Preserve lazy loading behavior when an image is side-loaded [chromium/src : main]
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
pdr, can you review this? dom@ has absconded.
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 3 comments![]( "Open in Gerrit")
Can you add a few lines about the high-level change?
Safari has the same behavior as us, so there is a chance that sites are relying on our current behavior. Can you put this behind an enabled-by-default runtime enabled flag just in case?
EnqueueImageLoadingMicroTask(kUpdateFromMicrotask);I think we still need to call stop monitoring here and in ElementStyleResources::LoadPendingImages for the following scenario:
1. pre-load cached image a.png
2. new image element with lazy and src=b.png and offscreen, starts monitoring
3. switch new image element src to a.png
4. no call to stop monitoring.
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 3 comments![]( "Open in Gerrit")
Safari has the same behavior as us, so there is a chance that sites are relying on our current behavior. Can you put this behind an enabled-by-default runtime enabled flag just in case?
Done
Can you add a few lines about the high-level change?
Done
I think we still need to call stop monitoring here and in ElementStyleResources::LoadPendingImages for the following scenario:
1. pre-load cached image a.png
2. new image element with lazy and src=b.png and offscreen, starts monitoring
3. switch new image element src to a.png
4. no call to stop monitoring.
I didn't remove a call to `StopMonitoring()` here or in ElementStyleResources. I think you are referring to the call I removed below in `ImageLoader::ImageNotifyFinished()`. That one needs to be removed, to ensure the `load` event is properly triggered when the image enters the viewport.
If you think there should be a call to `StopMonitoring()` here and/or in ElementStyleResources, then I think there should be a separate bug and separate CL.
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
EnqueueImageLoadingMicroTask(kUpdateFromMicrotask);Stefan ZagerI think we still need to call stop monitoring here and in ElementStyleResources::LoadPendingImages for the following scenario:
1. pre-load cached image a.png
2. new image element with lazy and src=b.png and offscreen, starts monitoring
3. switch new image element src to a.png
4. no call to stop monitoring.
I didn't remove a call to `StopMonitoring()` here or in ElementStyleResources. I think you are referring to the call I removed below in `ImageLoader::ImageNotifyFinished()`. That one needs to be removed, to ensure the `load` event is properly triggered when the image enters the viewport.
If you think there should be a call to `StopMonitoring()` here and/or in ElementStyleResources, then I think there should be a separate bug and separate CL.
Oh, I think I see what you mean. I'll add the calls to `StopMonitoring()`, can't hurt anyways.
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
EnqueueImageLoadingMicroTask(kUpdateFromMicrotask);Stefan ZagerI think we still need to call stop monitoring here and in ElementStyleResources::LoadPendingImages for the following scenario:
1. pre-load cached image a.png
2. new image element with lazy and src=b.png and offscreen, starts monitoring
3. switch new image element src to a.png
4. no call to stop monitoring.
Stefan ZagerI didn't remove a call to `StopMonitoring()` here or in ElementStyleResources. I think you are referring to the call I removed below in `ImageLoader::ImageNotifyFinished()`. That one needs to be removed, to ensure the `load` event is properly triggered when the image enters the viewport.
If you think there should be a call to `StopMonitoring()` here and/or in ElementStyleResources, then I think there should be a separate bug and separate CL.
Oh, I think I see what you mean. I'll add the calls to `StopMonitoring()`, can't hurt anyways.
Done
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
EnqueueImageLoadingMicroTask(kUpdateFromMicrotask);Stefan ZagerI think we still need to call stop monitoring here and in ElementStyleResources::LoadPendingImages for the following scenario:
1. pre-load cached image a.png
2. new image element with lazy and src=b.png and offscreen, starts monitoring
3. switch new image element src to a.png
4. no call to stop monitoring.
Stefan ZagerI didn't remove a call to `StopMonitoring()` here or in ElementStyleResources. I think you are referring to the call I removed below in `ImageLoader::ImageNotifyFinished()`. That one needs to be removed, to ensure the `load` event is properly triggered when the image enters the viewport.
If you think there should be a call to `StopMonitoring()` here and/or in ElementStyleResources, then I think there should be a separate bug and separate CL.
Stefan ZagerOh, I think I see what you mean. I'll add the calls to `StopMonitoring()`, can't hurt anyways.
Done
Thanks! LGTM with the fix you applied in your latest patchset (it fixes a local test I have).
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
LazyImageHelper::StopMonitoring(GetElement());```suggestion
LazyImageHelper::StopMonitoring(&element_);
```
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
LazyImageHelper::StopMonitoring(GetElement());Stefan Zager```suggestion
LazyImageHelper::StopMonitoring(&element_);
```
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/css/resolver/element_style_resources.cc
Insertions: 1, Deletions: 1.
@@ -436,7 +436,7 @@
if (new_image->IsLoaded() &&
RuntimeEnabledFeatures::
LazyImageConformantLoadEventTimingEnabled()) {
- LazyImageHelper::StopMonitoring(GetElement());
+ LazyImageHelper::StopMonitoring(&element_);
To<StyleFetchedImage>(new_image)->LoadDeferredImage(
element_.GetDocument());
} else {
```
Change information
Preserve lazy loading behavior when an image is side-loaded
According to the spec, if the resource for an <img loading="lazy"> is
loaded via other means (e.g. fetch()), then the <img> should not
fire its load event until it enters the viewport.
Putting this behind a flag in case sites are relying on the legacy
broken behavior.
- M third_party/blink/renderer/core/css/resolver/element_style_resources.cc
- M third_party/blink/renderer/core/loader/image_loader.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- D third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-img-element/update-the-image-data/lazy-out-of-band-load-expected.txt
Code-Review: +1 by Philip Rogers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 2 comments![]( "Open in Gerrit")
I realize this is landed. But after OOO I saw a message about this from Stephan decided to take a look.
According to the spec, if the resource for an <img loading="lazy"> isWhere in the spec? I believe the document's list of available images is consulted before lazy-load-ness is evaluated, in https://html.spec.whatwg.org/#updating-the-image-data.
Either way, I think the set-up described here is ambiguous. I can think of two different things that it could mean:
1. First create a loading=lazy image that's out-of-viewport, and while it's paused, fetch the same resource via `fetch()` or some other means, and observe that the loading=lazy img element does NOT get its load event fired;
2. First load some resource via `fetch()` or an eager img tag. Sometime later, create a loading=lazy image and note that it does not load immediately, despite its resource being in the document's list of available images cache.
I was hoping looking at the test would clear up which scenario we were referring to, but [the test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-img-element/update-the-image-data/lazy-out-of-band-load.html) appears to be testing NEITHER of these scenarios. Am I correct? It looks like it's trying to test (2), but I'm not sure it's doing it right.
(I think the inverse of (2) above should happen, per spec).
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager has created a revert of this change![]( "Open in Gerrit")
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
According to the spec, if the resource for an <img loading="lazy"> isWhere in the spec? I believe the document's list of available images is consulted before lazy-load-ness is evaluated, in https://html.spec.whatwg.org/#updating-the-image-data.
Either way, I think the set-up described here is ambiguous. I can think of two different things that it could mean:
1. First create a loading=lazy image that's out-of-viewport, and while it's paused, fetch the same resource via `fetch()` or some other means, and observe that the loading=lazy img element does NOT get its load event fired;
2. First load some resource via `fetch()` or an eager img tag. Sometime later, create a loading=lazy image and note that it does not load immediately, despite its resource being in the document's list of available images cache.I was hoping looking at the test would clear up which scenario we were referring to, but [the test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-img-element/update-the-image-data/lazy-out-of-band-load.html) appears to be testing NEITHER of these scenarios. Am I correct? It looks like it's trying to test (2), but I'm not sure it's doing it right.
(I think the inverse of (2) above should happen, per spec).
The test is testing (1). The `<img>` under test is not out of viewport but it's `display:none`, which should have the same behavior.
As for where the spec addresses this specifically: I don't know, I am relying on the conversation on the spec issue that motivated this CL:
https://github.com/whatwg/html/issues/10671
Do you think that this CL doesn't correctly implement the behavior described there? Or do you think the discussion on the spec issue misinterprets the spec?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
According to the spec, if the resource for an <img loading="lazy"> isStefan ZagerWhere in the spec? I believe the document's list of available images is consulted before lazy-load-ness is evaluated, in https://html.spec.whatwg.org/#updating-the-image-data.
Either way, I think the set-up described here is ambiguous. I can think of two different things that it could mean:
1. First create a loading=lazy image that's out-of-viewport, and while it's paused, fetch the same resource via `fetch()` or some other means, and observe that the loading=lazy img element does NOT get its load event fired;
2. First load some resource via `fetch()` or an eager img tag. Sometime later, create a loading=lazy image and note that it does not load immediately, despite its resource being in the document's list of available images cache.I was hoping looking at the test would clear up which scenario we were referring to, but [the test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-img-element/update-the-image-data/lazy-out-of-band-load.html) appears to be testing NEITHER of these scenarios. Am I correct? It looks like it's trying to test (2), but I'm not sure it's doing it right.
(I think the inverse of (2) above should happen, per spec).
The test is testing (1). The `<img>` under test is not out of viewport but it's `display:none`, which should have the same behavior.
As for where the spec addresses this specifically: I don't know, I am relying on the conversation on the spec issue that motivated this CL:
https://github.com/whatwg/html/issues/10671
Do you think that this CL doesn't correctly implement the behavior described there? Or do you think the discussion on the spec issue misinterprets the spec?
As for where the spec addresses this specifically: I don't know, I am relying on the conversation on the spec issue that motivated this CL:
> https://github.com/whatwg/html/issues/10671
I see, I didn't realize this CL was aimed at fixing Chromium's implementation of that HTML Standard bug. Based on the CL description I was unclear if this was trying to solve (1) or (2) above, thanks, and sorry!
Looking closer, I do think the test correctly tests that scenario—that a pending lazyload image does not suddenly free up and eagerly load a resource that was side-loaded. It looked funny to me since the test reuses the same variable for both imgs, but it's fine.
Do you think that this CL doesn't correctly implement the behavior described there?
This CL tries to get the same observable results as the spec, but the fundamental problem is described in https://github.com/whatwg/html/issues/10671#issuecomment-2450198052, and this CL doesn't "fix" it, but it tries to bypass the bad side-effects of it. What I'm saying is that for `loading=lazy` images, the spec stops the image loading pipeline super early, before it ever reaches the spec-equivalent of blink::ResourceFetcher. Blink has the fundamental problem of pausing/deferring a `loading=lazy` image quite late, in the blink::ResourceFetcher, after the "empty" blink::Resource representing the image has already been registered with the loading infra.
This is bad because then when an image is side-loaded by a different blink::Resource that happens to "match" the lazy-loaded one and get coalesced into the same one, all registered clients for coalesced blink::Resource get to share the freshly-loaded resource.
I can envision two solutions to this:
1. Heavy: refactor the lazy loading infra to defer an image way before blink::ResourceFetcher, so that it's not registered as a blink::Resource and cannot be coalesced into other side-loaded out-of-band resources for the same image.
2. Light: Modify the [`Resource::CanReuse()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource.cc;l=793;drc=cf35b90f85d2dc2bfbef0b6950207a8cd81f66d7) logic to somehow discriminate between lazyloaded resources and non-lazy-loaded resources. Maybe we want access to [`image_request_behavior_`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h;l=181-183;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f). This might already be possible based on the information on blink::Resource , but if not maybe we'll need add a bit somewhere.
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |