Bug 237703 – Native image Lazyloading sometimes does not load

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Mads Erik Forberg]

Tested on latest version of iOS 15.4 (19E241).

Steps to reproduce:
1. Goto https://www.aftenposten.no via Safari on iOS.
2. Scroll down somewhat «fast» or try a couple of refresh.
3. You’ll see empty image containers. Screenshot here: https://i.imgur.com/BGQ4dB6.png

If I turn off «Lazy image loading» it works as expected: https://i.imgur.com/zS1VaL3.png

Device info:
iPhone Xs
MT9H2QN/A
iOS 15.4 (19E241)

[Radar WebKit Bug Importer]

<rdar://problem/90106761>

[Mads Erik Forberg]

So I did some digging during the weekend, and I found that the issue. 

If a <img /> has a placeholder image in the `src` or `srcset` (small gif or smth), and it is replaced by the actual image to load, the new image doesn't seem to load. We use placeholder images to prevent "broken images" during loading and for legacy support for browsers that doesn't support native lazyloading.

[Martin Schön]

I can confirm that there is a connection between having an initial placeholder value for src and images not appearing. On our site https://www.spiegel.de, we use data URIs with encoded SVGs as placeholder `src` values for not-yet-loaded images. Once we remove either the loading="lazy" attribute or the default src, images appear correctly again.

We noted that the issue only occurs, if the images are not in the browser's cache.

Also, we use srcset/sizes to load different image variants per screen width. When the window with the loaded page is resized so the viewport matches a different variant, the image appears.

The issue occurs on Safari for Mac 15.4 (Mac OS 12.0.1) and on Safari Technology Previews 143 and higher (all STP versions that include native image lazyloading).

[karl]

There is a very similar issue which has been reported as a webcompat bug on webcompat.com.
https://github.com/webcompat/web-bugs/issues/101475

The JavaScript version of lazy loading on The Telegraph website seems to compete with the native lazy loading version. If I deactivate the native lazyload in Safari Release 143 (Safari 15.4, WebKit 17614.1.7.7), then the images are being displayed. If I reactivate, the issue reproduces. 

The initial reporter had all images not loading. I had only the first columns. A resize of the window creates an opportunity for loading the missing images.

[karl]

It would probably be worth to check on all the reported sites if the mechanism for lazy load is similar. 

aftenposten.no, spiegel.de and telegraph.co.uk are all press websites.

[Karl Dubost]

Hello,

Does it reproduce for you on the latest version of Safari Tech Preview on macOS and/or the latest version of Safari 15.5 on iOS?

Mads Erik for aftenposten?
Martin Schön for Spiegel?

I can't reproduce on the telegraph anymore.

[Martin Schön]

@Karl Dubost Yes, the issue still reproduces in Safari Tech Preview 147.

Note that we have disabled native lazyloading in production, so the issue is not reproducible on www.spiegel.de at the moment.

I have created this test page with native lazyloading turned on again: http://spiegel-safari-image-lazyloading.surge.sh – make sure to empty caches (Develop > Empty Caches) and then reload the page to reproduce the issue.

[Karl Dubost]

Thanks a lot Martin,

today on 20ish attempts, I was able to reproduce twice only with 
Release 147 (Safari 16.0, WebKit 18614.1.14.10.16)

http://spiegel-safari-image-lazyloading.surge.sh/

when it does reproduce, none of the fallback or the srcset images are loading. 

