-
Notifications
You must be signed in to change notification settings - Fork 88
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 4] Update existing specs #1966
Conversation
This covers changes from sass/sass#3819 and sass/sass#3823.
@@ -73,7 +73,7 @@ a {b: adjust-color(#993333, $blackness: 100%)} | |||
|
|||
<===> blackness/max/output.css | |||
a { | |||
b: rgb(42.5, 42.5, 42.5); | |||
b: rgb(31.875, 31.875, 31.875); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the rgb()
value change by more than 10 points on each channel? I don't think the adjust-color
function changed in a way that would cause this much of a change. Is it intended? I'm not sure what I missed 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! The short answer is that the old behavior was just wrong. The longer answer is that previously, color.adjust()
eagerly clipped each channel to its bounds before updating the color. #99333
is equivalent to hwb(0deg 20% 40%)
, so we took the 40%
blackness, added 100%
to get 140%
, but then clipped it down to get the final result of hwb(0deg 20% 100%)
. What we should have done, rather than clipping, was normalize the whiteness and blackness channels to get hwb(0deg 12.5% 87.5%)
, which is equivalent to hwb(0deg 20% 140%)
(both whiteness and blackness are scaled down linearly by the smallest value such that whiteness + blackness = 100%
).
Because we've changed all the infrastructure to support out-of-gamut channels anyway, it inherently fixes this issue and produces the correct, blacker grey listed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I guess this is still waiting on the updated gamut serialization tests
I'll be adding tests for those as part of |
This covers changes from sass/sass#3819 and sass/sass#3823.
[skip dart-sass]
[skip sass-embedded]