Harden logic to add `auto` keyword to `sizes` attribute to prevent duplicate keyword #1445

felixarntz · 2024-08-07T18:20:26Z

Currently, the Enhanced Responsive Images functions that conditionally add the auto keyword to the image sizes attribute only check for the attribute in one specific way. This works to prevent double processing of an image that the plugin itself has added, but may lead to problems if the auto keyword was added through another means, where it can appear in a different position within the sizes attribute.

Relevant technical choices

Create new helper function auto_sizes_attribute_includes_auto( string $sizes_attr ): bool that checks a sizes attribute for presence of the auto keyword, so that this logic can be reused between both filter callbacks.
Add test coverage for both the existing and new logic to prevent duplicate auto keywords.

…ate keyword.

plugins/auto-sizes/hooks.php

plugins/auto-sizes/tests/test-auto-sizes.php

Co-authored-by: Weston Ruter <[email protected]>

plugins/auto-sizes/tests/test-auto-sizes.php

…d in combination with auto.

felixarntz · 2024-08-07T19:21:48Z

plugins/auto-sizes/tests/test-auto-sizes.php

+			'within, without space'       => array(
+				'(max-width: 1024px) 100vw, auto,1024px',
+				false,
+			),
+			'within, with space'          => array(
+				'(max-width: 1024px) 100vw, auto, 1024px',
+				false,
+			),
+			'at the end, without space'   => array(
+				'(max-width: 1024px) 100vw,auto',
+				false,
+			),
+			'at the end, with space'      => array(
+				'(max-width: 1024px) 100vw, auto',
+				false,
+			),
+			'sole keyword'                => array(
+				'auto',
+				false,
+			),
+			'with space at beginning'     => array(
+				' auto, (max-width: 1024px) 100vw, 1024px',
+				false,
+			),
+			'with uppercase'              => array(
+				'AUTO, (max-width: 1024px) 100vw, 1024px',
+				false,
+			),


FYI: All of these would have failed prior to the production code changes in this PR.

github-actions · 2024-08-07T19:22:30Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: felixarntz <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: joemcgill <[email protected]>
Co-authored-by: dmsnell <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

joemcgill · 2024-08-07T19:31:59Z

This works to prevent double processing of an image that the plugin itself has added, but may lead to problems if the auto keyword was added through another means, where it can appear in a different position within the sizes attribute.

@felixarntz can you give an example of what problem this is actually solving? Per the HTML spec, auto should only appear as the first value in the sizes list. Placing it elsewhere in sizes would not be valid.

plugins/auto-sizes/hooks.php

felixarntz · 2024-08-07T19:41:46Z

@joemcgill

@felixarntz can you give an example of what problem this is actually solving? Per the HTML spec, auto should only appear as the first value in the sizes list. Placing it elsewhere in sizes would not be valid.

Is that correct? Will the value not be parsed if it's placed elsewhere?

Regardless, if that's the case, some valid usages were not covered by the implementation so far, such as sizes="auto" or sizes="AUTO, ...".

joemcgill

I'm happy to see us consolidate some of this early logic for checking whether sizes already exists with "auto" applied into a helper function, but we should only be looking for "auto" in the first position of the attribute. Otherwise, I don't believe the attribute will have the intended effect in the first place as it's expected to be the first thing in the attribute.

joemcgill · 2024-08-07T19:45:12Z

Regardless, if that's the case, some valid usages were not covered by the implementation so far, such as sizes="auto" or sizes="AUTO, ...".

If there are valid cases like that which we can demonstrate get parsed correctly by browsers, then I agree we should handle them. The standalone sizes="auto" value is a great catch, for example.

westonruter · 2024-08-07T20:07:01Z

can you give an example of what problem this is actually solving? Per the HTML spec, auto should only appear as the first value in the sizes list. Placing it elsewhere in sizes would not be valid.

I'm happy to see us consolidate some of this early logic for checking whether sizes already exists with "auto" applied into a helper function, but we should only be looking for "auto" in the first position of the attribute. Otherwise, I don't believe the attribute will have the intended effect in the first place as it's expected to be the first thing in the attribute.

@joemcgill It seems like the spec does allow auto anywhere:

<source-size-list> = <source-size>#? , <source-size-value>
<source-size> = <media-condition> <source-size-value> | auto
<source-size-value> = <length> | auto

plugins/auto-sizes/hooks.php

joemcgill · 2024-08-08T14:29:01Z

