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

Fix image sizes and srcset for intermediate images #2007

Closed
wants to merge 6 commits into from

Conversation

mikeyarce
Copy link
Member

Description

This PR uses a new filter available in WP 5.7 to properly pass image dimensions for the image srcset and sizes.
This needs to be tested and merged in coordination with the WP 5.7 release (March 9 is the current tentative release date).

New filter added in WP 5.7: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/media.php?rev=50136#L1643

How the fix works:

  • First we hook into wp_img_tag_add_width_and_height_attr which is early enough that our image source still has the query arguments available so we know what the image size is intended to be
  • We save that image size data and then pass it into wp_image_src_get_dimensions where we set image dimensions
  • If the image is a full size image, we set the default dimensions
  • If the image is not full size, we calculate the image size based on the given width and the calculated aspect ratio.

Changelog Description

Bug Fix: Fixed an issue where images were not properly sized when using the Block Editor and intermediate image sizes.

Previously, all images added through the Block Editor that were assigned intermediate image sizes ( e.g. Thumbnail ) would appear as full size images.

Checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally (or has an appropriate fallback).
  • This change works and has been tested on a Go sandbox.
  • This change has relevant unit tests (if applicable).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Steps to Test

  1. Patch your wp-includes/media.php file to include the changes to wp_image_src_get_dimensions found here: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/media.php?rev=50136#L1601
  2. Apply this code to your sandbox
  3. Make sure your test site has Gutenberg Enabled
  4. Create a new Post
  5. Select the Image Block, add an image
  6. From the Block Properties, select an intermediate image size like Thumbnail
  7. Add more image blocks for Full Size, Medium, Large, etc..
  8. Verify that the srcset and sizes attributes output are correct.

Before the change:
before-patch-srcset

After the change:
after-patch-srcset

Use a new filter available in WP 5.7 to properly pass image dimensions for the image srcset.
@mikeyarce mikeyarce requested a review from a team as a code owner March 1, 2021 22:50
@GaryJones
Copy link
Contributor

Will those who are still on WP versions before 5.7, continue to see the same full size image for each named image size (i.e. the Before)?

* @param string $context Additional context about how the function was called or where the img tag is.
* @param int $attachment_id The image attachment ID.
*/
function a8c_files_maybe_inject_dimensions( $value, $image, $context, $attachment_id ) {
Copy link
Contributor

@GaryJones GaryJones Mar 1, 2021

Choose a reason for hiding this comment

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

Any reason not to use type declarations on these parameters and return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! We're not really using most of these since we're just returning true for the value in this filter and we're not returning any of them either, so I wasn't sure if that would be helpful in this case. Would you suggest still adding them? I also wasn't sure if I should pass them all even if we're not using them?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're not using them, then there is no need to support them in the function signature, unless a param to the right is being used.

However, since you need $attachment_id, then they all need to be passed.

The non-first types should be known, as they are from core code, but the first type may have been filtered at an earlier priority into a different type, so although it's not being used here, the advantage of using types here is that it becomes an extra check in case some other code is doing something wrong. It's also good for IDEs and static analysis tools, like Psalm or Phpstan

Adding the return type also has no drawbacks.

a8c-files.php Outdated Show resolved Hide resolved
a8c-files.php Outdated Show resolved Hide resolved
a8c-files.php Outdated Show resolved Hide resolved
a8c-files.php Outdated Show resolved Hide resolved
a8c-files.php Show resolved Hide resolved
@t-wright t-wright linked an issue Mar 2, 2021 that may be closed by this pull request
@mikeyarce
Copy link
Member Author

Thanks for your review @GaryJones! I've addressed the issues and separated out the dimension logic into another function.

Will those who are still on WP versions before 5.7, continue to see the same full size image for each named image size (i.e. the Before)?

I tested this and for versions before 5.7, site's will continue to see the same before full size image for each named image.

@rinatkhaziev
Copy link
Contributor

We're investigating other avenues, in the meantime, we'll keep it open but with the appropriate status.

@mikeyarce mikeyarce marked this pull request as draft April 21, 2021 15:19
@mchanDev
Copy link
Contributor

mchanDev commented Jun 2, 2021

Hi @rinatkhaziev

Is there a work around for this atm?

There is an open ticket here #128167-z that I would like to reply to

@DustinHartzler
Copy link
Contributor

Hi @rinatkhaziev

Just leaving a note that this issue came up in #145657-z.

@sjinks sjinks force-pushed the develop branch 2 times, most recently from 529fa93 to 1ac9637 Compare July 26, 2022 10:59
@github-actions
Copy link
Contributor

This pull request 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 pull requests manageable and actionable and is not a comment on the quality of this pull request nor on the work done so far. Closed PRs are still valuable to the project and their branches are preserved.

@github-actions github-actions bot closed this Oct 2, 2022
@gudmdharalds gudmdharalds deleted the fix-image-size-srcset branch September 19, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter sizes value for intermediate images
5 participants