Add UI for 'loading' html attribute to images

Below is a simple mockup of a potential solution, which I proposed over in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core:

Proposed Image field formatter configuration UI

The options would map to the supported values: auto, lazy, eager, lazy being the default. Rendered markup would respond accordingly, based on this setting. I could also see this additional display field/UI being gated behind a higher level setting if its inclusion by default is considered too heavy handed.

My thinking centers around the fact that there are certain instances where setting the loading attribute to something other than lazy is desirable. For example, if a page has an image which makes up the Largest Contentful Paint, its loadingattribute should be set to loading="eager" to tell the browser to load that resource right away. Doing so should also help with First Contentful Paint, another important performance metric.

There might also be sites that handle this on their own and wish to disable Drupal core's defaults.

I am struggling to think of a better place to contain this functionality. I don't believe it belongs on the image style. Doing so could be far too broad an assumption. Imagine a hero_image_xl image style. It might be used across a site and across content types, in varying layout positions. Customizing its loading attribute to eager would thus have adverse effects. To me, this attribute it purely about display, and thus should be configurable on the image field's display formatter. In this way it is decoupled from the image style and configurable based on how/where the content is presented.

This approach makes sense to me and seems like the right place to address the problem.

I would be interested in helping create a patch if others are in support of the approach.

Here is a quick patch that add the UI for the Image field formatter. It's functional. Please test it.

We also need to see how to integrate this with CKeditor and put one eye on this issue: https://www.drupal.org/project/drupal/issues/2061377

The UI also should be implemented for the Responsive image module e.g adding the UI into its field formatter.

