Bug 191607 - Default the view background color and text color to different values when in dark mode
Summary: Default the view background color and text color to different values when in ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on: 191559
Blocks: 191697
  Show dependency treegraph
 
Reported: 2018-11-13 16:06 PST by Timothy Hatcher
Modified: 2018-11-15 09:52 PST (History)
12 users (show)

See Also:


Attachments
Patch (17.62 KB, patch)
2018-11-13 16:24 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (18.64 KB, patch)
2018-11-13 17:55 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (19.10 KB, patch)
2018-11-13 17:57 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (1.09 MB, application/zip)
2018-11-13 19:05 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (1.71 MB, application/zip)
2018-11-13 20:45 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (1.07 MB, application/zip)
2018-11-13 21:28 PST, EWS Watchlist
no flags Details
Patch (23.77 KB, patch)
2018-11-13 23:46 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (23.77 KB, patch)
2018-11-13 23:58 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (3.28 MB, application/zip)
2018-11-14 01:11 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (4.47 MB, application/zip)
2018-11-14 01:59 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (4.08 MB, application/zip)
2018-11-14 07:43 PST, EWS Watchlist
no flags Details
Patch (31.42 KB, patch)
2018-11-14 10:01 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (33.26 KB, patch)
2018-11-14 10:41 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (34.10 KB, patch)
2018-11-14 10:44 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Failing test (also on shipping Safari) (58.45 KB, image/png)
2018-11-14 10:47 PST, zalan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2018-11-13 16:06:45 PST
The view background color should not be white. And the text color should not be black. They should be [NSColor controlBackgroundColor] and [NSColor textColor].
Comment 1 Radar WebKit Bug Importer 2018-11-13 16:07:08 PST
<rdar://problem/46045854>
Comment 2 Timothy Hatcher 2018-11-13 16:24:27 PST Comment hidden (obsolete)
Comment 3 Simon Fraser (smfr) 2018-11-13 16:38:42 PST
Comment on attachment 354724 [details]
Patch

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

> Source/WebCore/css/html.css:41
> +    color: text;

Is this still black in light mode?

Also weird that we do something in the UA stylesheet which there is no standard way for authors to do in their pages.

> Source/WebCore/page/FrameView.cpp:3005
> +    Color backgroundColor = transparent ? Color::transparent : RenderTheme::singleton().systemColor(CSSValueAppleSystemControlBackground, styleColorOptions());

What's the value of CSSValueAppleSystemControlBackground in light mode? White, or are you affecting the default background color?
Comment 4 Timothy Hatcher 2018-11-13 16:39:52 PST
Comment on attachment 354724 [details]
Patch

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

>> Source/WebCore/css/html.css:41
>> +    color: text;
> 
> Is this still black in light mode?
> 
> Also weird that we do something in the UA stylesheet which there is no standard way for authors to do in their pages.

Yes, it is black in light mode. Test checks that.

>> Source/WebCore/page/FrameView.cpp:3005
>> +    Color backgroundColor = transparent ? Color::transparent : RenderTheme::singleton().systemColor(CSSValueAppleSystemControlBackground, styleColorOptions());
> 
> What's the value of CSSValueAppleSystemControlBackground in light mode? White, or are you affecting the default background color?

It is pure white in light mode. Test checks that.
Comment 5 Timothy Hatcher 2018-11-13 17:55:48 PST Comment hidden (obsolete)
Comment 6 Timothy Hatcher 2018-11-13 17:57:49 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-11-13 19:05:31 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-11-13 19:05:33 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-11-13 20:45:01 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-11-13 20:45:03 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-11-13 21:28:46 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-11-13 21:28:47 PST Comment hidden (obsolete)
Comment 13 Timothy Hatcher 2018-11-13 23:46:43 PST Comment hidden (obsolete)
Comment 14 Timothy Hatcher 2018-11-13 23:58:17 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-11-14 01:11:05 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-11-14 01:11:07 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-11-14 01:59:34 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-11-14 01:59:36 PST Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-11-14 07:43:27 PST Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-11-14 07:43:29 PST Comment hidden (obsolete)
Comment 21 Timothy Hatcher 2018-11-14 10:01:10 PST Comment hidden (obsolete)
Comment 22 Timothy Hatcher 2018-11-14 10:41:48 PST Comment hidden (obsolete)
Comment 23 Timothy Hatcher 2018-11-14 10:44:10 PST
Created attachment 354834 [details]
Patch
Comment 24 zalan 2018-11-14 10:47:41 PST
Created attachment 354835 [details]
Failing test (also on shipping Safari)

The test actually fails already on shipping Safari.
Comment 25 Simon Fraser (smfr) 2018-11-14 11:53:32 PST
Comment on attachment 354834 [details]
Patch

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

> Source/WebCore/css/html.css:29
> +    color: text;

Does this mean that our default text color is now semantic, and will not get color-filter applied?
Comment 26 Timothy Hatcher 2018-11-14 12:31:09 PST
Comment on attachment 354834 [details]
Patch

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

>> Source/WebCore/css/html.css:29
>> +    color: text;
> 
> Does this mean that our default text color is now semantic, and will not get color-filter applied?

Yes. The color-filter skips it, but it still auto switches with light and dark mode — yielding the same white text as the Mail color-filter.
Comment 27 WebKit Commit Bot 2018-11-14 17:48:30 PST
Comment on attachment 354834 [details]
Patch

Clearing flags on attachment: 354834

Committed r238212: <https://trac.webkit.org/changeset/238212>
Comment 28 WebKit Commit Bot 2018-11-14 17:48:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Truitt Savell 2018-11-15 08:22:00 PST
Looks like https://trac.webkit.org/changeset/238212/webkit

has caused two API failures on MacOS

build: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/12799

Failed

    TestWebKitAPI.WebKit.BackgroundColorNil
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/mac/BackgroundColor.mm:77
        Expected equality of these values:
          CGColorGetConstantColor(kCGColorWhite)
            Which is: 0x7f922871dee0
          [webView layer].backgroundColor
            Which is: 0x7f9228530400
        

    TestWebKitAPI.WebKit.BackgroundColorDefault
        
        /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/mac/BackgroundColor.mm:47
        Expected equality of these values:
          CGColorGetConstantColor(kCGColorWhite)
            Which is: 0x7fb24d4f5ff0
          [webView layer].backgroundColor
            Which is: 0x7fb2520065d0
Comment 30 Truitt Savell 2018-11-15 08:28:37 PST
Additionally Mojave has this api failure in addition:

Failed

    TestWebKitAPI.WebKit.BackgroundColorNoDrawsBackground
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/mac/BackgroundColor.mm:87
        Expected equality of these values:
          [NSColor whiteColor]
            Which is: 0x7f9562574c20
          backgroundColor
            Which is: 0x7f9562574440
Comment 31 Timothy Hatcher 2018-11-15 09:11:42 PST
Oops. I’ll get these fixed in the next hour.
Comment 32 Timothy Hatcher 2018-11-15 09:52:30 PST
Fix for API tests is bug 191697.