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 4] Update existing specs #1966

Merged
merged 5 commits into from
Apr 10, 2024
Merged

[Color 4] Update existing specs #1966

merged 5 commits into from
Apr 10, 2024

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Mar 29, 2024

This covers changes from sass/sass#3819 and sass/sass#3823.

[skip dart-sass]
[skip sass-embedded]

@nex3 nex3 marked this pull request as ready for review March 29, 2024 22:29
@nex3 nex3 requested a review from Goodwine March 29, 2024 22:30
@@ -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);
Copy link
Member

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 😅

Copy link
Contributor Author

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.

Copy link
Member

@Goodwine Goodwine left a 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

@nex3
Copy link
Contributor Author

nex3 commented Apr 1, 2024

I'll be adding tests for those as part of color.to-space and color.change tests, since as it stands there isn't a way with currently-tested functions to make an out-of-gamut clamped color.

@nex3 nex3 requested a review from Goodwine April 1, 2024 21:38
@nex3 nex3 merged commit fac7453 into feature.color-4 Apr 10, 2024
7 checks passed
@nex3 nex3 deleted the update-color-specs branch April 10, 2024 00:15
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

Successfully merging this pull request may close these issues.

None yet

2 participants