1926802 - loading lazy doesn't work with template cloning ( cloneNode or importNode )

Status: NEW

(Core :: DOM: Core & HTML, defect)

Description

[titob]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36

Steps to reproduce:

Loading set to lazy doesn't work when a template is cloned and appended to the document. At least, I couldn't figure out a way to make it work.

const tpl = document.createElement("template");
tpl.innerHTML = `<div style="height: 2500px">
      <style>html, body{ height: 2500px}</style>
      <div style="height: 2500px">Spacer</div>
      <br/>
      <img
        loading="lazy"
        src="https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png?${Date.now()}"
        height="108"
        width="320"
        style="height: 108px;width:320px;"
        onload="console.log('image loaded')"
      > 
    </div>`;

const useCloneNode = false;

const img = useCloneNode ? tpl.cloneNode(true) : document.importNode(tpl, true);

setTimeout(() => {
  document.body.append(img.content);
}, 2000);

Actual results:

The image is fetched from the server as soon as the template is appended.

Expected results:

The image should load when in near the viewport.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

testcases:

• importNode: https://jsfiddle.net/d_toybox/gyLusefk/1/
• cloneNode: https://jsfiddle.net/d_toybox/gyLusefk/2/

Hiro, could you take a look?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The problem here in HTMLImageElement::SetLazyLoading;

  // If scripting is disabled don't do lazy load.                               
  // https://whatpr.org/html/3752/images.html#updating-the-image-data           
  //                                                                            
  // Same for printing.                                                         
  Document* doc = OwnerDoc();                                                   
  if (!doc->IsScriptEnabled() || doc->IsStaticDocument()) {                     
    return;                                                                     
  } 

Unfortunately at that time, when the template's innerHTML is set, the !doc->IsScriptEnabled() is true. That's the cause. We need to tweak the condition.

I'd defer it to DOM team.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for the investigation. I'll ask who can work on this.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

The problem here in HTMLImageElement::SetLazyLoading;

  // If scripting is disabled don't do lazy load.                               
  // https://whatpr.org/html/3752/images.html#updating-the-image-data           
  //                                                                            
  // Same for printing.                                                         
  Document* doc = OwnerDoc();                                                   
  if (!doc->IsScriptEnabled() || doc->IsStaticDocument()) {                     
    return;                                                                     
  } 

Unfortunately at that time, when the template's innerHTML is set, the !doc->IsScriptEnabled() is true. That's the cause. We need to tweak the condition.

I'd defer it to DOM team.

Hi Henri, do you know where we can tweak the condition for the template's innerHTML case? Thanks.

Comment 8

[Robert Longson [:longsonr]]

Attachment: Bug 1926802 - don't disable lazy loading if we're in a script blocker r=hsivonen

Comment 9

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=ba3d65dacb8388ad20a6ea19669d9af076ad6c4f so it doesn't actually work.

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

Unfortunately at that time, when the template's innerHTML is set, the !doc->IsScriptEnabled() is true. That's the cause. We need to tweak the condition.

That's because the script enabled check is done with the owner document of template contents instead of the owner document of the template itself.

Perhaps documents should have a tri-state enum for scripting state: default: compute when IsScriptEnabled() is called. force-true, make IsScriptEnabled() return true. force-false, make IsScriptEnabled() return false. Then the document holding the template contents should get this set to force-true or force-false depending on what the document that owns the template element returned when the template was created.

zcorpan, does that seem reasonable? (Though we should probably find out what Chrome does.)

Comment 11

[Simon Pieters [:zcorpan]]

Chrome and Safari lazy-loads the image in the demo in the issue description: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/13284

I think we shouldn't force scripting enabled on template contents documents. It's intentional that scripting is disabled there (because the document has no browsing context).

The spec handles this as intended, as far as I can tell. Maybe there's a bug with what happens when the img element is cloned or imported, or maybe image loading checks "will lazy load element steps" to soon?

As long as the img element's node document is the template contents document, update the image data shouldn't start (or if it starts, it will be stuck in step 1 forever). With cloneNode(), the node document is the the same as the clonee until the clone is appended to body. With importNode() the node document is set to "this" (the document in document.importNode()), which causes update the image data to run at that point.

In either case, when the update the image data step 25 calls will lazy load element steps, the img element's node document should have scripting enabled (it's not the template contents document at this point).

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Thank you Simon.

So the real underlying problem is probably here in HTMLImageElement::NodeInfoChanged;

    if (mLazyLoading) {
      aOldDoc->GetLazyLoadObserver()->Unobserve(*this);
      mLazyLoading = false;
      SetLazyLoading();
    }

The SetLazyLoading() call should be outside of the if (mLazyLoading) block, I mean, the function needs to be evaluated whatever the current mLazyLoading is, I guess.