Bug 243601

I am able to reproduce this issue of "grey broder" in Safari Technical Preview 150 as well. Thanks!

<rdar://problem/98212411>

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 😊

Created attachment 461792 [details]
Patch

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

Comment on attachment 461792 [details]
Patch

Can we make a regression test?

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

Created attachment 461983 [details]
Patch

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

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

Created attachment 461989 [details]
Patch

@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 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?

Created attachment 462005 [details]
Patch

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.

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

Which version of Safari has the fix? 

I tried 16, 16.1, 16.2, and 16.3 but none of them work.

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

This fix shipped yesterday in Safari 16.4.
https://webkit.org/blog/13966/webkit-features-in-safari-16-4/#css