Bug 187574

Summary: Allow removal of white backgrounds
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, jonlee, rniwa, ryanhaddad, simon.fraser, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews117 for mac-sierra
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Archive of layout-test-results from ews112 for mac-sierra
none
Archive of layout-test-results from ews113 for mac-sierra
none
Patch
none
Patch simon.fraser: review+, simon.fraser: commit-queue-

Description Dean Jackson 2018-07-11 17:53:02 PDT
Allow removal of white backgrounds when color filters are enabled
Comment 1 Dean Jackson 2018-07-11 17:54:52 PDT
<rdar://problem/41146792>
Comment 2 Dean Jackson 2018-07-11 18:13:23 PDT
Created attachment 344803 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-07-11 18:52:13 PDT
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.
Comment 4 EWS Watchlist 2018-07-11 19:12:42 PDT
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
Comment 5 EWS Watchlist 2018-07-11 19:12:43 PDT
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
Comment 6 EWS Watchlist 2018-07-11 19:18:15 PDT
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
Comment 7 EWS Watchlist 2018-07-11 19:18:17 PDT
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
Comment 8 EWS Watchlist 2018-07-11 19:48:08 PDT
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
Comment 9 EWS Watchlist 2018-07-11 19:48:10 PDT
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
Comment 10 EWS Watchlist 2018-07-11 20:05:27 PDT
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
Comment 11 EWS Watchlist 2018-07-11 20:05:39 PDT
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
Comment 12 Dean Jackson 2018-07-12 09:42:38 PDT
Created attachment 344850 [details]
Patch
Comment 13 Dean Jackson 2018-07-12 10:05:01 PDT
Created attachment 344851 [details]
Patch
Comment 14 Dean Jackson 2018-07-12 10:24:39 PDT
Created attachment 344855 [details]
Patch
Comment 15 Dean Jackson 2018-07-12 10:41:58 PDT
Created attachment 344859 [details]
Patch
Comment 16 Dean Jackson 2018-07-12 11:16:36 PDT
Created attachment 344862 [details]
Patch
Comment 17 Jon Lee 2018-07-12 11:24:51 PDT
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?
Comment 18 Dean Jackson 2018-07-12 11:31:00 PDT
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.
Comment 19 EWS Watchlist 2018-07-12 12:11:44 PDT
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
Comment 20 EWS Watchlist 2018-07-12 12:11:46 PDT
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
Comment 21 EWS Watchlist 2018-07-12 12:16:40 PDT
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
Comment 22 EWS Watchlist 2018-07-12 12:16:42 PDT
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
Comment 23 EWS Watchlist 2018-07-12 13:00:19 PDT
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
Comment 24 EWS Watchlist 2018-07-12 13:00:21 PDT
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
Comment 25 Dean Jackson 2018-07-13 14:39:19 PDT
Created attachment 344983 [details]
Patch
Comment 26 Simon Fraser (smfr) 2018-07-13 14:44:31 PDT
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?
Comment 27 Dean Jackson 2018-07-16 15:42:55 PDT
Created attachment 345125 [details]
Patch
Comment 28 Simon Fraser (smfr) 2018-07-16 15:49:01 PDT
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.
Comment 29 Dean Jackson 2018-07-16 16:35:32 PDT
Committed r233869: <https://trac.webkit.org/changeset/233869>
Comment 30 Truitt Savell 2018-07-17 09:17:53 PDT
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)
Comment 31 Ryan Haddad 2018-07-17 09:59:31 PDT
(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