Skip to content

[css-fonts] [palettes] Context for resolving override-color colors #6680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
litherum opened this issue Sep 24, 2021 · 2 comments · Fixed by #6681
Closed

[css-fonts] [palettes] Context for resolving override-color colors #6680

litherum opened this issue Sep 24, 2021 · 2 comments · Fixed by #6681
Labels
css-fonts-4 Current Work

Comments

@litherum
Copy link
Contributor

litherum commented Sep 24, 2021 •
edited
Loading

Colors in CSS are resolved within a context. For example:

<div style="color-scheme: dark; color: text;">Hello, World!</div>
<div style="color-scheme: light; color: text;">Hello, World!</div>

In this example, the meaning of text is different for the two different elements, because of some other property: color-scheme. A similar phenomenon occurs for the currentColor keyword and the color property.

What is the context that colors in @font-palette-values are resolved within?

I think we have 3 options:

  1. Disable these context-sensitive color keywords outright. override-color: 0 text; would be a parse error.
  2. Resolve the color keywords in the context of the root element.
  3. Resolve the color keywords in the context of the element referencing the @font-palette-values rule.

Option 3 is significantly more difficult than the others to implement, because it means that a single @font-palette-values rule will behave differently when applied to different elements, even if the two different elements have the same values for font-family and font-palette. It's particularly difficult in WebKit, where our text engine is divorced from the CSS engine - it would entail treating color, color-scheme, and anything else that can affect color resolution as "font properties" which get threaded throughout the text engine. This also means that our font caches will have to be sensitive to a bunch of (seemingly unrelated) color properties - and even if the cache hit rate won't decrease, simply the act of computing hashes and comparisons of bigger objects has real cost.

Option 1 would be the best, I think, at least to start with, until authors specifically ask for this feature. I can see how it might be useful to have your palette update based on the document, but I think this functionality would essentially be a niche within a niche. We can always add in additional flexibility later, if authors ask for it.
@litherum litherum added the css-fonts-4 Current Work label Sep 24, 2021
litherum added a commit to litherum/csswg-drafts that referenced this issue Sep 24, 2021
@litherum
[css-fonts] Context for resolving override-color colors
ebcc6a6 
Fixes w3c#6680.
@litherum litherum mentioned this issue Sep 24, 2021
Merged
@litherum
Copy link
Contributor Author

Pull request: #6681

litherum added a commit to litherum/csswg-drafts that referenced this issue Sep 24, 2021
@litherum
[css-fonts] Context for resolving override-color colors
733d3dc 
Fixes w3c#6680.
litherum added a commit to litherum/csswg-drafts that referenced this issue Sep 24, 2021
@litherum
[css-fonts] Context for resolving override-color colors
c790f28 
Fixes w3c#6680.
@litherum litherum changed the title [css-fonts] Context for resolving override-color colors [css-fonts] [palettes] Context for resolving override-color colors Sep 24, 2021
litherum added a commit to litherum/wpt that referenced this issue Sep 24, 2021
@litherum
[css-fonts] Add some more override-color tests for different color fo…
e42f743 
…rmulations

I didn't include the context-sensitive colors because
w3c/csswg-drafts#6680 isn't resolved yet.
@litherum litherum mentioned this issue Sep 24, 2021
Merged
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Sep 24, 2021
@litherum
Color keywords in override-color cause a crash
74db241 
https://bugs.webkit.org/show_bug.cgi?id=230605
<rdar://problem/83389290>

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

Tests are being upstreamed at web-platform-tests/wpt#30941.

* web-platform-tests/css/css-fonts/parsing/font-palette-values-valid-expected.txt:
* web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html:

Source/WebCore:

I was assuming that consumeColor() would always produce a color. Instead, if the color
was specified as a keyword, consumeColor() would produce the keyword.

This passes in the default context for color resolution, because
w3c/csswg-drafts#6680 isn't resolved yet.

Test: imported/w3c/web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html

* css/parser/CSSParserImpl.cpp:
(WebCore::CSSParserImpl::consumeFontPaletteValuesRule):

Canonical link: https://commits.webkit.org/242113@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283053 268f45cc-cd09-0410-ab3c-d52691b4dbfc
litherum added a commit to web-platform-tests/wpt that referenced this issue Sep 25, 2021
@litherum
[css-fonts] Add some more override-color tests for different color fo…
b55dda0 
…rmulations (#30941)

I didn't include the context-sensitive colors because
w3c/csswg-drafts#6680 isn't resolved yet.
@svgeesus
Copy link
Contributor

I can see how it might be useful to have your palette update based on the document,

Yes, but that could also be done inside some conditional at-rule like @supports etc. So yeah, certainly option 1 is the simplest.

Off to look at your PR

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 4, 2021
@litherum @moz-wptsync-bot
Bug 1733161 [wpt PR 30941] - [css-fonts] Add some more override-color…
4a4ec7d 
… tests for different color formulations, a=testonly

Automatic update from web-platform-tests
[css-fonts] Add some more override-color tests for different color formulations (#30941)

I didn't include the context-sensitive colors because
w3c/csswg-drafts#6680 isn't resolved yet.
--

wpt-commits: b55dda0cb5dc76635fbb14069168a45b9740b9d8
wpt-pr: 30941
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 6, 2021
@litherum @moz-wptsync-bot
Bug 1733161 [wpt PR 30941] - [css-fonts] Add some more override-color…
826ccfc 
… tests for different color formulations, a=testonly

Automatic update from web-platform-tests
[css-fonts] Add some more override-color tests for different color formulations (#30941)

I didn't include the context-sensitive colors because
w3c/csswg-drafts#6680 isn't resolved yet.
--

wpt-commits: b55dda0cb5dc76635fbb14069168a45b9740b9d8
wpt-pr: 30941
bertogg pushed a commit to Igalia/webkit that referenced this issue Oct 9, 2021
Color keywords in override-color cause a crash
aa878dc 
https://bugs.webkit.org/show_bug.cgi?id=230605
<rdar://problem/83389290>

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

Tests are being upstreamed at web-platform-tests/wpt#30941.

* web-platform-tests/css/css-fonts/parsing/font-palette-values-valid-expected.txt:
* web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html:

Source/WebCore:

I was assuming that consumeColor() would always produce a color. Instead, if the color
was specified as a keyword, consumeColor() would produce the keyword.

This passes in the default context for color resolution, because
w3c/csswg-drafts#6680 isn't resolved yet.

Test: imported/w3c/web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html

* css/parser/CSSParserImpl.cpp:
(WebCore::CSSParserImpl::consumeFontPaletteValuesRule):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283053 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this issue Nov 18, 2021
@litherum @Gabisampaio
[css-fonts] Add some more override-color tests for different color fo…
57e603d 
…rmulations (web-platform-tests#30941)

I didn't include the context-sensitive colors because
w3c/csswg-drafts#6680 isn't resolved yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
css-fonts-4 Current Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[css-fonts] Context for resolving override-color colors litherum/csswg-drafts
2 participants
@litherum @svgeesus