Bug 243601

Summary:

Images with loading="lazy" have uncontrollable gray border while loading

Product:

WebKit

Reporter:

styfle <steven>

Component:

Images

Assignee:

Rob Buis <rbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

709922234, ahmad.saleem792, bfulgham, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, glenn, jensimmons, kondapallykalyan, mmaxfield, pdr, rbuis, rniwa, sabouhallawa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

Safari 15

Hardware:

All

OS:

macOS 12

See Also:

https://bugs.webkit.org/show_bug.cgi?id=171326

Attachments:

Description	Flags
HTML page that reproduces the issue	none
Patch	none
Patch	none
Patch	none
Patch	none

styfle

Reported 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>

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ahmad Saleem

Comment 1 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!

Radar WebKit Bug Importer

Comment 2 2022-08-05 14:31:07 PDT

<rdar://problem/98212411>

styfle

Comment 3 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 😊

Rob Buis

Comment 4 2022-08-22 01:33:35 PDT

Created attachment 461792 [details] Patch

Rob Buis

Comment 5 2022-08-22 06:13:31 PDT

I added a patch to remove this behaviour, let me know if a test is needed.

Darin Adler

Comment 6 2022-08-22 08:41:34 PDT

Comment on attachment 461792 [details] Patch Can we make a regression test?

Rob Buis

Comment 7 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.

Rob Buis

Comment 8 2022-08-29 09:32:15 PDT

Created attachment 461983 [details] Patch

Darin Adler

Comment 9 2022-08-29 11:22:31 PDT

Exciting that one of the WPT tests seems to have detected the improvement

Rob Buis

Comment 10 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.

Rob Buis

Comment 11 2022-08-29 12:28:52 PDT

Created attachment 461989 [details] Patch

Rob Buis

Comment 12 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...

Darin Adler

Comment 13 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?

Rob Buis

Comment 14 2022-08-30 05:44:13 PDT

Created attachment 462005 [details] Patch

Rob Buis

Comment 15 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.

EWS

Comment 16 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].

styfle

Comment 17 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.

Chris Dumez

Comment 18 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.

Jen Simmons

Comment 19 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

Note You need to log in before you can comment on or make changes to this bug.