Bug 234199

Summary: Allow override of system's preferred color scheme
Product: WebKit Reporter: Dean Jackson <dino>
Component: WebKit APIAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, japhet, jer.noble, macpherson, menard, philipj, sergio, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch thorton: review+

Description Dean Jackson 2021-12-11 09:05:31 PST
Allow override of system's preferred color scheme
Comment 1 Radar WebKit Bug Importer 2021-12-11 09:07:46 PST
<rdar://problem/86366044>
Comment 2 Dean Jackson 2021-12-11 09:17:55 PST
Created attachment 446891 [details]
Patch
Comment 3 Anders Carlsson 2021-12-11 13:36:38 PST
Looks like all the EWS bots are red.
Comment 4 Dean Jackson 2021-12-12 08:45:05 PST
(In reply to Anders Carlsson from comment #3)
> Looks like all the EWS bots are red.

Very impressive. Do I win a prize?
Comment 5 Dean Jackson 2021-12-13 11:08:47 PST
Created attachment 447028 [details]
Patch
Comment 6 Dean Jackson 2021-12-13 11:09:50 PST
Would appreciate feedback on the SPI naming - I put the word "explicitly" in there to make it clear that this isn't the preference coming from the system.
Comment 7 Tim Horton 2021-12-13 11:22:26 PST
Comment on attachment 447028 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIWebsitePolicies.h:135
> +    WebCore::ColorSchemePreference userExplicitlyPrefersColorScheme() const { return m_userExplicitlyPrefersColorScheme; }

This getter name feels slightly odd (it sounds like it would return a boolean that the "user explicitly prefers (a) color scheme").
Comment 8 Dean Jackson 2021-12-13 11:24:58 PST
Comment on attachment 447028 [details]
Patch

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

>> Source/WebKit/UIProcess/API/APIWebsitePolicies.h:135
>> +    WebCore::ColorSchemePreference userExplicitlyPrefersColorScheme() const { return m_userExplicitlyPrefersColorScheme; }
> 
> This getter name feels slightly odd (it sounds like it would return a boolean that the "user explicitly prefers (a) color scheme").

Do you think I should call it colorSchemePreference?
Comment 9 Tim Horton 2021-12-13 12:39:29 PST
(In reply to Dean Jackson from comment #8)
> Comment on attachment 447028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447028&action=review
> 
> >> Source/WebKit/UIProcess/API/APIWebsitePolicies.h:135
> >> +    WebCore::ColorSchemePreference userExplicitlyPrefersColorScheme() const { return m_userExplicitlyPrefersColorScheme; }
> > 
> > This getter name feels slightly odd (it sounds like it would return a boolean that the "user explicitly prefers (a) color scheme").
> 
> Do you think I should call it colorSchemePreference?

or preferredColorScheme? Or maybe shove the word "override" in there somewhere because it overrides the system preference?
Comment 10 Dean Jackson 2021-12-13 13:44:42 PST
Created attachment 447056 [details]
Patch
Comment 11 Tim Horton 2021-12-13 17:16:03 PST
Comment on attachment 447056 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:811
> +    bool useDarkAppearance = [&] () -> auto {

Are there other things that read useDarkAppearance that need to change? Should useDarkAppearance itself respect the bit on DocumentLoader?
Comment 12 Antoine Quint 2021-12-14 02:46:11 PST
There is an existing `ColorScheme` enum with holds the `Light` and `Dark` options. Maybe instead of adding a new enum you could use an std::optional<ColorScheme> where std::nullopt would indicate the lack of a preference?
Comment 13 Dean Jackson 2021-12-14 10:21:28 PST
Committed r287030 (?): <https://commits.webkit.org/r287030>