Open Bug 2016233 Opened 4 months ago Updated 2 months ago

An invisible `<img loading=lazy>` element can get `.complete=true`if the resource is loaded elsewhere on Chrome but not on Firefox

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

People

(Reporter: saschanaz, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: parity-chrome, webcompat:platform-bug)

User Story

user-impact-score:800

Attachments

(3 files)

2014480-2.html
2.91 KB, text/html
Details
example.html
625 bytes, text/html
Details
test.html
1000 bytes, text/html
Details
Attached file 2014480-2.html —
  1. Open the attached page
  2. Check the output in the devtools console

On Chrome: Initially the invisible element has .complete being false, but after getting an empty src value it gets true.
On Firefox: It stays false.

On Safari it also stays false.

Do you know what the spec says here?

User Story: (updated)

https://html.spec.whatwg.org/#when-to-obtain-images:relevant-mutations

When obtaining images on demand, the user agent must update the image data of an img element whenever it needs the image data (i.e., on demand), but only if the img element's current request's state is unavailable. When an img element has experienced relevant mutations, if the user agent only obtains images on demand, the img element's current request's state must return to unavailable.

"relevant mutations" includes "src" being changed per the next session. Meaning, .src = "" should set the state as unavailable. But it shouldn't run "update the image data" as a hidden image element has no need to obtain the image.

I think we should add WPT here.

I think "obtaining images on demand" is a distinct concept from loading="lazy". Step 25 of https://html.spec.whatwg.org/multipage/images.html#updating-the-image-data is "If the will lazy load element steps given the img return true, then:" which specifically talks about loading="lazy".

But I think the conclusion is similar. When we run updating-the-image-data the first time for img we stop at step 25 before fetching the image request, so it stays in unavailable state. Then we run updating-the-image-data again after the img.src="" and we do basically the same thing. Note step 7 of updating-the-image-data means we skip checking the list of available images in the srcset case, so the fact we are using srcset changes the expected outcome.

I really have now idea how bad this is. Can you assign a severity Kagami?

Flags: needinfo?(krosylight)

Copying the bits from the site report. The site report also has webcompat priority P1.

But the question is, is this bug in the right component? Wonder what tnikkel thinks.

Severity: -- → S2
Flags: needinfo?(krosylight) → needinfo?(tnikkel)
Priority: -- → P1

The code that would be changed lives in dom/ and previous bugs in this area lived in DOM: Core & HTML, so the component seems fine to me, although I'm not too picky on where it lives.

Flags: needinfo?(tnikkel)

Hi Leo, mind taking this bug? Thanks!
Edgar thought that .complete has other inconsistency between browsers and also spec, but he suggested we could just deal with that specific case in this bug.

Flags: needinfo?(ltenenbaum)
Attached file example.html —

It looks like there is some kind of inconsistency between src and srcset.
In the attached example #img gets complete = false but changing it to use src instead of srcset makes it true.
HTMLImageElement::Complete is returning false because mCurrentRequest is null … I think there should be a request at that point based on step 17 of updating the image data (and I do think we are supposed update-the-image-data even if the element is invisible and lazy-loading based on this).
I'll try to see if I can fix this

Assignee: nobody → ltenenbaum
Flags: needinfo?(ltenenbaum)

Hmm actually I think our behavior is correct here.
In this case, the image request's state has to be "completely available" for complete to be true (https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-complete)
But it is only set to completely available in update the image data steps 7.4.4 (which only applies if src is set to a non-empty value) and 27 (which only happens after the lazy-loading image comes into view).
Probably best to add a WPT for this and file a Chromium bug then…

Chrome's behavior seems to also be spec-compliant… The spec allows the UA to react to changes in the environment at any time, which bypasses lazy loading. I suspect this might be what Chrome is doing based on this which is called here. Probably the spec should just be changed to respect lazy loading in those steps. There is an issue about it here: https://github.com/whatwg/html/issues/7255

(In reply to Leo Tenenbaum [:ltenenbaum] from comment #9)

It looks like there is some kind of inconsistency between src and srcset.

From comment 4, the spec is different in the srcset case, it seems like it could be intentional as I could see the logic in doing it that way.

(In reply to Leo Tenenbaum [:ltenenbaum] from comment #11)

Chrome's behavior seems to also be spec-compliant… The spec allows the UA to react to changes in the environment at any time, which bypasses lazy loading. I suspect this might be what Chrome is doing based on this which is called here. Probably the spec should just be changed to respect lazy loading in those steps. There is an issue about it here: https://github.com/whatwg/html/issues/7255

Bypassing the loading attribute via reacting to environment changes seems to be a spec bug as you've linked there, so basing an implementations behaviour on that part of the spec seems like a bad idea.

But does that even apply here? There are no environment changes happening? The src is being changed. The place you linked to SelectSourceURL being called is when the src, srcset, or sizes attribute changes, which are relevant mutations and trigger a normal update of the image data, so running the "react to environment changes" in that case would be wrong.

Hmm I think Chrome has essentially merged "update the image data" and "react to changes in the environment" in their code which was confusing me. (e.g. SelectSourceURL is also called when the viewport is changed: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_image_element.cc;l=139;drc=71e98710db31f5ee4c666eccd09fe14ff27514e7)
Even though the spec doesn't say what counts as a change in the environment, I think it's reasonable to assume that just changing srcset doesn't count (since it should trigger update the image data anyways and it would be weird to do both). So probably it is okay to add a WPT for this.

Attached file test.html —

This is another reduced test page. On Chrome, not only the .complete returns true, it also loads the lazy-load-image eagerly.

When src is changed (in this case it is changed to empty string), the update the image data steps run synchronously per spec. And spec doesn't mention the lazy load img element should load the image.

On Chrome, if the selected source is already in the image cache, the lazy load image loads the cached image eagerly. It looks like a Chrome bug to me, similar to https://github.com/whatwg/html/issues/10671, but using a different way to confuse it.

On Chrome, if the selected source is already in the image cache, the lazy load image loads the cached image eagerly. It looks like a Chrome bug to me, similar to https://github.com/whatwg/html/issues/10671, but using a different way to confuse it.

I'm on the phone so I haven't dug too much, but we also have similar code here: https://searchfox.org/firefox-main/rev/4bc9c2f9b62e7cb44894d581c8171edbf0f7e27f/dom/base/nsImageLoadingContent.cpp#194

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)

On Chrome, if the selected source is already in the image cache, the lazy load image loads the cached image eagerly. It looks like a Chrome bug to me, similar to https://github.com/whatwg/html/issues/10671, but using a different way to confuse it.

I'm on the phone so I haven't dug too much, but we also have similar code here: https://searchfox.org/firefox-main/rev/4bc9c2f9b62e7cb44894d581c8171edbf0f7e27f/dom/base/nsImageLoadingContent.cpp#194

Yes checking the list of available images (and setting complete/firing load events synchronously if found) is part of the spec (step 7.4 of update the image data). Although aSrcURI is null if only srcset is set, so it gets skipped (which is what the spec says). I think the chrome bug is unrelated though because it looks like they also only check src for available images (image_source_kurl here is derived just from the src attribute).

Priority: P1 → --
Assignee: ltenenbaum → nobody

Not Webcompat:Core (Core Countries) P1, lowering the severity.

Severity: S2 → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: