-
Notifications
You must be signed in to change notification settings - Fork 140
Fix setting sizes attribute on PICTURE > SOURCE elements #1354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Can a test be added which demonstrates the bug if the changes aren't applied? |
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
@westonruter the bug only generated when the Enhanced Responsive Images (auto-sizes) plugin also activated. |
joemcgill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the most robust approach to fixing this issue. If I am understanding the problem correctly, the issue is caused because the Enhanced Responsive Images plugin modifies the sizes attribute using the wp_content_img_tag filter, since we're not getting all of the information we need to filter the wp_calculate_image_sizes() value directly.
However, when Modern Image Formats applies webp_uploads_wrap_image_in_picture() it adds sizes attributes to the source elements by calling wp_calculate_image_sizes() directly, rather than using the actual sizes attribute that is on the img element that it is wrapping.
I think that a better solution would be to update webp_uploads_wrap_image_in_picture() to try to copy the sizes attribute from the img that it's wrapping, if available, rather than using wp_calculate_image_sizes() directly. This would ensure that any modifications that were made to the sizes attribute, regardless of what filter was used, will be applied to the source elements as well.
Alternately, we need to make sure the enhanced sizes attribute ends up getting applied via the wp_calculate_image_sizes filter so that any other code that calls wp_calculate_image_sizes() directly will have the optimizations applied as well.
As a side note: when encountering compatibility issues like this one, we should be skeptical about modifying filter order as a way of fixing the problem, since we can't control the filter order of all third party code that might modify the HTML markup that our plugins affect.
|
Thanks @joemcgill for the feedback. I have updated PR that use |
| continue; | ||
| } | ||
| // Get the sizes from the IMG tag. | ||
| $sizes = $processor->get_attribute( 'sizes' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
westonruter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Almost there.
|
Thanks @westonruter @adamsilverstein (Joe OoO) for the feedback. PR ready for review. |
westonruter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple nits, but pre-approving as LGTM.
adamsilverstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Summary
Fixes #1349
The PR changes the filter priority for the picture element filter so it will be generated before the sizes attribute updates. Add new filter for picture element in auto-sizes plugin that add accurate sizes change for
sourcetags