Responsive image width/height values are not used from fallback image style

Here is a patch.

The last submitted patch, 3: 3377420-responsive-image-dimension-fallback-style-3.patch, failed testing. View results →

Test fix.

The last submitted patch, 5: 3377420-responsive-image-dimension-fallback-style-5.patch, failed testing. View results →

More test fixing.

The last submitted patch, 7: 3377420-responsive-image-dimension-fallback-style-7.patch, failed testing. View results →

One more try for test fix.

Tested this quite a bit and all reported problems in issue description are fixed.

If you upload small image and image styles are configured in that way that bigger styles can not produce big images then width and height will be calculated only against valid styles. This works as expected.

However, I did found one edge case when new behavior is not correct. The setup is like this:
1. We have 2 image styles 'Medium (220x220)' and 'Large (480x480)'
2. We have responsive image style with next setup
- 1x Mobile []: Use single image style 'Medium'
- 1.5x Mobile []: Use single image style 'Large'
- 2x Mobile []: Use single image style 'Large'
- Fallback image style is set to 'Large'

On the page we upload two images, one with the size of 480px width, and second with dimensions of 900px width.

When testing for a device with 250px width and DPR: 1 the first smaller image will be a bit bigger because width of image is calculated to 480px (from fallback 'Large' style) but browser will use 'Medium' style. This looks like this:

As you can see image is slightly scaled (250px) instead of original 220px.

Ideally for this case, we should use width of 220 and show image like this:

This is not that big of a deal, I guess for some special cases image could be stretched more, so you could start to see visual degradation because of scaling.

Not sure how to proceed because of this. IMHO I think that a new behavior (using correct values from fallback image) is better than the current one, but there are still some problems.

I am wondering should we remove width and height attribs and instead of them use inline aspect-ratio. The original reason we added width and height were to help browser to correctly calculate image dimensions for lazy loaded images, so we can avoid layout shifting when images are loaded. Feels a bit awkward to use inline style to solve this problem, but does it make actual sense?

Greetings!
Is this correct standard behavior? (look at screenshot, please).
"Width" and "height" attributes freeze image at size of the "fallback image".
No any "css" modification here.
When width of the screen becomes bigger - in browsers web-developer tool is visible that now is used larger image with 570px. But on the page image is still small 190px.

@sergey-serov yes, you exactly described the same problem we have and patch in this issue is fixing this. However there is one edge case explained in comment #10 which is a new problem when this patch is applied - it is smaller problem then current core behavior but still a problem and not fully correct behavior.

@pivica Thank You, for explanation!
I spent several hours at last weekend with this thing :))

Updating for responsive images. Also tagging for the submaintainer thoughts.

But based on the fact this causes a regression not sure it go in until that gets resolved. With Drupal's BC policy that regression seems like it could negatively impact existing sites.

Update from @heddn from https://www.drupal.org/project/drupal/issues/3359421#comment-15186181 🐛 (Re-)Add width / height also on fallback image Fixed - we probably got this problem from that issue.

Seems like the issue summary may need an update since 🐛 (Re-)Add width / height also on fallback image Fixed was committed?

Beyond that, I'm confused why you'd want the width and height of an img src set to be the fallback image style? I have a responsive image style configured as such:

1x viewport sizing
type: select multiple image styles and use the sizes attribute
sizes: (min-width: 1440px) 1440px, 100vw
image styles: 8x3_2880w_1080h, 8x3_1440w_540h, 8x3_750w_281h
fallback image style: 8x3_1440w_540h

I don't have my fallback image style set to be the smallest image style. I chose the middle size as a compromise. If there's a browser that didn't support them, I don't want it to have a super grainy image, or assume that a super high res version is the best, so I chose the middle one.

> I don't have my fallback image style set to be the smallest image style. I chose the middle size as a compromise.

Maybe you use picture tag and it works fine for you? But in our case we use img tag with srcset and in this case it does not matter which fallback style you select, smallest or the biggest, currently you will always get width and height from the smallest style, which is not something you want as you said also.

Ultimately it's not a huge deal for us, as will use CSS to force the correct width of the image as needed.

But there's in incorrect statement in the issue summary:

This will overwrite fallback image style width and height with width and height which are calculated from smallest image style. Because of this images will be rendered with the smallest image dimension in browser.

It's not selecting the smallest. It's selecting the dimensions from the last image style in the set, which is not necessarily the smallest width. The list is sorted alphanumerically by machine name of the image style and processed in that order. You can observe this with the standard install profile in Drupal 10.1:

1. Install 10.1 fresh using standard profile and enable Responsive Image
2. Configure entity view display for Article and set the image field to use responsive image set to "Wide"
3. Edit the "Wide" responsive image and observe it's set up with 1x viewport sizing, "Select multiple image styles and use the sizes attribute.", sizes set to "(min-width: 1290px) 1290px, 100vw" and Image styles selected Max 1300x1300, Max 2600x2600, Max 325x325, Max 650x650. Fallback set to Max 325x325. Don't change anything.
4. Create an article and upload a very wide image (at least 2600 px wide)
5. View the article and observe that the image is output as 650px width (the last image style listed).

Here's a screenshot to further demonstrate the issue:

This patch overrides that to have ensure the fallback style is chosen as used for the dimensions instead.

I suppose that's fine, but I think this will need a change record, and the responsive image style form for the "Fallback Image Style" should probably be updated to indicate that whatever style is chosen as the fallback will be used for the image dimensions.

Thank you for putting this together @pivica! After testing, this does seem like the correct fix for the expected functionality of the fallback image. As it is mentioned on https://web.dev/patterns/web-vitals-patterns/images/responsive-images, it is recommended to "set the width and height attributes to match the dimensions of the fallback image" and this patch fixes that problem.

Hiding images in the files to make the patch easier to find.

Rerolled and created a merge request.

Going to elevate to framework manager.

Wondering what the contention is here? The MR looks straight forward - is the Needs FM review tag just because there's no Subsystem maintainer?

Got sign off in slack https://drupal.slack.com/archives/C04CHUX484T/p1717515262152809

Am doing a rebase as the MR is 500+ commits behind HEAD.

Ran test-only feature after rebase https://git.drupalcode.org/issue/drupal-3377420/-/jobs/1778003 and shows failure with test coverage

Tests all green!

nod_ → closed merge request !7248

Committed 460a2dd and pushed to 11.x. Thanks!