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

color-mix() tests look wrong. #34158

Open
emilio opened this issue May 21, 2022 · 15 comments
Open

color-mix() tests look wrong. #34158

emilio opened this issue May 21, 2022 · 15 comments

Comments

@emilio
Copy link
Contributor

emilio commented May 21, 2022

The tests introduced in #31902 and #32711 look wrong to me. They expect color-mix() to go away at parse-time, which seems incorrect since currentColor can't go away at parse time, so at the very least color-mix should be resolved at computed-value time.

cc @weinig @svgeesus @nt1m

@svgeesus
Copy link
Contributor

Looks like https://drafts.csswg.org/css-color-5/#resolving-color-values needs a section on color-mix().

Now we don't have to track all the system colors, the result should be either a <color> or the whole function (if one of the colors being mixed is currentColor), right?

@svgeesus
Copy link
Contributor

Or if one of them is a custom property. So yeah, at computed-value time.

@emilio
Copy link
Contributor Author

emilio commented May 22, 2022

Custom properties would work at parse time... I don't know, maybe it'd be ok to resolve as eagerly as possible, sorta like calc() does. Maybe @tabatkins has opinions?

@emilio
Copy link
Contributor Author

emilio commented May 22, 2022

But yeah system colors and currentColor would need to wait until computed value time at least, because color-scheme and such affect how they resolve.

@weinig
Copy link
Contributor

weinig commented May 24, 2022

the tests introduced in #31902 and #32711 look wrong to me

Hi @emilio. Can you give me an example of one of the tests (perhaps a line in color-mix-computed.html?) that you think is wrong? I left our any tests of currentColor because I though the serialization was not fully spec'd yet, so I think all the tests are just on color values.

@weinig
Copy link
Contributor

weinig commented May 24, 2022

(actually, one thing that does look wrong, or rather probably just outdated, is that the color-mix(in hsl..). and color-mix(in hwb...) are resolving to rgb(...) but should be resolving to color(srgb ...) now I think, per the changes in https://drafts.csswg.org/css-color-5/#serial-color-mix)

@emilio
Copy link
Contributor Author

emilio commented May 24, 2022

@weinig All the tests in parsing/color-mix-valid.html and so on expect rgb output in the specified style, which confused me. But then I realized the spec isn't particularly clear, thus I filed w3c/csswg-drafts#7302.

FWIW in general I'd rather serialize srgb colors as rgb/rgba, both for simplicity, but also because that way we can work on the different color-5 features more independently (we don't need to implement color() for color-mix() etc.

But I guess that's a different topic altogether anyways :)

@weinig
Copy link
Contributor

weinig commented May 24, 2022

@emilio I think the spec is clear on how to serialize all the examples of color-mix() in the tests cases thought, see https://drafts.csswg.org/css-color-5/#serial-color-mix, as that states the style it is expected in.

As for not needing color(), I think that would be detriment to color-mix() as it would mean elongated the life of legacy color syntax and clamping precision below where it is in today's spec.

@emilio
Copy link
Contributor Author

emilio commented May 24, 2022

@emilio I think the spec is clear on how to serialize all the examples of color-mix() in the tests cases thought, see https://drafts.csswg.org/css-color-5/#serial-color-mix, as that states the style it is expected in.

Well, sure. The thing is whether we should serialize the result at specified-value time vs at computed-value time, right? Which is w3c/csswg-drafts#7302. currentColor and system colors at least need to be serialized as a color-mix(), in the specified style (though it seems WebKit doesn't support it).

As for not needing color(), I think that would be detriment to color-mix() as it would mean elongated the life of legacy color syntax and clamping precision below where it is in today's spec.

That's fair enough.

@emilio
Copy link
Contributor Author

emilio commented May 24, 2022

(To clarify, I think depending on the answer of w3c/csswg-drafts#7302, those examples in color-mix-valid.html should be moved to color-mix-computed.html to check the computed value).

@weinig
Copy link
Contributor

weinig commented May 24, 2022

Sorry, so, I just want to make sure I understand, so let's just take one test line, like this one from color-mix-valid.html:

test_valid_value(`color`, `color-mix(in hsl, hsl(120deg 10% 20%), hsl(30deg 30% 40%))`, `rgb(84, 92, 61)`);

Is your assertion that the this should instead be:

test_valid_value(`color`, `color-mix(in hsl, hsl(120deg 10% 20%), hsl(30deg 30% 40%))`, `color-mix(in hsl, hsl(120deg 10% 20%), hsl(30deg 30% 40%))`);

@emilio
Copy link
Contributor Author

emilio commented May 25, 2022

Yes (or just omitting the third argument)

@emilio
Copy link
Contributor Author

emilio commented May 25, 2022

Well actually it should probably use rgb() because browsers historically convert hsl to rgb at parse time... Colors are quite messy.. :(

@weinig
Copy link
Contributor

weinig commented May 25, 2022

It's not clear to me there is much value in preserving all that for serialization when a more compact representation is available. Sure, for cases like currentColor it would be necessary to preserve more, but for the cases I wrote test cases for I think the compact representation is preferred.

As for the using legacy rgb() for hsl/hwb color-mix(). This was how things worked originally, but it was explicitly change to use color(srgb ...) to avoid the precision loss associated with legacy rgb() (which would be unfortunate in a mixing use case).

@weinig
Copy link
Contributor

weinig commented May 25, 2022

I guess this really comes down to how import is preserving the full fidelity or the original source text input when round tripping in this context.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 31, 2022
I made sure the new fixes are covered. There are still a bunch of failures.

Those can be explained by:

 * web-platform-tests/wpt#34158 (tests expecting color-mix() to go away at parse time)
 * Off-by-one in the color channels (so float precision errors).
 * Unimplemented features like the color() function.

Differential Revision: https://phabricator.services.mozilla.com/D147008
aosmond pushed a commit to aosmond/gecko that referenced this issue Jun 3, 2022
I made sure the new fixes are covered. There are still a bunch of failures.

Those can be explained by:

 * web-platform-tests/wpt#34158 (tests expecting color-mix() to go away at parse time)
 * Off-by-one in the color channels (so float precision errors).
 * Unimplemented features like the color() function.

Differential Revision: https://phabricator.services.mozilla.com/D147008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants