Enhance modern component serialization with inf and nan support #351
Conversation
Modern syntax colors support NaN and infinity values. These values are now allowed, where previously for some lab/lch/oklab/oklch components, NaN values were truncated to 0.0. NaN and infinity now also correctly serializes to calc(NaN) and calc(infinity) respectively.
There was a problem hiding this comment.
This seems related to web-platform-tests/interop#369 / w3c/csswg-drafts#8629 which is still under discussion. Could you elaborate on the behavior you're implementing?
In particular, the clamping infinity but not NaN at specified value time seems like a bug?
src/color.rs
Outdated
| @@ -1495,7 +1521,7 @@ where | |||
| P::parse_number_or_percentage, | |||
| )?; | |||
|
|
|||
| let lightness = lightness.map(|l| l.value(lightness_range).max(0.0)); | |||
| let lightness = lightness.map(|l| max_preserve_nan(l.value(lightness_range), 0.0)); | |||
There was a problem hiding this comment.
So it is really weird to clamp negatives and so on but not nan, I think we should be consistent.
There was a problem hiding this comment.
The clamping is now being done in Gecko here: https://phabricator.services.mozilla.com/D183983
I only kept in the clamping to 0 for chroma and lightness, because it used to be here. I would prefer to remove these validations here so that the parser only gives your the raw values and not a value that is a "specified" value.
Once the colors are converted to "computed" more rules apply, like NaN becomes 0.0.
So this change is more about allowing calc(nan) values to be passed along to Gecko.
Maybe for consistency I can remove validations here like the clamping, because if I start doing validations here, then it feels like the "computed" value validations are missing.
wdyt?
|
Ok now without any of the adjusting or clamping. |
| if let Some(value) = self.0 { | ||
| if value.is_infinite() { | ||
| if value.is_sign_negative() { | ||
| dest.write_str("calc(-infinity)") |
There was a problem hiding this comment.
Hmm, so the behavior of out-of-range negative values like -3 would be different than -infinity? Or would it be consistent?
There was a problem hiding this comment.
-3 would serialize to -3 and -infinity would serialize to calc(-infinity).
As far as the parser is concerned now, there is no range, just numbers between -inf..inf. Up to the user of the lib to clamp/decide what is a range.
There was a problem hiding this comment.
Ok, we should make sure that Gecko is also consistent in how we treat these but that seems fine.
| use super::*; | ||
|
|
||
| #[test] | ||
| fn serialize_modern_components() { |
There was a problem hiding this comment.
(Not convinced this test is particularly useful but...)
There was a problem hiding this comment.
What would you change to be convinced?
There was a problem hiding this comment.
I mean, the test is just testing fairly straight-forward code directly, so I don't think it brings much value, but...
|
@bors-servo delegate+ |
|
✌️ @tiaanl can now approve this pull request |
|
Ah I guess we're not doing the bors thing. Anyways please confirm this is consistent and hit merge yourself? I think you can, if not happy to do that :) |
Modern syntax colors support NaN and infinity values. These values are now allowed, where previously for some lab/lch/oklab/oklch components, NaN values were truncated to 0.0.
NaN and infinity now also correctly serializes to calc(NaN) and calc(infinity) respectively.