(Re-)Add width / height also on fallback image

In #228 📌 Apply width and height attributes to allow responsive image tag use loading="lazy" Fixed 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'];
+  }

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.

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.

Here's the diff as static patch.

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

Thank you for the patch!
I can confirm the missing values in Drupal 10.0.9. With this (and other patches from this issue 📌 Apply width and height attributes to allow responsive image tag use loading="lazy" Fixed ) 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"
    },

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

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

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

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

Here's a screenshot of your codepen test:

with Firefox 113.0.1 (64-Bit) on Windows 11

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

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.

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

1 Tests still fails, the others seem unrelated.

@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!

Nice, tests pass now!

@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.

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

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

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

RTBC'ing this.

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

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

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

Thanks @catch!

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

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

Great, thx @catch!

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

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

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

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

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