Changing color scheme does not update gradients with system colors or `light-dark()` #25394

pxlcoder · 2024-03-03T22:22:43Z

`5596316`

Changing color scheme does not update gradients with system colors or `light-dark()`
https://bugs.webkit.org/show_bug.cgi?id=267790
rdar://121285450

Reviewed by Matthieu Dubet.

`CSSGradientValue`s cache their generated `StyleImage` when possible. Currently,
caching is disallowed when the gradient contains one of the following color
keywords as a color stop: "-internal-document-text", "-webkit-link",
"-webkit-active-link", and "currentcolor".

However, that denylist is not comprehensive with respect to dynamic colors.
Notably, it excludes almost all system color keywords, and `light-dark()`.
Consequently, the same cached `StyleImage` is used even when the used color
scheme of an element changes.

Fix by updating `BuilderState::isColorFromPrimitiveValueDerivedFromElement`
to account for other scenarios where colors change dynamically.

* LayoutTests/imported/w3c/web-platform-tests/css/css-images/gradient/color-scheme-dependent-color-stops-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/gradient/color-scheme-dependent-color-stops-ref.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/gradient/color-scheme-dependent-color-stops.html: Added.
* Source/WebCore/css/StyleColor.cpp:
(WebCore::StyleColor::colorFromKeyword):

Drive-by style fix.

(WebCore::StyleColor::containsColorSchemeDependentColor):

System colors, `light-dark()`, and any other unresolved colors containing
either of those, are color-scheme dependent colors.

* Source/WebCore/css/StyleColor.h:
* Source/WebCore/css/color/CSSUnresolvedColor.cpp:
(WebCore::CSSUnresolvedColor::containsColorSchemeDependentColor const):
* Source/WebCore/css/color/CSSUnresolvedColor.h:
* Source/WebCore/style/StyleBuilderState.cpp:
(WebCore::Style::BuilderState::isColorFromPrimitiveValueDerivedFromElement):

Update to cover colors that are color-scheme dependent.

Canonical link: https://commits.webkit.org/275645@main

bbaae52

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-03T22:22:51Z

EWS run on previous version of this PR (hash d0cd549)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 gtk
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-03T22:25:37Z

EWS run on previous version of this PR (hash 8568573)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

mdubet

Can we convert the test to a WPT one ?

mdubet · 2024-03-04T03:31:39Z

Source/WebCore/css/StyleColor.cpp

@@ -132,6 +132,17 @@ bool StyleColor::containsCurrentColor(const CSSPrimitiveValue& value)
    return false;
 }