The sizes attribute gets parsed left to right and uses the first value that is applicable. It may be technically possible to put "auto" somewhere other than the first value without breaking the parser, but I don't think it would work as intended. Regardless, for our purposes we're only interested in cases where people are using the intended value so that we aren't applying auto-sizes to a sizes attribute that is already using "auto" correctly, so checking the first position is sufficient.

felixarntz · 2024-08-08T15:52:17Z

I'm happy to update this PR to only look for an auto occurrence in the beginning. The spec technically allows for it to be in any position, but at the same time it's clearly formulated that it must be at the beginning or the entire attribute.

Most of this PR is test coverage anyway, and it'll still add support for the valid case of sizes="auto", which would currently lead to the plugin creating sizes="auto, auto", which doesn't make sense. :)

felixarntz · 2024-08-08T16:03:43Z

@joemcgill In f34ab23 I've updated the PR to only look for auto in the beginning.

I kept all tests from before and just changed the expected value, so that it is captured that in the case of auto being in an invalid position, it should still be added by the plugin.

plugins/auto-sizes/hooks.php

Co-authored-by: Weston Ruter <[email protected]>

Co-authored-by: dmsnell <[email protected]>

Only the initial token is now examined for being auto

dmsnell · 2024-08-12T19:40:07Z

plugins/auto-sizes/hooks.php

+ */
+function auto_sizes_attribute_includes_valid_auto( string $sizes_attr ): bool {
+	$token = strtok( strtolower( $sizes_attr ), ',' );
+	return false !== $token && 'auto' === trim( $token, " \t\f\r\n" );


@westonruter just curious, did you confirm in the browsers that ,,,,,,,auto is still registered as auto for the size?

I did not confirm, no.

Since I know basically nothing about this feature, I had to look up a lot (and discovered that only Chrome supports this feature).

Also I relied on this post in addition to the HTML spec to figure out what's going on.

Ultimately I confirmed that Chrome respects the spec, and strangely enough, if the attribute value is ,,,,,auto then no image is lazy-loaded or displayed at all. In other words, we're still trading one broken auto for another. Now, I don't want to be a broken record, but the code I posted before doesn't suffer this same problem.

Setup Chrome Safari

sizes=auto ✅ 🟡 Loads the 1200w image

sizes=",auto" 🚫 Loads nothing 🟡 Loads the 1200w image

sizes="auto," ✅ 🟡 Loads the 1200w image

Interestingly enough this broken behavior only seems to affect lazy-loaded images in Chrome. That is, if the page/tab is visible while loading the page, then no image loads. However, if you can get the page to load the first time when the tab is hidden, then the 1200w image will load, matching Safari's behavior. To reproduce this, open up a test page where Chrome fails to render any image with the invalid attribute value, open another tab, quit Chrome, and then re-open where it restores the session. The first srcset image will now be displayed. I suspect this is a bug in Chrome, but its behavior is worse under the presence of this attribute value defect than Safari's, which lacks support for the auto value.

 <img loading="lazy" sizes=",,,,,auto" srcset=" https://o.img.rodeo/w_1200,h_1200,b_tomato/t_WxH/_.png 1200w, https://o.img.rodeo/w_900,h_900,b_goldenrod/t_WxH/_.png 900w, https://o.img.rodeo/w_600,h_600,b_forestgreen/t_WxH/_.png 400w, https://o.img.rodeo/w_300,h_300,b_dodgerblue/t_WxH/_.png 300w" src="https://o.img.rodeo/w_300,h_300,b_forestgreen/t_WxH/_.png" alt="An example image that reports its natural dimensions" >

This also seemed to have shipped without checking if the attribute is sizes or only ends in sizes, so anything like data-wp-sizes="state.imageSizes" will also be corrupted by it, and the sizes attribute may not be updated properly.

See #1476 which handles the case of leading ,,, chars.

Harden logic to add auto keyword to sizes attribute to prevent duplic…

eb82158

…ate keyword.

felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) labels Aug 7, 2024

felixarntz added this to the auto-sizes n.e.x.t milestone Aug 7, 2024

Add two more test scenarios.

23687cd

westonruter reviewed Aug 7, 2024

View reviewed changes

plugins/auto-sizes/hooks.php Outdated Show resolved Hide resolved

plugins/auto-sizes/hooks.php Show resolved Hide resolved

plugins/auto-sizes/tests/test-auto-sizes.php Show resolved Hide resolved

plugins/auto-sizes/tests/test-auto-sizes.php Outdated Show resolved Hide resolved

Further hardening of logic (also support non-lowercase).

ec490e8

Co-authored-by: Weston Ruter <[email protected]>

westonruter reviewed Aug 7, 2024

View reviewed changes

plugins/auto-sizes/tests/test-auto-sizes.php Outdated Show resolved Hide resolved

