Add image preload option to help boost actual and perceived performance

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rerolling the patch respective to the 10.1.x along with the reroll.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Since this is changing the configuration it will need an upgrade path for existing sites.

Also will need a change record.

Hi @smustgrave
Yes, the configuration has been updated, but the new option is FALSE by default, so it won't have any impact on existing sites. Therefore, we don't need to make any additional changes such as hook_update or hook_deploy

@HitchShock thanks for all the work on this!

Even though the default is FALSE if I go into the field formatter, make no change and click save. If I do config export and it changes the config it will need an upgrade path.

Removing credit for #14 as it's expected to check a patch before uploading.
You can check for build errors make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel... →

Hi @smustgrave
I got your point.
Provided an upgrade path and some tests.

upgrade looks good.

Can mark it after the change record.

Hi @smustgrave
Change record - done.

Unpublished the CR, because when it is published this is announced trough rss and that should only be done when the issue is committed.

Will this be the same functionality as provided by https://www.drupal.org/project/responsive_image_preload → ?

There are a lot of tests updated with '#image_preload' => FALSE,, does this mean that all custom/contrib code will have to do that as well? Can we not rely on the default value instead? I'd love to see less changes in the tests for this PR.

For the new open threads in the MR

Went through threads.

Big thanks to @borisson_ for the quality review. Some points were really good

How is the work here vs in ✨ Add support of "preload" attribute into FileMediaFormatterBase Needs work different? It isn't clear.

@heddn the differents is in formats that will be affected.

So, these are different solutions for different formatters

Will also post into #ux channel for feedback.

Posted in #ux channel and it hasn't had a full review but here is comment from @rkoller who was nice enough to take a quick look.

i don’t have much time to look in depth but i sort of agree with lucas heddn. it feels odd having the preload checkbox outside the image loading field set. i dont know the technical details and why it may be semantically more precise to separate both according to hitchshock. but from a users perspective preload and lazy load are both in the context of loading the image. and as lucas said it is advanced functionality.
currently you have the preload option exposed outside the collapsed fieldset. and users tend not to read lengthy descriptions. therefore it might happen that exactly that happens what the description advices not to do. people clicking the checkbox.
and yes i agree the description is long and hard to read. havent posted a comment on the issue cuz i havent come up with an alternative draft yet.
but one detail. …because not everything are critical… sounds grammatically wrong. shouldn’t that be is critical?

So seems like there is more votes leaning towards moving this into the image loading fieldset.
Will need a shorter description (if possible)
And nit picky "everything are critical" should be updated.

@smustgrave Okay, you've convinced me.
- I moved the preload option into image_loading
- Updated the tests

Also, I banned adding the loading attribute when preload is used because in this case, it won't work anyway.

@HitchShock:

Good timing! @ericerubinonet and I are looking at this issue at DrupalCon Pittsburgh today. We will try to give a UX review today.

Tested 3309016-add-image-preload branch using DrupalPod in core version 10.1.x

Viewed UI changes through the image field present on the Article content type (Structure → Content types → Article → Manage display).
The UI here looks great; Expanding the image loading dropdown in format reveals the "Preload" checkbox above the Image loading attribute radio. Hiding the attributes section upon checking "Preload" clearly indicates the other attribute options are rejected.

Compared page source before and after updating the image field format with preload active to verify that the respective link tags were placed in HEAD with rel="preload" as="image". Everything appears as described.

Working great! Very useful enhancement to the image field formatter.

Overall this looks good, added some comments on the MR though. It's still tagged for usability review, has that been done?

Hi @catch Wrote answers in the MR, Check them, please.

Also, it looks like @andrew_b has done a usability review in #35.
Also, the earlier version of the patch has been applied and tested on the real project.
But I don't know the approval procedure for this, so I'm not sure if I should remove "Needs usability review" tag or not.

Usability review

I do not have an opinion on the implementation of this feature, or whether it is useful enough to be added to Drupal core. (If I understand correctly, this feature is already provided by the blazy contrib module. If only a few sites need the feature, then that should be where it stays.) I am just reviewing the usability of the configuration form.

The current MR introduces a very unusual pattern. By default, the Preload option is un-checked and the "Image loading attribute" radio buttons are visible. If Preload is selected, then the radio buttons are hidden. The usual "progressive disclosure" pattern is different: initially, some options are hidden, keeping the form simple, but they appear if some other non-default option is selected.

If I read the code correctly, then the choice of loading attribute is ignored when Preload is selected:

  if ($image_src !== '' && $variables['image_preload']) {
    // Make sure that we don't have loading attribute when we use preload.
    unset($variables['attributes']['loading']);
    // ...
  }

I think it makes more sense to have three radio buttons. Putting Preload third makes sense: then the options are ordered by how aggressive we are about loading. We will have to change the label "Image loading attribute" since one of the choices does not set the loading attribute.

Here is a first draft:

Lazy render images with native image loading attribute (loading="lazy"). This improves performance by allowing browsers to lazily load images.

Image loading

• Lazy (loading="lazy")
  
  Delays loading the image until that section of the page is visible in the browser. When in doubt, lazy loading is recommended.
• Eager (loading="eager")
  
  Force browsers to download an image as soon as possible. This is the browser default for legacy reasons. Only use this option when the image is always expected to render.
• Preload (rel="preload")
  
  Preload to optimize the loading of late-discovered resources. Normally large or hero images below the fold.

Select how images are loaded. Learn more about the loading attribute for images and the preload attribute for links.

I do not love having descriptive text in three places (above the fieldset, after each option, and at the end of the fieldset). But I think that reorganizing it is out of scope for this issue.

The labels I suggest are imprecise because they do not indicate that the loading and rel attributes apply to different HTML elements. But I want to keep the labels short, and the link text at the bottom suggests what is going on.

Hi @benjifisher
About your comment:

• You are right it was implemented in the blazy contrib module. But it's worth remembering that lazy and eager loadings were also introduced quite recently and were previously only part of the contrib module. So I had a logical question: why wasn't the full solution implemented? Why weren't all possible loading options provided? This is weird for me. That's why I spend time creating this patch along with the tests and an upgrade path for existing sites instead of the simple patch with a simple solution.
• I don't agree with 'unusual pattern'. The use of states should be logical and user-friendly. This is exactly what it is here.
• I have to say thank you for the description tip, I will really use it. I'm not good with short and localized wording, so your help with this is really helpful for me.
• As for moving a new option to the radios list: this is indeed a matter for discussion. I'll try to describe what exactly worries me and why it was implemented this way cuz this is not only an issue of visual appearance, but also the consequences of the changes:
  • The solution should be convenient and understandable not only for users but also for developers, so the structure of the settings should be logical.
  • I don't know why the structure of the settings was invented this way 'image_loading' => [ 'attribute' => 'lazy', ],, but since preload is not an attribute, it cannot be stored in the same place as other loading attributes, cuz this is unlogical. So to implement your proposal, we need one of two ways:
    • You are right it was implemented in the blazy contrib module. But it's worth remembering that lazy and eager loadings were also introduced quite recently and were previously only part of the contrib module. So I had a logical question: why wasn't the full solution implemented? Why weren't all possible loading options provided? This is weird for me. That's why I spend time creating this patch along with the tests and an upgrade path for existing sites instead of the simple patch with a simple solution.
    • I don't agree with 'unusual pattern'. The use of states should be logical and user-friendly. This is exactly what it is here.
    • I have to say thank you for the description tip, I will really use it. I'm not good with short and localized wording, so your help with this is really helpful for me.
    • As for moving a new option to the radios list: this is indeed a matter for discussion. I'll try to describe what exactly worries me and why it was implemented this way cuz this is not only an issue of visual appearance, but also the consequences of the changes:
      • The solution should be convenient and understandable not only for users but also for developers, so the structure of the settings should be logical.
      • I don't know why the structure of the settings was invented this way 'image_loading' => [ 'attribute' => 'lazy', ],, but since preload is not an attribute, it cannot be stored in the same place as other loading attributes, cuz this is unlogical. So to implement your proposal, we need one of two ways:
        • Rework the schema to customize the loading to make it suitable for all cases. The major disadvantage of this option is that we need to completely update the schema on existing sites. This can also strongly affect the custom code and possibly some of the contrib modules.
        • Override the result of saving the settings to keep the structure of the scheme and change the form at the same time. But I really don't like this complication of the solution.

      That's why I decided to make this option separate, so as not to overcomplicate the decision where it shouldn't be. It would be interesting to hear someone else's opinion on this issue.