Bug 191319 - Add experimental support for a `supported-color-schemes` CSS property
Summary: Add experimental support for a `supported-color-schemes` CSS property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-06 12:22 PST by Timothy Hatcher
Modified: 2018-11-08 13:57 PST (History)
7 users (show)

See Also:


Attachments
WIP (42.07 KB, patch)
2018-11-06 12:22 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (64.08 KB, patch)
2018-11-07 17:01 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (64.12 KB, patch)
2018-11-07 17:14 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (74.68 KB, patch)
2018-11-08 01:02 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (73.71 KB, patch)
2018-11-08 01:07 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (73.75 KB, patch)
2018-11-08 01:19 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (74.80 KB, patch)
2018-11-08 07:12 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (74.81 KB, patch)
2018-11-08 11:31 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (74.83 KB, patch)
2018-11-08 13:18 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2018-11-06 12:22:18 PST
Current proposal, based on our current meta tag implementation:

supported-color-schemes: [light? || dark? || <ident>?]* || only?

Inherited: yes
Initial value: auto

"auto" (initial value) — the UA may choose to transform the content to match the user's preference.
"light dark" — the UA will choose the light or dark theme to match the user's preference. If the user's preference does not match something in the list, the UA is allowed to apply transformations to the content.
"only" — synonym for "only light"; the UA will only ever render the content in the light color scheme, and never apply transformations.
"light dark only" — the UA will choose the first of the listed schemes that it supports taking user preference into account, and never apply transformations.
"os-specific-scheme only" — if the UA doesn't support “os-specific-scheme” it's allowed to fall back to its default rendering “light”, and will never apply transformations.

UAs are always allowed to apply minimal transformations, for example making the root background transparent, but not change the overall appearance of the content.
Comment 1 Timothy Hatcher 2018-11-06 12:22:53 PST Comment hidden (obsolete)
Comment 2 Radar WebKit Bug Importer 2018-11-06 12:23:20 PST
<rdar://problem/45852261>
Comment 3 Simon Fraser (smfr) 2018-11-07 10:54:11 PST Comment hidden (obsolete)
Comment 4 Timothy Hatcher 2018-11-07 17:01:31 PST Comment hidden (obsolete)
Comment 5 Timothy Hatcher 2018-11-07 17:05:51 PST
Simon wrote up the GitHub issue at: https://github.com/w3c/csswg-drafts/issues/3299
Comment 6 Timothy Hatcher 2018-11-07 17:07:32 PST
My patch supports an `auto` value to let the CSS property interact with the <meta> value if both are used.
Comment 7 Timothy Hatcher 2018-11-07 17:14:09 PST Comment hidden (obsolete)
Comment 8 Simon Fraser (smfr) 2018-11-07 17:22:46 PST Comment hidden (obsolete)
Comment 9 Timothy Hatcher 2018-11-08 01:02:28 PST Comment hidden (obsolete)
Comment 10 Timothy Hatcher 2018-11-08 01:03:01 PST
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 354185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354185&action=review
> 
> I don't see any code that keeps support for the CSS property behind the
> DarkModeCSS feature setting?

Fixed.

> > Source/WebCore/rendering/style/RenderStyleConstants.h:1026
> > +enum class ColorSchemes : uint8_t {
> > +    Only = 1 << 0,
> > +    Light = 1 << 1,
> > +    Dark = 1 << 2
> > +};
> 
> This doesn't read very well. "only" isn't a scheme.

