Automattic / vip-go-mu-plugins

The development repo for mu-plugins used on the WordPress VIP Platform.
https://docs.wpvip.com/
GNU General Public License v2.0
188 stars 103 forks source link

Filter `sizes` value for intermediate images #1391

Open t-wright opened 4 years ago

t-wright commented 4 years ago

Desired Behavior

Responsive images should have a value on the sizes attribute which reflects the width of the chosen intermediate size.

Actual Behavior

When images are added to posts using the block editor, the value of the sizes attribute currently reflects the width of the original/full-size image. This can cause these images to display at the incorrect dimensions.

For more information:

P2: p9zhOE-Qw-p2 Ticket: #102087-zd-wordpressvip

simonwheatley commented 4 years ago

Noted in JIRA as GH-3353

yolih commented 3 years ago

This issue persists and has been reported by Greg from Alley as well.

Ticket: #119159-zd-wordpressvip

joemcgill commented 3 years ago

đź‘‹ Hi all. It looks like what is most likely happening here is that something in the a8c-files plugin is causing the dimension matching that would normally happen in wp_image_src_get_dimensions to fail. Unfortunately, there isn't currently a filter available to fix this issue when it occurs, but I've opened a Trac ticket to add this as a possible fix.

Ideally, this is something that could be fixed in the VIP plugin repo more quickly since this bug currently causes almost all images in content to be rendered with incorrect height, width, and sizes attributes—causing browsers to request much larger file sizes than necessary.

lschuyler commented 3 years ago

This issue mentioned in ticket #123565-zd-wordpressvip Any blockers for committing the proposed fix? https://github.com/Automattic/vip-go-mu-plugins/pull/1900

nickdaugherty commented 3 years ago

Hey @lschuyler - we discussed internally and want to explore other ways of addressing this that ideally don't need to filter the_content.

mikeyarce commented 3 years ago

This has been addressed in WP 5.7 with https://core.trac.wordpress.org/ticket/51865 Once that's released we can close this issue out.

mikeyarce commented 3 years ago

Ok, turns out we can't close this quite yet even with the new filter.

The main issue right now is that by the time wp_image_src_get_dimensions runs, our image path has been stripped of the requested dimensions that are found in the URL query string (e.g. ?w=400). WordPress then assumes it's a full size image and acts accordingly.

Where the parameters are stripped: list( $image_src ) = explode( '?', $image_src );

From: https://github.com/WordPress/WordPress/blob/0c5a3fa2ed4a6c773363984f61abd6b6d9641a69/wp-includes/media.php#L1915

Jetpack's Photon doesn't have the same issue because it does the interpolation really late on the_content whereas VIP does it really early, when the image is added in the editor.

@joemcgill - do you have any insight into why WordPress wants to remove these parameters? I wonder if we could add a filter here that could allow us to bypass this when we need these arguments to determine image dimensions.

joemcgill commented 3 years ago

I don't know exactly why that line was added to this function, except that WordPress uses the file name to determine what the expected dimensions should be, by comparing against the sizes array for that attachment in post meta. Since you're overriding the URL earlier, we may need to add another filter here or pass the original src to the wp_image_src_get_dimensions function so you can override things there. It's worth opening a Trac ticket for, because you're not the only plugin that is filtering the src value before these calculations are made.

github-actions[bot] commented 2 years ago

This issue has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep issues manageable and actionable and is not a comment on the quality of this issue nor on the work done so far. Closed issues are still valuable to the project and are available to be searched.

kevinfodness commented 2 years ago

@mikeyarce @joemcgill has there been any movement on this issue? We're currently coding our way around it in themes, but it would be great to have it work out of the box. Is there anything I can do to help?

joemcgill commented 2 years ago

@kevinfodness We've attempted to polyfill this solution into our own projects, but it didn't seem to work in the couple of places we tested it, but I've not revisited the implementation in several months. I assume we would need a new solution at this point.

github-actions[bot] commented 1 year ago

This issue has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep issues manageable and actionable and is not a comment on the quality of this issue nor on the work done so far. Closed issues are still valuable to the project and are available to be searched.