```
<picture>
  <source
    data-srcset="
      https://picsum.photos/id/469/872/490.webp 872w,       
      https://picsum.photos/id/469/699/393.webp 699w, 
      https://picsum.photos/id/469/520/292.webp 520w"
    data-sizes="
      (max-width: 519px) 100vw, 
      (min-width: 520px) and (max-width: 719px) 520px, 
      (min-width: 720px) and (max-width: 871px) 100vw, 
      (min-width: 872px) and (max-width: 1011px) 872px, 
      (min-width: 1012px) 699px"
    sizes="
      (max-width: 519px) 100vw, 
      (min-width: 520px) and (max-width: 719px) 520px, 
      (min-width: 720px) and (max-width: 871px) 100vw, 
      (min-width: 872px) and (max-width: 1011px) 872px, 
      (min-width: 1012px) 699px"
    srcset="
      https://picsum.photos/id/469/872/490.webp 872w,
      https://picsum.photos/id/469/699/393.webp 699w,
      https://picsum.photos/id/469/520/292.webp 520w
    "
  />
  <img
    data-image-el="img"
    class="lazyload rounded loaded"
    loading="lazy"
    src="https://picsum.photos/id/469/872/490.jpg"
    data-src="https://picsum.photos/id/469/872/490.jpg"
    width="872"
    height="490"
    title="»Durch die Erkrankung ist mein Herz geschrumpft« - Robert Michael / picture alliance / dpa"
    data-ll-status="loaded"
  />
</picture>

```

if I scroll to the bottom the lazy loaded icons are loading, but still not the second picture.


Martin, The site uses both a JS library and the native lazy loading mechanism. When removing the library can you reproduce the issue? (asking because you seem to be able to reproduce more systematically than me.)

[Martin Schön]

Hi Karl! Thanks for checking, your observations match mine. When the bug occurs, it is usually the first n images that don't appear – for me round about 10 images remain blank. Images further down the page do load and show properly.

When I remove the JS Library, I cannot reproduce the issue. However, this requires me to remove the placeholder SVG from the src attribute as well. So this:

```
<img 
    ...
    loading="lazy"
    src="data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 872 490' width='872' height='490' %3E%3C/svg%3E"
    data-src="https://picsum.photos/id/469/872/490.jpg"
    ...
/>
```

becomes this:

```
<img 
    ...
    loading="lazy"
    src="https://picsum.photos/id/469/872/490.jpg"
    ...
/>
```

The placeholder is important though. We use it to prevent the "broken image" icon from appearing and to reserve space for the image to avoid layout shifts once it loads.

When the JS library detects that the browser supports native lazyloading, the first thing it does is copy the URL from data-src to src. As far as I can tell, this is where the problem occurs.

[Yehonatan Daniv]

I can confirm it still reproduces on Safari 15.5.

At Wix we use LQIPs on all sites, with our own JS library for lazy loading, and it's making loading="lazy" simply impossible to use ):

Here's an example: https://www.axeumtrading.com/?petri_ovr=specs.thunderbolt.lazyLoadImages:true

If you scroll down you'll see that images below the fold stay with the LQIP content.

[Simon Fraser (smfr)]

I can sometimes reproduce with https://www.axeumtrading.com/?petri_ovr=specs.thunderbolt.lazyLoadImages:true. Investigating.

[Simon Fraser (smfr)]

I think the issue is in ImageLoader::updateFromElement(). For images that have an initial source, this code:

        // Use url from original request for lazy loads that are no longer in deferred state.
        URL imageURL = m_lazyImageLoadState == LazyImageLoadState::LoadImmediately ? m_image->url() : document.completeURL(sourceURI(attr));
        ResourceRequest resourceRequest(imageURL);
        resourceRequest.setInspectorInitiatorNodeIdentifier(InspectorInstrumentation::identifierForNode(m_element));

continues to use the old image URL depending on the state.

[Simon Fraser (smfr)]

That code was added to fix bug 207902.

[Simon Fraser (smfr)]

I don't understand the changes in https://commits.webkit.org/220967@main. They preserve the entire URL, not use the baseURL.

[Simon Fraser (smfr)]

I'm also confused because I can't find any references to the base URL in https://html.spec.whatwg.org/multipage/images.html#update-the-image-data

However, https://html.spec.whatwg.org/multipage/urls-and-fetching.html#dynamic-changes-to-base-urls does say:
"For instance, changing the base URL doesn't affect the image displayed by img elements, although subsequent accesses of the src IDL attribute from script will return a new absolute URL that might no longer correspond to the image being shown."

[Simon Fraser (smfr)]

https://html.spec.whatwg.org/multipage/images.html#update-the-image-data seems to imply that changing the image's src should just reset the lazy load algorithm.

[Rob Buis]

Created attachment 461840 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 461840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461840&action=review

> COMMIT_MESSAGE:9
> +fail as well in Chrome and Firefox.

Does this need a WPT change to remove these tests or mark as "tentative"?

[Rob Buis]

Comment on attachment 461840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461840&action=review

>> COMMIT_MESSAGE:9
>> +fail as well in Chrome and Firefox.
> 
> Does this need a WPT change to remove these tests or mark as "tentative"?

I talked with Anne, we can probably decide what to do tomorrow after he has taken a better look at tests and spec.

[Rob Buis]

Created attachment 461855 [details]
Patch

[Anne van Kesteren]

Okay, so the idea definitely was that parsing happens eagerly (upon element insertion, setting of the `src` attribute, etc.) so that if the base URL changes between that point in time and the point in time we decide we need to fetch the image, it doesn't affect the results. That makes it quite a bit more deterministic.

So ideally we wouldn't regress on that test. And the issue from comment 0 doesn't seem to be related to base URLs so I don't think we have to? However, it does seem problematic that other browsers haven't prioritized this test. Might be worth asking Emilio and Dominic...

[Rob Buis]

(In reply to Anne van Kesteren from comment #21)
> Okay, so the idea definitely was that parsing happens eagerly (upon element
> insertion, setting of the `src` attribute, etc.) so that if the base URL
> changes between that point in time and the point in time we decide we need
> to fetch the image, it doesn't affect the results. That makes it quite a bit
> more deterministic.

Did this idea make it into the current spec? If so can you point to it?

Can the base URL idea be ignored for protocol changes in the url? For example the spiegel.de test site the src goes from data url to a http(s) url. Possibly that is what https://commits.webkit.org/220967@main got wrong?

[Anne van Kesteren]

If you look at the "update the image data" algorithm it'll have parsed the URL input early on (step 11). It's only just before the actual fetch that the lazy load logic kicks in (step 22).

(There is still some timing weirdness in the algorithm with respect to the microtask it queues in step 7. I'm not sure that's correct. The algorithm is quite old and could use an overhaul.)

[Rob Buis]

Created attachment 461876 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 461889 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 461889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461889&action=review

> COMMIT_MESSAGE:6
> +Skip the "base url at parse time" logic when the previous src was not in the http family.

I don't think that the previous patch correctly tests changes in baseURL (there is no change of the document baseURL in any of the tests), so describing this as "base URL at parse time" logic is misleading.

I think lazy loading should allow replacement of, say, "https://img.foo.com/tiny.gif" with "https://img.foo.com/assests/bignewsimage.jpgf", and assigning the src of the image should reset the lazy load algorithm.

[Rob Buis]

Created attachment 461894 [details]
Patch

[Darin Adler]

Comment on attachment 461894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=461894&action=review

> Source/WebCore/loader/ImageLoader.cpp:228
> +        URL imageURL = m_lazyImageLoadState == LazyImageLoadState::LoadImmediately && m_image->url().protocolIsInHTTPFamily()

This doesn’t seem quite right. What makes the non-HTTP family URLs special? Maybe lazy loading doesn’t actually happen? Why is that specific to HTTP. What about file URLs? Or custom URLs that have loading behavior analogous to HTTP?

Can you explain why this is wrong for data URLs? This code change makes the code subtly different from the comment above in a way I find unclear.

[Simon Pieters (:zcorpan)]

(In reply to Anne van Kesteren from comment #23)
> (There is still some timing weirdness in the algorithm with respect to the
> microtask it queues in step 7. I'm not sure that's correct. The algorithm is
> quite old and could use an overhaul.)

This is due to <picture>, we need to wait for all elements and attributes to be available (if building a DOM with script) before making a resource selection to avoid eager fetches of the wrong URL.

This is tested in https://wpt.fyi/results/html/semantics/embedded-content/the-img-element/update-the-image-data/current-request-microtask.html?label=experimental&label=master&aligned

[Rob Buis]

Created attachment 462200 [details]
Patch

[Rob Buis]

Created attachment 462201 [details]
Patch

[Rob Buis]

Created attachment 462202 [details]
Patch

[Darin Adler]

Comment on attachment 462202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=462202&action=review

> COMMIT_MESSAGE:8
> +Fix the problem that data url placeholders are not replaced with the target image by
> +keeping track of pending url loads ; if the new request is done with the same url we keep
> +the original url since it contains the original base url, otherwise we use the new url.

Lets call these "URL" in comments, not "url".

> Source/WebCore/loader/ImageLoader.cpp:166
> +    m_currentURL = nullAtom();

Could use { } instead of nullAtom(), which is actually even more efficient.

> Source/WebCore/loader/ImageLoader.cpp:229
> +        // Use url from original request for same url loads in order to preserve the original base url.

Lets call these "URL" in comments, not "url".

> Source/WebCore/loader/ImageLoader.cpp:274
> +            m_currentURL = nullAtom();

Ditto.

> Source/WebCore/loader/ImageLoader.cpp:282
> +        m_currentURL = nullAtom();

Ditto.

> Source/WebCore/loader/ImageLoader.cpp:392
> +    m_currentURL = nullAtom();

Ditto.

> Source/WebCore/loader/ImageLoader.h:127
> +    AtomString m_currentURL;

I’m not sure that "current" is the right name for this URL; nothing about "current" makes it clear that we’d have to clear "current URL" in all those places.

[Darin Adler]

Given the subtlety here I’d like to know we had enough test cases that removing any of one our cases of setting m_currentURL would result in a test failure.

[Rob Buis]

Created attachment 462281 [details]
Patch

[Rob Buis]

Created attachment 462282 [details]
Patch

[Rob Buis]

Created attachment 462309 [details]
Patch

[Rob Buis]

Created attachment 462311 [details]
Patch

[Rob Buis]

Comment on attachment 462202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=462202&action=review

>> COMMIT_MESSAGE:8
>> +the original url since it contains the original base url, otherwise we use the new url.
> 
> Lets call these "URL" in comments, not "url".

Done.

>> Source/WebCore/loader/ImageLoader.cpp:166
>> +    m_currentURL = nullAtom();
> 
> Could use { } instead of nullAtom(), which is actually even more efficient.

Nice, though it turns out not needed, since at appropriate times (load cancel/error/finished) m_image is reset.

>> Source/WebCore/loader/ImageLoader.cpp:229
>> +        // Use url from original request for same url loads in order to preserve the original base url.
> 
> Lets call these "URL" in comments, not "url".

Fixed.

>> Source/WebCore/loader/ImageLoader.cpp:274
>> +            m_currentURL = nullAtom();
> 
> Ditto.

Not needed in the end.

>> Source/WebCore/loader/ImageLoader.cpp:282
>> +        m_currentURL = nullAtom();
> 
> Ditto.

Not needed in the end.

>> Source/WebCore/loader/ImageLoader.cpp:392
>> +    m_currentURL = nullAtom();
> 
> Ditto.

I kept this one, fixed.

>> Source/WebCore/loader/ImageLoader.h:127
>> +    AtomString m_currentURL;
> 
> I’m not sure that "current" is the right name for this URL; nothing about "current" makes it clear that we’d have to clear "current URL" in all those places.

I chose pending.

[EWS]

Committed 254471@main (0c40ba62b482): <https://commits.webkit.org/254471@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 462311 [details].