+bool StyleColor::containsColorSchemeDependentColor(const CSSPrimitiveValue& value)
+{
+    if (StyleColor::isSystemColorKeyword(value.valueID()))


Does all system colors depend on color scheme ?

Not all of them do, and even the ones that do can be platform-specific. On Cocoa platforms, the majority of them are color-scheme dependent.

However, I think that using an allowlist or denylist for that set of system colors would be complicated to maintain, for limited benefit. Both an allowlist and denylist would have the problem of knowing the keep the list up-to-date, as new colors are added, or colors change their dependency on color-scheme.

Let me know if you feel strongly about a more specific list.

Indeed, having a platform specific list would require some more code (stuff in /platform..etc..) I will follow your judgment for the cost-benefit ratio

Source/WebCore/css/color/CSSUnresolvedColor.cpp

nt1m · 2024-03-04T06:23:55Z

I agree with Matthieu, would be nice to create WPTs

webkit-early-warning-system · 2024-03-04T06:25:23Z

EWS run on current version of this PR (hash bbaae52)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

pxlcoder · 2024-03-04T06:27:14Z

Rewrote the test as a WPT.

… `light-dark()` https://bugs.webkit.org/show_bug.cgi?id=267790 rdar://121285450 Reviewed by Matthieu Dubet. `CSSGradientValue`s cache their generated `StyleImage` when possible. Currently, caching is disallowed when the gradient contains one of the following color keywords as a color stop: "-internal-document-text", "-webkit-link", "-webkit-active-link", and "currentcolor". However, that denylist is not comprehensive with respect to dynamic colors. Notably, it excludes almost all system color keywords, and `light-dark()`. Consequently, the same cached `StyleImage` is used even when the used color scheme of an element changes. Fix by updating `BuilderState::isColorFromPrimitiveValueDerivedFromElement` to account for other scenarios where colors change dynamically. * LayoutTests/imported/w3c/web-platform-tests/css/css-images/gradient/color-scheme-dependent-color-stops-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-images/gradient/color-scheme-dependent-color-stops-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-images/gradient/color-scheme-dependent-color-stops.html: Added. * Source/WebCore/css/StyleColor.cpp: (WebCore::StyleColor::colorFromKeyword): Drive-by style fix. (WebCore::StyleColor::containsColorSchemeDependentColor): System colors, `light-dark()`, and any other unresolved colors containing either of those, are color-scheme dependent colors. * Source/WebCore/css/StyleColor.h: * Source/WebCore/css/color/CSSUnresolvedColor.cpp: (WebCore::CSSUnresolvedColor::containsColorSchemeDependentColor const): * Source/WebCore/css/color/CSSUnresolvedColor.h: * Source/WebCore/style/StyleBuilderState.cpp: (WebCore::Style::BuilderState::isColorFromPrimitiveValueDerivedFromElement): Update to cover colors that are color-scheme dependent. Canonical link: https://commits.webkit.org/275645@main

webkit-commit-queue · 2024-03-04T20:34:14Z

Committed 275645@main (5596316): https://commits.webkit.org/275645@main

Reviewed commits have been landed. Closing PR #25394 and removing active labels.

pxlcoder self-assigned this Mar 3, 2024

pxlcoder added the CSS Cascading Style Sheets implementation label Mar 3, 2024

pxlcoder requested review from nt1m, mdubet and anttijk March 3, 2024 22:23

pxlcoder force-pushed the eng/267790 branch from d0cd549 to 8568573 Compare March 3, 2024 22:25

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 4, 2024

pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Mar 4, 2024

mdubet approved these changes Mar 4, 2024

View reviewed changes

mdubet reviewed Mar 4, 2024

View reviewed changes

Source/WebCore/css/color/CSSUnresolvedColor.cpp Outdated Show resolved Hide resolved

mdubet reviewed Mar 4, 2024

View reviewed changes

Source/WebCore/css/color/CSSUnresolvedColor.cpp Outdated Show resolved Hide resolved

pxlcoder force-pushed the eng/267790 branch from 8568573 to bbaae52 Compare March 4, 2024 06:25

pxlcoder added the merge-queue Applied to send a pull request to merge-queue label Mar 4, 2024

webkit-commit-queue force-pushed the eng/267790 branch from bbaae52 to 5596316 Compare March 4, 2024 20:34

webkit-commit-queue merged commit 5596316 into WebKit:main Mar 4, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 4, 2024

francescorn mentioned this pull request Mar 6, 2024

Update color.json- STP 189 supports light-dark, updating to "preview" francescorn/browser-compat-data#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing color scheme does not update gradients with system colors or `light-dark()` #25394

Changing color scheme does not update gradients with system colors or `light-dark()` #25394

pxlcoder commented Mar 3, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Mar 3, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 3, 2024 •

edited

Loading

mdubet left a comment

mdubet Mar 4, 2024

pxlcoder Mar 4, 2024

mdubet Mar 4, 2024

nt1m commented Mar 4, 2024

webkit-early-warning-system commented Mar 4, 2024 •

edited

Loading

pxlcoder commented Mar 4, 2024

webkit-commit-queue commented Mar 4, 2024

Changing color scheme does not update gradients with system colors or light-dark() #25394

Changing color scheme does not update gradients with system colors or light-dark() #25394

Conversation

pxlcoder commented Mar 3, 2024 • edited by webkit-early-warning-system Loading

5596316

webkit-early-warning-system commented Mar 3, 2024 • edited Loading

webkit-early-warning-system commented Mar 3, 2024 • edited Loading

mdubet left a comment

Choose a reason for hiding this comment

mdubet Mar 4, 2024

Choose a reason for hiding this comment

pxlcoder Mar 4, 2024

Choose a reason for hiding this comment

mdubet Mar 4, 2024

Choose a reason for hiding this comment

nt1m commented Mar 4, 2024

webkit-early-warning-system commented Mar 4, 2024 • edited Loading

pxlcoder commented Mar 4, 2024

webkit-commit-queue commented Mar 4, 2024

Changing color scheme does not update gradients with system colors or `light-dark()` #25394

Changing color scheme does not update gradients with system colors or `light-dark()` #25394

pxlcoder commented Mar 3, 2024 •

edited by webkit-early-warning-system

Loading

`5596316`

webkit-early-warning-system commented Mar 3, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 3, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 4, 2024 •

edited

Loading