WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 months ago

#34736 accepted defect (bug)

wp_calculate_image_srcset filter: provide the individual srcset image size

Reported by: kraftbj Owned by: joemcgill
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch
Focuses: Cc:
PR Number:

Description

When using the wp_calculate_image_srcset filter, plugins have access to the value (typically the width), but no easy way to access the height of the image being used for that particular srcset source.

Since Core already knows the height of the image being considered, instead of asking a plugin to backwards-engineer it when needed, I suggest we should be explicitly sending the size to make it more functional.

Slack convo: https://wordpress.slack.com/archives/feature-respimg/p1447899460000167
Plugin example: https://github.com/Automattic/jetpack/pull/3040

Attachments (3)

34736.patch (1.0 KB) - added by kraftbj 4 years ago.
Initial patch to add a size array to the sources array.
34736.2.patch (1.7 KB) - added by mikeschroder 4 years ago.
Adjusted indentation
34736.3.patch (1.7 KB) - added by mikeschroder 4 years ago.
Remove extraneous "@type type" mentions

Download all attachments as: .zip

Change History (12)

@kraftbj
4 years ago

Initial patch to add a size array to the sources array.

#1 @kraftbj
4 years ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #feature-respimg by kraft. View the logs.


4 years ago

#3 @joemcgill
4 years ago

I don't have any immediate concerns with this enhancement, and if it's helpful for other developers to explicitly have this information, we should probably make it available. I would have reservations about changing the 'value' property of the array, since values could also be used to describe density values (e.g. 1x, 2x), but that's not what's being proposed here.

#4 @joemcgill
4 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner set to joemcgill
  • Status changed from new to accepted

@mikeschroder
4 years ago

Adjusted indentation

#5 @mikeschroder
4 years ago

This seems like a legitimate use case/request. Adjusted the indentation for 34736.2.patch.

Including the height here somewhere makes sense. It strikes me as odd to include the width twice -- both as a key, and in the size -- but I can see how it'd be easier to work with.

@mikeschroder
4 years ago

Remove extraneous "@type type" mentions

#6 @mikeschroder
4 years ago

Cleaned up @type type artifacts from previously added comments for 34736.3.patch.

#7 @mikeschroder
4 years ago

Chatted with @joemcgill, who mentioned he'd like to hold this for a discussion, rather than rushing it into RC1.

He'll follow up here with a comment, but wanted to make sure that it didn't get committed in the meantime.

#8 @joemcgill
4 years ago

  • Milestone changed from 4.4 to Future Release

It strikes me as odd to include the width twice -- both as a key, and in the size -- but I can see how it'd be easier to work with.

I agree that including the width multiple times in the same array is awkward. If we make this change, we essentially have the width listed three times: the array key for each source, the value of each source, and again in a size array. We could only add a height value to each source instead of size array, but it seems equally awkward to have a height value without a width value. Additionally, if h descriptors ever become supported by browsers—something that has been requested—we'd run into this all over again, but from the opposite perspective.

I spoke with @kraftbj and came up with an alternate solution for their specific use case, which is the use the $size_array of the src image to calculate the image ratio, and then applying the ratio to the source value to calculate the height of each source image.

Since there is a decent workaround, I'm going to punt this to a future release rather than rushing this for RC1. If we receive more feedback that passing the height for each source would be helpful to developers, then we can revisit.

Somewhat unrelated, the @type doc fixes from 34736.3.patch should probably go in. Either @DH-Shredder or I will create a new patch for that change on #34733.

#9 @mikeschroder
4 years ago

+1

Thanks @joemcgill!

Note: See TracTickets for help on using tickets.