Bug 243601 - Images with loading="lazy" have uncontrollable gray border while loading
Summary: Images with loading="lazy" have uncontrollable gray border while loading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Safari 15
Hardware: All macOS 12
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-08-05 12:41 PDT by styfle
Modified: 2023-03-28 17:08 PDT (History)
19 users (show)

See Also:


Attachments
HTML page that reproduces the issue (368 bytes, text/html)
2022-08-05 12:41 PDT, styfle
no flags Details
Patch (1.82 KB, patch)
2022-08-22 01:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.50 KB, patch)
2022-08-29 09:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.90 KB, patch)
2022-08-29 12:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.06 KB, patch)
2022-08-30 05:44 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description styfle 2022-08-05 12:41:50 PDT
Created attachment 461428 [details]
HTML page that reproduces the issue

Images that use native lazy loading (the feature added in #196698) show an unexpected gray border while loading.

This border disappears once the image is loaded.

The border does not seem to be controllable with CSS as seen here: https://stackoverflow.com/q/71992334/266535

This issue can be reproduced by adding `loading="lazy"` to any `<img>` and watching it load, preferable a large and slowly loaded image.



<!DOCTYPE html>
<html>
  <head>
    <title>Lazy Loading</title>
  </head>
  <body style="background:black">
    <img 
      alt="This is alt text"
      loading="lazy"
      width="791"
      height="989"
      src="https://images.unsplash.com/photo-1496694971191-58e2450fc841?ixlib=rb-1.2.1&ixid=MnwxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8"
   />
  </body>
</html>
Comment 1 Ahmad Saleem 2022-08-05 14:08:10 PDT
I am able to reproduce this issue of "grey broder" in Safari Technical Preview 150 as well. Thanks!
Comment 2 Radar WebKit Bug Importer 2022-08-05 14:31:07 PDT
<rdar://problem/98212411>
Comment 3 styfle 2022-08-19 08:52:22 PDT
I was trying to find where this gray border is assigned but I can't find it.

I did however find a test that mentions "Fully loaded image should not be a placeholder which is drawn as a translucent gray rectangle in placeholder_image.cc"

https://github.com/WebKit/WebKit/blob/bcacfd6ffbd07a582adc2af9a4ceae429e14eb1c/LayoutTests/http/tests/lazyload/placeholder.js

So it seems like webkit intentionally added this gray border (its not clear why).

Perhaps there is some other image lib that is assigning the gray border?

Regardless, the image should never have a gray border 😊
Comment 4 Rob Buis 2022-08-22 01:33:35 PDT
Created attachment 461792 [details]
Patch
Comment 5 Rob Buis 2022-08-22 06:13:31 PDT
I added a patch to remove this behaviour, let me know if a test is needed.
Comment 6 Darin Adler 2022-08-22 08:41:34 PDT
Comment on attachment 461792 [details]
Patch

Can we make a regression test?
Comment 7 Rob Buis 2022-08-23 09:30:56 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 461792 [details]
> Patch
> 
> Can we make a regression test?

I tried both in WPT (takeScreenshot) and http/tests frameworks but got nothing working. I suspect the dumpAfterWaitAttributeIsRemoved logic cancels the pending lazy load and we end up with a screenshot of the broken image load instead of the outline placeholder. I propose to accept the patch as-is since it is low risk, i.e. it does not break any existing tests.
Comment 8 Rob Buis 2022-08-29 09:32:15 PDT
Created attachment 461983 [details]
Patch
Comment 9 Darin Adler 2022-08-29 11:22:31 PDT
Exciting that one of the WPT tests seems to have detected the improvement
Comment 10 Rob Buis 2022-08-29 12:10:22 PDT
(In reply to Darin Adler from comment #9)
> Exciting that one of the WPT tests seems to have detected the improvement

I agree. I just had to look extra carefully since the test paints an outline and paintIncompleteImageOutline as well. So the only difference is that without this patch the total outline was too thick compared to the expected result. Still, image-loading-lazy-slow.html ends verifying the change.
Comment 11 Rob Buis 2022-08-29 12:28:52 PDT
Created attachment 461989 [details]
Patch
Comment 12 Rob Buis 2022-08-29 12:30:38 PDT
@rniwa maybe you have an opinion on the test runner changes, I am not very familiar with it. Also I wonder if anything similar is needer for DRT...
Comment 13 Darin Adler 2022-08-29 14:07:20 PDT
Comment on attachment 461989 [details]
Patch

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

> COMMIT_MESSAGE:12
> +Do not paint border while an image is in deferred state.
> +The test image-loading-lazy-slow.html covers this. However, the current
> +test runner logic always stops page loads before pixel dumping, causing the
> +image to be painted as a broken image instead of the empty image at the
> +time of calling takeScreenshot. To fix this, postpone the stopping of page
> +loads (except when the test tests printing) and instead always stop page
> +loads when reseting after the test.

The code doesn’t actually match this comment. The patch postpones the stopping of the page loads only when the test is dumping a pixel result, so the special case is dumping pixels, not printing. I am guessing this comment documents an earlier plan.

> Source/WebCore/rendering/RenderImage.cpp:486
> +        if (!context.contenfulPaintDetected() && cachedImage() && cachedImage()->canRender(this, deviceScaleFactor) && !contentSize.isEmpty())

Just noticed a typo in the code (the unmodified part): contenfulPaintDetected. Someone could fix that later.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:553
> +    if (!m_pixelResultIsPending)
> +        page()->stopLoading();

I think this is not obviously correct, so it needs a comment. Why ever stop loading? Why make an exception for pixel results?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:408
> +    stopLoading();

Seems unclear exactly why we *need* this here, although obviously we need it for the pixel result case. But it seems we could instead stop loading at the same time we set m_pixelResultIsPending to false. The trick is to keep this code as logical as possible so we don’t get more confused in the future.

Might even need a comment here that explains why this is usually a no-op.

> LayoutTests/TestExpectations:-803
> -imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/image-loading-lazy-slow.html [ ImageOnlyFailure ]

Given we are changing WebKitTestRunner and not DumpRenderTree, it seems likely this is still broken under Legacy WebKit. Do we have the correct test expectation for that?
Comment 14 Rob Buis 2022-08-30 05:44:13 PDT
Created attachment 462005 [details]
Patch
Comment 15 Rob Buis 2022-08-30 14:02:28 PDT
Comment on attachment 461989 [details]
Patch

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

>> COMMIT_MESSAGE:12
>> +loads when reseting after the test.
> 
> The code doesn’t actually match this comment. The patch postpones the stopping of the page loads only when the test is dumping a pixel result, so the special case is dumping pixels, not printing. I am guessing this comment documents an earlier plan.

I reworded the commit meesage.

>> Source/WebCore/rendering/RenderImage.cpp:486
>> +        if (!context.contenfulPaintDetected() && cachedImage() && cachedImage()->canRender(this, deviceScaleFactor) && !contentSize.isEmpty())
> 
> Just noticed a typo in the code (the unmodified part): contenfulPaintDetected. Someone could fix that later.

I fixed it in https://bugs.webkit.org/show_bug.cgi?id=244531.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:553
>> +        page()->stopLoading();
> 
> I think this is not obviously correct, so it needs a comment. Why ever stop loading? Why make an exception for pixel results?

Done.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:408
>> +    stopLoading();
> 
> Seems unclear exactly why we *need* this here, although obviously we need it for the pixel result case. But it seems we could instead stop loading at the same time we set m_pixelResultIsPending to false. The trick is to keep this code as logical as possible so we don’t get more confused in the future.
> 
> Might even need a comment here that explains why this is usually a no-op.

Added comment.

>> LayoutTests/TestExpectations:-803
>> -imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/image-loading-lazy-slow.html [ ImageOnlyFailure ]
> 
> Given we are changing WebKitTestRunner and not DumpRenderTree, it seems likely this is still broken under Legacy WebKit. Do we have the correct test expectation for that?

It works fine in wk1. I also noticed it passes in ios, so I removed the expectation to fail there too.
Comment 16 EWS 2022-08-30 14:52:16 PDT
Committed 253960@main (7d844a4e948d): <https://commits.webkit.org/253960@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 462005 [details].
Comment 17 styfle 2023-01-23 15:40:17 PST
Which version of Safari has the fix? 

I tried 16, 16.1, 16.2, and 16.3 but none of them work.
Comment 18 Chris Dumez 2023-01-23 15:41:52 PST
(In reply to styfle from comment #17)
> Which version of Safari has the fix? 
> 
> I tried 16, 16.1, 16.2, and 16.3 but none of them work.

This fix hasn't shipped in Safari yet.
Comment 19 Jen Simmons 2023-03-28 17:08:45 PDT
This fix shipped yesterday in Safari 16.4.
https://webkit.org/blog/13966/webkit-features-in-safari-16-4/#css