Bug 191319 – Add experimental support for a `supported-color-schemes` CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 353985 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/45852261>

[Simon Fraser (smfr)]

Let's write up the GitHub issue before we land this.

[Timothy Hatcher]

Created attachment 354183 [details]
Patch

[Timothy Hatcher]

Simon wrote up the GitHub issue at: https://github.com/w3c/csswg-drafts/issues/3299

[Timothy Hatcher]

My patch supports an `auto` value to let the CSS property interact with the <meta> value if both are used.

[Timothy Hatcher]

Created attachment 354185 [details]
Patch

[Simon Fraser (smfr)]

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?

> 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.

[Timothy Hatcher]

Created attachment 354212 [details]
Patch

[Timothy Hatcher]

(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.

[Timothy Hatcher]

Created attachment 354214 [details]
Patch

[Timothy Hatcher]

Created attachment 354219 [details]
Patch

[Timothy Hatcher]

Created attachment 354238 [details]
Patch

[Simon Fraser (smfr)]

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.

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 354256 [details]
Patch

[Timothy Hatcher]

Comments addressed (other than the computed style use in canvas).

[Dean Jackson]

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?

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 354265 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354265 [details]
Patch

Clearing flags on attachment: 354265

Committed r238001: <https://trac.webkit.org/changeset/238001>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.