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] Tests assume old hsl()/hsla() clipping behavior or depend on unclear gamut mapping to sRGB #45814

Open
weinig opened this issue Apr 20, 2024 · 10 comments

Comments

@weinig
Copy link
Contributor

weinig commented Apr 20, 2024

There are few tests that assume the old hsl()/hsla() clipping behavior (specifically, clipping saturation values greater than 100% to 100%):

css/css-color/t424-hsl-clip-outside-gamut-b.xht
css/css-color/t425-hsla-clip-outside-device-gamut-b.xht

html/canvas/element/fill-and-stroke-styles/2d.fillStyle.parse.hsl-clamp-1.html
html/canvas/element/fill-and-stroke-styles/2d.fillStyle.parse.hsla-clamp-1.html
html/canvas/offscreen/fill-and-stroke-styles/2d.fillStyle.parse.hsl-clamp-1.html
html/canvas/offscreen/fill-and-stroke-styles/2d.fillStyle.parse.hsl-clamp-1.worker.html
html/canvas/offscreen/fill-and-stroke-styles/2d.fillStyle.parse.hsla-clamp-1.html
html/canvas/offscreen/fill-and-stroke-styles/2d.fillStyle.parse.hsla-clamp-1.worker.html

The tests will pass if the host is running with an sRGB display with some gamut mappings, but it doesn't seem like that is something the tests should be depending on.

@nt1m
Copy link
Member

nt1m commented Apr 21, 2024

@mysteryDate

@nt1m
Copy link
Member

nt1m commented Apr 23, 2024

cc @tiaanl @svgeesus @mdubet as well

@mysteryDate
Copy link
Contributor

Yes, I'd be pro deleting them as the debate around gamut mapping is still swirling, though clipping like this is almost certainly not what we want to be doing.

They're generated here:

('hsl-clamp-1', 'hsl(120, 200%, 50%)', 0,255,0,255, ""),

@tiaanl
Copy link
Contributor

tiaanl commented Apr 24, 2024

Agreed, until we're clear on what exactly to expect from colors falling outside of some gamut we should avoid testing for it.

@svgeesus
Copy link
Contributor

These tests are based on two things in CSS Color 3, section 4.2.4 which apply to both HSL and HSLA:

If saturation is less than 0%, implementations must clip it to 0%.

This was discussed in the CSS Color 4 context:

and as a result, for interop, modern syntax HSL and HSLA also require clipping negative saturation.

However the paragraph continues:

If the resulting value is outside the device gamut, implementations must clip it to the device gamut. This clipping should preserve the hue when possible, but is otherwise undefined. (In other words, the clipping is different from applying the rules for clipping of RGB colors after applying the algorithm below for converting HSL to RGB.)

On the face of it this is tautologous: a display is not required to produce colors it can't produce. The mechanism is intentionally unspecified, so the actual result can't be directly tested; in practice implementations of Color 3 clipped each RGB component to [0, 255] and also assumed all displays were sRGB; and that is what the WPT assume and test for. Which is incorrect. The tests note this explicitly:

<p><strong>WARNING: This test assumes that the device gamut is sRGB
	(as it will be for many CRT monitors).</strong></p>

In terms of what do do about it, in my view subtests that check for negative saturation becoming 0 should be kept because this is also in CSS Color 4:

For historical reasons, if the saturation is less than 0% it is clamped to 0% at parsed-value time, before being converted to an sRGB color.

and all the subtests which assume an sRGB display and assume a particular method to force colors into the display gamut should be deleted, as the current tests go beyond what is specified.

These subtests contradict the resolution from

specifically, if a Display P3 color gets converted to HSL then it now round trips, and should be displayed as that color not forced into sRGB on a P3 display.

@svgeesus
Copy link
Contributor

svgeesus commented Apr 25, 2024

Also, the slimmed-down tests

css/css-color/t424-hsl-clip-outside-gamut-b.xht
css/css-color/t425-hsla-clip-outside-device-gamut-b.xht

should be renamed

css/css-color/hsl-clamp-negative-saturation.html
css/css-color/hsla-clamp-negative-saturation.html

and should link to the relevant clause in CSS Color 4, which replaces CSS Color 3 according to the CSS Snapshot.

@svgeesus
Copy link
Contributor

They're generated here:

('hsl-clamp-1', 'hsl(120, 200%, 50%)', 0,255,0,255, ""),

