Follow-up issue on #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy" from comment #296:

Problem/Motivation

Google Pagespeed is complaning about missing width and height attributes on the fallback image now.

The commit contained these lines:

+    // 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'];

In the issue summary and comment #15 I found

Firefox has open issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1694741 (linked from https://github.com/mozilla/standards-positions/issues/485)

when looking for "Firefox".
Both are closed. Might have been fixed between this issue and commen was created in 2021 and now.

Pagespeed says:

Image elements do not have explicit width and height
Set an explicit width and height on image elements to reduce layout shifts and improve CLS

for some <img loading="lazy" ... />'s within picture tags. This makes sense because if the image is lazy, it should also have width/height. And it should be the browsers job to handle the values correctly.

So should we add a follow-up to investigate on removing this exclusion?

Steps to reproduce

Proposed resolution

Remove the exclusion for fallback images as this seems to be now fixed in FF and PageSpeed complains occasionally.

Remaining tasks

  1. Check if this is really fixed in FF
  2. Remove the special handling

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#15 codepen-width-height-firefox-113.0.1 (64-Bit).png38.39 KBanybody
#13 3359421-13.patch2 KBgrevil
#8 3359421-8.patch2 KBanybody

Issue fork drupal-3359421

Command icon Show commands

Start 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:

About issue forks

Comments

Anybody created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented

In #228 you can see the code before the FF exclusion was added:

+  // Get width and height from fallback responsive image style and transfer them
+  // to img tag so browser can do aspect ratio calculation and prevent
+  // recalculation of layout on image load.
+  if (isset($variables['width'], $variables['height'])) {
+    $dimensions = responsive_image_get_image_dimensions($responsive_image_style->getFallbackImageStyle(), ['width' => $variables['width'], 'height' => $variables['height']], $variables['uri']);
+    // Unset current image attributes, so the new ones that are set will work.
+    unset($variables['img_element']['#attributes']['width']);
+    unset($variables['img_element']['#attributes']['height']);
+    $variables['img_element']['#width'] = $dimensions['width'];
+    $variables['img_element']['#height'] = $dimensions['height'];
+  }

Anybody opened merge request !3974

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Status: Active » Needs review

Ok, let's see if I got it right. First had to understand, that the block just has to be moved below the if-clauses, but not totally sure yet it's correct.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Parent issue: » #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy"
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Title: Add width / height also on fallback image » (Re-)Add width / height also on fallback image
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Issue tags: +Needs manual testing

Just tried this in dev in Firefox on some of our projects, where it would be helpful and it works as expected so far!
Without the patch, width / height are gone. With this patch, they're there and output if still fine in FF.

But definitely needs manual testing on FF and confirmation that this is 100% working as expected. Not just in my cases.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
StatusFileSize
new 3359421-8.patch2 KB

Here's the diff as static patch.

Status: Needs review » Needs work

The last submitted patch, 8: 3359421-8.patch, failed testing. View results

tfranz’s picture

tfranz CreditAttribution: tfranz as a volunteer commented

Thank you for the patch!
I can confirm the missing values in Drupal 10.0.9. With this (and other patches from this issue) it works: Width and Height are back again.

My patches:

  "patches": {
    "drupal/core": {
      "Order image mappings by breakpoint ID and numeric multiplier": "https://www.drupal.org/files/issues/2022-05-19/3267870-9.5.x-47.patch",
      "3192234: Apply width and height attributes to responsive image tag": "https://www.drupal.org/files/issues/2023-04-07/3192234-293-10.0.x.patch",
      "3359421: (Re-)Add width / height also on fallback image": "https://www.drupal.org/files/issues/2023-05-10/3359421-8.patch"
    },
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Issue tags: -Needs manual testing

Manual testing works perfectly! Now we should finish the automated tests and try to get this in before 10.1!

Grevil made their first commit to this issue’s fork.

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented
StatusFileSize
new 3359421-13.patch2 KB

Rebased, as the MR did not apply on 10.1.x anymore. Attached the changes as a temporary patch.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Category: Task » Bug report

Re-categorizing as a bug report. Was the manual testing from #3192234-246: Apply width and height attributes to allow responsive image tag use loading="lazy" re-performed against FF?

See https://codepen.io/heddn/pen/LYmKway

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
StatusFileSize
new codepen-width-height-firefox-113.0.1 (64-Bit).png38.39 KB

Sorry @heddn I tested the patch, but didn't explicitly retest the Firefox behavior.

Here's a screenshot of your codepen test:
Firefox
with Firefox 113.0.1 (64-Bit) on Windows 11 and browser width of 1700px the image width / height rendered is: 800x300

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

Looks like it is no longer an issue. Yeah! Let's get the tests fixed to back this up.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Version: 11.x-dev » 10.1.x-dev

Switching back to 10.1.x as this is a bugfix.
The test-fixes I tried didn't seem to be correct, taking a new approach with @Grevil today to push this forward.

klemendev’s picture

klemendev CreditAttribution: klemendev as a volunteer and at Pylo commented

This indeed should target 10.1.x, preferably fist release or the change to this will break CLS scoring for responsive images

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented
Status: Needs work » Needs review
grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented
Status: Needs review » Needs work

1 Tests still fails, the others seem unrelated.

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented
Status: Needs work » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented

@Grevil: Yeah we have to be careful, as there are failing tests in core 10.1.x-dev currently. So yes, surely unrelated if not from this module!

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented

Nice, tests pass now! (The 2 remaining failing tests are unrelated)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented

@heddn would you be so kind to review this perhaps? Hopefully the 10.1.x failing tests will be solved upstream soon, so we can trigger a retest with true results and mark this RTBC.

Status: Needs review » Needs work

The last submitted patch, 13: 3359421-13.patch, failed testing. View results

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Status: Needs work » Needs review

Tests are green now :) (dev core has been fixed)

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented

I agree with @Anybody, I don't think we need to remove the alt and loading attribute.

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented
Status: Needs review » Reviewed & tested by the community

RTBC'ing this.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

  • catch committed 1b99cf3d on 10.1.x 
    Issue #3359421 by Anybody, Grevil: (Re-)Add width / height also on...

  • catch committed 15755733 on 11.x 
    Issue #3359421 by Anybody, Grevil: (Re-)Add width / height also on...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Linked this issue from the existing CR at https://www.drupal.org/node/3279032

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
CreditAttribution: anybody at DROWL.de commented
Status: Reviewed & tested by the community » Fixed

Funny, the issue status shows as RTBC, while the comments say "Fixed". What's that? :D
Trying to re-set this fixed.

Thanks @catch!

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

So glad to see this fixed before 10.1.0. Thanks everyone.

klemendev’s picture

klemendev CreditAttribution: klemendev as a volunteer and at Pylo commented

Indeed really nice to see this in 10.1.0. This release will be awesome, thanks to all involved!

grevil’s picture

grevil CreditAttribution: grevil as a volunteer and at DROWL.de commented

Great, thx @catch!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented

I believe this commit has resulted in undefined array key errors for responsive_image themeing that don't contain a width or a height. At the very top we do this:

  if (isset($variables['width']) && empty($variables['width'])) {
    unset($variables['width']);
    unset($variables['height']);
  }
  elseif (isset($variables['height']) && empty($variables['height'])) {
    unset($variables['width']);
    unset($variables['height']);
  }

And now we always do

  $dimensions = responsive_image_get_image_dimensions($responsive_image_style->getFallbackImageStyle(), [
    'width' => $variables['width'],
    'height' => $variables['height'],
  ],
    $variables['uri']
  );

Whereas previously we only did that if there was a single source which was probably almost never.

Upgrading to 10.1 has started throwing 

Undefined array key "width"

/data/app/core/modules/responsive_image/responsive_image.module:209
acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented

Looks like reverting this commit locally produces the same error so the error was introduced in #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy" rolling back to 10.0 doesn't have this issue.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented

Opened #3375721: Undefined array key errors for responsive_image themeing without width or height also found #3375638: Undefined array key "height" and "width" when using "Responsive Background Image" formatter so other people are experiencing it.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

The changes here seem to have introduced #3377420: Responsive image width/height values are not used from fallback image style. Testing over there is welcome.