-
Notifications
You must be signed in to change notification settings - Fork 664
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-4] Parse-time clip of HSL negative saturation for modern syntax? #9222
Comments
hsl
can result in negative saturationhsl
can result in negative saturation with the sample code
I will note that not every algorithm for a color space is limitless, sometimes there are simply inherent limits to a color space >>> Color('srgb', [1.5, 1, 0.5]).convert('hsl').convert('srgb')
color(srgb 1 1 1 / 1)
>>> Color('srgb', [1.49, 1, 0.5]).convert('hsl').convert('srgb')
color(srgb 1.49 1 0.5 / 1) Negative saturation is how HSL deals with colors with a lightness that exceeds 100%. Large saturation is how it generally deals with colors saturated beyond its gamut. Now, none of this is by a design as it is only designed for in gamut colors, but more incidentally. The example you've given is actually outside the visible gamut. I don't think such a value is a real world concern. If you are talking about a gamut such as Rec. 2020, I believe it should handle all colors in the gamut fine, but the shape will be far from a cylinder 🙂. |
Yes, It was this test that triggered a failure on my end : https://github.com/web-platform-tests/wpt/blob/00e27884bfe6426de9c3e7c33e32db76bc1faed8/css/css-color/parsing/relative-color-out-of-gamut.html#L52
This eventually becomes When clamping the saturation to |
Ah, I haven't looked as closely at the sequencing of steps for relative color syntax, so I can't speak to whether the tests accurately represent the spec. I'm not sure exactly when things get clamped and/or gamut mapped during the process. |
Previously we did a gamut mapping step when converting (maybe my understanding of that resolution was wrong :) ) Gamut mapping helped here because it ensured that any input color would have sane value ranges for a given color model/space. |
Well, define sane. They were restrained to sRGB. I don't know if the tests are doing what they should or not, but I know that >>> hsl = Color('lab(100 104.3 -50.9)').convert('hsl')
>>> hsl
color(--hsl 311.21 -5.5486 1.0906 / 1)
>>> hsl.convert('lab')
color(--lab 100 104.3 -50.9 / 1) I also know if you gamut map in OkLCh referencing the color as HSL vs sRGB that you get different results. I'm not exactly sure what CSS does here. Does it use sRGB when gamut mapping HSL or does it gamut map HSL colors keeping them as HSL? I assume HSL. >>> hsl = Color('lab(100 104.3 -50.9)').convert('hsl')
>>> hsl.clone().fit('hsl', fit='oklch-chroma').convert('srgb').to_string()
'rgb(255 255 255)'
>>> hsl.clone().fit('srgb', fit='oklch-chroma').convert('srgb').to_string()
'rgb(255 253.18 255)' In neither case did I get > new Color('lab(100 104.3 -50.9)').to('srgb').toGamut({method: 'oklch.chroma'})
Color { coords: [ 1, 1, 1 ], alpha: 1 }
> new Color('lab(100 104.3 -50.9)').to('srgb').toGamut('srgb', {method: 'oklch.chroma'})
Color { coords: [ 1, 0.9951746276359744, 1 ], alpha: 1 } It is possible there is something about RCS that I don't understand, some step(s) that I'm not executing. I haven't really given CSS color level 5 the same critical eye that I've given level 4. |
🤔 After a bit of poking I think it's something completely different. Most color functions describe clipping of values outside certain ranges. https://www.w3.org/TR/css-color-3/#hsl-color
But this should be done only during parsing. In our implementation we always do this step, but after #8444 I think it becomes clear that we must only do it when parsing, and not when computing. Are these the same or different colors?
Closing and filing a new issue to avoid confusion :) |
see : #9259 |
Re-opening because clipping on parsing negative saturation in
|
It may be possible that the HSL conversion algorithm can be altered to ensure that negative saturation is never returned and still have completely accurate results. Then negative saturation can be clamped at zero with no ceiling. When negative saturation occurs, it just rotates the hue by 180. So if you rotate the hue by 180, you can set the saturation to a positive value during conversion. Then there is no need to allow parsing negative saturation. Going with this earlier example, we can correct the saturation and hue. Then when we convert back, we get the same value we had before. >>> c1 = Color('lab(100 104.3 -50.9)').convert('hsl')
>>> c1
color(--hsl 311.21 -5.5486 1.0906 / 1)
>>> c1.set('s', lambda x: abs(x)).set('h', lambda x: x + 180).convert('lab')
color(--lab 100 104.3 -50.9 / 1) I suspect this is generally true, but more experiments would need to be performed to ensure this is the case. I assume if there are outliers, the reasons would need to be explored as HSL has hard limits. Once the limits are exceeded, the results will never round trip back. But if you are using such extreme values, which are well outside the visible spectrum, with HSL, you get what you get 🙂. >>> Color('color(srgb 1.5 1 0.5)').convert('hsl').convert('srgb')
color(srgb 1 1 1 / 1)
>>> Color('color(srgb 1.5 1 0.5)').convert('xyz-d65')
color(xyz-d65 1.4425 1.2701 0.37169 / 1) |
As a note, I used the rec2100-pq HDR color space to generate 1,030,301 colors which I then converted to HSL. 363,630 returned negative saturation, every single one was able to successfully round trip to the original color after correcting the negative saturation and adjusting the hue. So it seems HSL algorithm can be altered successfully to correct the negative saturation without introducing issues if negative saturation is a problem. |
We now have: /**
* @param {number} red - Red component 0..1
* @param {number} green - Green component 0..1
* @param {number} blue - Blue component 0..1
* @return {number[]} Array of HSL values: Hue as degrees 0..360, Saturation and Lightness in reference range [0,100]
*/
function rgbToHsl (red, green, blue) {
let max = Math.max(red, green, blue);
let min = Math.min(red, green, blue);
let [hue, sat, light] = [NaN, 0, (min + max)/2];
let d = max - min;
if (d !== 0) {
sat = (light === 0 || light === 1)
? 0
: (max - light) / Math.min(light, 1 - light);
switch (max) {
case red: hue = (green - blue) / d + (green < blue ? 6 : 0); break;
case green: hue = (blue - red) / d + 2; break;
case blue: hue = (red - green) / d + 4;
}
hue = hue * 60;
}
// Very out of gamut colors can produce negative saturation
// If so, just rotate the hue by 180 and use a positive saturation
// see https://github.com/w3c/csswg-drafts/issues/9222
if (sat < 0) {
hue += 180;
sat = Math.abs(sat);
}
if (hue >= 360) {
hue -= 360;
}
return [hue, sat * 100, light * 100];
} |
So we no longer return negative saturation. I'm wondering whether it is better to
Tending towards the belt-and-braces option 2, opinions welcome |
Thank you for the update 🙇 Option 2 seems fine to me, I can't think of any case where you would need the uncorrected saturation. But I don't have a strong opinion on this :) |
Keep in mind this approach works for most cylindrical color spaces, OkLCh and LCh included. We could, theoretically, better handle OkLCh and LCh negative chroma when converting back to Oklab and Lab without having to hard clamp to zero. As I side note, I did mention most cylindrical spaces. Depending on the model, this may not always be true, as a non CSS example CAM16 does not work this way due to the complex way in which chroma is calculated for a given lightness and hue. The only way to resolve such saturation is to convert it back to the XYZ base and then convert it forward to CAM16 again, though once you are out of the visible spectrum, you can still get negative chroma no matter what you do. The algorithm just doesn't handle colors so far out. This is true for some other spaces like HSV as well. It is better to convert HSV back to sRGB and then back to HSV, and if the colors happen to resolve, great, if not 🤷🏻. I mainly mention this for anyone thinking this works for every cylindrical space, without exception. |
I will note that since OkLCh and LCh never return negative chroma, the only way this could occur is through manually specifying it or maybe |
In general the approach now is to do some sanitizing on input, and try not to do any on intermediate values during color conversion and interpolation; and we leave "make this make sense as a color" to display gamut mapping. |
Okay, then if CSS wanted to not clamp chroma in a manually input chroma value of |
I can see merit in both approaches; either treat a specified negative chroma as an error (and clamp it to zero) or fix up the hue and absolutize chroma. But as you say, unlike HSL, the conversion code is not producing negative values so I feel the best thing is to not fix up the hue. |
CSS Color 3 said to clamp negative saturation at parse time (and was silent about generated values, since it was sRGB-only). This is tested in and browsers are interoperable. Looks like that parse-time check should be added back to CSSColor 4, for interoperability. |
… to 0, which is current interop behavior from CSS Color 3. #9222
hsl
can result in negative saturation with the sample code
(Title updated to cover the one remaining issue) |
Oh, my. In CSS Color 3, for
(No mention of clipping lightness). And for
The wording in both is somewhat confusing, as it mixes up parse-time clipping and used-value mapping to the display gamut. |
The CSS Working Group just discussed
The full IRC log of that discussion<fantasai> chris: Long thread because originally about something else, but discovered a problem so about that<fantasai> chris: CSS Color L3 required that negative saturation be clipped <fantasai> chris: all implementations do that, tested in css-color-3 tests <fantasai> chris: We have since decided that we want to round-trip through HSL <fantasai> chris: which requires representing out-of-gamut colors <fantasai> chris: and HSL bakes sRGB <fantasai> chris: Because color-4 said don't clip, and color-3 said clip <fantasai> chris: UAs interpreted, clip for old syntaxes and don't clip for new syntaxes <fantasai> chris: There are pros and cons to various approaches <fantasai> chris: My preference is it's better to not clip in modern syntax, but we need to agree on that <fantasai> chris: also this is a parse-time clip, values from the stylesheet. If you happen to wander into HSL because of color conversion, then we don't clip <emilio> q+ <emilio> fantasai: is there a web compat constraint for not clipping in the old syntax? <emilio> ... could we not clip always? <fantasai> chris: It's been baked in for so long, it might break some sites ... I haven't checked, but it would be a change <emilio> chris: it's been there for a while and interoperable <emilio> fantasai: it's fairly rare to type negative values <emilio> ... it seems it might be possible to just not clip <emilio> ... maybe we should try to pursue <astearns> ack fantasai <emilio> chris: it's not non-sensical, it just means that you've gone past the axis <fantasai> astearns: We could resolve on modern syntax, and then check if old syntax could be changed <astearns> ack emilio <fantasai> emilio: Can you remind me, what status is on colors serializing to rgb? <fantasai> emilio: IIRC at least some of the HSL syntax serializes to rgb() <fantasai> emilio: if it serializes to rgb() then it should be consistent and clip <fantasai> chris: Correct, and that's why for color-mix() when you go through HSL you come out in color() rather than hsl() <fantasai> emilio: I suggest either do Firefox behavior (always clip) or make modern HSL syntax not serialize to rgb() <fantasai> emilio: but that does seem more risky <chris> s/rather than hsl()/rather than rgb() <fantasai> emilio: because otherwise the color can't round-tirp <fantasai> s/tirp/trip <fantasai> chris: Worried about Web-compat for not serializing to rgb() <fantasai> emilio: I think ideally the color should round-trip. <fantasai> emilio: div.style.color = div.style.color shouldn't change the color <fantasai> chris: Would Chrome change to always clip? <fantasai> [none of the Chrome engineers step up to answer] <fantasai> chris: Based on legacy syntax tests, we have interop on it <fantasai> chris: Was going to commit tests for new syntax, and dbaron was unsure when reviewing so we have this issue <fantasai> matthieud: for color-mix() we still [missed] so we clip <matthieud> for color-mix, we still output rgb syntax and not color(in srgb...) <fantasai> emilio: since legacy syntax (and modern) syntax convert to rgb(), more consistent to clip always <fantasai> chris: I would be OK with that. If you use HSL, you brought this on yourself. <fantasai> astearns: given no Blink opinion, shall we resolve to clip all HSL syntaxes? <fantasai> fantasai: All HSL or all HSL except color() ? <fantasai> chris: color() doesn't affect HSL, this is hsl() only <fantasai> PROPOSED: All hsl() clip to non-negative numbers <fantasai> for saturation <fantasai> RESOLVED: All hsl() clip to non-negative numbers for saturation |
The current specification already conforms to this resolution (it does not distinguish legacy and modern syntax):
So this does not need spec edits, but does need review of my WPR PR which tests this in modern syntax. |
WPT updated, thanks @dbaron for swift re-review |
Should it be considered a bug if saturation is also capped at 100% before conversion to RGB? to8Bit(hslToRgb(0, 150, 40)) // [255,-51,-51]
to8Bit(hslToRgb(0, 100, 40)) // [204,0,0]
element.style.color = 'hsl(0%, 150%, 40%)'
element.style.color // rgb(204, 0, 0) in Chrome and FF But I cannot find where clamping values outside of |
Yes, and that should be observable via
It is in 5.1 The RGB functions:
and, hmm,
I agree that it doesn't say explicitly about after conversion. In particular,
Which is true, but does not cover the case where the color space (in this case, sRGB) is defined over the extended range but a serialization form ( |
I would definitely expect channel values to not be clamped before mixing colors (unless otherwise specified at parse time). The definition of
And its saturation/lightness argument should be in reference range [0,100]. Now, It returns And the specified/computed value of element.style.color = 'hsl(0, 150%, 40%)'
getComputedStyle(element).color; // Chrome-FF -> rgb(204, 0, 0)
element.style.color = 'hsl(0 150% 40%)'
getComputedStyle(element).color; // Chrome-FF -> rgb(204, 0, 0)
element.style.color = 'color-mix(in srgb, hsl(0, 150%, 40%), hsl(0, 150%, 40%))'
getComputedStyle(element).color; // Chrome-FF -> color(srgb 0.8 0 0)
element.style.color = 'color-mix(in srgb, hsl(0 150% 40%), hsl(0 150% 40%))'
getComputedStyle(element).color; // Chrome -> color(srgb 1 -0.2 -0.2) ; FF -> color(srgb 0.8 0 0)
element.style.color = 'rgb(from hsl(0, 150%, 40%) r g b)'
getComputedStyle(element).color; // Chrome-FF -> color(srgb 0.8 0 0)
element.style.color = 'rgb(from hsl(0 150% 40%) r g b)'
getComputedStyle(element).color; // Chrome -> color(srgb 1 -0.2 -0.2) ; FF -> color(srgb 0.8 0 0) Unfortunately, I cannot find any test on WPT with a saturation greater than 100% in |
Confirmed with Chrome Canary Version 123.0.6312.0 (Official Build) canary (64-bit) $0.style.color = 'hsl(0, 150%, 40%)'
'hsl(0, 150%, 40%)'
getComputedStyle($0).color
'rgb(204, 0, 0)'
$0.style.color ='color-mix(in srgb, hsl(0, 150%, 40%), hsl(0, 150%, 40%))'
'color-mix(in srgb, hsl(0, 150%, 40%), hsl(0, 150%, 40%))'
getComputedStyle($0).color
'color(srgb 0.8 0 0)'
$0.style.color ='color-mix(in srgb, hsl(0 150% 40%), hsl(0, 150%, 40%))'
'color-mix(in srgb, hsl(0 150% 40%), hsl(0, 150%, 40%))'
getComputedStyle($0).color
'color(srgb 0.9 -0.1 -0.1)'
$0.style.color ='color-mix(in srgb, hsl(0 150% 40%), hsl(0 150% 40%))'
'color-mix(in srgb, hsl(0 150% 40%), hsl(0 150% 40%))'
getComputedStyle($0).color
'color(srgb 1 -0.2 -0.2)'
I will add some to css/css-color/parsing/color-computed-hsl.html and css/css-color/parsing/color-mix-out-of-gamut.html |
"normalized to the range [0, 1]" used to be true but now needs to be restated. "Colors in the sRGB gamut will be in the range [0, 1]" perhaps. |
Sorry, I am still confused... so what should be the serialized value of
Answer 3 is eliminated because it does not round-trip (rgb channel values must be clamped at parse time).
I conclude that answer 1 is eliminated because the saturation is capped before conversion... unless I am missing how it happens during conversion to Assuming answer 2 is expected:
The only way I see for this to apply would be to convert |
csswg-drafts/css-color-4/rgbToHsl.js
Line 16 in 61da22e
This only happens when the sum of all channels exceeds
3
.rgbToHsl(1.5, 1, 0.5)
-> negative saturationrgbToHsl(1.499999, 1, 0.5)
-> very high, but positive saturation of99999900
I get better results when I clamp saturation to
0
.I don't know if clamping to
0
had unintended side effects here.The text was updated successfully, but these errors were encountered: