Problem/Motivation

We went through a few iterations here. The solution landed on is a filter format. See comments in #8 and #10

Proposed resolution

Add a new filter format that replaces the functionality already added to editor_file_reference and move lazy loading logic into it. Enable this new filter, but only if editor_file_reference was enabled. This gives all sites that currently have lazy loading, that same ability. But now gives them the ability to disable lazy loading if they do not want it. This new filter only adds loading="lazy" if the markup on the <img> tag doesn't already designate something else.

History:
In #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core, lazy loading was added for editor file referenced files. Back at that point, the understanding from many in the industry was that lazy loading was a great win. But then some things were learned and in certain situations, it would be great to disable that. That is what this new filter does. And it does it by moving that feature out of editor_file_reference into a dedicated editor_image_lazy_load filter.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

A new text filter was added to support <img loading="lazy">. For any sites that previously had Track images uploaded via a Text Editor enabled, this filter will be enabled by default.

CommentFileSizeAuthor
#53 interdiff_51-53.txt4.05 KBheddn
#53 3247795-53.patch41.51 KBheddn
#51 reroll_diff_3247795_47_3247795_51.txt5.91 KByogeshmpawar
#51 3247795-51.patch37.79 KByogeshmpawar
#47 diff_45-47.txt7.36 KBheddn
#47 3247795-47.patch37.64 KBheddn
#45 interdiff_42-45.txt2.14 KBheddn
#45 3247795-45.patch35.57 KBheddn
#42 3247795-42.patch33.77 KBheddn
#42 interdiff_40-42.txt629 bytesheddn
#40 interdiff_38-40.txt2.37 KBheddn
#40 3247795-40.patch33.15 KBheddn
#38 3247795-37.patch33.15 KBheddn
#38 interdiff_35-37.txt23.78 KBheddn
#35 interdiff_32-35.txt2.53 KBheddn
#35 3247795-35.patch30.57 KBheddn
#32 interdiff_29-32.txt1.52 KBheddn
#32 3247795-32.patch28.37 KBheddn
#29 interdiff_27-29.txt4.03 KBheddn
#29 3247795-29.patch28.32 KBheddn
#27 interdiff_25-27.txt1.43 KBheddn
#27 3247795-27.patch23.84 KBheddn
#25 3247795-25.patch22.24 KBheddn
#25 interdiff_24-25.txt2.08 KBheddn
#24 3247795-24.patch21.9 KBheddn
#24 interdiff_19-24.txt12.6 KBheddn
#19 interdiff_13-19.txt9.41 KBheddn
#19 3247795-19.patch12.95 KBheddn
#14 2021-11-17_13-50.png23.36 KBheddn
#13 3247795-14.patch7.51 KBheddn
#13 interdiff_12-13.txt4.59 KBheddn
#12 3247795-12.patch3.44 KBheddn
#2 3247795.patch6.92 KBheddn

Issue fork drupal-3247795

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

Status: Active » Needs review
FileSize
6.92 KB
Wim Leers’s picture

Category: Task » Feature request

Same questions as I asked at https://github.com/ckeditor/ckeditor5/issues/10784#issuecomment-961700192 apply here.

I asked:

Is the idea here to allow content creators to signal which images in their content are higher priority? Isn't it always going to be the first one? Or inline images, because reflows would be bad?

I guess it's the other way around actually, since `eager` is the default — that'd allow content creators to mark certain images as lower priority?

I'm concerned about overburdening the content creator with these decisions. Depending on the context in which they're editing, they may not even be able to make a fully informed decision.

You responded:

One suggestion proposed was to add a marker for "below the fold" so a content editor could mark where they think below will occur so ckeditor can "do the right thing" for images and iframes that are embedded.


The viewport size and theme determines where "below the fold" occurs. So … I have doubts about this. Strong doubts. Discussion needed! 😊

Only one thing I'm certain about: the current patch would complicate the UX for 100% of sites. That means this can't land with the current UX.

heddn’s picture

I can't find it in D9/D9, but there used to be a trim indicator in D7 (maybe it was a contrib module?) that you could place on the body to tell where to trim the body field. It was for use with trim/summary in teaser view modes. I could see us doing something similar here. Where we tell ckeditor 600 characters as a default. Then gave the editor an option to insert a marker/indicator where the fold "really" is if they need to tweak the page in one-off situations. Anything above the indicator would be eager. Below, it would be lazy.

heddn’s picture

@Wim Leers any feedback on #4?

heddn’s picture

Discussed this with Fabian Franz and upstream w/ Piotrek Koszuliński this morning on a call. We finally came up with a suggestion direction, which I'm documenting here to get wider feedback.

The suggestion is:

  • no upstream ckeditor plugin work
  • write a drupal input filter, that when enabled, will cause images to be lazy loaded
  • this input filter will not overwrite any already existing markup that denotes lazy or eager
  • this last option gives knowledgeable editors to manually inspect the markup and trigger eager in those cases where the default is to load lazy and they need to override this to be eager

The benefit to an input filter is that the markup stays very clean. It just has the markup for image tags, and nothing else. The benefit to not providing a UI widget to disable the filter's effect are that this is an advanced feature-set that could be confusing to many editors. One note on overriding the input filter, the allowed markup of the text format would need to be updated in Drupal so that loading="lazy|eager" is allowed.

Wim Leers’s picture

Title: drupalimage support for loading="lazy" » Text filter plugin to support <img loading="lazy">
Component: ckeditor.module » filter.module
Issue tags: +front-end performance

#6 addresses the concerns I had on #4 😊

But how would the @Filter plugin work? What heuristics would it use? Or would it simply make every image have loading=lazy?

Note that extra care will need to be applied when the image is not using <img> markup but <drupal-media>, i.e. when it is an image from Drupal's Media Library. In that case, you'll need to ensure this new filter runs late enough. Probably setting a very high default weight is sufficient.

heddn’s picture

re #7: the filter as we envisioned it would be called "Lazy load images". It would check markup for <img> tags and add loading="lazy" to them. But it wouldn't overwrite any existing loading="*" designation. This lets folks that have a real need to manually effect lazy loading to 1) adjust the allowed markup then 2) inspect markup 3) manually add the designation to the image tag(s) on individual pages.

This feature would be a core filter. It doesn't stop contrib from trying to do something more heuristic. In our discussions, it was very complicated on mobile vs desktop vs widescreen vs responsive image vs (you get the picture) to figure out a deterministic way to decide when to mark something as lazy. Especially when there are teaser, hero, full page, etc view modes. The edge case of turning off lazy load is for Knowledgeable Content Editors who have been directed to improve their SEO Lighthouse/Speedtest scores and are given defined directions how to improve the LCP for specific pages. For those edge cases, they can decide if it is easier to 1) disable the filter and add loading="lazy" to a few pages that need it still or 2) keep the filter enabled and set loading="eager". Alternatively, they could write a custom or develop a contrib module that has some heuristics to only add loading="lazy" based on some type of indeterminate criteria.

benmorss’s picture

I'd also be curious to learn why you decided not to work within CKEditor itself, but to focus on a filter on the Drupal side instead.

I haven't hacked on CKEditor in a while. Did it simply seem easier to work in an area where the Drupal community has more control... within Drupal?

heddn’s picture

re #9: we don't want to adjust the actual markup of the ckeditor backed body field. That makes for changing/altering the markup globally at a later point complicated. Ergo, enter Drupal's filter system. It has long been a golden rule of body field markup to keep clean markup. And use filters to augment the data. Having this be a filter, we can easily enable/disable lazy loading across an entire drupal site with a simple toggle and flushing cache.

If we make the change in ckeditor directly, we could not do that. It doesn't have the concept of filters. The lazy loading directive gets directly sprinkled in each and every body field.

As a parallel, we discussed a ckeditor plugin that adds target="_blank" to all links. This core ckeditor plugin muddies the markup (no filter). And it is one we as a Drupal community are actively looking to block. For all the reasons I just expounded upon.

CKEditor is open to adding it to its core at a later date, similar to the link plugin just mentioned. But since loading="lazy" is so new on the market, they want to get more industry input on its addition before implementing. If it did get implemented, Drupal would still block its usage (similar to the link plugin just mentioned) and recommend usage of a Drupal filter plugin instead.

benmorss’s picture

That makes sense to me. The more complex the markup you inherit from CKEditor, the harder it is to change attributes or undo features you didn't want. I've dealt with that target="_blank" issue before in content generation, so I get what you mean!

Of course I do still like the idea of CKEditor adding this an option anyway. But that's a separate issue :)

heddn’s picture

No interdiff as this is an entirely new direction. This is the filter. We should consider if we want to add this filter to the default text formats provided by core's install profile.

heddn’s picture

Here we add the filter to the install profiles.

heddn’s picture

Issue summary: View changes
FileSize
23.36 KB
heddn’s picture

Issue summary: View changes

Status: Needs review » Needs work

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

Wim Leers’s picture

#11:

The more complex the markup you inherit from CKEditor, the harder it is to change attributes or undo features you didn't want.

It's not just that. Complexity of markup isn't the problem. Embedded assumptions in the content is the problem.

