WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 10 days 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: Future Release 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 (8)

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 11 months ago.
36982.2.diff (14.0 KB) - added by adamsilverstein 10 months ago.
36982.3.diff (14.6 KB) - added by adamsilverstein 10 months ago.
36982.4.diff (14.7 KB) - added by adamsilverstein 10 months ago.
36982.5.diff (15.8 KB) - added by adamsilverstein 10 months ago.
36982.6.diff (15.8 KB) - added by adamsilverstein 4 weeks ago.

Download all attachments as: .zip

Change History (49)

@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
5 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
5 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
5 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
15 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
11 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
11 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
11 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.


11 months ago

#12 @Mista-Flo
10 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.


10 months ago

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


10 months ago

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


10 months ago

#17 @adamsilverstein
10 months ago

In 36982.2.diff I added back a missing line from resolving the merge conflicts.

Pushed current state to PR to run all tests.

Version 0, edited 10 months ago by adamsilverstein (next)

#18 @adamsilverstein
10 months ago

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

#20 @Mista-Flo
10 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.


10 months ago

#22 @antpb
10 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
10 months ago

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

#24 @adamsilverstein
10 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
10 months ago

36982.4.diff also corrects phpcs warnings.

#26 @adamsilverstein
10 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
10 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
10 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
10 months ago

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

#30 @adamsilverstein
10 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.


10 months ago

#32 @hellofromTonya
10 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
10 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.

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


6 months ago

#35 @desrosj
6 months ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 feature freeze. Since there has been no progress this release cycle, I'm going to punt to Future Release. When someone is able to tackle this, it can be moved back to a numbered milestone.

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


5 weeks ago

#37 @adamsilverstein
4 weeks ago

Trying to get tests to pass I noticed two issues:

  • Test string is missing loading="lazy" (added since patch)
  • Sizes are not filtered correctly in because calls to wp_calculate_image_sizes in wp_image_add_srcset_and_sizes do not pass the image attributes, so checks like looking for the class in the test filters fail.

I think a little more work is needed to make the filters work properly there with attributes as well if we want to consider this complete. @kylereicks, I know it has been a long time since you worked on this, any chance you want to pick it up again?

#38 @adamsilverstein
4 weeks ago

36982.6.diff is a refresh against trunk and adds loading="lazy" to the expected image test string. Tests still not passing.

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


3 weeks ago

#40 @antpb
3 weeks ago

  • Milestone changed from 5.9 to Future Release

Thanks for the refresh Adam! I think this one may be a little too close to 5.9 but I'm optimistic the next release is a good target.

If anyone wants to move this back into 5.9 please feel free.

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


10 days ago

Note: See TracTickets for help on using tickets.