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-color] Resolving color-mix() #7302

Open
emilio opened this issue May 23, 2022 · 33 comments
Open

[css-color] Resolving color-mix() #7302

emilio opened this issue May 23, 2022 · 33 comments
Labels
css-color-5 Color modification

Comments

@emilio
Copy link
Collaborator

emilio commented May 23, 2022

https://drafts.csswg.org/css-color-5/#resolving-contrast has some rules to resolve color-contrast() values, however I'm not sure they are clear or they match what implementations want or can do.

web-platform-tests/wpt#34158 is an example of this, where it seems like WebKit resolves color-mix() at parse-time (they don't seem to support currentColor inside color-mix()).

In Gecko, I opted for the approach of keeping color-mix() in the specified value, but resolve at computed-value time (except for currentColor which is special of course, but that's not observable anyways because getComputedStyle returns resolved colors).

It's clear that some amount of these functions needs to be preserved all the way to computed-value time (system colors), or even used-value time, for currentColor.

However, it's not clear what amount needs to be preserved at parse-time vs. not. Generally, we preserve stuff at parse-time, (so element.style.color would return color-mix(red, blue) for example. But then there's the precedent of calc() where we normalize generally as much as possible.

I tend to think preserving the function at parse-time is simpler and maybe less confusing for authors, but I don't particularly mind either way.

cc @weining @svgeesus @lilles

@emilio emilio added Agenda+ css-color-5 Color modification labels May 23, 2022
@weinig
Copy link

weinig commented May 24, 2022

I think you meant to cc me (@weinig).

I am a bit unclear on where CSS stands with regards to preserving input. For instance, I CSS Images 4 says this in its serialization section:

To serialize any function defined in this module, serialize it per its individual grammar, in the order its grammar is written in, omitting components when possible without changing the meaning, joining space-separated tokens with a single space, and following each serialized comma with a single space.

I would love for this to be more explicit in more places.

@emilio
Copy link
Collaborator Author

emilio commented May 24, 2022

Gah, yeah, got tricked by the autocomplete, sorry!

@svgeesus
Copy link
Contributor

@emilio said:

In Gecko, I opted for the approach of keeping color-mix() in the specified value, but resolve at computed-value time (except for currentColor which is special of course, but that's not observable anyways because getComputedStyle returns resolved colors).

That seems preferable to me; and currentColor gets resolved at used-value time.

@weinig I assume the fact that WebKit currently resolves color-mix() at parse time, and the fact that it currently doesn't handle currentColor in color-mix(), are related?

CSS Color 5 doesn't mention color-mix() in the Resolving Color Values section, and it should. Maybe we can nail this down this week (in this issue, and resolve on the call)?

@svgeesus
Copy link
Contributor

svgeesus commented May 31, 2022

CSS Color 5 doesn't mention color-mix() in the Resolving Color Values section, and it should. Maybe we can nail this down this week (in this issue, and resolve on the call)?

Proposed text for review is now at

7.2. Resolving color-mix() values

and revised text and example at

7.1 Resolving color-contrast() values

Both of these specifically call out currentColor, which does not resolve to a corresponding color in a specific color space.

Comments and corrections welcome.

Once we have agreement I will add more examples to both sections.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed css-color] Resolving color-mix() / color-contrast().

The full IRC log of that discussion <dael> Topic: css-color] Resolving color-mix() / color-contrast()
<dael> github: https://github.com//issues/7302
<chris> People should also have https://github.com//issues/6168 open as they cover basically the same thing
<dael> chris: 7302 is about resolving color-mix and color-contrast. Have spec text but didn't say how to resolve
<dael> chris: Have another issue that talks about same thing. I'll copy discussion to both
<dael> chris: Suggested wording, emilio said it's slightly wrong and I agree. There is prop wording in color 5
<dael> chris: lea did a little codepen that implt color-mix so you can see how it works and get calc
<dael> chris: Useful to see what's happening, particularly when currentColor involved.
<dael> chris: If we can agree here I'll put this in the spec
<dael> chris: Color-mix has to inferit in as the whole thing
<chris> https://codepen.io/svgeesus/pen/QWQrQqr
<dael> jensimmons: codepen URL? I don't see it in the issue
<fantasai> +1 to computing currentColor components to themselves
<fantasai> even if that requires maintaining notations through inheritance
<dael> chris: It's in the other issue; discussion is happening in both
<dael> chris: I put in concrete wording. emilio pointed out it's a bit wrong, but other then that do people like the wording?
<dael> chris: smfr or jensimmons do you know what Sam is thinking?
<dael> smfr: Not sure I do. Can you give prop wording comment?
<chris> https://drafts.csswg.org/css-color-5/#resolving-color-values
<dael> chris: Dropping a link
<dael> chris: emilio didn't like the section being called resolving color values and I asked him about that. Didn't hear back yet. That's what it's named in color 4
<dael> fantasai: Call it computing color values?
<dael> chris: Resolving makes it sound used. This is both computed and used value
<dael> fantasai: I don't know what to do. It's fine then.
<dael> chris: It's different if currentColor is involved. In WK this is at parse time and WK doesn't do currentColor b/c it didn't know what to do. I suspect WK will need to change
<dael> fantasai: If I understand intention, it's that you compute the winning color and that becomes computed. If currentColor is used you compute individual values but don't resolve until used value time
<dael> fantasai: That makes sense. Not exactly what you wrote but if that's what you're going for good
<dael> fantasai: You wrote another sentence about resolving the used value to compute...confusing.
<dael> chris: That is intent. used is the winning color for contrast and the mix value for mix
<dael> fantasai: used is always a color. current is not always for currentColor
<dael> chris: Yes.
<fantasai> s/current is/computed is/
<dael> chris: Other thing is emilio opened an issue a while ago that if we resolve another bug happy to ship. bug closed today so I take as happyt o ship from Moz. Want to know if WK is happy to ship too
<dael> chris: Both browsers you have to run with flag enabled to get riht result so all tests flag
<dael> smfr: I think with WPT it runs with flags enabled
<dael> jensimmons: I think it enables for chrome and firefox but not safari
<dael> smfr: May want to fix currentColor before shipping on by default
<dael> chris: But sounds near-term thing on what spec should say?
<dael> fantasai: Seems like we should fix up spec to say what we want shipped and once that's ready we say go ahead
<dael> Rossen_: Do you want to go back and do that and let us know once ready
<dael> chris: Have to assume that's done to cover item 3 on agenda
<dael> Rossen_: We can resolve to accept text that is not quite correct. Let's consider the request once more next week and we can go from there
<dael> chris: Okay