The content in entities (here it's specifically markup in body fields) should always be as reusable as possible, and definitely not be coupled to (because tuned for) single-observer specifics.

If a content author optimizes the "lazy vs eager" flags for images in his markup, it may be coupled to:

  • the viewport the content author used
  • the back end ("admin") theme they created the content in (hopefully not, that'd be really bad)
  • the front end theme they previewed/tested the content in

This gets in the way when reusing content in multiple channels (it may be served on multiple websites, consumed through APIs, announcement screens … — IIRC the NYC subway's info screens run on Drupal for example, so this isn't too far-fetched!). IOW: the author should never tweak their content to look perfect on their device. That's the mantra in Drupal.

This tends to be called structured content. I wrote about it a very long time ago: https://wimleers.com/article/drupal-8-structured-content-authoring-exper..., and also wrote about a great podcast episode about this topic: https://wimleers.com/blog/eaton-urbina-structured-intelligent-adaptive-c....

You're right that it still makes sense to add to CKEditor! Many sites use CKEditor to generate the majority of their markup, with no post-processing. And they often do not aim to have reusable content. In that case, that's totally reasonable :)

Wim Leers’s picture

Issue tags: +Needs tests

Thorough review:

  1. +++ b/core/modules/filter/config/install/filter.format.plain_text.yml
    @@ -31,3 +31,9 @@ filters:
    +  filter_image_lazy_load:
    +    id: filter_image_lazy_load
    +    provider: filter
    +    status: true
    +    weight: 10
    +    settings: {  }
    

    -1 to adding this to the plain_text format 😅 Makes no sense because no HTML is allowed, hence also not images:

      # Escape all HTML.
      filter_html_escape:
        id: filter_html_escape
        provider: filter
        status: true
        weight: -10
        settings: {  }
    
  2. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,49 @@
    + *   description = @Translation("Direct browsers to lazy load all images, unless overridden by <code>&lt;img loading=&quot;eager&quot;&gt;

    ."),

    "Direct" is ambiguous. I think "Instruct" would be clearer?

  3. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,49 @@
    +  private function transformImages(string $text): string {
    

    Needs docblock.

  4. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,49 @@
    +    // Only set loading="lazy" if no existing loading attribute is specified.
    +    foreach ($xpath->query('//img[not(@loading)]') as $image) {
    

    🤩👍 — super elegant, perfect!

  5. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,49 @@
    +      \assert($image instanceof \DOMElement);
    

    Nit: We don't need the leading backslash.

  6. +++ b/core/modules/filter/tests/src/Unit/FilterImageLazyLoadTest.php
    @@ -0,0 +1,61 @@
    +  public function testProcess(string $html, string $expected): void {
    +    $this->assertSame($expected, $this->filter->process($html, 'en')->getProcessedText());
    +  }
    

    Solid basic test 👍

    Missing test where it is verified that this also correctly combined with media_embed.

  7. +++ b/core/modules/filter/tests/src/Unit/FilterImageLazyLoadTest.php
    @@ -0,0 +1,61 @@
    +  public function providerHtml() {
    

    Nit: return type hint could be added :)

  8. +++ b/core/modules/filter/tests/src/Unit/FilterImageLazyLoadTest.php
    @@ -0,0 +1,61 @@
    +      // The first few should not be touched.
    +      ['<p><img src="foo.png" loading="lazy"></p>', '<p><img src="foo.png" loading="lazy" /></p>'],
    +      ['<p><img src="foo.png" loading="eager"/></p>', '<p><img src="foo.png" loading="eager" /></p>'],
    +      ['<p>Lorem ipsum...</p>', '<p>Lorem ipsum...</p>'],
    +      // This one should be processed to lazy because no loading directive was
    +      // provided.
    +      ['<p><img src="foo.png"></p>', '<p><img src="foo.png" loading="lazy" /></p>'],
    

    We generally describe our test cases in data providers. That makes debugging failing tests easier.

    If we do that, then we can also get rid of these comments: the case labels become the comments.

  9. +++ b/core/profiles/demo_umami/config/install/filter.format.basic_html.yml
    @@ -16,7 +16,7 @@ filters:
    -      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption> <drupal-media data-entity-type data-entity-uuid data-view-mode data-align data-caption alt title>'
    +      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt loading height width data-entity-type data-entity-uuid data-align data-caption> <drupal-media data-entity-type data-entity-uuid data-view-mode data-align data-caption alt title>'
    

    No upgrade path concerns here because this is a demo-only install profile. 👍

  10. +++ b/core/profiles/demo_umami/config/install/filter.format.basic_html.yml
    @@ -49,6 +49,12 @@ filters:
    +  filter_image_lazy_load:
    +    id: filter_image_lazy_load
    +    provider: filter
    +    status: true
    +    weight: 10
    +    settings: { }
    

    This should have a higher weight than the media_embed filter. Because only then can it also mark media-that-are-images to load lazily.

  11. +++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
    @@ -15,7 +15,7 @@ filters:
    -      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption>'
    +      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt loading height width data-entity-type data-entity-uuid data-align data-caption>'
           filter_html_help: false
           filter_html_nofollow: false
       filter_align:
    @@ -36,6 +36,12 @@ filters:
    
    @@ -36,6 +36,12 @@ filters:
         status: true
         weight: 9
         settings: {  }
    +  filter_image_lazy_load:
    +    id: filter_image_lazy_load
    +    provider: filter
    +    status: true
    +    weight: 15
    +    settings: {  }
    

    This looks sensible to me. 👍

    But … shouldn't we try to get this to work on every Drupal 9 site out-of-the-box? IOW: shouldn't we provide an upgrade path?

    At minimum, we should do that for sites who haven't modified the default basic_html or full_html text formats.

heddn’s picture

Status: Needs work » Needs review
FileSize
12.95 KB
9.41 KB

18.10 (basic_html): media toggles lazy loading already. Until #3173180: Add UI for 'loading' html attribute to images lands, the default is for all images to lazy load. So the order of the lazy loading vs media embed doesn't matter. After that issue lands, it still won't matter the order. Because the media item will already to lazy load or not.

:thinking: should _that_ issue change to have 3 options? 1) loading="lazy", 2) loading="eager" 3) not specified? Actually, that feels like scope creep for _that_ issue. Another follow-up to add another option to debate that is in order. I can see pros/cons of a not specified option. Regardless, we can move the order of lazy_loading after media embed, just know that it won't really do anything. At least not right away.

:thinking: another thought, what if we put the lazy_loading _before_ media embed so it injects the attribute for lazy loading and media then just takes the values passed to it from the media markup? that seems complicated and scope creep for _this_ issue. I think I'm going to _not_ change anything on filter order to help media embed for now. Too many edge cases.

18.11 (default OOB upgrade path): We forced that on site owners in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core. And it was mildly successful. But it also lead to increases in LCP scores. I'm not sure we really want to do that again. Add this feature to release notes and give site owners the option to enable it. They should do it, but then again, maybe they don't want to.

18.6 (embed testing): not added, see long stream of conscience above in response to 18.10.

Attached new patch with test fixes. It also addresses all feedback in #18.

Wim Leers’s picture

Well … it'd be great if that issue had been linked from the issue summary since day one then. That's crucially important information for reviewing this issue!

Wim Leers’s picture

Category: Feature request » Task
Status: Needs review » Needs work
Issue tags: +Needs title update, +Needs issue summary update
Related issues: +#3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core

Huh, that issue expands on what #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core already landed in Drupal core … and I'm very surprised that #3167034 managed to modify the EditorFileReference filter to also set loading="lazy", even though that is 100% unrelated to what it says it does.

It also means that the original and current issue summary are extremely misleading. 😔 Because … it is already supported, just in a really confusing way, through a completely unrelated filter, whose description does not mention it at all! The description of this filter does what #3167034 made another filter do! The difference is that this filter works for any/all images, the other filter only works for Drupal-owned and Drupal-uploaded images.

Another consequence: the test coverage here is not remotely sufficient, my concern in #18.6 is just a subset of all the combinations that should be tested now.

I'm even more confused now because that was careful to only set the default of "lazy" if an image has dimensions to avoid CLS (Content Layout Shift), but here there's no mention of that.


thought, what if we put the lazy_loading _before_ media embed so it injects the attribute for lazy loading and media then just takes the values passed to it from the media markup?

Are you suggesting to set <drupal-media loading="lazy"> — and then have the media_embed filter respect that?


18.6 (embed testing): not added, see long stream of conscience above in response to 18.10.

Sorry, but the implementation details of #3173180: Add UI for 'loading' html attribute to images do not really matter here — we just need to verify that these different filters work as expected when combined. If they are touching the same attribute, there's no way around that, that is essential to guarantee correct behavior.

Because you argued that the filter order should not matter, that also implies even more test coverage is needed: we need to verify that both filters work correctly together regardless of their order.


18.11 (default OOB upgrade path): We forced that on site owners in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core. And it was mildly successful. But it also lead to increases in LCP scores.

So … the replies to @xjm's concerns in #3167034-54: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core about disruption were actually inaccurate then? 😨 There was disruption?! But there's nothing in that issue about it. Where can I read more about this?

Wim Leers’s picture

IMHO the changes made in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core that were made in EditorFileReference should be reverted from (I think its impact was not yet fully understood) and moved into this new filter. #3167034 made it impossible to not set loading="lazy" on images, by doing it on a required filter (required if you are using image uploads). It made it have unexpected and undocumented side effects.

P.S.: I'm sorry for the pushback 😞 I was eagerly reviewing this because I really want to see this happen in Drupal. 😄 But then I just learned that this sort of already landed in Drupal 9.1 😳 (not explained in the issue summary at all), this is just different (in ways not explained anywhere), in a way that makes it impossible to opt out 👎and introduced regressions 😱. So I find myself in a place where I have to push back, even though I don't want to, but it's crucial for the maintainability (and understandability) of this functionality. 😬

heddn’s picture

I've been playing with things for the past little bit and thought before I move much further, I should post some notes. Some of what was done in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core is starting to come back. Pulling back the layers, not having a managed file it becomes complicated to get the dimensions for images.

    foreach ($xpath->query('//img[not(@loading)]') as $element) {
      assert($element instanceof \DOMElement);
      $path = ltrim(urldecode($element->getAttribute('src')), '/');

The above code works to get a path for locally hosted files that are placed in a standard location without any htaccess rewritting of paths. Meaning, it works because /sites/default/files/inline-images/foo%20bar%20baz.png removes the leading slash and becomes sites/default/files/inline-images/foo bar baz.png. But what if the file has special characters or isn't a local image? Or is rewritten via htaccess from sites/default/files to some other location on the server? Then we run into lots of fun when we attempt to gather the dimensions:

      $image = $this->imageFactory->get($path);
      $width = $image->getWidth();
      $height = $image->getHeight();

In the case of remote images, we can get errors like: Warning: filesize(): stat failed for http://placekitten.com/200/300 in Drupal\Core\Image\Image->__construct() (line 54 of core/lib/Drupal/Core/Image/Image.php)..

What this tells me, is we really don't want to have lazy load images that aren't entity references. Or at least, if we don't want to experience CLS. And we are back to the entity reference filter again. But we should probably untangle lazy loading from that filter. For all the reasons already mentioned. And re-add it to a new lazy load filter for managed files, so we can calculate image dimensions off the managed file.

And maybe we leave a dumb "lazy loading" filter that doesn't do any image dimension logic. And leave a note on that filter that it could lead to CLS if no dimensions are specified.

heddn’s picture

Status: Needs work » Needs review
FileSize
12.6 KB
21.9 KB

This removes lazy loading from EditorFileReference. All that logic is added into _this new_ filter.

Still remaining:

  • update hook to enable this new filter if EditorFileReference is enabled
  • test coverage for update hook
  • test coverage of deprecated removal of image factory in EditorFileReference
  • update change record to remove image factory

I've been thinking about it and I'm not really sure I see a need for a dumb text filter for remote and non managed files (in core). These aren't as common and could be handled by a contrib module. Distinguishing when to use it and the possible CLS implications if we can't determine the dimensions are complicated.

heddn’s picture

FileSize
2.08 KB
22.24 KB

Status: Needs review » Needs work

The last submitted patch, 25: 3247795-25.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
23.84 KB
1.43 KB

this should be green again.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

heddn’s picture

FileSize
28.32 KB
4.03 KB

This responds to my personal review of next steps in #24. All those actionable items are completed in this upload.

heddn’s picture

Title: Text filter plugin to support <img loading="lazy"> » Add text filter plugin to support <img loading="lazy"> and remove it from editor_file_reference
Issue summary: View changes
Issue tags: -Needs tests, -Needs title update, -Needs issue summary update

Updated tags, IS and title. Ready for another round of reviews.

Status: Needs review » Needs work

The last submitted patch, 29: 3247795-29.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
28.37 KB
1.52 KB
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC - nice cleanup

Some things:

- Can we add lazing=loady for images that have width and height attributes set, but no loading attribute?

e.g. two ways for automatically make images lazy loading:

- One when they have a data-uuid
- Two when they already have width and height attributes set

That said:

Setting width and height attributes is usually always a good idea, so maybe it might be best to make a follow-up to set width and height always (when available) and then have the filter piggy back from that?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 3247795-32.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
30.57 KB
2.53 KB
Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
    @@ -393,7 +393,16 @@ public function provider() {
    +            'ckeditor5_sourceEditing' => [
    +              'allowed_tags' => [
    +                '<img loading>',
    +              ],
    +            ],
    

    So now this is affecting the CKEditor 5 upgrade path too!

    Interesting! 😄

    🤞 Is there a CKEditor 5 plugin that supports opting in to this?

    🤔 Overall, what do you intend the UX to be? Do you expect users to set this flag in the "source" view in CKEditor 4 and 5? Is this obscure enough that it warrants not being exposed in the UI (EditorImageDialog for CKEditor 4, the image balloon/popover toolbar for CKEditor 5)? Did that get approved by the Usability topic maintainers?

  2. +++ b/core/modules/editor/src/Plugin/Filter/EditorFileReference.php
    @@ -95,25 +92,6 @@ public function process($text, $langcode) {
    -            if ($node->nodeName == 'img') {
    -              // Without dimensions specified, layout shifts can occur,
    -              // which are more noticeable on pages that take some time to load.
    -              // As a result, only mark images as lazy load that have dimensions.
    -              $image = $this->imageFactory->get($file->getFileUri());
    -              $width = $image->getWidth();
    -              $height = $image->getHeight();
    -              if ($width !== NULL && $height !== NULL) {
    -                if (!$node->hasAttribute('width')) {
    -                  $node->setAttribute('width', $width);
    -                }
    -                if (!$node->hasAttribute('height')) {
    -                  $node->setAttribute('height', $height);
    -                }
    -                if (!$node->hasAttribute('loading')) {
    -                  $node->setAttribute('loading', 'lazy');
    -                }
    -              }
    -            }
    

    👏 YES!

  3. +++ b/core/modules/filter/filter.post_update.php
    @@ -0,0 +1,39 @@
    +      \assert($format instanceof FilterFormatInterface);
    ...
    +      \assert($collection instanceof FilterPluginCollection);
    +      if (\array_key_exists('editor_file_reference', $configuration)) {
    

    We don't add these leading backslashes in Drupal. They're in the global namespace, this is just noise.

  4. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,124 @@
    + *   weight = 15
    

    Why 15?

  5. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,124 @@
    +   * Constructs a \Drupal\filter\Plugin\Filter\FilterImageLazyLoad object.
    

    Nit: the namespace should be omitted here.

  6. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,124 @@
    +   *   The plugin_id for the plugin instance.
    

    s/plugin_id/plugin ID/

  7. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,124 @@
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition): self {
    

    Missing {@inheritdoc}

  8. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,124 @@
    +    if (stripos($text, '<img ') === FALSE && stripos($text, 'data-entity-type="file"') === FALSE) {
    

    Should we have a follow-up to expand this to also happen for embedded image Media entities?

  9. +++ b/core/modules/filter/src/Plugin/Filter/FilterImageLazyLoad.php
    @@ -0,0 +1,124 @@
    +        // Set dimensions to avoid content layout shift (CLS).
    

    An @see for this would be nice :)

  10. +++ b/core/modules/filter/tests/src/Functional/Update/FilterConfigTest.php
    @@ -0,0 +1,48 @@
    +   * @see \filter_post_update_image_lazy_load()
    

    Leading backslash again.

  11. +++ b/core/modules/filter/tests/src/Functional/Update/FilterConfigTest.php
    @@ -0,0 +1,48 @@
    +    // Run updates.
    

    The comment literally says what the method name says … let's delete this.

  12. +++ b/core/profiles/demo_umami/config/install/filter.format.basic_html.yml
    @@ -49,6 +49,12 @@ filters:
    +    weight: 10
    

    10 does not match the default weight of 15.

    Does that mean that existing sites just checking the checkbox to enable this filter will get the results they expected?

    Perhaps we need to add explicit validation for this, just like we did for the media_embed filter?

heddn’s picture

Status: Needs work » Needs review

This picks up feedback from 36. Namely, 36.3, 36.5, 36.6, 36.7, 36.10, 36.11.

Items from 36 not needing addressing: 36.2.

36.1 (expected UX in ckeditor):
See findings in #6 - #8

36.4 (why weight 15) &
36.12 (weight mismatch): I don't recall. I guess it is related to attempting to make sure it runs after track uploaded files and not trigger the (new) validation logic just added. There is test coverage of this though, so the weights seems correct.

36.8 (follow-up for image media embed):
media embed items are rendered via media's view mode. its rendering and whether lazy load is added is handled by the configuration on the media. ckeditor has zero impact on that.

36.9 (@see):
See what? I can't tell from context where someone should be directed.

The interdiff is a little noisy as I've moved some hunks around from filter to editor module to better align functionality. The main changes are the nit fixes and changes to editor.module. All feedback in #36 are addressed.

heddn’s picture

FileSize
23.78 KB
33.15 KB

Oops, And here are the missing files.

Status: Needs review » Needs work

The last submitted patch, 38: 3247795-37.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
33.15 KB
2.37 KB

Status: Needs review » Needs work

The last submitted patch, 40: 3247795-40.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
629 bytes
33.77 KB
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Approved, changes look good back to RTBC for another round of maintainer reviews! :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 42: 3247795-42.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
35.57 KB
2.14 KB

Due to this getting mixed up into the ckeditor5 upgrade path, we get to keep enhancing test coverage as more of ckeditor5 gets converted. That's what this latest re-roll is about.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good, back to RTBC

heddn’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
37.64 KB
7.36 KB

Needed a re-roll. Diff attached for the re-roll. Mainly we are still chasing CKEditor5 test changes.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Reviewed & tested by the community » Needs review

I'd like the opportunity to sign off on this again. Will try to review by the end of the week. This has not had a green for well over a month, so fortunately you've not been blocked on me :)

heddn’s picture

Can we leave this in the RTBC queue so re-tests get triggered while reviewing this?

yogeshmpawar’s picture

Rerolled patch & added reroll diff for reviewers.

Status: Needs review » Needs work

The last submitted patch, 51: 3247795-51.patch, failed testing. View results

heddn’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs subsystem maintainer review

Back to RTBC, leaving assigned to Wim

Adding tag for subsystem maintainer review (that Wim will perform)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 3247795-53.patch, failed testing. View results

heddn’s picture

Status: Needs work » Reviewed & tested by the community

And back to RTBC with same caveats as #54. Note, there are 2 branches here to support 9.4 and 10.0.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs subsystem maintainer review
  1. 🤔 I'm missing a plan for how this will support images embedded from the Media Library. That is the future way, and I think all high-impact front-end performance features like this one should consider that. They should at minimum have a plan, and ideally just also solve that. I first asked about this in #21, then again in #36 and it unfortunately has not yet been addressed.
  2. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
    @@ -158,7 +158,7 @@ protected function setUp(): void {
    -    $new_value = str_replace('<img src alt height width data-entity-type data-entity-uuid data-align data-caption>', '<img src alt height width data-*>', $current_value);
    +    $new_value = str_replace('<img src alt loading height width data-entity-type data-entity-uuid data-align data-caption>', '<img src alt height width data-* loading>', $current_value);
    

    Nit: let's keep the order the same. loading was the third attribute (after src and alt), no need to change that.

  3. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
    @@ -571,14 +580,21 @@ public function provider() {
    -        'plugins' => $basic_html_test_case['expected_ckeditor5_settings']['plugins'],
    +        'plugins' => [
    +            'ckeditor5_sourceEditing' => [
    +              'allowed_tags' => array_merge(
    +                $basic_html_test_case['expected_ckeditor5_settings']['plugins']['ckeditor5_sourceEditing']['allowed_tags'],
    +                ['<img loading>'],
    +              ),
    +            ],
    +          ] + $basic_html_test_case['expected_ckeditor5_settings']['plugins'],
    

    🤓 Indentation issue.

  4. +++ b/core/modules/editor/editor.module
    @@ -182,6 +182,64 @@ function editor_form_filter_format_form_alter(&$form, FormStateInterface $form_s
    + * Validate callback to ensure filter order and allowed_html are compatible.
    

    🐛 This comment does not quite make sense I think? I think it meant to say editor_image_lazy_load and editor_file_reference?

  5. +++ b/core/modules/editor/editor.post_update.php
    @@ -13,3 +17,30 @@ function editor_removed_post_updates() {
    + * Enable filter_image_lazy_load if editor_file_reference is enabled.
    

    🐛 Isn't it editor_image_lazy_load?

  6. +++ b/core/modules/editor/editor.post_update.php
    @@ -13,3 +17,30 @@ function editor_removed_post_updates() {
    +function editor_post_update_image_lazy_load(): void {
    

    🤔 Why is the post-update hook in the editor module if the new filter is provided by the filter module?

  7. +++ b/core/modules/editor/editor.post_update.php
    @@ -13,3 +17,30 @@ function editor_removed_post_updates() {
    +      \assert($format instanceof FilterFormatInterface);
    

    🤔 Nothing else in Drupal core uses a leading backslash before assert().

  8. +++ b/core/modules/editor/editor.post_update.php
    @@ -13,3 +17,30 @@ function editor_removed_post_updates() {
    +        $configuration['editor_image_lazy_load'] = [
    +          'id' => 'editor_image_lazy_load',
    +          'provider' => 'filter',
    

    🤔 Why is the filter called editor_image_lazy_load if it's provided by the filter module?

    And … why then not just call it image_lazy_load?

  9. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    +namespace Drupal\editor\Plugin\Filter;
    ...
    + *   id = "editor_image_lazy_load",
    ...
    +final class EditorImageLazyLoad extends FilterBase implements ContainerFactoryPluginInterface {
    

    🤔 This editor prefix definitely does not make sense.

  10. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    + *   title = @Translation("Lazy load tracked images uploaded via a Text Editor"),
    

    🤔 This filter is completely unrelated to a text editor? So why is it then labeled as such?

    Either it should not mention that, or it should be moved into the editor module. Right now it's very confusing.

  11. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    +   * Constructs a FilterImageLazyLoad object.
    

    🐛 Name mismatch.

  12. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    +    );
    +
    +  }
    

    🐛 Extraneous empty line.

  13. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    +    // If there are no images, exit early.
    

    🤓 Not exit, but return.

  14. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    +   * @return string
    +   */
    

    🐛 Incomplete documentation.

  15. +++ b/core/modules/editor/src/Plugin/Filter/EditorImageLazyLoad.php
    @@ -0,0 +1,127 @@
    +        // Set dimensions to avoid content layout shift (CLS).
    

    🙏 Let's add an @see to explain what "CLS" is.

  16. +++ b/core/modules/editor/tests/src/Functional/Update/EditorFilterConfigTest.php
    @@ -0,0 +1,47 @@
    + * Tests updating editor module's configuration.
    

    That's not what this is actually testing.

  17. +++ b/core/modules/editor/tests/src/Functional/Update/EditorFilterConfigTest.php
    @@ -0,0 +1,47 @@
    +class EditorFilterConfigTest extends UpdatePathTestBase {
    

    🤔 This name is also not conveying what it's actually testing.

  18. +++ b/core/modules/editor/tests/src/Functional/Update/EditorFilterConfigTest.php
    @@ -0,0 +1,47 @@
    +   * @see filter_post_update_image_lazy_load()
    

    🐛 Apparently this used to be in the filter module and now it's in the editor module. Out of sync.

  19. +++ b/core/modules/editor/tests/src/Unit/EditorFileReferenceTest.php
    @@ -0,0 +1,35 @@
    + * Tests editor file reference.
    

    🤔 That's not what this tests.

  20. +++ b/core/profiles/demo_umami/config/install/filter.format.basic_html.yml
    @@ -49,6 +49,12 @@ filters:
    +    weight: 10
    

    🤔 Why 10 and not also 15?

Wim Leers’s picture

The issue summary could also use an update, to make this actually reasonable to review and commit.

This definitely also needs a change record and a release note. Because this will be a high-impact change! 😄

catch’s picture

I'm missing a plan for how this will support images embedded from the Media Library. That is the future way, and I think all high-impact front-end performance features like this one should consider that. They should at minimum have a plan, and ideally just also solve that. I first asked about this in #21, then again in #36 and it unfortunately has not yet been addressed.

I haven't been following this particular issue very closely, but I just committed #3173180: Add UI for 'loading' html attribute to images and also keeping an eye on #3245720: [drupalMedia] Support choosing a view mode for <drupal-media> and related issues.

<drupal-media> is embedding media (which might contain an image, but isn't just the image), with a defined view mode.

The media display mode configuration will include whether the image is rendered lazily or not (responsive image formatter still needs #2947796: Responsive image format for media).

If we do this issue and the others, but no specific follow-up, then it looks like this:

- If you add an image directly in the editor, you either specify the loading attribute, or this filter makes it lazy if nothing is specified.
- If you add an image via media embed, the media view display configuration determines the loading attribute

The responsibility for the loading attribute (and everything else about how the embedded media is displayed) is therefore on the media display configuration. This means that care needs to be taken about where any particular media view mode is likely to be used, since it may or may not appear above or below the fold, but that is the case for non-editor media references too.

There seem to be a couple of other options:

1. Make this filter apply the same change to images rendered via media embed too - however since every image rendered via media view mode will have a loading attribute of either lazy or eager, it would be a no-op.

2. Add configuration on media embed to specify the loading attribute <drupal-media loading="lazy">, as mentioned in #3247795-21: Add text filter plugin to support <img loading="lazy"> and remove it from editor_file_reference, this would have to rewrite the rendered media entity HTML to override the attribute.

3. Take an approach closer to #2947796: Responsive image format for media: allow the media reference to show media as a 'thumbnail' instead of with a view mode, and then make the image style/responsive image style and loading attribute configurable directly in the editor <media-embed thumbnail responsive_image_style="wide"> or something like that. This would have to be an option in the media embed widget in that case, although that allows the loading attribute to be unspecified, then this filter could then kick in on those images.

Option #2. seems possible, but counter-intuitive to have the rendered entity HTML rewritten by the filter.

Option #3 seems like it might be viable, although it adds a lot of options and complexity to the media embed widget.

Wim Leers’s picture

#3173180: Add UI for 'loading' html attribute to images made it possible to configure it site-wide (for a formatter of a specific field, for all entities containing that field), not per use (i.e. in the widget to configure it per entity).

Which indeed leads to

The responsibility for the loading attribute (and everything else about how the embedded media is displayed) is therefore on the media display configuration.

I already wrote concerns about option 3 elsewhere, quoting the relevant bits here:

I was not aware of #2947796: Responsive image format for media . That reminds me of #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5 and #2822389: Allow responsive image style to be selected in Text Editor's image dialog (necessary for structured content) which I've been trying to land ever since CKEditor 4 was added to Drupal 8 almost 10 years ago 😔 #2947796 has no integration at all yet for CKEditor (4 or 5) — it's only for media fields. It would also need to update \Drupal\media\Plugin\Filter\MediaEmbed to allow overriding the new responsive_image_style field property. But the good news is that we already have something like this for alt and title — see \Drupal\media\Plugin\Filter\MediaEmbed::applyPerEmbedMediaOverrides() (introduced in #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`). And for that, the CKEditor 5 integration (just like the CKEditor 4 integration) already has the ability to override the image alt stored in the Media Library. We could do the same for the responsive image style. But that'd definitely result in a confusing UX. That's always the trade-off: which level of granularity do we expose? In MediaEmbed (#2940029: Add an input filter to display embedded Media entities), we chose to expose view modes, not responsive image styles — in no small part because responsive image styles do not work for media other than images. There's nothing stopping you from creating one view mode per responsive image style, and have that view mode only be meaningful (i.e. have config) for images. CKEditor 5 will only show those view modes for the Image media type that actually have a view display configured. Besides, some responsive image styles may not make sense for image media — maybe they're specifically used for user profile pictures for example. So I do not think #2947796: Responsive image format for media is a wise approach.

Option 2 would be very different if #3173180: Add UI for 'loading' html attribute to images had made it possible to choose the laziness per entity/use. IMHO that is the logical next step. Then what I wrote above in response to option 3 would become trivial to do: expand what \Drupal\media\Plugin\Filter\MediaEmbed::applyPerEmbedMediaOverrides() does to allow overriding not just alt and title but also loading.

Curious what you think 🤓

catch’s picture

Option 2 would be very different if #3173180: Add UI for 'loading' html attribute to images had made it possible to choose the laziness per entity/use.

I don't think we should allow configuration in the field widget (i.e. per-entity), seems like the wrong place.

Let's say I have an article content type, with a media image field, it's shown in the following situations:

1. A hero image on the front page of my site (because it's in a layout via views/entityqueue, or some kind of entity reference block thing).
2. A wide image at the top when the article is shown at node/n just under the title.
3. A card at the bottom of a different article as 'related content'
4. Additionally, the media image gets re-used in a different article via ckeditor.

Per entity doesn't help here at all, it makes things less flexible than view mode, because at least the node view display configuration can handle different media view display configurations.

I could see layout and views (and paragraphs) potentially providing some kind of override mechanism (per-use), but that doesn't play well with render caching of the view mode, and not sure we want to get into layout and views doing DOM-based post-processing of HTML.

There's also an existing usability issue open for the media override logic: #3084011: The source of alt text in embedded image media is not clear.

The idea with 'thumbnail' or if not thumbnail then some kind of 'directly render the image without using a view mode' option was to more cleanly separate the two cases, so that it's not actually overriding anything but just a different way of doing it. The issue with that though is it doesn't provide you the option of 'view mode + tweaks' which the current logic does, but it's closer to the views 'fields' mode (not that I'm a fan of that one though outside tables, but again it's clear what's happening).

heddn’s picture

Additional point for media embed, for videos that are loaded in an iframe and oembed, we have #3212351: Lazy load OEmbed formatter. Which also would get the lazy load treatment based on the view mode configuration.

heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Here's the start of fixes from the feedback over the past few days. But first 1000+ thanks for posting a review. I really appreciate it.

  1. Updated IS
  2. 61.1: I'm still digesting all the thoughts shared by @catch and Wim. Will post some thoughts in a bit.
  3. 61.2: fixed
  4. 61.3: hopefully fixed, I found one place with indent problems but if phpcs doesn't find it, these can be tricky to find
  5. 61.4: fixed
  6. 61.5: No, the filter is named editor_image_lazy_load. see below for more about, "Why editor module?"
  7. 61.6: The filter is provided by editor module. It used to be in filter, but I moved it several months ago. See below for the reason.
  8. 61.7: fixed
  9. 61.8: It is provided by the editor module. All other filter plugins in core seem to namespace with the module name as the prefix. This does the same.
  10. 61.9: All other filters in core do this. This just follows tradition.
  11. 61.10: The wording here is almost exactly the same as editor_file_reference. The filter is in editor module.
  12. 61.11: fixed
  13. 61.12: fixed
  14. 61.13: fixed
  15. 61.14: fixed
  16. 61.15: fixed. Usually we try to use MDN as our source. But the closest I could find in MDN was not about CLS but firefox's API to deal with layout shift and a reference to web.dev. If its good enough for MDN, hopefully it is good enough for Drupal.
  17. 61.16: I don't understand. It seems to be testing this from all I can tell.
  18. 61.17: I don't understand.
  19. 61.18: fixed
  20. 61.19: I don't understand.
  21. 61.20: fixed

Why is the filter added to editor module?

Because it depends on file module. Filter doesn't have that dependency. But editor does (added in 2013, see #2119775: EditorImageDialog assumes File module is enabled: add dependency on File module). All the same reasons that editor_file_reference are in editor module apply here. This issue just pulls the bits that were doing lazy loading out of that filter and moves them into a dedicated purposed filter.

heddn’s picture

heddn’s picture

To make it easier to review, I've closed the 10.x branch. Too hard to keep things in sync and review different MRs. If we need a distinct MR for 10.x, we can make it later.

heddn’s picture

My promised response to #61.1:

Media entities embedded in ckeditor have a view mode with a configured lazy/eager loading attribute defined in that view mode. I consider that to be the first phase of adoption for configuring the loading attribute. In the short term, folks can apply the work from #3245720: [drupalMedia] Support choosing a view mode for <drupal-media> and fork their view modes to have a lazy and eager option. And we can learn some things. We don't have to know everything before we adopt the first phase. If we need to add more phases to override media embed values (captions, loading, view mode, etc), that can all be done in a follow-up that takes the entire body of knowledge that is present at that time and adopts a reasonable path forward. So I guess I push back on us needing to fully fleshed out plan for media embed. Many other things have to line up and it is hard to predict the future.

Graber’s picture

Status: Needs review » Needs work

Tried to test this on demo_umami profile and all images added in wysiwyg don't have the height, width or loading attributte. Those are responsive images with srcset. I think it's worth to support those as well.

heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs release note

Responsive images from embedded media are going to be handled by the view mode selected for the media. It isn't in scope of this issue to solve that. Rather, that is in #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy". Back to NR.

Graber’s picture

Status: Needs review » Reviewed & tested by the community

Good :+1:

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

heddn’s picture

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

Switching issue version as well as a rebase of MR.

lauriii’s picture

It looks like the CI is failing.

Is there a specific reason for restricting the filter for tracked images? Wouldn't it be possible to enable that across all images now that it's in a separate filter?

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Responding to myself; of course because we couldn't retrieve the dimensions for other images. Marking NW for addressing the CI failures.

ravi.shankar made their first commit to this issue’s fork.

ravi.shankar’s picture

I've tried to rebase the current MR as it was not mergeable, please review.

heddn’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC after updating the fixture path from 9.3 => 9.4.

edurenye’s picture

Status: Reviewed & tested by the community » Needs work

MR is not mergeable.

heddn’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC. It didn't need a rebase. I did refresh the MR with 10.1.x, but there were no conflicts.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Added a code review in gitlab. Everything seems to be in place. A couple of nits and some missing test coverage.

heddn’s picture

Status: Needs work » Needs review

All feedback from MR (hopefully) addressed.

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

Graber’s picture

Status: Needs review » Needs work

A few things to address 👆

heddn’s picture

Status: Needs work » Needs review

The deprecation using DeprecatedServicePropertyTrait seems to be a topic of question. There doesn't seem to be existing test coverage for it. Not sure the right path forward on it.

alexpott’s picture

Issue tags: +Needs followup

@heddn re #90 I think what you've done is fine. We should file a follow-up to discuss how to improve this - but it should not block this issue.

heddn’s picture

Opened #3315816: Improve deprecated service trait as a follow-up. I've requeued tests as I am pretty sure the js test failures are randoms.

Graber’s picture

Status: Needs review » Reviewed & tested by the community

Tests passed - random test fails in core are a different issue, the solution here is ok. Back to RTBC.

alexpott’s picture

Updating issue credit to credit constructive and detailed comments plus code review.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Reviewed & tested by the community » Needs work

Left a review on the merge request.

High-level observations:

  1. 🤓 There is an unhandled edge case that would make the filter more robust.
  2. 👍 Excellent update path test coverage!
  3. 🙏 Thanks for opening #3314972: Filter plugin requirements handling!
  4. #61.1 still is not addressed, but was discussed in #63 and subsequent comments. I do think we still need a follow-up for this, with all the relevant information from the discussion here copied into it. If it's not in scope for this issue, it's in scope for this issue to ensure that we can efficiently work on it in the future, to avoid redoing this discussion. #75 says that's what #3192234: Apply width and height attributes to allow responsive image tag use loading="lazy" is for. Great! 👍 But that issue does not have a single mention of #3247795 (this issue). Not great 😔
heddn’s picture

Issue tags: -Needs followup

I opened #3319514: Handle overriding loading attribute in embedded media to handle ckeditor loading attribute overrides.

heddn’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Needs work
Related issues: +#3319514: Handle overriding loading attribute in embedded media

I assume you marked this as needing review because you addressed all feedback? I can't know for sure because due to the current d.o GitLab instance infra problems, it's impossible to see commits until >10 hours (maybe even >48) after they've been pushed 😅

In any case, the one thing you pushed back on means there's still at least one bug present, so marking Needs work for that.

heddn’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Needs work
Graber’s picture

Status: Needs work » Reviewed & tested by the community

Switching to RTBC as this needs a maintainer response / decision on one unresolved thread.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, I think this is still not in a consistent state: https://git.drupalcode.org/project/drupal/-/merge_requests/2096#note_130460 😅

heddn’s picture

Added some comments to the MR. This is a tough space. Not sure that either option is ideal. But I've proposed to keep lazy_load in editor. And move dimension setting into editor_file_reference.

heddn’s picture

Discussed this with Marcin and Nat today and the tentative plan is to move dimensions into editor_file_reference and move lazy_load into filter.module. The requirements check will disappear and we'll add description wording similar to html_corrector that lazy_load filter should execute after all other image filters. We can leave it to #3314972: Filter plugin requirements handling to handle warning messaging in a more clear way.

heddn’s picture

Status: Needs work » Needs review
Graber’s picture

Status: Needs review » Needs work

A few nits.

ravi.shankar’s picture

Resolved a few threads on MR, still needs work for the remaining threads.

heddn’s picture

Status: Needs work » Needs review

Sorry, this should be NR.

Graber’s picture

Status: Needs review » Reviewed & tested by the community

All addressed, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a266ba6 and pushed to 10.1.x. Thanks!

  • alexpott committed a266ba6 on 10.1.x
    Issue #3247795 by heddn, Graber, ravi.shankar, yogeshmpawar,...
heddn’s picture

Thank you everyone for all your assistance in getting this to land!

Wim Leers’s picture

👏

The end result looks great!

Status: Fixed » Closed (fixed)

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