Fixed.
Comment 11 Timothy Hatcher 2018-11-08 01:07:12 PST Comment hidden (obsolete)
Comment 12 Timothy Hatcher 2018-11-08 01:19:49 PST Comment hidden (obsolete)
Comment 13 Timothy Hatcher 2018-11-08 07:12:18 PST Comment hidden (obsolete)
Comment 14 Simon Fraser (smfr) 2018-11-08 10:11:27 PST
Comment on attachment 354238 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3976
> +        case CSSPropertySupportedColorSchemes: {

This should check RuntimeEnabledFeatures::sharedFeatures().darkModeCSSEnabled(), otherwise the presence of the property is detectable via getComputedStyle.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:89
> +    context->drawFocusRing(path, 1, 1, RenderTheme::focusRingColor(element.document().styleColorOptions(canvas().computedStyle())));

That call to computedStyle() seems like it could be expensive, when all we need is one property.

> Source/WebCore/rendering/style/StyleSupportedColorSchemes.h:65
> +    unsigned m_colorSchemes : ColorSchemesBits; // ColorSchemes

Can this be an OptionSet<>? Using 2 bits here isn't saving you anything because you'll have 6 bits of padding before the bool.
Comment 15 Timothy Hatcher 2018-11-08 11:21:07 PST
Comment on attachment 354238 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3976
>> +        case CSSPropertySupportedColorSchemes: {
> 
> This should check RuntimeEnabledFeatures::sharedFeatures().darkModeCSSEnabled(), otherwise the presence of the property is detectable via getComputedStyle.

Yep.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:89
>> +    context->drawFocusRing(path, 1, 1, RenderTheme::focusRingColor(element.document().styleColorOptions(canvas().computedStyle())));
> 
> That call to computedStyle() seems like it could be expensive, when all we need is one property.

I don't know any other option to avoid it. A few other places in CanvasRenderingContext2D.cpp access computedStyle() too.

>> Source/WebCore/rendering/style/StyleSupportedColorSchemes.h:65
>> +    unsigned m_colorSchemes : ColorSchemesBits; // ColorSchemes
> 
> Can this be an OptionSet<>? Using 2 bits here isn't saving you anything because you'll have 6 bits of padding before the bool.

Sure. Makes sense.
Comment 16 Timothy Hatcher 2018-11-08 11:31:20 PST Comment hidden (obsolete)
Comment 17 Timothy Hatcher 2018-11-08 11:31:56 PST Comment hidden (obsolete)
Comment 18 Dean Jackson 2018-11-08 12:13:49 PST
Comment on attachment 354256 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3598
> +            supportedColorSchemes = { };

Is this needed since you already guarded for auto being the only member of the list?

> Source/WebCore/inspector/InspectorOverlay.cpp:183
> +    GraphicsContextStateSaver stateSaver(context);

Why was this added?

> Source/WebCore/rendering/style/RenderStyleConstants.h:1027
> +static const size_t ColorSchemesBits = 2;

Shame we can't do a static_assert on these :(

> Source/WebCore/rendering/style/StyleSupportedColorSchemes.h:40
> +        : m_colorSchemes(other.m_colorSchemes), m_allowsTransformations(other.m_allowsTransformations) { }

Do we usually put all this on one line?

> Source/WebCore/rendering/style/StyleSupportedColorSchemes.h:43
> +        : m_colorSchemes(colorSchemes), m_allowsTransformations(allowsTransformations) { }

Ditto?
Comment 19 Timothy Hatcher 2018-11-08 13:16:28 PST
Comment on attachment 354256 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:3598
>> +            supportedColorSchemes = { };
> 
> Is this needed since you already guarded for auto being the only member of the list?

This is the meta tag parsing, which is its own thing.

>> Source/WebCore/inspector/InspectorOverlay.cpp:183
>> +    GraphicsContextStateSaver stateSaver(context);
> 
> Why was this added?

I moved the LocalDefaultSystemAppearance below to use existing view var. Patch lies.

>> Source/WebCore/rendering/style/StyleSupportedColorSchemes.h:40
>> +        : m_colorSchemes(other.m_colorSchemes), m_allowsTransformations(other.m_allowsTransformations) { }
> 
> Do we usually put all this on one line?

No, i'll fix.

>> Source/WebCore/rendering/style/StyleSupportedColorSchemes.h:43
>> +        : m_colorSchemes(colorSchemes), m_allowsTransformations(allowsTransformations) { }
> 
> Ditto?

Ditto.
Comment 20 Timothy Hatcher 2018-11-08 13:18:37 PST
Created attachment 354265 [details]
Patch
Comment 21 WebKit Commit Bot 2018-11-08 13:57:50 PST
Comment on attachment 354265 [details]
Patch

Clearing flags on attachment: 354265

Committed r238001: <https://trac.webkit.org/changeset/238001>
Comment 22 WebKit Commit Bot 2018-11-08 13:57:51 PST
All reviewed patches have been landed.  Closing bug.