Looks like those tests all check for saturation greater than 100 (so, are wrong) and there are no tests for clamping negative saturation. So an easy fix would be to change that generator to use a negative saturation, and check the rgb value comes from hsl with a zero saturation.

@svgeesus
Copy link
Contributor

We do have some modern-syntax CSS tests for clamping negative HSL saturation, because of this change

weinig pushed a commit to weinig/wpt that referenced this issue Apr 27, 2024
… on unclear gamut mapping to sRGB

web-platform-tests#45814

Removes tests that were assuming the CSS Color 3 rules for hsl()/hsla()
where lightness and saturation values outside of 0-100 were clamped to
that range. CSS Color 4 states that only saturation below 0 is clamped.

To make it clear that only the saturation clamping is being tested, the
tests where that is still tested have been renamed to be more clear.
@weinig
Copy link
Contributor Author

weinig commented Apr 27, 2024

I put up a PR for the ones discussed so far.

In addition though, there are

css/css-color/lch-009.html
css/css-color/lch-010.html
css/css-color/oklch-009.html
css/css-color/oklch-010.html

which depend on gamut mapping always mapping lightness levels of 100% always mapping to srgb white, and lightness levels of 0% always mapping to srgb black.

weinig pushed a commit that referenced this issue Apr 29, 2024
… on unclear gamut mapping to sRGB (#45941)

#45814

Removes tests that were assuming the CSS Color 3 rules for hsl()/hsla()
where lightness and saturation values outside of 0-100 were clamped to
that range. CSS Color 4 states that only saturation below 0 is clamped.

To make it clear that only the saturation clamping is being tested, the
tests where that is still tested have been renamed to be more clear.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 2, 2024
…) clipping behavior or depend on unclear gamut mapping to sRGB, a=testonly

Automatic update from web-platform-tests
[css-color] Tests assume old hsl()/hsla() clipping behavior or depend on unclear gamut mapping to sRGB (#45941)

web-platform-tests/wpt#45814

Removes tests that were assuming the CSS Color 3 rules for hsl()/hsla()
where lightness and saturation values outside of 0-100 were clamped to
that range. CSS Color 4 states that only saturation below 0 is clamped.

To make it clear that only the saturation clamping is being tested, the
tests where that is still tested have been renamed to be more clear.
--

wpt-commits: fd6e317871ed6baeedfe25b03b79b73eb898d15c
wpt-pr: 45941
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue May 2, 2024
…) clipping behavior or depend on unclear gamut mapping to sRGB, a=testonly

Automatic update from web-platform-tests
[css-color] Tests assume old hsl()/hsla() clipping behavior or depend on unclear gamut mapping to sRGB (#45941)

web-platform-tests/wpt#45814

Removes tests that were assuming the CSS Color 3 rules for hsl()/hsla()
where lightness and saturation values outside of 0-100 were clamped to
that range. CSS Color 4 states that only saturation below 0 is clamped.

To make it clear that only the saturation clamping is being tested, the
tests where that is still tested have been renamed to be more clear.
--

wpt-commits: fd6e317871ed6baeedfe25b03b79b73eb898d15c
wpt-pr: 45941
@svgeesus
Copy link
Contributor

svgeesus commented Aug 15, 2024

@wenig thanks for fixing the WPT tests we discussed

In addition though, there are

css/css-color/lch-009.html
css/css-color/lch-010.html
css/css-color/oklch-009.html
css/css-color/oklch-010.html

which depend on gamut mapping always mapping lightness levels of 100% always mapping to srgb white, and lightness levels of 0% always mapping to srgb black.

Currently in the spec we have

In Lab, the first argument specifies the CIE Lightness, L. This is a number between 0% or 0 and 100% or 100 Values less than 0% or 0 must be clamped to 0% at parsed-value time; values greater than 100% or 100 are clamped to 100% at parsed-value time.

and

If the lightness of a Lab color (after clamping) is 0%, or 100% the color will be displayed as black, or white, respectively due to gamut mapping to the display.

The second quoted part was changed to be that way because previously there was a discontinuity: 1.0 would give white while 0.9999 would not. Expressing it as a consequence of gamut mapping removed that discontinuity, while not relying on any details of the gamut mapping beyond lightness preservation. Just the natural consequence in standard dynamic range that media white is the brightest displayable color.

It isn't clear to me how to express that in a better way in the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants