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

"parse a CSS <color> value" should take into account that currentColor can be nested in color-mix() #9940

Closed
Loirooriol opened this issue Nov 19, 2023 · 16 comments · Fixed by #10175

Comments

@Loirooriol
Copy link
Contributor

Loirooriol commented Nov 19, 2023

What is the issue with the HTML Standard?

https://html.spec.whatwg.org/multipage/infrastructure.html#parsed-as-a-css-color-value

It assumes that currentColor, if present, needs to be the whole value of the color property.

However, this is perfectly valid:

color: yellow;
background-color: color-mix(in srgb, currentcolor, cyan); /* color(srgb 0.5 1 0.5) */

So consider

<canvas style="color: yellow"></canvas>
<script>
var canvas = document.querySelector("canvas");
var ctx = canvas.getContext("2d");
ctx.shadowColor = "red";
ctx.shadowColor = "color-mix(in srgb, currentcolor, cyan)";
document.body.append(ctx.shadowColor);
</script>

The shadowColor ends up being #ff0000 in both Blink and WebKit, i.e. they reject color-mix(in srgb, currentcolor, cyan), even though they accept it in CSS. They also reject color-mix(in srgb, cyan, cyan) even without a nested currentcolor.

In Firefox, shadowColor becomes #008080, so I guess it treats currentcolor as #000000, even though a lone currentcolor would become #ffff00.

In Servo I plan to do what I think is more reasonable (modulo #8917) and produce #80ff80.

@annevk
Copy link
Member

annevk commented Nov 20, 2023

This seems like another case where new CSS features regressed HTML. That argues for changing the layering such that HTML calls even more directly into CSS and CSS maintains the API such that it doesn't break when new features are added.

cc @tabatkins @svgeesus @whatwg/css

@nt1m
Copy link
Member

nt1m commented Nov 20, 2023

I think the expected result here is clear, it is the behavior that you are planning to implement in Servo. Which is just replacing the usages of currentColor with what the HTML spec defines, then taking the computed color.

I would consider this as implementation bugs rather than a spec issue. I don't think the spec has any ambiguity here.

@nt1m
Copy link
Member

nt1m commented Nov 20, 2023

I guess the CSS spec could define a "resolve a color algorithm" that takes in a parameter that is the current color.

@Loirooriol
Copy link
Contributor Author

I still think it's a spec issue, because https://html.spec.whatwg.org/multipage/infrastructure.html#parsed-as-a-css-color-value says

If color is currentcolor, then:

So it's not covering currentcolor nested inside color-mix().

CSS already has https://drafts.csswg.org/css-color/#resolving-color-values, I guess HTML could invoke that but overriding the resolution of currentcolor in https://drafts.csswg.org/css-color/#resolving-other-colors

@annevk
Copy link
Member

annevk commented Nov 20, 2023

It would be good if CSS had a dedicated entry point for this that it would commit to supporting long term. We've seen here that locally overriding something is not tenable so we should not move to a solution that still does that.

@tabatkins
Copy link
Collaborator

Yeah, we can make an algo for that, that can take an optional element to supply the context-specific data. (There's more than just currentcolor; the system keywords need an element ref to know what color scheme to be.)

@tabatkins
Copy link
Collaborator

Okay, check https://drafts.csswg.org/css-color-4/#parse-color in a few minutes.

@Loirooriol
Copy link
Contributor Author

Seems to assume there is a used value. Do we have a used value with display: none?

@tabatkins
Copy link
Collaborator

For colors? Not sure why we wouldn't, yeah.

@annevk
Copy link
Member

annevk commented Nov 21, 2023

@tabatkins I think that implies a change from opaque black to CanvasText. And I guess by default that does the correct thing, but not sure it always does?

@tabatkins
Copy link
Collaborator

It will always do the "correct" thing, aka "give the same color you'd get if you put that value into a CSS property on the element". And without an element for context, in theory it could give something besides opaque black, but in practice with color-scheme: light (the initial value, which is used when there's no context element) CanvasText will be opaque black.

@svgeesus
Copy link

svgeesus commented Dec 4, 2023

Okay, check https://drafts.csswg.org/css-color-4/#parse-color in a few minutes.

Nice, but also needs a copy in CSS Color 5, because Color 4 doesn't have, for example, Resolving color-mix()

@svgeesus
Copy link

svgeesus commented Dec 4, 2023

I still think it's a spec issue, because https://html.spec.whatwg.org/multipage/infrastructure.html#parsed-as-a-css-color-value says

If color is currentcolor, then:

So it's not covering currentcolor nested inside color-mix().

Also not covering currentcolor inside relative color syntax which now has a bunch of tests

@tabatkins
Copy link
Collaborator

I'm not sure I understand. The text covers future additions to <color> just fine as-is, yeah? It just invokes "resolve", and Color 5 adds additional ways to resolve a color.

When we un-delta Color 5 it'll move over, but I don't see any particular need to duplicate it right now.

@svgeesus
Copy link

svgeesus commented Dec 5, 2023

Okay, just checking.

annevk added a commit that referenced this issue Mar 4, 2024
Our definition had issues such as not resolving currentcolor correctly when used as part of CSS color functions.

Fixes #9940.
@annevk
Copy link
Member

annevk commented Mar 4, 2024

Thanks everyone. I put up #10175.

annevk added a commit that referenced this issue Mar 5, 2024
Our definition had issues such as not resolving currentcolor correctly when used as part of CSS color functions.

Fixes #9940.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants