Bug 187574 – Allow removal of white backgrounds

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Allow removal of white backgrounds when color filters are enabled

[Dean Jackson]

<rdar://problem/41146792>

[Dean Jackson]

Created attachment 344803 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 344803 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:1365
> +    if (renderer().document().settings().punchOutWhiteBackgrounds() && Color::isWhiteColor(color) && lineStyle.hasAppleColorFilter())

I think we may want this to be independent of color filter. Let's wait until we find out.

[EWS Watchlist]

Comment on attachment 344803 [details]
Patch

Attachment 344803 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8510387

New failing tests:
displaylists/replay-skip-clipped-rect.html
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html
css3/color-filters/punch-out-white-backgrounds.html
displaylists/layer-dispay-list.html

[EWS Watchlist]

Created attachment 344806 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344803 [details]
Patch

Attachment 344803 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8510397

New failing tests:
displaylists/replay-skip-clipped-rect.html
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html
displaylists/layer-dispay-list.html

[EWS Watchlist]

Created attachment 344807 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344803 [details]
Patch

Attachment 344803 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8510438

New failing tests:
displaylists/replay-skip-clipped-rect.html
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html
css3/color-filters/punch-out-white-backgrounds.html
displaylists/layer-dispay-list.html

[EWS Watchlist]

Created attachment 344809 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344803 [details]
Patch

Attachment 344803 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8510697

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 344812 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Jackson]

Created attachment 344850 [details]
Patch

[Dean Jackson]

Created attachment 344851 [details]
Patch

[Dean Jackson]

Created attachment 344855 [details]
Patch

[Dean Jackson]

Created attachment 344859 [details]
Patch

[Dean Jackson]

Created attachment 344862 [details]
Patch

[Jon Lee]

Comment on attachment 344859 [details]
Patch

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

> Source/WebCore/rendering/RenderTableCell.cpp:1291
> +    if (color.isValid()) {

Is it a problem that before we would color-filter regardless of whether the color is valid, but here we only filter if it is valid? And why do we not check the validity of the color prior to filtering in all of the other cases?

[Dean Jackson]

Comment on attachment 344859 [details]
Patch

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

>> Source/WebCore/rendering/RenderTableCell.cpp:1291
>> +    if (color.isValid()) {
> 
> Is it a problem that before we would color-filter regardless of whether the color is valid, but here we only filter if it is valid? And why do we not check the validity of the color prior to filtering in all of the other cases?

This is a good point. isWhiteColor should check validity, as well as colorByApplyingColorFilter, so this code is not necessary.

[EWS Watchlist]

Comment on attachment 344862 [details]
Patch

Attachment 344862 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8517653

New failing tests:
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html

[EWS Watchlist]

Created attachment 344868 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344859 [details]
Patch

Attachment 344859 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8517455

New failing tests:
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html

[EWS Watchlist]

Created attachment 344871 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344862 [details]
Patch

Attachment 344862 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8517936

New failing tests:
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html

[EWS Watchlist]

Created attachment 344873 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Dean Jackson]

Created attachment 344983 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 344983 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:952
> +                context.fillRect(backgroundRectForPainting, bgColor, operation); // FIXME: use op?

Do we need to fix?

[Dean Jackson]

Created attachment 345125 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 345125 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:1367
> +        WTFLogAlways("punching out inline");

Nope.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:952
> +                context.fillRect(backgroundRectForPainting, bgColor, operation); // FIXME: use op if it is DestinationOut?

Remove or fix the fixme.

> Source/WebCore/rendering/RenderTableCell.cpp:1293
> +        WTFLogAlways("punching out table cell");

Nope.

> Source/WebCore/rendering/RenderTheme.h:256
> +    virtual bool usingDarkAppearance(const Page&) const { return false; }

Can RenderTheme really not get to Page()? Maybe just pass the renderer?

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:173
> +    if (options.punchOutWhiteBackgroundsInDarkMode)
> +        m_mainWebView->setDrawsBackground(false);

Hmmmm, weird. Those things naively seem unrelated.

You also need to reset this between tests.

[Dean Jackson]

Committed r233869: <https://trac.webkit.org/changeset/233869>

[Truitt Savell]

Looks like we have two failing tests after r233869 <https://trac.webkit.org/changeset/233869/webkit>

Test Histories:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=displaylists%2Fextent-includes-transforms.html%20displaylists%2Fextent-includes-shadow.html

Test Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/displaylists/extent-includes-shadow-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/displaylists/extent-includes-shadow-actual.txt
@@ -10,8 +10,10 @@
   (shadow-offset width=10 height=20)
   (shadows-use-legacy-radius 0)
   (should-subpixel-quantize-fonts 0))
-(fill-rect-with-color
+(fill-composited-rect
   (extent at (43,50) size 134x137)
   (rect at (50,50) size 100x100)
-  (color #0000FF))
+  (color #0000FF)
+  (composite-operation source-over)
+  (blend-mode normal))
 (restore)

[Ryan Haddad]

(In reply to Truitt Savell from comment #30)
> Looks like we have two failing tests after r233869
> <https://trac.webkit.org/changeset/233869/webkit>

I went ahead and rebaselined these tests for mac-wk1 in https://trac.webkit.org/r233887