WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#36982 assigned enhancement

Add image attributes and additional context to the wp_calculate_image_srcset and wp_calculate_image_sizes filters

Reported by: kylereicks Owned by: adamsilverstein
Milestone: 5.8 Priority: normal
Severity: normal Version: 4.6
Component: Media Keywords: has-patch 2nd-opinion has-unit-tests
Focuses: template Cc:

Description

Because the srcset and sizes attributes try to give the browser guidance on how CSS is going to handle images, we should be able to take into account image class attributes (and other attributes that could be used as CSS selectors) when using the wp_calculate_image_srcset and wp_calculate_image_sizes filters to set the appropriate srcset and sizes.

It would also be helpful to be able to supply a context to functions like wp_image_add_srcset_and_sizes and wp_make_content_images_responsive so we can calculate different srcset and sizes attributes if a medium sized image is in the main content section vs. in an aside or other section.

Attachments (7)

media-srcset-sizes.patch (6.7 KB) - added by kylereicks 5 years ago.
Adds image attributes and additional context to wp_calculate_image_srcset and wp_calculate_image_sizes filters
media-srcset-sizes.2.patch (13.3 KB) - added by kylereicks 4 years ago.
Update patch to remove the "context" attribute. Add a test to make sure that srcsets and sizes can be successfully updated via filters.
36982.diff (13.2 KB) - added by adamsilverstein 4 months ago.
36982.2.diff (14.0 KB) - added by adamsilverstein 3 months ago.
36982.3.diff (14.6 KB) - added by adamsilverstein 3 months ago.
36982.4.diff (14.7 KB) - added by adamsilverstein 3 months ago.
36982.5.diff (15.8 KB) - added by adamsilverstein 3 months ago.

Download all attachments as: .zip

Change History (40)

@kylereicks
5 years ago

Adds image attributes and additional context to wp_calculate_image_srcset and wp_calculate_image_sizes filters

#1 @kylereicks
5 years ago

As an example, if a theme had a multi column layout like the following:

img, figure{
	max-width: 100%;
	height: auto;
}
.alignright{
	max-width: calc(100% + 3em);
	float: right;
	margin-right: -6em;
}
.entry-content{
	max-width: 350px;
	margin: 0 auto;
	padding: 1em 0;
	columns: 1;
	column-gap: 12em;
}
@media screen and (min-width: 600px){
	.alignright{
		max-width: 150%;
		margin-right: -50%;
	}
}
@media screen and (min-width: 900px){
	.alignright{
		max-width: calc(100% + 6em);
		float: right;
		margin-right: -6em;
	}
	.entry-content{
		max-width: none;
		columns: 2;
		margin: 0 6em;
	}
}
@media screen and (min-width: 1200px){
	.entry-content{
		columns: 3;
		column-gap: 6em;
	}
	.alignright{
		max-width: calc(100% + 3em);
		float: right;
		margin-right: -3em;
	}

}

An image with no alignment class might have a sizes attribute like (min-width: 1200px) calc((100vw / 3) - 3em), (min-width: 900px) calc((100vw / 2) - 6em), (min-width: 350px) 350px, 100vw whereas an image with the class .alignright might need a slightly different sizes attribute a more like (min-width: 1200px) calc(100vw / 3), (min-width: 900px) calc(100vw / 2), (min-width: 600px) 525px, (min-width: 350px) calc(350px + 3em), 100vw

Without access to the image attributes (class, id etc.) when filtering srcset and sizes, we cannot tell the difference between a medium sized image and a medium sized image with the class .alignright even though they may be sized differently at some breakpoints.

#2 @kylereicks
5 years ago

  • Keywords dev-feedback added

#3 @herrschuessler
4 years ago

I second @kylereicks considerations. There are many use cases where responsive images will need a different sizes attribute based on their alignment value, e.g. when floated images are displayed smaller than non-aligned, full-column images. Therefore, the alignment value should be available in wp_calculate_image_sizes.

#4 @greatestview
4 years ago

+1

Currently there is no way to tell exactly, which image is small and wide in wp_calculate_image_sizes. It’s the same with wp_get_attachment_image_attributes. I would appreciate providing the classes of that image as an additional argument, or maybe better, the whole image html code.

#5 @adamsilverstein
4 years ago

  • Keywords has-patch needs-unit-tests 2nd-opinion added; dev-feedback removed
  • Owner set to adamsilverstein
  • Status changed from new to assigned

@kylereicks Thanks for your ticket and patch and welcome to trac!

Overall the patch looks good and the use case makes sense... this seems like a valuable addition to the filter. Can you give an example of how you would use the context variable, or why you added that?

It would be good to add some unit tests that demonstrate how filter change is used: how the patch enables altering the sizes based on both attr values and context.

@kylereicks
4 years ago

Update patch to remove the "context" attribute. Add a test to make sure that srcsets and sizes can be successfully updated via filters.

