In #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy" we improved responsive image support and added width and height attributes to image to prevent layout shift for lazy loaded images.
In the same issue the code is added that calculates width and height dimensions. First we will get the width and height from the smallest responsive image style and we put width and height in $variables['attributes']
foreach ($variables['sources'][0] as $attribute => $value) {
if ($attribute != 'type') {
$variables['attributes'][$attribute] = $value;
}
}
and then in next code we are getting width and height from fallback image style which are put in $variables['img_element']
if (isset($variables['sources']) && count($variables['sources']) === 1 && !isset($variables['sources'][0]['media'])) {
...
// We don't set dimensions for fallback image if rendered in picture tag.
// In Firefox, it results in sizing the entire picture element to the size
// of the fallback image, instead of the size on the source element.
$dimensions = responsive_image_get_image_dimensions($responsive_image_style->getFallbackImageStyle(), [
'width' => $variables['width'],
'height' => $variables['height'],
],
$variables['uri']
);
$variables['img_element']['#width'] = $dimensions['width'];
$variables['img_element']['#height'] = $dimensions['height'];
}
The problem is on the end of this function where $variables['attributes'] and $variables['img_element'] values are merged:
if (isset($variables['attributes'])) {
...
$variables['img_element']['#attributes'] = $variables['attributes'];
}
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. This is not ideal and we can not fix this with CSS. We need more accurate width and height values set here that are coming from fallback image style.
Make sure that width and height from fallback image style is used and not the width and height from smallest image style.
None.
None.
None.
None.
TODO.
| Comment | File | Size | Author |
|---|---|---|---|
| #39 | 3377420-responsive-image-dimension-fallback-style-10.3.x-39.patch | 3.66 KB | szato |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !7248
Comments
Comment #2
pivica commentedComment #3
pivica commentedHere is a patch.
Comment #5
pivica commentedTest fix.
Comment #7
pivica commentedMore test fixing.
Comment #9
pivica commentedOne more try for test fix.
Comment #10
pivica commentedTested 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?
Comment #11
sergey-serovGreetings!
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.
Comment #12
sergey-serovComment #13
pivica commented@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.
Comment #14
sergey-serov@pivica Thank You, for explanation!
I spent several hours at last weekend with this thing :))
Comment #15
martijn de witComment #16
pivica commentedComment #17
smustgrave commentedUpdating 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.
Comment #18
pivica commentedUpdate from @heddn from https://www.drupal.org/project/drupal/issues/3359421#comment-15186181 - we probably got this problem from that issue.
Comment #19
bkosborneSeems like the issue summary may need an update since #3359421: (Re-)Add width / height also on fallback image 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.
Comment #20
pivica commented> 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.
Comment #21
bkosborneUltimately 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:
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:
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.
Comment #22
bspeare commentedThank 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.
Comment #23
thejimbirch commentedHiding images in the files to make the patch easier to find.
Comment #27
berdirRerolled and created a merge request.
Comment #28
smustgrave commentedGoing to elevate to framework manager.
Comment #29
larowlanWondering what the contention is here? The MR looks straight forward - is the Needs FM review tag just because there's no Subsystem maintainer?
Comment #30
smustgrave commentedGot sign off in slack https://drupal.slack.com/archives/C04CHUX484T/p1717515262152809
Am doing a rebase as the MR is 500+ commits behind HEAD.
Comment #31
smustgrave commentedRan test-only feature after rebase https://git.drupalcode.org/issue/drupal-3377420/-/jobs/1778003 and shows failure with test coverage
Tests all green!
Comment #36
nod_Committed 460a2dd and pushed to 11.x. Thanks!
Comment #38
jannakha commentedJust a note - fallback image style should be the largest image style in the set.
Wide responsive image has fallback image style "max 325x325" which sets
<img ... width="325px">, even if the large image is loaded.Setting an image `width="325px"` makes it render at this width, although it needs to be rendered at whatever the max width is required.
Comment #39
szato commented@nod_, if I'm correct, it wasn't committed to 10.3.x.
Patch attached.
Comment #40
mariohernandez commentedComing a little late to this discussion because I recently started experiencing issues when I use the img tag with srcset and sizes attributes. By default, the width and height attributes of the img tag is being updated to use the fallback image dimensions, which means my responsive images configuration is being totally ignored.
I am having the same issue as @sergey-serov (#11).
I typically use a medium or small image as my fallback image so if everything fails at least I get a decent image to render. However, this is breaking all my existing responsive images configuration because the width and height of the img tag is now the fallback image.
Everything seems to work if I use
<picture>, but this is not always the recommended approach for responsive images. I use resolution switching as my default method.Anyone else having this issue?
Comment #41
bkosborneThis is the problem I described in #19. I "fixed" this by using CSS to overwrite the size of the images.
Comment #42
fallenturtle commentedI'm having the same issue as https://www.drupal.org/project/drupal/issues/3377420#comment-15293179 using Drupal 10.5.3. Patch didn't work, so I guess I'll need to roll my own?
Edit: Patch didn't work because the changes in the patch are already made to 10.5.3.... so I have no idea what the problem is.