Fix incorrect image size selection in `PICTURE` element #1885

b1ink0 · 2025-02-25T18:13:32Z

Summary

Relevant technical choices

The srcset is generated differently for the SOURCE and IMG tags when there are two image sizes with the same width but different heights. This discrepancy occurs because the wp_image_matches_ratio check is applied during wp_calculate_image_srcset for generating the srcset of the IMG tag. However, for the SOURCE tag, the srcset is generated within the webp_uploads_wrap_image_in_picture function, which then overrides the sources created by the wp_get_attachment_image_srcset function using wp_calculate_image_srcset filter. The issue was caused by the absence of the wp_image_matches_ratio check in webp_uploads_wrap_image_in_picture, leading to incorrect image size selection.

Below is the code snippet to reproduce the bug.

add_image_size( 'square', 768, 768, true );
add_filter( 'pre_option_medium_large_size_w', function() { return '768'; } );
add_filter( 'pre_option_medium_large_size_h', function() { return '512'; } );

codecov · 2025-02-25T18:22:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.01%. Comparing base (ad15727) to head (2c92462).
Report is 14 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1885      +/-   ##
==========================================
+ Coverage   70.99%   71.01%   +0.01%     
==========================================
  Files          85       85              
  Lines        6958     6959       +1     
==========================================
+ Hits         4940     4942       +2     
+ Misses       2018     2017       -1

Flag	Coverage Δ
multisite	`71.01% <100.00%> (+0.01%)`	⬆️
single	`40.93% <93.75%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…_src

github-actions · 2025-02-26T20:03:10Z

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @1ucay.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Unlinked contributors: 1ucay.

Co-authored-by: b1ink0 <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: westonruter <[email protected]>

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

felixarntz

@b1ink0 The production code changes look good to me, but it would be great if you could add a unit test to verify that it fixes the originally reported bug.

felixarntz · 2025-02-26T22:45:48Z

plugins/webp-uploads/tests/test-picture-element.php

+	/**
+	 * Test handling of case when wp_get_attachment_image_src returns false.
+	 */
+	public function test_wrap_image_in_picture_with_false_image_src(): void {


This test is good to have, but it's not actually testing the more important fix that this PR implements. It would be great if you could add a test that verifies that what is reported in #1882 gets fixed by this PR. You could probably use the code snippet you have in the PR description in that test itself.

A great way to do that is test-driven development:

Work on the test based on the codebase without this fix. (For example temporarily comment out or revert the changes in webp_uploads_wrap_image_in_picture() locally.)

Once you implemented the test, it should fail.

Re-enable the changes in webp_uploads_wrap_image_in_picture().

Now the same test should pass without any changes to it.

This way you verify the bug gets fixed by this PR.

Thanks for the detailed guidance! I will make sure to integrate the step provided for test-driven development into my workflow from now on.

…hes img tag srcset image sizes

felixarntz

@b1ink0 Thank you for the updates, the test looks great! Just a few minor points of feedback on following some best practices for PHPUnit tests in WordPress.

plugins/webp-uploads/tests/test-picture-element.php

felixarntz · 2025-03-04T19:55:35Z

plugins/webp-uploads/tests/test-picture-element.php

+		wp_delete_attachment( $attachment_id, true );
+
+		if ( $tear_down instanceof Closure ) {
+			$tear_down();
+		}


Having this cleanup logic called here is not reliable, since any assertion failing would result in this code not being reached. Can we instead incorporate this in an actual tear_down() method on the class that will always get invoked after each test?

You could store the attachment IDs generated in a class property, so that after each check any new IDs result in their attachment being deleted.

felixarntz · 2025-03-04T19:58:53Z

plugins/webp-uploads/tests/test-picture-element.php

+				'tear_down' => static function (): void {
+					remove_image_size( 'square' );
+					remove_all_filters( 'pre_option_medium_large_size_w' );
+					remove_all_filters( 'pre_option_medium_large_size_h' );
+				},


Since this is the only usage of the tear_down closure, we can probably work without them:

Filters added in a test are removed automatically after the test, so I don't think we need to clean them up here.

For the image size, maybe you can handle it in a tear_down() method on the class? Alternatively if it makes sense to keep it, we need to move when this tear_down closure is invoked in the test (see above comment).

…e management

felixarntz

@b1ink0 This looks great, thank you for the updates!

I have one more feedback, which would be great to address, but I'm already going to approve it as it overall is good.

plugins/webp-uploads/tests/test-picture-element.php

…y data provider

felixarntz · 2025-03-13T21:43:14Z

Thanks for the updates @b1ink0, looks good!

@westonruter Did you already create a branch for the next release set on Monday, or can this still be merged into trunk? This should be included in Monday's releases.

westonruter · 2025-03-14T01:34:04Z

@felixarntz No, I didn't create branch yet. There are still several PRs pending merge.

Fix incorrect image size selection in PICTURE element

6a5c0df

b1ink0 added the [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) label Feb 25, 2025

b1ink0 added this to the webp-uploads n.e.x.t milestone Feb 25, 2025

b1ink0 added the [Type] Bug An existing feature is broken label Feb 25, 2025

Add test for handling false return value from wp_get_attachment_image…

bfd7d2b

…_src

b1ink0 marked this pull request as ready for review February 26, 2025 20:02

b1ink0 requested review from adamsilverstein, felixarntz and westonruter as code owners February 26, 2025 20:02

felixarntz reviewed Feb 26, 2025

View reviewed changes

b1ink0 added 2 commits March 4, 2025 14:53

Add test to ensure picture element source tag srcset image sizes matc…

928a872

…hes img tag srcset image sizes

Merge branch 'trunk' into fix/wrong-image-size-picture-element

f645402

b1ink0 requested a review from felixarntz March 4, 2025 09:49

felixarntz reviewed Mar 4, 2025

View reviewed changes

b1ink0 and others added 2 commits March 6, 2025 17:57

Enhance test case by adding temporary attachment and custom image siz…

2118cf0

…e management

Merge branch 'trunk' into fix/wrong-image-size-picture-element

f544101

felixarntz approved these changes Mar 6, 2025

View reviewed changes

plugins/webp-uploads/tests/test-picture-element.php Outdated Show resolved Hide resolved

plugins/webp-uploads/tests/test-picture-element.php Outdated Show resolved Hide resolved

b1ink0 added 2 commits March 12, 2025 22:17

Refactor test case to remove custom image sizes parameter and simplif…

da82760

…y data provider

Merge branch 'trunk' into fix/wrong-image-size-picture-element

2c92462

b1ink0 requested a review from felixarntz March 12, 2025 16:59

westonruter merged commit 5600db9 into WordPress:trunk Mar 14, 2025
15 checks passed

Fix incorrect image size selection in PICTURE element #1885

Fix incorrect image size selection in PICTURE element #1885

Uh oh!

Conversation

b1ink0 commented Feb 25, 2025

Summary

Relevant technical choices

Uh oh!

codecov bot commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b1ink0 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixarntz Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

felixarntz Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixarntz commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Mar 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix incorrect image size selection in `PICTURE` element #1885

Fix incorrect image size selection in `PICTURE` element #1885

codecov bot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 26, 2025 •

edited

Loading

felixarntz Feb 26, 2025 •

edited

Loading

felixarntz commented Mar 13, 2025 •

edited

Loading