#6 @kylereicks
4 years ago

  • Keywords needs-unit-tests removed

I added a new patch to remove the "context" attribute. It may not end up being useful for most cases and is not a critical addition. The new patch also adds unit tests to confirm that the wp_calculate_image_srcset and wp_calculate_image_sizes filters can access an image's class attribute and update their values based on that attribute.

#7 @strarsis
9 months ago

Currently, I have to resort to either the ratio (when hard crop is used for that particular image size) or other tricks to deduce the usage of a particular image in the wp_calculate_image_sizes filter.
Basically the usage context of the image has to be "encoded" as image size dimensions or ratio, there are no other means. Just knowing the markup/Gutenberg block the image is used in would help tremendously.

#8 @adamsilverstein
4 months ago

  • Milestone changed from Awaiting Review to 5.7

Revisiting tickets and this still seems useful. Marking as 5.7 as a reminder to land this.

#9 @adamsilverstein
4 months ago

@kylereicks I couldn't quite understand the regex your patch includes to extract the image attributes. Can we just not pass attributes in that case? Developers still get the source and could extract attributes if they need them.

If we do want to try to extract the attributes, I would ask:

  • Document the RegEx part by part so it can be understood more easily.
  • Move the attribute extraction into a helper.
  • Add tests that validate the helper works correctly in all cases.

Otherwise this patch looks good: when we have the image attributes, we pass them to the wp_calculate_image_sizes and wp_calculate_image_srcset filters to make those filters more useful for developers.

#10 @adamsilverstein
4 months ago

36982.diff is a refresh against trunk with the regex bit removed. I resolved several merge conflicts so this deserves careful review.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 months ago

#12 @Mista-Flo
3 months ago

  • Keywords has-unit-tests added

Patch looks good, well done Adam

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 months ago

#17 @adamsilverstein
3 months ago

In 36982.2.diff I added back a missing line from resolving the merge conflicts. Also added @since annotations for the new parameter.

Pushed current state to PR to run all tests.

Last edited 3 months ago by adamsilverstein (previous) (diff)

#18 @adamsilverstein
3 months ago

36982.2.diff is a refresh against trunk. Rerunning all tests on the PR.

#20 @Mista-Flo
3 months ago

Thanks for the refresh Adam, the removed line came back, code looks good, unit tests are pretty solid, I think it's ok to go.

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


3 months ago

#22 @antpb
3 months ago

I think the PR is false green because the changes are different from the uploaded patch.

I'm seeing test failures locally around undefined $image_attr attributes. Going to investigate fixing these on my end.

example failure
57) Tests_XMLRPC_wp_newPost::test_post_thumbnail Undefined variable: image_attr

I suspect it's a quick find/replace sort of fix for these tests.

Would appreciate a review before commit!

#23 @adamsilverstein
3 months ago

Yea, @antpb apologies that is the wrong PR linked, let me find the right one :)

#24 @adamsilverstein
3 months ago

This is the correct PR where the tests are currently running after I pushed an updated with fixes for phpcs: https://github.com/WordPress/wordpress-develop/pull/968.

#25 @adamsilverstein
3 months ago

36982.4.diff also corrects phpcs warnings.

#26 @adamsilverstein
3 months ago

I'm seeing test failures locally around undefined $image_attr attributes. Going to investigate fixing these on my end.

I see these as well in GitHub, thanks for investigating.

#27 @antpb
3 months ago

Just investigating the first failure around the get_custom_logo()
1) Tests_WP_Customize_Manager::test_import_theme_starter_content Undefined variable: image_attr

I tested the function on a fresh install with a custom logo set and am seeing errors when using the function.

Getting the notice
Notice: Undefined variable: image_attr in /var/www/src/wp-includes/media.php on line 1529

I need to dig deeper into the other ~50 failures to see if theres another function we'll need to factor this fix into. Likely a repeated function through our tests. get_custom_logo doesnt seem to be more than a 5 of them.

#28 @adamsilverstein
3 months ago

I see the issue, we are using $image_attr in wp_calculate_image_sizes without actually passing it in. Checking history to see where that was lost.

#29 @adamsilverstein
3 months ago

36982.5.diff fixes the missing $image_attr issue in wp_calculate_image_sizes.

#30 @adamsilverstein
3 months ago

Still seeing some failing tests, these are the tests added in the patch. I'll have to dig in further to see exactly why they are failing.

Hey @kylereicks, if you are available I would appreciate your help getting this over the finish line!

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 months ago

#32 @hellofromTonya
3 months ago

  • Milestone changed from 5.7 to 5.8

5.7 Beta 1 is happening in a few minutes. Ran out of time to get this ticket into the release. Punting to 5.8.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#33 @adamsilverstein
3 months ago

5.7 Beta 1 is happening in a few minutes. Ran out of time to get this ticket into the release. Punting to 5.8.

Thanks @hellofromTonya - I'll pick this up again for 5.8.

Note: See TracTickets for help on using tickets.