felixarntz added 2 commits August 7, 2024 12:10

Fix whitespace.

51e607d

Fix failing test scenarios after update, so media query cannot be use…

471a1d0

…d in combination with auto.

felixarntz commented Aug 7, 2024

View reviewed changes

felixarntz marked this pull request as ready for review August 7, 2024 19:22

felixarntz requested review from joemcgill and mukeshpanchal27 as code owners August 7, 2024 19:22

westonruter reviewed Aug 7, 2024

View reviewed changes

plugins/auto-sizes/hooks.php Outdated Show resolved Hide resolved

joemcgill previously requested changes Aug 7, 2024

View reviewed changes

westonruter mentioned this pull request Aug 7, 2024

Injection of sizes=auto into images can be more robust with HTML Tag Processor #1446

Closed

dmsnell reviewed Aug 8, 2024

View reviewed changes

plugins/auto-sizes/hooks.php Outdated Show resolved Hide resolved

Only look for auto as the first entry in the sizes attribute.

f34ab23

felixarntz requested review from joemcgill, westonruter and dmsnell August 8, 2024 16:07

westonruter reviewed Aug 8, 2024

View reviewed changes

plugins/auto-sizes/hooks.php Outdated Show resolved Hide resolved

felixarntz and others added 2 commits August 8, 2024 13:40

Simplify check for sizes attribute presence.

a18021f

Co-authored-by: Weston Ruter <[email protected]>

Merge branch 'trunk' into auto-sizes/harden-attribute-logic

644cef5

felixarntz requested a review from westonruter August 9, 2024 20:39

Use strtok() since only the first instance of auto is considered

18f00cc

Co-authored-by: dmsnell <[email protected]>

westonruter approved these changes Aug 9, 2024

View reviewed changes

mukeshpanchal27 approved these changes Aug 12, 2024

View reviewed changes

felixarntz merged commit aab5038 into trunk Aug 12, 2024
15 checks passed

felixarntz deleted the auto-sizes/harden-attribute-logic branch August 12, 2024 14:35

dmsnell reviewed Aug 12, 2024

View reviewed changes

This was referenced Aug 14, 2024

Use more robust HTML Tag Processor for auto sizes injection #1471

Merged

Move Auto Sizes logic from Enhanced Responsive Images to Image Prioritizer #1476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harden logic to add `auto` keyword to `sizes` attribute to prevent duplicate keyword #1445

Harden logic to add `auto` keyword to `sizes` attribute to prevent duplicate keyword #1445

felixarntz commented Aug 7, 2024

felixarntz Aug 7, 2024

github-actions bot commented Aug 7, 2024 •

edited

Loading

joemcgill commented Aug 7, 2024

felixarntz commented Aug 7, 2024

joemcgill left a comment

joemcgill commented Aug 7, 2024

westonruter commented Aug 7, 2024

joemcgill commented Aug 8, 2024

felixarntz commented Aug 8, 2024

felixarntz commented Aug 8, 2024

dmsnell Aug 12, 2024

westonruter Aug 12, 2024

dmsnell Aug 12, 2024 •

edited

Loading

westonruter Aug 16, 2024

Setup	Chrome	Safari
`sizes=auto`	✅	🟡 Loads the 1200w image
`sizes=",auto"`	🚫 Loads nothing	🟡 Loads the 1200w image
`sizes="auto,"`	✅	🟡 Loads the 1200w image

Harden logic to add auto keyword to sizes attribute to prevent duplicate keyword #1445

Harden logic to add auto keyword to sizes attribute to prevent duplicate keyword #1445

Conversation

felixarntz commented Aug 7, 2024

Relevant technical choices

felixarntz Aug 7, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 7, 2024 • edited Loading

joemcgill commented Aug 7, 2024

felixarntz commented Aug 7, 2024

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill commented Aug 7, 2024

westonruter commented Aug 7, 2024

joemcgill commented Aug 8, 2024

felixarntz commented Aug 8, 2024

felixarntz commented Aug 8, 2024

dmsnell Aug 12, 2024

Choose a reason for hiding this comment

westonruter Aug 12, 2024

Choose a reason for hiding this comment

dmsnell Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

westonruter Aug 16, 2024

Choose a reason for hiding this comment

Harden logic to add `auto` keyword to `sizes` attribute to prevent duplicate keyword #1445

Harden logic to add `auto` keyword to `sizes` attribute to prevent duplicate keyword #1445

github-actions bot commented Aug 7, 2024 •

edited

Loading

dmsnell Aug 12, 2024 •

edited

Loading