Skip to content
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

Filter sizes value for intermediate images #1391

Open
t-wright opened this issue Dec 31, 2019 · 12 comments
Open

Filter sizes value for intermediate images #1391

t-wright opened this issue Dec 31, 2019 · 12 comments

Comments

@t-wright
Copy link
Contributor

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
Copy link
Contributor

simonwheatley commented Dec 31, 2019

Noted in JIRA as GH-3353

@yolih
Copy link

yolih commented Nov 19, 2020

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

Ticket: #119159-zd-wordpressvip

@joemcgill
Copy link

👋 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
Copy link

This issue mentioned in ticket #123565-zd-wordpressvip
Any blockers for committing the proposed fix?
#1900

@nickdaugherty
Copy link
Member

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

@mikeyarce
Copy link
Member

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.

@t-wright t-wright linked a pull request Mar 2, 2021 that will close this issue
5 tasks
@mikeyarce
Copy link
Member

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2022
@t-wright t-wright reopened this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants