Skip to content
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.

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

[css-fonts-4] absolute-color in override-colors unspecified #7010

Closed
drott opened this issue Feb 3, 2022 · 10 comments
Closed

[css-fonts-4] absolute-color in override-colors unspecified #7010

drott opened this issue Feb 3, 2022 · 10 comments
Labels
css-fonts-4 Current Work

Comments

@drott
Copy link
Collaborator

drott commented Feb 3, 2022

9.2.3. Overriding a colors from a palette: The override-colors descriptor

says:

Value: | [<integer [0,∞]> <absolute-color> ]#

But I can't find a reference for absolute-color. Has the color spec changed after this? I assume the intention is to allow <<absolute-color-base>> | currentcolor but not | <<system-color>> | <<device-cmyk()>>.

CC @litherum @svgeesus

Edit: See below for clarifications re currentcolor.

@drott drott added the css-fonts-4 Current Work label Feb 3, 2022
@litherum
Copy link
Contributor

litherum commented Feb 3, 2022

Yes, IIRC the change that added that grammar also modified the color spec.

@svgeesus
Copy link
Contributor

svgeesus commented Feb 4, 2022

Bikeshed reports, on building CSS Fonts 4

LINE ~6533: No 'type' refs found for '<absolute-color>'.
<<absolute-color>>

probably because CSS Color 4 has

<pre class='prod'>
	&lt;color> = <<absolute-color-base>> | currentcolor | <<system-color>> | <<device-cmyk()>>

	<dfn>&lt;absolute-color-base></dfn> = <<hex-color>> | <<named-color>> | transparent |
	    <<rgb()>> | <<rgba()>> | <<hsl()>> | <<hsla()>> | <<hwb()>> |
	    <<lab()>> | <<lch()>> |
	    <<oklab()>> | <<oklch()>> |
	    <<color()>>
	</pre>

So, the fix is to change Fonts 4 to use the actual exported term :)

@svgeesus
Copy link
Contributor

svgeesus commented Feb 4, 2022

Oh I just noticed

I assume the intention is to allow <<absolute-color-base>> | currentcolor

If that is correct, I need to make a different edit. Is currentColor to be included?

@svgeesus svgeesus reopened this Feb 4, 2022
@litherum
Copy link
Contributor

litherum commented Feb 5, 2022

I intentionally omitted currentColor . c790f28 is the edit. You can see how currentColor was supposed to be part of the <color> production, not the <absolute-color> production.

@svgeesus
Copy link
Contributor

svgeesus commented Feb 7, 2022

Okay in that case, what we currently have is correct

@svgeesus svgeesus closed this as completed Feb 7, 2022
@drott
Copy link
Collaborator Author

drott commented Feb 7, 2022

Thanks for fixing the reference.

Okay in that case, what we currently have is correct

But if it was <<absolute-color>> before, and now we use <<absolute-color-base>>, it means dropping <<absolute-device-cmyk>> - is that intended (I may have missed a continuation of the conversation in #6681 (review).

@svgeesus
Copy link
Contributor

svgeesus commented Feb 9, 2022

Ah, I see. At the time of c790f28 device-cmyk() had a fallback color so it was necessary to split off an <<absolute-device-cmyk>> with no fallback.

So yes this needs a further edit to add back device-cmyk.

@svgeesus svgeesus reopened this Feb 9, 2022
@drott
Copy link
Collaborator Author

drott commented Feb 10, 2022

I am in favour of excluding currentColor, see also my comment: web-platform-tests/wpt#31517 - font-palette-35.html and font-palette-36.html WPT tests should probably be deleted. (CC @faceless2 )

From an implementation perspective, I find it difficult to resolve currentColor at the time of font instantiation, or vice versa: The time of being able to resolve currentColor is not the time of when the font is getting instantiated (for which resolved/absolute colors are needed). For the purpose of current color usage, I suggest a COLRv0/COLRv1 font that uses color index 0xFFFF in the glyph descriptions (which can't be overridden).

@svgeesus
Copy link
Contributor

svgeesus commented Feb 10, 2022

Yes, I think we are all agreed that currentcolor should (continue to be) excluded. So yes, I agree WPT using it should be removed.

The remaining question is whether to add the device-cmyk back, which I guess we should do.

drott added a commit to drott/csswg-drafts that referenced this issue Feb 11, 2022
Current color excluded from possible values for override-colors:
entries, see discussion in
w3c#7010
svgeesus pushed a commit to web-platform-tests/wpt that referenced this issue Feb 13, 2022
svgeesus pushed a commit that referenced this issue Feb 13, 2022
Current color excluded from possible values for override-colors:
entries, see discussion in
#7010
mattwoodrow pushed a commit to mattwoodrow/wpt that referenced this issue Feb 15, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 26, 2022
… in @font-palette-values", a=testonly

Automatic update from web-platform-tests
Revert "New tets for use of currentColor in @font-palette-values"

This reverts commit e14ed8613f431e528b83f074ce37cf6e22dd95b6.

As discussed in
web-platform-tests/wpt#31517 and
w3c/csswg-drafts#7010

--

wpt-commits: 13b16f2df09ae8b579fbf9ebeca5f6153f494d55
wpt-pr: 32812
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Feb 28, 2022
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 1, 2022
… in @font-palette-values", a=testonly

Automatic update from web-platform-tests
Revert "New tets for use of currentColor in @font-palette-values"

This reverts commit e14ed8613f431e528b83f074ce37cf6e22dd95b6.

As discussed in
web-platform-tests/wpt#31517 and
w3c/csswg-drafts#7010

--

wpt-commits: 13b16f2df09ae8b579fbf9ebeca5f6153f494d55
wpt-pr: 32812
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 8, 2022
… in @font-palette-values", a=testonly

Automatic update from web-platform-tests
Revert "New tets for use of currentColor in @font-palette-values"

This reverts commit e14ed8613f431e528b83f074ce37cf6e22dd95b6.

As discussed in
web-platform-tests/wpt#31517 and
w3c/csswg-drafts#7010

--

wpt-commits: 13b16f2df09ae8b579fbf9ebeca5f6153f494d55
wpt-pr: 32812
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 8, 2022
… in @font-palette-values", a=testonly

Automatic update from web-platform-tests
Revert "New tets for use of currentColor in @font-palette-values"

This reverts commit e14ed8613f431e528b83f074ce37cf6e22dd95b6.

As discussed in
web-platform-tests/wpt#31517 and
w3c/csswg-drafts#7010

--

wpt-commits: 13b16f2df09ae8b579fbf9ebeca5f6153f494d55
wpt-pr: 32812
@svgeesus
Copy link
Contributor

svgeesus commented Nov 2, 2023

The remaining question is whether to add the device-cmyk back, which I guess we should do.

@drott see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-fonts-4 Current Work
Projects
None yet
Development

No branches or pull requests

3 participants