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.
Created attachment 353985 [details] WIP
<rdar://problem/45852261>
Let's write up the GitHub issue before we land this.
Created attachment 354183 [details] Patch
Simon wrote up the GitHub issue at: https://github.com/w3c/csswg-drafts/issues/3299
My patch supports an `auto` value to let the CSS property interact with the <meta> value if both are used.
Created attachment 354185 [details] Patch
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.
Created attachment 354212 [details] Patch
(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.
Created attachment 354214 [details] Patch
Created attachment 354219 [details] Patch
Created attachment 354238 [details] Patch
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 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.
Created attachment 354256 [details] Patch
Comments addressed (other than the computed style use in canvas).
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 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.
Created attachment 354265 [details] Patch
Comment on attachment 354265 [details] Patch Clearing flags on attachment: 354265 Committed r238001: <https://trac.webkit.org/changeset/238001>
All reviewed patches have been landed. Closing bug.