1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -128,6 +129,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +      'auto' => t('Auto'),
   +      'lazy' => t('Lazy'),
   +      'eager' => t('Eager'),
   ...
   +      '#title' => t('Lazy loading priority'),
   
   @@ -161,6 +173,9 @@ public function settingsSummary() {
   +    $summary[] = t('Lazy loading priority: @priority', ['@priority' => $lazy_loading_priority_setting]);
   

   it should use $this->t() as inside of class

2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -128,6 +129,17 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +      '#options' => $lazy_load_options,
   

   Would be great to add description with a link to docs about what each option means

Addressing #5 ;)

Added new setting schema.

I tested 3173180-8.patch and it works as advertised. I was able to successfully create a new display mode called "Hero" for an existing Media entity type (Image) and from within said display mode, configure it's default Image field to have a lazy loading value of eager, auto, and lazy:

I was then able to verify that when rendering this "Hero" display mode, the chosen value is rendered as expected.

That said, I think it would be following existing standards to link to Mozilla's documentation instead of Google's. I see that Drupal core already does so 18 times. I see no references to the web.dev website.

As such attached is a patch (and interdiff) that changes the referenced documentation to that of Mozilla.

Thanks for testing!
Does the new link list the loading options?
IMHO in that case we could use https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loadi... instead. We already have used this website in core too.

Oh good point. The Mozilla page isn’t nearly as clear on that point. I’ll do some more research and get another patch going if need be.

Updated link as suggested in #12

The whatwg.org/ spec is probably the right/better documentation page to link to. That said, it does not list auto as one of the supported attributes. Since auto in Chrome is equivalent to the loading attribute not existing, and since the spec defines eager as the both the missing value and invalid value default, it would probably make sense to remove auto as an option. Thoughts?

Additionally, I still wonder if exposing the "Lazy loading priority" field on the display formatter should be gated by some higher level configuration. I think the best place for this would be under "Media settings". This could also be a great place to better describe Drupal's default's for native lazy loading.

I'm going to work on a patch for this right now.

Here is draft of a patch that adds a configuration option within the Media module for globally enabling/disabling per image formatter lazy loading settings.

The Image formatter now checks to see if customization is enabled before displaying lazy loading configuration options.

After getting this all working, I realized that the Media module might not be the best place for this configuration because it cannot be assumed that the Media module will be enabled on all sites. Or in other words, the Image module might be in use, but the Media module might not. This draft assumes Media module as a dependency.

Since the main lazy loading bits are all contained in the Image module, I suppose that might be a better place for this configuration UI, but I wonder then about how the responsive_image module will play into this (See #3173231: Enable lazy-load by default for responsive images). In that case, maybe it's ok to allow this level of customization to live on in a separate place (such as the Media module)?

In any case, I took a pass at what the UI might look like, including help text to describe native lazy loading, overriding.

This will also need tests, but I am not versed in creating such things.

1. +++ b/core/modules/image/config/schema/image.schema.yml
   @@ -142,6 +142,9 @@ field.formatter.settings.image:
   +    lazy_loading_priority:
   
   +++ b/core/modules/media/config/install/media.settings.yml
   @@ -2,3 +2,4 @@ icon_base_uri: 'public://media-icons/generic'
   +lazy_loading_customization: false
   

   new config needs upgrade path and upgrade tests to be ready for existing sites

2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -129,6 +142,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +      $description_link = $this->t('Select the lazy-load priority for load images. <a href="@link">Learn more.</a', ['@link' => 'https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes']);
   

   Links must use ":" for URL - <a href=":link">

Here's an updated patch and interdiff, fixing the link in the help text.

Whoops, I didn't use the proper syntax for URLs. This patch fixes that.

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -43,6 +44,13 @@ class ImageFormatter extends ImageFormatterBase {
+   * @var \Drupal\Core\Config\ImmutableConfig
...
+  protected $config;

@@ -64,11 +72,14 @@ class ImageFormatter extends ImageFormatterBase {
+    $this->config = $config_factory->get('media.settings');

Please don't store immutable config in protected property, no reason to read config at plugin creation time and it makes serialization weird

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -129,6 +142,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+      $description_link = $this->t('Select the lazy-load priority for load images. <a href="@link">Learn more.</a', ['@link' => 'https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes']);

Link still rendered with wrong selector, see #18

Fix #22 but leaving NW for #18.1

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -129,6 +146,26 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    $custom_loading_enabled = $this->configFactory->get('media.settings')
+      ->get('lazy_loading_customization');
+    if ($custom_loading_enabled) {
...
+      $lazy_load_options = [
+        'auto' => $this->t('Auto'),
+        'lazy' => $this->t('Lazy'),
+        'eager' => $this->t('Eager'),
+      ];

btw instead of global setting it could use "empty/none" option as default for formatter

Thanks @andypost. I wasn't fully sure how to handle all of the config bits. I was following the lead of the oembed field formatter. A bit of which was over my head.

I'll recruit some help on my end to work on the upgrade path stuff.

#16 and #24.
I also think auto doesn't make too sense in the UI, maybe just keeping lazy and eager and set the fallback option to auto.
e.g:

    $lazy_load_options = [
      'lazy' => $this->t('Lazy'),
      'eager' => $this->t('Eager'),
    ];
    $element['lazy_loading_priority'] = [
      '#title' => $this->t('Lazy loading priority'),
      '#type' => 'select',
      '#default_value' => $this->getSetting('lazy_loading_priority'),
      '#options' => $lazy_load_options,
      '#description' => $description_link,
      '#empty_option' => '- None -', // by default is - None - . So we won't need it, at least we want another word.
      '#empty_value' => 'auto',
    ];

I think @heddn is right. It would significantly reduce the complexity here in this issue, but also seems like a better user experience for site owners. The '#type' => 'details' idea is a ++ from me.

Moved changes into the details element. Also applied #26.

Fixing Image.Image tests.

Change Image to lowercase.
We have used html in the translatable markup in many other places.

Fixed some tests and added new ones to address #34.2.

Changing to Needs review for now to see how test are working.

Fixing tests.

Adding screenshots.

Upgrade path added.

+++ b/core/modules/image/image.install
@@ -78,3 +78,24 @@ function image_requirements($phase) {
+function image_update_9001(&$sandbox) {

The correct schema version should be 9201 :)

The #44 works fine ,
After patch

RTBC.

Wouldn't it make sense to move the new setting to the ImageFormatterBase class? Doing that https://www.drupal.org/project/drupal/issues/3173231 could also use it and we don't have code duplication.
Currently ImageFormatterBase is exteded only by ImageFormatter and ResponsiveImageFormatter classes, so both will share the same setting.

I am also thinking on mixing both issues into this one, because they are very related.

Thoughts?

+1 to combining these closely related issues.

Working on a patch.

Added patch implementing #48. Moving settings to base class and mixing with 3173231 issue.

https://www.drupal.org/project/drupal/issues/3173231#comment-13876634 was addressed here.

Patch added.

Working on #54

Resolved #54 1, 4, 5, and 6 points plus some responsive images test fixes.

Still needs work for 2 and 3.

Cleaning displayed files.

Let tests to run.

The patch from #51 is no longer applicable on Core 9.1.10

The patch from #51 is no longer applying for me either on Core 9.1.10

@yivanov + @darren.fisher - that's because development on this issue moved to gitlab. See https://git.drupalcode.org/project/drupal/-/merge_requests/43 for the latest code. You can no longer simply apply this fix as a patch on your site.

You can get an auto-generated patch for the latest changes here:

https://git.drupalcode.org/project/drupal/-/merge_requests/43.diff

However, it's a really bad idea to add that to composer for your site, since any random person can push more changes to that merge request and the next time you run composer install, the 43.diff file will be different and contain the new code. So you'll end up always running whatever code has been pushed to this issue fork, which is definitely unwise. There's no way to get a link to a gitlab patch locked to a specific commit hash you want to install. Welcome to the "new improved" contribution workflow using Gitlab! ;)

If you want to safely deal with this, you'll want to download that "43.diff" file, rename it to something unique like $issue_num.$hash.patch (e.g. '3173180.cd2041ff2a7c7be97059a4d77afa71eddd9f1e9f.patch'). Commit *that* to your site's source code somewhere "safe", and tell composer to install the patch from this local file, not from a link.

Good luck,
-Derek

Here's a good read on why this is important:

The performance effects of too much lazy-loading by @rick_viscomi and @felixarntz https://web.dev/lcp-lazy-loading/

The article focuses on WordPress but the same would apply to Drupal. Lazyloading images that appear in the viewport saves on bytes but negatively affects the page's LCP score when things like hero images are lazyloaded instead of eagerly loaded.

Patch in #51 and version in merge request was not applying to 9.2.4.
Re-rolled to apply.

Is the merge request's outstanding discussions the last bits to get this across the finish line? My team might have some time available to finish this up if so.

As I'm currently working on a high-performance Drupal site, I would like to add my 2 cents here:

- all images need dimensions to avoid CLS
- most images should use the "responsive image" formatter
- a responsive image needs dimensions to avoid CLS
- the module https://www.drupal.org/project/css_aspect_ratio provides dimensions for responsive images. Using this module I achieved a score of 0 CLS, that's why I think this should be integrated into core (this aspect_ratio option is missing from the responsive image formatter in views when rendering fields)
- all images need the lazy attribute as an option, obviously, there are some images for which lazy-loading is not adequate. I think this should be integrated into the image formatters, normal image formatters and responsive image formatters

- an additional need is to have lazy-loading responsive images as backgrounds.

Url to caniuse seems brittle to add in a help test, and i'm not convinced at the default being lazy.

the issue of image dimensions for responsive images.
see https://www.drupal.org/project/drupal/issues/3173180#comment-14204256
we could use aspect ratio.

MR isn't mergeable:

Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally.

So someone needs to rebase before this is RTBC. Tagging for "Needs reroll" although maybe we should introduce a "Needs rebase" instead. 🤓😉

It's a bit disappointing that this MR won't support Responsive Images at all, but I understand that it's easier to get smaller commits merged on a fast moving project.

Hoping this one get's merged soon so we can take a look at Responsive Images support too!

I wasn't able to apply the MR, tagging for reroll

RTBC - This patch looks great to me now.

Great work! Thank you all! Will be nice to have this in 9.4.x! :)

For those that want to use this patch for Drupal 9.2 here is a backported one

Small update

Having tried to pull this apart for 9.3.x, I believe that the attached patch should apply.

Back to RTBC - interdiff looks good to me

Thanks everyone for adding this feature!

Doing a review.

I am not familiar with the display for managing images and it is not clear if the images in the Issue Summary are before or after screenshots. I checked on a local test site and now know they are after screenshots. I applied the patch and what I see on my test site does not match the screenshots in the Issue Summary. This needs up-to-date before and after screenshots in the Issue Summary. Adding tag

This is adding help text for the user and I do not see a review of that text from the usability team, adding tag.

Since this is changing the UI, adding a new feature, let's have a change record.

I took a very brief look at the MR and asked a question.

Amazing addition, thank you for working on that! I've applied the patch and also taken a look. Two quick observations:

1. If you are inside the format settings and you expand the Image loading settings then they are not only expanded vertically but also horizontally to an unexpected degree. Since it happened already on a smaller viewport I've expanded the browser window to a unusual degree to test if it is happening there as well and also used that for the screenshot - I would have expected only a vertical expansion. (Tested with Safari 13.1.2 and 98.0.2 with identical results)

2. The settings summary for the image field formatter in an article content type in a standard Drupal 9.4.x-dev installation states:

Image style: Wide (1090)
Loading attribute: lazy

But the titles inside the field formatter are: Image Style, Link image to and Image loading. Would it make sense to make the titles consistent and use Image loading instead of loading attribute in the summary or the other way around?

p.s. should the issue be added to next weeks ux meeting issue as @quietone asked for a review by the usability team?

Wording "image handling" needs discussion as contrib can extend it - for example module focal-point

We'll be able to expand that to manage the decoding attribute too.

not comfortable RTBC but it's looking good to me.

Got one question, were there talk about not adding the attribute at all in case it's configured as eager? it seems like an unnecessary output change since it doesn't do anything different.

eager does not mean the same thing as not having the attribute.
When the attribute is not there, the browser can use heuristics to determine if it should use lazy loading or not, whereas the attribute forces a specific mode.

right, so we're missing an option of not defining the attribute then?

specs says default = eager though The attribute's missing value default and invalid value default are both the Eager state. so it's not supposed to be up to the browser.

My bad, what I said applies to the decoding attribute (decoding="sync" is not the same as not having the attribute).
But for the loading attribute, the spec says:

The attribute's missing value default and invalid value default are both the Eager state.

Thanks again for all the work on this nice feature!

It also wouldn't matter much to me whether we set loading="eager" or omit the loading attribute when 'eager' is selected. If we choose the latter tho, I'd recommend expanding the description of the 'eager' option by saying (in as few words as possible) that it is the default browser behavior. I only mention this because unexperienced users might be surprised that no loading attribute is set and perceive this as a bug, when in fact it would be the desired behavior.

Agreed with #108, I almost wonder if we should say 'Default' instead of 'eager' in the UI, so it's just 'default' vs. 'lazy' (but keep it eager in config just in case a third one shows up one day).

The MR needs updating for the new database dumps in 10.x/9.4.x (and removal of the old ones in 10.x). Couple of other minor questions, but in general this looks good to me. We originally thought we didn't need a UI for this, but google pagespeed disagrees.

I am not sure if changing the wording from eager to default wouldnt be too confusing for the user of any experience level. what eager and lazy means is defined in the description for each radio button for less experienced users, in contrast for more experienced users eager and lazy are prevalent terms.
by changing from eager to default experience users would have to think what is meant by default? and there is another problem in the introductory line it is stated image assets are rendered with native browser loading attribute of (loading="lazy") by default in the learn more-link in contrast within the spec eager is defined as the default state. so leaving the terms eager and lazy as is would be the safer and more clear in combination with the description text of the radio buttons we currently have.
and i agree with @antoniya in #108 as well. in case for eager there isn't a need to set an attribute then that should be noted in the description for the radio button.

I mean we can also put a checkbox that says "lazy" with the description of what it does and just not mention the default/eager thing. In that case it would make sense to not add the attribute if the checkbox is not checked and that reduce the amount of text to read to understand what it means.

Back to RTBC - that UI looks super clean!

Changes look great, I think having the radios to keep things explicit is fine.

Went to commit this, but it's failing phpstan:

Running PHPStan on changed files.
 ------ --------------------------------------------------------------------- 
  Line   modules/views/views.post_update.php                                  
 ------ --------------------------------------------------------------------- 
  51     Class ViewsConfigUpdater not found.                                  
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  52     Class ConfigEntityUpdater not found.                                 
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  52     Parameter $view of anonymous function has invalid type               
         ViewEntityInterface.                                                 
 ------ --------------------------------------------------------------------- 

Pretty sure this is because views.post_update.php has had some use statements removed since updates were removed from Drupal 10.

We also need a 10.x test run here - will probably need a separate MR/patch with the use statement changes.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Added a release notes snippet.

This broke testing on PHP 7.3, so I've rolled it back from 9.4.x (only).

Without looking into it too closely, best guess is there is a typehint conflict on PHP 7.3 somewhere in here:

function image_post_update_image_loading_attribute(array &$sandbox = NULL): void {
  $image_config_updater = \Drupal::classResolver(ImageConfigUpdater::class);
  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'entity_view_display', function (EntityViewDisplayInterface $view_display) use ($image_config_updater): bool {
    return $image_config_updater->processImageLazyLoad($view_display);
  });
}

Easiest thing to do would be to either manually test the upgrade path on 7.3, or run the failing upgrade path test on 7.3 and look at the HTML results.

Yeah void is 7.4 + I think

I've opened #3275093: Drupal 9.3.0 dumps were created with PHP 8, needs to be PHP 7.3 to sort the dumps out.

Since this ended up being just switching to the 8.8 fixture (which doesn't hurt anyway to update more things at once as part of the test), I've gone ahead and re-committed the new patch to 9.4.

Is there a disruption from this issue such that it needs to be in the release notes, or is it meant to be a release highlight as a new feature?

Agreed, more of a highlight.

Updating tag

This change influences the formatter "URL to image", and causes PHP warning.
Please see #3291622: Formatter 'URL to image' from ImageUrlFormatter shows PHP warning due to the newly introduced loading attribute on image field, thank you.

Getting content.thumbnail.settings.image_loading missing schema, in detail:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.media.document.media_library with the following errors: core.entity_view_display.media.document.media_library:content.thumbnail.settings.image_loading missing schema

in https://www.drupal.org/pift-ci-job/2476536

for tests using the "standard" installation profile (to install core default media entity types).

The content section of
core/profiles/standard/config/optional/core.entity_view_display.media.document.media_library.yml looks like this and should match the schema:

content:
  thumbnail:
    type: image
    label: hidden
    settings:
      image_style: thumbnail
      image_link: ''
      image_loading:
        attribute: lazy
    third_party_settings: {  }
    weight: 0
    region: content

So I'm unsure if this is a follow-up issue with the "standard" installation profile after this change. If anyone has the same issue or can see what's causing this, we should create a separate issue for that. Not yet 100% sure the isn't on our side.

Thought I'd better leave a note here, for anyone experiencing the same.

Patch update for D9.3 compatibility

Update patch 9.3.22

Sorry, some fixes. Update patch 9.3.22