Preserve lazy loading behavior when an image is side-loaded, take two [chromium/src : main]
Stefan Zager (Gerrit)
Stefan Zager added 4 comments![]( "Open in Gerrit")
if (new_image && new_image->IsLazyloadPossiblyDeferred()) {Stefan ZagerCan you help me understand this code? When is `IsLazyloadPossiblyDeferred()` ever true here? I ask because right above, we manually set:
```
image_request_behavior = FetchParameters::ImageRequestBehavior::kNone;
```... and pass it into `loader.Load(...)`. `StyleImageLoader::Load()` immediately delegates to `CSSImageValue::CacheImage()` which just passes the `kNone` image_request_behavior` into `CSSImageValue::PrepareFetch()`, which I guess *never* [sets the image to be lazy loaded](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_image_value.cc;l=78-81;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a)—at least I cannot follow a path where this would ever result in a lazy loaded CSS image (I've gotta be missing something, right?).
Nevertheless, I can't see how we'd enter this block. But assuming that's possible, what are we trying to do here? It seems like we're trying to say: if synchronously after we tried to fetch this lazy loaded image, it's `IsLoaded()` is true, then that means it must've been already fetched by some other means, and we just need to finish the loading from our client's perspective (i.e., call `LoadDeferredImage()`). Is that right?
Indeed, it seems that lazy load behavior for CSS images was disabled entirely by:
https://chromium-review.googlesource.com/c/chromium/src/+/3316090
I've removed my diff from this section, added a CHECK to verify it, and added a TODO to clean up this dead code.
To<StyleFetchedImage>(new_image)->LoadDeferredImage(Stefan ZagerHow do we know that `new_image` is a `StyleFetchedImage` specifically? Sorry, I'm just really not familiar with `element_style_resources` so this might be a dumb question.
Because `property == CSSPropertyID::kBackgroundImage` in the `case` statement above, which means that this condition will be `true`:
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Here is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
delay_until_image_notify_finished_ = nullptr;Stefan ZagerI wonder if we should just prevent the creation of this object in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=771-772;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a specifically for lazyload-deferred images, since I don't think they should ever block the load event, right? In fact, today lazy loaded images don't block the load event when they eventually load, so why is this actually necessary? Is the edge case for lazy loaded images that were loaded out of band before the document load event is fired—then `ImageChanged()` is called because the original `<img>` and the side-loaded one share the same underlying resource / observer chain, and then we accidentally end up creating a `delay_until_image_notify_finished_` object for these images?
Yes, I assume that's how `delay_until_image_notify_finished_` gets vivified. I don't know this code well enough to be confident that modifying `ImageLoader::ImageChanged` is the right thing to do. so I opted instead for a more localized.
Related details
- Dominic Farolino
- Mason Freed
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")
delay_until_image_notify_finished_ = nullptr;Stefan ZagerI wonder if we should just prevent the creation of this object in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=771-772;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a specifically for lazyload-deferred images, since I don't think they should ever block the load event, right? In fact, today lazy loaded images don't block the load event when they eventually load, so why is this actually necessary? Is the edge case for lazy loaded images that were loaded out of band before the document load event is fired—then `ImageChanged()` is called because the original `<img>` and the side-loaded one share the same underlying resource / observer chain, and then we accidentally end up creating a `delay_until_image_notify_finished_` object for these images?
Yes, I assume that's how `delay_until_image_notify_finished_` gets vivified. I don't know this code well enough to be confident that modifying `ImageLoader::ImageChanged` is the right thing to do. so I opted instead for a more localized.
I went ahead and made the change you suggested, to not populate `delay_until_image_notify_finished_` for a lazy-loaded image.
Related details
- Dominic Farolino
- Mason Freed
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 2 comments![]( "Open in Gerrit")
if (new_image && new_image->IsLazyloadPossiblyDeferred()) {Stefan ZagerCan you help me understand this code? When is `IsLazyloadPossiblyDeferred()` ever true here? I ask because right above, we manually set:
```
image_request_behavior = FetchParameters::ImageRequestBehavior::kNone;
```... and pass it into `loader.Load(...)`. `StyleImageLoader::Load()` immediately delegates to `CSSImageValue::CacheImage()` which just passes the `kNone` image_request_behavior` into `CSSImageValue::PrepareFetch()`, which I guess *never* [sets the image to be lazy loaded](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_image_value.cc;l=78-81;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a)—at least I cannot follow a path where this would ever result in a lazy loaded CSS image (I've gotta be missing something, right?).
Nevertheless, I can't see how we'd enter this block. But assuming that's possible, what are we trying to do here? It seems like we're trying to say: if synchronously after we tried to fetch this lazy loaded image, it's `IsLoaded()` is true, then that means it must've been already fetched by some other means, and we just need to finish the loading from our client's perspective (i.e., call `LoadDeferredImage()`). Is that right?
Indeed, it seems that lazy load behavior for CSS images was disabled entirely by:
https://chromium-review.googlesource.com/c/chromium/src/+/3316090
I've removed my diff from this section, added a CHECK to verify it, and added a TODO to clean up this dead code.
Great, thanks a lot!
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Here is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Prior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Related details
- Mason Freed
- 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")
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Dominic FarolinoHere is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Prior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Sorry, I explained that incorrectly. This code path *does* fire the load event immediately, both before and after this CL.
Because `IsLoaded()` is true, `NotifyFinished()` will be called synchronously from `DoUpdateElement()` in this call stack, via the call to `AddObserver()` ([stack](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=619)). Prior to this patch, `lazy_image_load_state_` would still be `kDeferred` at that time, but `ImageNotifyFinished()` didn't have an early return clause, so we would proceed through the rest of that method and fire the `load` event. With this patch, `lazy_image_load_state_` will be `kFullImage`, so we skip the early-return clause and fire the `load` event just as before.
The early-return clause in `ImageNotifyFinished()` handles the case where it's called while `lazy_image_load_state_` is still `kDeferred`, which is the case for a `loading="lazy"` image that is being monitored for viewport intersection. In that case, we now early-return. When the `LazyLoadImageObserver` signals that the image should be loaded, it updates `lazy_load_image_state_` [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=1015), which ensures that a subsequent call to `ImageNotifyFinished()` will not early-return.
Related details
- Dominic Farolino
- Mason Freed
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")
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Dominic FarolinoHere is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Stefan ZagerPrior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Sorry, I explained that incorrectly. This code path *does* fire the load event immediately, both before and after this CL.
Because `IsLoaded()` is true, `NotifyFinished()` will be called synchronously from `DoUpdateElement()` in this call stack, via the call to `AddObserver()` ([stack](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=619)). Prior to this patch, `lazy_image_load_state_` would still be `kDeferred` at that time, but `ImageNotifyFinished()` didn't have an early return clause, so we would proceed through the rest of that method and fire the `load` event. With this patch, `lazy_image_load_state_` will be `kFullImage`, so we skip the early-return clause and fire the `load` event just as before.
The early-return clause in `ImageNotifyFinished()` handles the case where it's called while `lazy_image_load_state_` is still `kDeferred`, which is the case for a `loading="lazy"` image that is being monitored for viewport intersection. In that case, we now early-return. When the `LazyLoadImageObserver` signals that the image should be loaded, it updates `lazy_load_image_state_` [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=1015), which ensures that a subsequent call to `ImageNotifyFinished()` will not early-return.
Here's the [call stack](https://screenshot.googleplex.com/6hzv5shQREaNW8f) for `ImageNotifyFinished()` being called when the image resource finishes loading, without going through `DoUpdateElement()`.
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
Still LGTM from my point of view. Needs resolution of the open comment though.
// TODO: This can never be true after:nit: maybe file a bug to do this cleanup?
Related details
- Dominic Farolino
- 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")
nit: maybe file a bug to do this cleanup?
Done
Related details
- Dominic Farolino
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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
if (new_image_content->IsLoaded() &&Stefan ZagerWhich caller of this method do we expect to pass in `new_image_content` that `IsLoaded()` already?
The only one that makes sense to me is the [callsite in `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=619;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a) which ordinarily will be called with an ImageResourceContent whose content status is `kPending`, not `kCached`, IIUC. My understanding is that this would only be `kCached` here (and thus `IsLoaded()`) if we were loading an image whose underlying resource was already fetched beforehand, and cached.
But that's a different case than what's described in the HTML Standard issue about an earlier-lazy-loaded `<img>` (that's currently paused) suddenly being freed up because the same resource happened to be loaded elsewhere. Can you help me understand how it relates?
Dominic FarolinoHere is the [call stack](https://screenshot.googleplex.com/CT3fMajmVPYHQV9). A possible scenario is that an `<img>` has `loading="lazy"` and subsequently its `src` attribute is changed to a URL that is in the cache. Prior to this CL, we would proceed to (incorrectly) fire the `load` event for the `<img>` immediately.
Stefan ZagerPrior to this CL, we would proceed to (incorrectly) fire the load event for the <img> immediately.
No I think that would be correct behavior. This is what I raised in https://chromium-review.googlesource.com/c/chromium/src/+/7240346/comment/2c7bedbe_3fcc964e/ (see scenario (2), and the very last sentence of that reply).
Per spec, the "list of available images" [is consulted](https://html.spec.whatwg.org/#updating-the-image-data:list-of-available-images) [*before* the `loading` attribute is considered](https://html.spec.whatwg.org/#updating-the-image-data:will-lazy-load-element-steps). Our implementation correctly does this via a [fast-path from `UpdateFromElement()` => `DoUpdateFromElement()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=684-690;drc=ec789581318d16a04973f11a08ddf2200140a3e0), IF `ShouldLoadImmediately()` is true, which contains [a check with the list of available images](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=747-751;drc=ec789581318d16a04973f11a08ddf2200140a3e0). I don't think we want to change that.
Stefan ZagerSorry, I explained that incorrectly. This code path *does* fire the load event immediately, both before and after this CL.
Because `IsLoaded()` is true, `NotifyFinished()` will be called synchronously from `DoUpdateElement()` in this call stack, via the call to `AddObserver()` ([stack](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=619)). Prior to this patch, `lazy_image_load_state_` would still be `kDeferred` at that time, but `ImageNotifyFinished()` didn't have an early return clause, so we would proceed through the rest of that method and fire the `load` event. With this patch, `lazy_image_load_state_` will be `kFullImage`, so we skip the early-return clause and fire the `load` event just as before.
The early-return clause in `ImageNotifyFinished()` handles the case where it's called while `lazy_image_load_state_` is still `kDeferred`, which is the case for a `loading="lazy"` image that is being monitored for viewport intersection. In that case, we now early-return. When the `LazyLoadImageObserver` signals that the image should be loaded, it updates `lazy_load_image_state_` [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=ec789581318d16a04973f11a08ddf2200140a3e0;l=1015), which ensures that a subsequent call to `ImageNotifyFinished()` will not early-return.
Here's the [call stack](https://screenshot.googleplex.com/6hzv5shQREaNW8f) for `ImageNotifyFinished()` being called when the image resource finishes loading, without going through `DoUpdateElement()`.
OK I think I get what you're saying, but let me make sure I fully understand. You're saying that prior to this patch, a hung lazy loaded image could have its `ImageNotifyFinished()` method called via a totally separate (side-loaded, we're calling it) image request for the same underlying resource. When that second side-loaded image request fulfills, the hung lazy loaded image's `ImageNotifyFinished()` is called due to:
```
ImageResourceContent::UpdateImage() <-- the side-loaded image
ImageResourceContent::NotifyObservers(kShouldNotifyFinish)
ImageResourceContent::HandleObserverFinished()
ImageLoader::ImageNotifyFinished() <-- Deferred ImageLoader gets "finished"
```
To restate it, the deferred ImageLoader gets notify-finished through the process of loading a non-deferred image that targets the same blink::Resource that the deferred image loader is an observer of. Before this CL, `ImageNotifyFinished()` has no early-return, so the deferred image's load event would mysteriously fire due to the side-loading. Is this correct? If so, can you please add these details to the CL description?
----
All of the above makes sense to me, and it explains the changes in `ImageNotifyFinished()`, but I'm still not totally seeing the need for `EnqueueImageLoadingMicroTask()` here. Maybe it's something like this:
When you try and lazy-load an image that's already been loaded via other means, we go through this path:
```
ImageLoader::UpdateFromElement()
ImageLoader::DoUpdateFromElement() <-- syncly, bc of `ShouldLoadImmediately()`
* Set new image content to content whose `IsLoaded()` is true
ImageLoader::UpdateImageState(new_image_content)
* This NOW queues a microtask to re-enter DoUpdateFromElement() later
ImageResourceContent::AddObserver() <-- i.e., new_image_content->AddObserver()
ImageResourceContent::HandleObserverFinished()
ImageLoader::ImageNotifyFinished()
* This now early-returns, which is why we had to re-queue the
* microtask earlier
```
Is this why we have to re-queue the image loading steps here? In anticipation of `ImageNotifyFinished()` (which gets called shortly after this method) now early returning instead of proceeding to fire the load event? Actually, this doesn't make sense to me, since the call to `ImageNotifyFinished()` that happens synchronously after `DoUpdateFromElement() -> UpdateImageState()` shouldn't hit the early-return path anyways, since `UpdateImageState()` sets `lazy_image_load_state_ = LazyImageLoadState::kFullImage`, thus avoiding the early-return path. I'm confused.
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")
That's the right mental model for this change, and I updated the commit message with a description.
After `lazy_image_load_state_` is modified, we need a call to `DoUpdateFromElement()` to fully propagate the new state. The reason for calling `EnqueueImageLoadingMicroTask()` is to avoid recursion into `DoUpdateFromElement()`, which I think would add risk and complexity.
Related details
- Dominic Farolino
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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
What does "fully propagate the new state" mean exactly though? Shouldn't the call to `AddObserver() -> ImageNotifyFinished()` that happens after `UpdateImageState()` change the status from deferred to fully loaded result in the load event being called, and the new image being loaded? If not, what happens instead? (I agree we don't want to recurse directly into `DoUpdateFromElement()`).
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")
Currently, when a lazy-loaded element enters the viewport, `lazy_image_load_state_` is updated and then `DoUpdateFromElement` is scheduled via micro-task:
It's not clear to me what the effect would be if `lazy_image_load_state_` were to transition to `kFullyLoaded` in the *middle* of an invocation of `DoUpdateFromElement()`. `lazy_image_load_state_` is referenced in various places of the `ImageLoader` code, and I don't know exactly what parts of that code are reachable from `DoUpdateFromElement()`, which enapsulates a lot of hairy logic with large fanout.
My mental model for this change is basically "do the same thing that LazyLoadImageObserver would do if the element entered the viewport." That seems like the safest thing to do.
Related details
- Dominic Farolino
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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
It's not clear to me what the effect would be if `lazy_image_load_state_` were to transition to `kFullyLoaded` in the _middle_ of an invocation of `DoUpdateFromElement()`.
That's a little scary to me, since that's exactly what L418 below does. I'm not sure that we need to re-enter the loading process (asyncly) from here, especially since we continue normally just after this method. What happens if we avoid the microtask here (delete L419) and keep L418 where we transition to kFullImage? When `DoUpdateFromElement()` calls `AddObserver()`, we'd still hit `ImageNotifyFinished()` synchronously with `DoUpdateFromElement()` on the stack, and we'd skip the early-return you added there, and we'd still queue the load event, right? Does everything pass with that change? If so, I'd be more comfortable with that.
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")
I'll try your suggestion, but to offer a counterpoint:
`UpdateFromElement()` is designed to run as an isolated task and fully update everything to a consistent state. There is no harm in running it multiple times; and in fact, it is called redundantly/unnecessarily many times during normal operation. The conventional wisdom seems to be: when in doubt, run `UpdateFromElement()` and everything will be OK. From that perspective, scheduling `UpdateFromElement()` after modifying `lazy_image_load_state_` is the conservative and safe thing to do.
It is possible that updating `lazy_image_load_state_` here could leave things in an inconsistent state. However, we may not have test coverage for such a situation. Or the inconsistent state may be hidden/camouflaged because we spam `UpdateFromElement()` so much from elsewhere in the code.
Related details
- Dominic Farolino
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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 9 comments![]( "Open in Gerrit")
steps.IIUC, This CL is to make
`lazy_image_load_state_ == LazyImageLoadState::kDeferred`
to correspond with the spec state where the image element is waiting for intersection observer at Step 25 of
https://html.spec.whatwg.org/#update-the-image-data, and fixes multiple behavior bugs (where I commented separately).
Am I understanding correctly? If so, we should:
- Update the comments at `LazyImageLoadState`,
- Update the commit message to mention the multiple behavior changes.
CHECK(!new_image || !new_image->IsLazyloadPossiblyDeferred());Just to check: Is this part (and `third_party/blink/renderer/core/style/style_image.h`) related to this change?
Should we do these separately?
(Because this change is not guarded by the feature so I suspect this is separate)
if (lazy_image_load_state_ == LazyImageLoadState::kDeferred) {Fix #4. Exempt the list-of-available-images hit cases from Fix #3.
(I comment the details separately)
Is this covered by a WPT? If not, could you add one?
I'm a little concerned with re-triggering the loading process (e.g. calling `EnqueueImageLoadingMicroTask()` here in the previous patch set) because of risk of infinite loop, so the current patch set 12 looks good to me.
Also, similar code is already around Line 410 (while Line 410 is anyway for error cases and thus I don't expect errorenous behavior at Line 410 is caught by real users though).
As far as I briefly looked at, changing `lazy_image_load_state_` to `kNone` as I suggested below (or `kFullImage` as-is in the patch set 12) is probably fine:
It's more like we first entering `LazyImageLoadState::kDeferred` and then reverting it back to `kNone`, more like as if the image has been staying at `kNone` state.
The primary (or only?) side effect of temporarily setting to `kDeferred` is calling `params.SetLazyImageDeferred()` at
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;l=559;drc=64b417264ff2f93e61931ef67b439687923c44df;bpv=1;bpt=1
but this flag should be no-op after we hit an already-loaded image on MemoryCache.
if (new_image_content->IsLoaded() &&`IsLoaded()` can't distinguish if the cache hit is due to the list-of-available-images or not (e.g. general MemoryCache hit). Is this OK? (I suppose perhals this is practically OK -- at least better than not fixing at all)
lazy_image_load_state_ = LazyImageLoadState::kFullImage;Maybe this should be
`lazy_image_load_state_ = LazyImageLoadState::kNone;`
- To align with Line 410, and
- To align with my view of `lazy_image_load_state_ == LazyImageLoadState::kDeferred` == Step 25 of the spec. When hitting the list of available images, the lazy loading logic in the spec isn't triggered at all.
I expect this doesn't impact the behavior though.
if (lazy_image_load_state_ != LazyImageLoadState::kDeferred ||Fix #1: Do not `delay the load event` by a lazy-loaded <img> while pending at Step 25.
Analysis:
Spec: `delay the load event` is executed only at Step 26, not during Step 25.
Impl: Previously, a lazy-loaded <img> delays the load event once the loading
is started BEFORE coming into the viewport (i.e. while in `LazyImageLoadState::kDeferred`).
Fix: This CL (`ImageLoader::ImageChanged()` part here) fixes this, which I feel reasonable.
Still there can be corner cases where such lazy loading images might delay the load event (not confirmed, e.g. when the underlying ImageResource is used from another Document which triggers loading start), but it would be anyway complicated hard-to-fix case.
So:
Could you add a WPT (and see how other browsers are behaving)?
Could you add a comment to explain this, including how this corresponds to the spec?
delay_until_image_notify_finished_ =Possible Fix #2. Do not `delay the load event` by a lazy-loaded <img> after unblocked from Step 25.
(Similar to but separate from Fix #1. But I haven't confirmed the actual behavior though, so I might be misunderstanding)
Analysis:
Spec: Step 24 of https://html.spec.whatwg.org/#update-the-image-data
sets `delay load event` to false, and thus even after coming into the viewport, the image shouldn't delay the load event.
Impl: Previously, a lazy-loaded <img> delays the load event once the loading
is started AFTER coming into the viewport (i.e. while in `LazyImageLoadState::kFullImage`).
A partial fix is done at
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=64b417264ff2f93e61931ef67b439687923c44df;l=564
but perhaps still delaying the load event due to `delay_until_image_notify_finished_`.
Fix: Not fixed by this CL. Changing the code here (changing to `lazy_image_load_state_ == LazyImageLoadState::kNone || ...`) would probably fix this, while I'm not super sure we should fix this altogether here.
Test: missing? not sure.
if (RuntimeEnabledFeatures::LazyImageConformantLoadEventTimingEnabled()) {Fix #3. Do not fire the <img> load event until a lazy loading image proceed to Step 26 of the spec.
Analysis:
Spec: unless the image hits the list of available images, the load event is fired only after Step 26.
Impl: The load event is fired once the underlying ImageResourceContent is loaded.
Fix: This CL (`ImageLoader::ImageNotifyFinished()` part) tries to fix this by not firing the load event.
This relies on `LazyLoadImageObserver` to eventually call `ImageLoader::LoadDeferredImage()`, restarting the image loading from `UpdateFromElement()`.
(Restarting itself has spec-conformance issues but should be orthogonal to this CL, and anyway it is how lazy image loading is implemented right now.)
Probably this should work. I was a little concerned if there are corner cases where `LazyLoadImageObserver` is not activated even if `LazyImageLoadState::kDeferred`, while I couldn't figure out actual examples.
Test: wpt/html/semantics/embedded-content/the-img-element/update-the-image-data/lazy-out-of-band-load.html
(Also, do we have a WPT to confirm the load event is fired after coming into viewport?)
However, note that this only fixes the <img> load/error event, and the image loading is still happening (e.g. updates to img's width/height is observable via JavaScript).
Probably this is intended, but it's better to explicitly state in the comment here.
e.g.
```
// Some other content may have triggered the load of this image, but that
// shouldn't fire this <img loading="lazy">'s load event at this time, because
// in the spec this <img> is still in Step 25 of
// https://html.spec.whatwg.org/#update-the-image-data.
// When LazyLoadImageObserver reports it to be intersecting (or close to) the
// viewport later (i.e. this <img> proceeds to Step 26 of the spec), actual
// load/error event will be fired, by going through the loading process again
// from `UpdateFromElement()`.
// Note that in Chromium implementation (unlike in the spec), the image content
// itself can be still loaded/updated even in this case, which can be observed
// via e.g. <img>'s width/height.
```
Related details
- Dominic Farolino
- 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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
Basically looks good, while more tests/clarification might be needed.
Stefan Zager (Gerrit)
Stefan Zager added 9 comments![]( "Open in Gerrit")
IIUC, This CL is to make
`lazy_image_load_state_ == LazyImageLoadState::kDeferred`
to correspond with the spec state where the image element is waiting for intersection observer at Step 25 of
https://html.spec.whatwg.org/#update-the-image-data, and fixes multiple behavior bugs (where I commented separately).Am I understanding correctly? If so, we should:
- Update the comments at `LazyImageLoadState`,
- Update the commit message to mention the multiple behavior changes.
The purpose of this CL is to enact the resolution of this spec issue:
https://github.com/whatwg/html/issues/10671
It is not my intention to do anything beyond that. If this CL fixes other incorrect behaviors, that's a bonus.
CHECK(!new_image || !new_image->IsLazyloadPossiblyDeferred());Just to check: Is this part (and `third_party/blink/renderer/core/style/style_image.h`) related to this change?
Should we do these separately?(Because this change is not guarded by the feature so I suspect this is separate)
domfarolino@ commented earlier that `IsLazyLoadPossibleDeferred()` can never be `true` here. I agree with that assessment, so I added the `CHECK` to enforce it. The next CL in the dependency chain removes the method `IsLazyLoadPossiblyDeferred()` entirely along with this `CHECK`.
if (lazy_image_load_state_ == LazyImageLoadState::kDeferred) {Fix #4. Exempt the list-of-available-images hit cases from Fix #3.
(I comment the details separately)
Is this covered by a WPT? If not, could you add one?
This may be true, but it's beyond the scope of what I'm trying to do here, and it's not clear to me how to write such a WPT.
I don't understand the "infinite loop" concern. This code block is only reached when `lazy_image_load_state_==kDeferred`, and once `lazy_image_load_state_` transitions away from `kDeferred` it can never return to that value unless the element state changes in a way that triggers a new fetch and gets a new `ImageResourceContent`.
To reiterate my prior points:
- `UpdateFromElement()` is already called redundantly *many* times during normal operation.
- `EnqueueImageLoadingMicroTask()` is *already* the code path used by `LazyLoadImageObserver()` to trigger a fetch after modifying `lazy_image_load_state_`.
PS12, which removes the call to `EnqueueImageLoadingMicroTask()`, causes a browser test failure. For all these reasons, I would argue that `EnqueueImageLoadingMicroTask()` is the *safe and conservative thing to do.* I've restored the call in PS13, but changed the `behavior` argument to `kUpdateNormal`, which should have the same behavior as `kUpdateFromMicrotask` here but hopefully will alleviate your concern about infinite looping.
`IsLoaded()` can't distinguish if the cache hit is due to the list-of-available-images or not (e.g. general MemoryCache hit). Is this OK? (I suppose perhals this is practically OK -- at least better than not fixing at all)
Acknowledged
lazy_image_load_state_ = LazyImageLoadState::kFullImage;Maybe this should be
`lazy_image_load_state_ = LazyImageLoadState::kNone;`
- To align with Line 410, and
- To align with my view of `lazy_image_load_state_ == LazyImageLoadState::kDeferred` == Step 25 of the spec. When hitting the list of available images, the lazy loading logic in the spec isn't triggered at all.
I expect this doesn't impact the behavior though.
Done
if (lazy_image_load_state_ != LazyImageLoadState::kDeferred ||Fix #1: Do not `delay the load event` by a lazy-loaded <img> while pending at Step 25.
Analysis:
Spec: `delay the load event` is executed only at Step 26, not during Step 25.
Impl: Previously, a lazy-loaded <img> delays the load event once the loading
is started BEFORE coming into the viewport (i.e. while in `LazyImageLoadState::kDeferred`).
Fix: This CL (`ImageLoader::ImageChanged()` part here) fixes this, which I feel reasonable.
Still there can be corner cases where such lazy loading images might delay the load event (not confirmed, e.g. when the underlying ImageResource is used from another Document which triggers loading start), but it would be anyway complicated hard-to-fix case.So:
Could you add a WPT (and see how other browsers are behaving)?
Could you add a comment to explain this, including how this corresponds to the spec?
I'm not sure how to write such a test, and it's beyond the scope of what I'm trying to do here. If this CL fixes the narrow issue described in the linked bug and doesn't regress any existing tests, that's all I'm trying to do.
delay_until_image_notify_finished_ =Possible Fix #2. Do not `delay the load event` by a lazy-loaded <img> after unblocked from Step 25.
(Similar to but separate from Fix #1. But I haven't confirmed the actual behavior though, so I might be misunderstanding)
Analysis:
Spec: Step 24 of https://html.spec.whatwg.org/#update-the-image-data
sets `delay load event` to false, and thus even after coming into the viewport, the image shouldn't delay the load event.
Impl: Previously, a lazy-loaded <img> delays the load event once the loading
is started AFTER coming into the viewport (i.e. while in `LazyImageLoadState::kFullImage`).
A partial fix is done at
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=64b417264ff2f93e61931ef67b439687923c44df;l=564
but perhaps still delaying the load event due to `delay_until_image_notify_finished_`.Fix: Not fixed by this CL. Changing the code here (changing to `lazy_image_load_state_ == LazyImageLoadState::kNone || ...`) would probably fix this, while I'm not super sure we should fix this altogether here.
Test: missing? not sure.
This is also beyond the scope of what I'm trying to do here.
if (RuntimeEnabledFeatures::LazyImageConformantLoadEventTimingEnabled()) {Fix #3. Do not fire the <img> load event until a lazy loading image proceed to Step 26 of the spec.
Analysis:
Spec: unless the image hits the list of available images, the load event is fired only after Step 26.
Impl: The load event is fired once the underlying ImageResourceContent is loaded.
Fix: This CL (`ImageLoader::ImageNotifyFinished()` part) tries to fix this by not firing the load event.
This relies on `LazyLoadImageObserver` to eventually call `ImageLoader::LoadDeferredImage()`, restarting the image loading from `UpdateFromElement()`.
(Restarting itself has spec-conformance issues but should be orthogonal to this CL, and anyway it is how lazy image loading is implemented right now.)Probably this should work. I was a little concerned if there are corner cases where `LazyLoadImageObserver` is not activated even if `LazyImageLoadState::kDeferred`, while I couldn't figure out actual examples.
Test: wpt/html/semantics/embedded-content/the-img-element/update-the-image-data/lazy-out-of-band-load.html
(Also, do we have a WPT to confirm the load event is fired after coming into viewport?)However, note that this only fixes the <img> load/error event, and the image loading is still happening (e.g. updates to img's width/height is observable via JavaScript).
Probably this is intended, but it's better to explicitly state in the comment here.e.g.
```
// Some other content may have triggered the load of this image, but that
// shouldn't fire this <img loading="lazy">'s load event at this time, because
// in the spec this <img> is still in Step 25 of
// https://html.spec.whatwg.org/#update-the-image-data.
// When LazyLoadImageObserver reports it to be intersecting (or close to) the
// viewport later (i.e. this <img> proceeds to Step 26 of the spec), actual
// load/error event will be fired, by going through the loading process again
// from `UpdateFromElement()`.
// Note that in Chromium implementation (unlike in the spec), the image content
// itself can be still loaded/updated even in this case, which can be observed
// via e.g. <img>'s width/height.
```
I've added your code comment here to the patch. Not sure about the test coverage issue.
Related details
- Dominic Farolino
- Hiroshige Hayashizaki
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
Failed tests on PS13 show that setting the state to `kNone` rather than `kFullImage` here doesn't work. Note that `LazyLoadImageObserver` changes the value to `kFullImage` as soon as the fetch is initiated, so using `kFullImage` here smells correct to me.
Related details
- Dominic Farolino
- Hiroshige Hayashizaki
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stefan Zager
| Commit-Queue | +1 |
1 comment
PTAL. The latest patchset is essentially the same as PS9. The following suggestions from review comments were tried but abandoned due to test failures:
- Remove call to `EnqueueImageLoadingMicroTask()`
- Set `lazy_load_image_state_` to `kNone` rather than `kFullImage`
The one actual change is that the argument to `EnqueueImageLoadingMicroTask()` is now `kUpdateNormal` rather than `kUpdateFromMicroTask`. This has no functional effect but hopefully alleviates concerns about a potential infinite loop.
Related details
- Dominic Farolino
- Hiroshige Hayashizaki
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 5 comments![]( "Open in Gerrit")
This one has marinated long enough; let's land it and move on.
if (lazy_image_load_state_ == LazyImageLoadState::kDeferred) {Stefan ZagerFix #4. Exempt the list-of-available-images hit cases from Fix #3.
(I comment the details separately)
Is this covered by a WPT? If not, could you add one?
This may be true, but it's beyond the scope of what I'm trying to do here, and it's not clear to me how to write such a WPT.
Marked as resolved.
if (new_image_content->IsLoaded() &&Marked as resolved.
if (lazy_image_load_state_ != LazyImageLoadState::kDeferred ||Stefan ZagerFix #1: Do not `delay the load event` by a lazy-loaded <img> while pending at Step 25.
Analysis:
Spec: `delay the load event` is executed only at Step 26, not during Step 25.
Impl: Previously, a lazy-loaded <img> delays the load event once the loading
is started BEFORE coming into the viewport (i.e. while in `LazyImageLoadState::kDeferred`).
Fix: This CL (`ImageLoader::ImageChanged()` part here) fixes this, which I feel reasonable.
Still there can be corner cases where such lazy loading images might delay the load event (not confirmed, e.g. when the underlying ImageResource is used from another Document which triggers loading start), but it would be anyway complicated hard-to-fix case.So:
Could you add a WPT (and see how other browsers are behaving)?
Could you add a comment to explain this, including how this corresponds to the spec?
I'm not sure how to write such a test, and it's beyond the scope of what I'm trying to do here. If this CL fixes the narrow issue described in the linked bug and doesn't regress any existing tests, that's all I'm trying to do.
Marked as resolved.
delay_until_image_notify_finished_ =Stefan ZagerPossible Fix #2. Do not `delay the load event` by a lazy-loaded <img> after unblocked from Step 25.
(Similar to but separate from Fix #1. But I haven't confirmed the actual behavior though, so I might be misunderstanding)
Analysis:
Spec: Step 24 of https://html.spec.whatwg.org/#update-the-image-data
sets `delay load event` to false, and thus even after coming into the viewport, the image shouldn't delay the load event.
Impl: Previously, a lazy-loaded <img> delays the load event once the loading
is started AFTER coming into the viewport (i.e. while in `LazyImageLoadState::kFullImage`).
A partial fix is done at
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/image_loader.cc;drc=64b417264ff2f93e61931ef67b439687923c44df;l=564
but perhaps still delaying the load event due to `delay_until_image_notify_finished_`.Fix: Not fixed by this CL. Changing the code here (changing to `lazy_image_load_state_ == LazyImageLoadState::kNone || ...`) would probably fix this, while I'm not super sure we should fix this altogether here.
Test: missing? not sure.
This is also beyond the scope of what I'm trying to do here.
Marked as resolved.
Related details
- Dominic Farolino
- Hiroshige Hayashizaki
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 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
8 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/style/style_image.h
Insertions: 1, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/loader/image_loader.cc
Insertions: 11, Deletions: 4.
The diff is too large to show. Please review the diff.
```
Change information
Preserve lazy loading behavior when an image is side-loaded, take two
Re-landing after revert:
https://chromium-review.googlesource.com/c/chromium/src/+/7240346
Patchset 1 is the previously-landed change, patchset 2 contains the fix
for crbug.com/469002488. Root cause: an <img> was loading="lazy" and
outside the viewport; then the underlying resource was loaded anyway
(due to being fetched by other means); and the lazy <img> did not fire
its load event but it blocked the document from being marked as fully
loaded.
When two elements use the same underlying resource, each element gets
its own ImageLoader, but they point to the same underlying
ImageResourceContent. Information about the loading lifecycle is
propagated from ImageResourceContent to ImageLoader(s) via the
ImageResourceObserver interface, which ImageLoader implements. Thus,
ImageNotifyFinished will be propagated to all observing elements,
regardless of whether they are loading="lazy" or not. This CL causes
ImageLoader to recognize when this happens and early-return rather than
running the "image loaded" steps. Later, when the element enters the
viewport and the lazy-loading machinery requests the image,
ResourceFetcher will get a cache hit and trigger ImageNotifyFinished
again; but this time the element will proceed with the "image loaded"
steps.
- 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/core/style/style_image.h
- 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 Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |