Skip to content

Conversation

@mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Jul 17, 2024

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 source tags

@mukeshpanchal27 mukeshpanchal27 added [Type] Bug An existing feature is broken [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) labels Jul 17, 2024
@mukeshpanchal27 mukeshpanchal27 added this to the auto-sizes n.e.x.t milestone Jul 17, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Jul 17, 2024
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review July 17, 2024 09:09
@github-actions
Copy link

github-actions bot commented Jul 17, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter
Copy link
Member

Can a test be added which demonstrates the bug if the changes aren't applied?

mukeshpanchal27 and others added 3 commits July 18, 2024 09:56
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
@mukeshpanchal27
Copy link
Member Author

Can a test be added which demonstrates the bug if the changes aren't applied?

@westonruter the bug only generated when the Enhanced Responsive Images (auto-sizes) plugin also activated.

Copy link
Member

@joemcgill joemcgill left a 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.

@mukeshpanchal27
Copy link
Member Author

Thanks @joemcgill for the feedback.

I have updated PR that use sizes value from the IMG tag and reverted the filter priority changes. I will open issue for wp_calculate_image_sizes()

@mukeshpanchal27 mukeshpanchal27 removed the [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) label Jul 19, 2024
@mukeshpanchal27 mukeshpanchal27 removed this from the auto-sizes n.e.x.t milestone Jul 19, 2024
continue;
}
// Get the sizes from the IMG tag.
$sizes = $processor->get_attribute( 'sizes' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@westonruter westonruter left a 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.

@mukeshpanchal27
Copy link
Member Author

Thanks @westonruter @adamsilverstein (Joe OoO) for the feedback. PR ready for review.

Copy link
Member

@westonruter westonruter left a 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.

Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

mukeshpanchal27 and others added 4 commits July 26, 2024 09:23
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
@mukeshpanchal27 mukeshpanchal27 merged commit 1216ead into trunk Jul 26, 2024
@mukeshpanchal27 mukeshpanchal27 deleted the fix/1349-sizes-picture-element branch July 26, 2024 04:34
@westonruter westonruter changed the title Fix sizes issue for picture element Fix sizes issue for PICTUER element Aug 16, 2024
@westonruter westonruter changed the title Fix sizes issue for PICTUER element Fix sizes issue for PICTURE element Aug 16, 2024
@westonruter westonruter changed the title Fix sizes issue for PICTURE element Fix setting sizes attribute on PICTURE > SOURCE elements Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Bug An existing feature is broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Picture element: The accurate sizes improvement for images not working

5 participants