@josepharhar
Copy link
Contributor

I implemented color-contrast() in chromium a little while ago, but I've been holding off on shipping it because I got feedback that I was doing the whole calculation at parse time instead of used-value time, which was making it not work properly with currentColor.

I guess I sort of assumed that color-contrast was already specced to be resolved at used-value time instead of parse time, but now that I see this issue I'm not so sure. I'm having a hard time fully following the discussion here, but is the idea that we should make color-contrast get resolved at used-value time and support currentColor? Are there any WPTs for this?

(I don't know what the difference is between used-value time and computed-value time)

cc @andruud

@svgeesus
Copy link
Contributor

svgeesus commented Jun 2, 2022

(I don't know what the difference is between used-value time and computed-value time)

Computed values inherit to children, so that affects currentColor

@svgeesus
Copy link
Contributor

@emilio
Copy link
Collaborator Author

emilio commented Jun 13, 2022

@svgeesus The specified value shouldn't resolve colors. color-mix(blue, red) should be color-mix(blue, red), not color-mix(rgb(0, 0, 255), rgb(255, 0, 0)).

@svgeesus
Copy link
Contributor

Ah, I see. So, just remove the text there about specified values?

@svgeesus
Copy link
Contributor

@emilio now?

@emilio
Copy link
Collaborator Author

emilio commented Jun 14, 2022

I think so, thanks!

@svgeesus
Copy link
Contributor

fantasai: Seems like we should fix up spec to say what we want shipped and once that's ready we say go ahead
Rossen_: Do you want to go back and do that and let us know once ready

Its ready, here is the text on resolving color-mix() and resolving color-contrast()

@astearns astearns removed the Agenda+ label Jun 15, 2022
@svgeesus svgeesus changed the title [css-color] Resolving color-mix() / color-contrast() [css-color] Resolving color-mix() Jun 15, 2022
@mysteryDate
Copy link

From https://drafts.csswg.org/css-color-5/#resolving-mix:

Otherwise (if currentColor was used in the function), the computed value is the color-mix() function with each parameter...

Is currentColor the only issue? What about system colors that depend on the color-scheme computed value, e.g.:

div { color: color-mix(in lch, canvastext, blue) }
div.dark { color-scheme: dark; }

Depending on whether an element has the class "dark" or not, the computed value will be different. Serializing the color declaration in the div rule above cannot give a reasonable serialization as a <color>.

@lilles, who I am basically quoting here, might have more to say.

@mysteryDate
Copy link

@svgeesus The specified value shouldn't resolve colors. color-mix(blue, red) should be color-mix(blue, red), not color-mix(rgb(0, 0, 255), rgb(255, 0, 0)).

@svgeesus Should we add some tests here to reflect that?
https://github.com/web-platform-tests/wpt/blob/master/css/css-color/parsing/color-valid-color-mix-function.html

@svgeesus
Copy link
Contributor

Should we add some tests here to reflect that?

That would be great

@svgeesus
Copy link
Contributor

svgeesus commented Oct 11, 2022

System colors are already defined:

Each <system-color> keyword computes to the corresponding color in its color space. However, such colors must not be altered by 'forced colors mode'.

@mysteryDate
Copy link

System colors are already defined:

Each <system-color> keyword computes to the corresponding color in its color space. However, such colors must not be altered by 'forced colors mode'.

What is this in response to?

@mysteryDate
Copy link

Should we add some tests here to reflect that?

That would be great

See: crrev.com/c/3945930

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 11, 2022
From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 11, 2022
From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
@mysteryDate
Copy link

It seems like firefox nightly never eagerly evaluates the resulting color mix, input values are always returned as-is at specified value time. I'm beginning to think that the expectations in this test are entirely incorrect?

https://github.com/web-platform-tests/wpt/blob/master/css/css-color/parsing/color-valid-color-mix-function.html

@svgeesus
Copy link
Contributor

What is this in response to?

It was in response to

Is currentColor the only issue? What about system colors that depend on the color-scheme computed value, e.g.:

@svgeesus
Copy link
Contributor

div { color: color-mix(in lch, canvastext, blue) }
div.dark { color-scheme: dark; }

Depending on whether an element has the class "dark" or not, the computed value will be different. Serializing the color declaration in the div rule above cannot give a reasonable serialization as a <color>.

Wouldn't the serialization on the div use the initial value of color-scheme which is normal?

@mysteryDate
Copy link

Okay, I'm still wondering about this test though:

https://github.com/web-platform-tests/wpt/blob/master/css/css-color/parsing/color-valid-color-mix-function.html

Is it valid? Firefox is not eagerly computing the colors at specified value time, though Safari is.

@mysteryDate
Copy link

Another open question: what should we do for negative calc values? From https://w3c.github.io/csswg-drafts/css-values-4/#calc-range:

Parse-time range-checking of values is not performed within math functions, and therefore out-of-range values do not cause the declaration to become invalid. However, the value resulting from an expression must be clamped to the range allowed in the target context. Clamping is performed on computed values to the extent possible, and also on used values if computation was unable to sufficiently simplify the expression to allow range-checking. (Clamping is not performed on specified values.)

This would mean that for something like color-mix(in srgb, red calc(-10%), blue);, calc(-10%) will get accepted, then clamped to zero, so the resulting color will be blue. This is what Firefox is doing, Safari rejects the input.

@svgeesus
Copy link
Contributor

svgeesus commented Oct 14, 2022

Another open question: what should we do for negative calc values?

Hmm. I thought that was already covered:

Percentages are required to be in the range 0% to 100%. Negative percentages are specifically disallowed.

But then the list of steps for percentage normalization doesn't say what to do if they happen anyway. It needs a new step between 2 and 3 where any value < 0% gets clamped to 0%, and any value > 100% gets clamped to 100%

@LeaVerou
Copy link
Member

Another open question: what should we do for negative calc values?

This is not something that needs to be defined specifically for color-mix(), it's the same handling as any other property or function that only allows positive numbers, e.g. padding or <border-radius> in inset().

@lilles
Copy link
Member

lilles commented Oct 14, 2022

Another open question: what should we do for negative calc values?

This is not something that needs to be defined specifically for color-mix(), it's the same handling as any other property or function that only allows positive numbers, e.g. padding or <border-radius> in inset().

Yes, this is generally covered by the css-values spec which says e.g. calc(-10%) should not be invalid at parse/specified time here even if negative percentages are not allowed, but should be clamped to the allowed range at computed value time.

There is one tricky part that I'm not sure if we have for other properties, and that is that color-mix() can be invalid even when both percentage values are separately within the allowed range. If you have two negative percentages (or 0%), and they are clamped at computed value time, you will end up with two 0% values which is still invalid per spec.

@svgeesus
Copy link
Contributor

If you have two negative percentages (or 0%), and they are clamped at computed value time, you will end up with two 0% values which is still invalid per spec.

I agree that this case needs to be explicitly specified. I suggest it is treated the same as "both percentages omitted" which results in a 50:50 mix.

@svgeesus
Copy link
Contributor

it's the same handling as any other property or function that only allows positive numbers, e.g. padding or <border-radius> in inset()

True, although some examples demonstrating this wouldn't hurt.

@lilles
Copy link
Member

lilles commented Oct 17, 2022

If you have two negative percentages (or 0%), and they are clamped at computed value time, you will end up with two 0% values which is still invalid per spec.

I agree that this case needs to be explicitly specified. I suggest it is treated the same as "both percentages omitted" which results in a 50:50 mix.

I don't have any strong opinions here.

@lilles
Copy link
Member

lilles commented Oct 17, 2022

If you have two negative percentages (or 0%), and they are clamped at computed value time, you will end up with two 0% values which is still invalid per spec.

I agree that this case needs to be explicitly specified. I suggest it is treated the same as "both percentages omitted" which results in a 50:50 mix.

I don't have any strong opinions here.

Another possibility is to make it invalid at computed value time. That would be the first case of values being invalid at computed value time for values not involving var() references.

@svgeesus
Copy link
Contributor

I can see the case for IACVT. Deliberately omitting both percentages has clear authorial intent (mix these two colors, equally) while two negatives does not.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 2, 2022
From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 2, 2022
From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 2, 2022
From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3945930
Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
Reviewed-by: Juanmi Huertas <juanmihd@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066591}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 2, 2022
From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3945930
Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
Reviewed-by: Juanmi Huertas <juanmihd@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066591}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 12, 2022
…lor-mix function, a=testonly

Automatic update from web-platform-tests
Add tests for not resolving colors in color-mix function

From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3945930
Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
Reviewed-by: Juanmi Huertas <juanmihd@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066591}

--

wpt-commits: a9e397396e32d91d52344ea61ea3bdd5c361dcf5
wpt-pr: 36404
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 14, 2022
…lor-mix function, a=testonly

Automatic update from web-platform-tests
Add tests for not resolving colors in color-mix function

From the discussion here:
w3c/csswg-drafts#7302

Keyword colors shouldn't be resolved as specified colors.

Bug: 1362022
Change-Id: I3e4f3dd7220ac84777a1edddd54f87ead217d6b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3945930
Commit-Queue: Juanmi Huertas <juanmihd@chromium.org>
Reviewed-by: Juanmi Huertas <juanmihd@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066591}

--

wpt-commits: a9e397396e32d91d52344ea61ea3bdd5c361dcf5
wpt-pr: 36404
@svgeesus
Copy link
Contributor

Looping back to this, was the consensus to make negative values which then get clamped to 0 and sum to 0 IACVT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-color-5 Color modification
Projects
None yet
Development

No branches or pull requests

9 participants