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 tests for color.mix() and color.complement() #1843

Merged
merged 34 commits into from
Jan 31, 2023

Conversation

dvdherron
Copy link
Contributor

Part of new/updated tests for color spaces.

See: sass/sass#2831

[skip dart-sass]

@dvdherron dvdherron changed the title Add tests for color.mix() and color.complement() Update tests for color.mix() and color.complement() Nov 24, 2022
@nex3 nex3 self-requested a review November 28, 2022 21:43
spec/core_functions/color/mix/hue_interpolation.hrx Outdated Show resolved Hide resolved
spec/core_functions/color/complement.hrx Outdated Show resolved Hide resolved
@dvdherron dvdherron changed the base branch from main to feature.color-4 December 1, 2022 13:25
@dvdherron dvdherron requested a review from nex3 December 2, 2022 15:44
@dvdherron dvdherron changed the title Update tests for color.mix() and color.complement() [Color 4] Update tests for color.mix() and color.complement() Dec 9, 2022
@@ -39,6 +65,12 @@ a {b: mix(lch(20% -20 0), red)}
a {b: mix(red, lch(20% -20 0))}

<===> null_method/non_legacy/color2/error
Error: $color1: To use color.mix() with non-legacy color red, you must provide a $method.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$color2 should be the cause of the error here, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good catch

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add physical directories for spec/core_functions/color/mix, you'll need to delete mix.hrx—otherwise the test runner won't know which is the real one and will throw an error.

spec/core_functions/color/complement.hrx Outdated Show resolved Hide resolved
spec/core_functions/color/complement.hrx Outdated Show resolved Hide resolved
spec/core_functions/color/mix/error.hrx Show resolved Hide resolved
spec/core_functions/color/mix/error.hrx Outdated Show resolved Hide resolved

<===> specified/output.css
a {
b: rgb(145, 116, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dart Sass returns rgb(145.2072868242, 115.9916273156, -48.1699603699) here. Is it possible that the source of truth you're using is clamping the RGB channels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the source of truth you're using is clamping the RGB channels?

Correct. I've updated the places where my source of truth isn't as precise. I'll keep an eye out on these going forward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this still has a non-negative blue channel. Should I just go ahead and update it with Dart Sass's output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated with Dart Sass's output.

spec/core_functions/color/mix/mixed_spaces.hrx Outdated Show resolved Hide resolved
spec/core_functions/color/mix/predefined.hrx Outdated Show resolved Hide resolved

<===> rgb_explicit_method/output.css
a {
b: color(display-p3 0.9 0.93 0.3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dart Sass returns color(display-p3 0.9849707148 0.9141322646 0.3079881122) here, which is kind of close but still surprisingly divergent given that. Do you know which one is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to look more into this but I think the Dart Sass version is correct. This mix comes out to color(display-p3 0.8959 0.9307 0.3031) on the ColorJS playground and then gets rounded up it seems. Like you said, all surprisingly different!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Dart Sass output is the correct one here.

spec/core_functions/color/mix/predefined.hrx Outdated Show resolved Hide resolved

<===> xyz_explicit_method/output.css
a {
b: color(xyz-d50 0.3 0.21 0.02);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another close but weirdly different output: color(xyz-d50 0.3464867621 0.2923393699 0.0410088495)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed the precision to match Dart Sass here.

<===> error/null_space/non_legacy/options.yml
:todo:
- dart-sass

<===> error/null_space/non_legacy/input.scss
a {b: complement(oklch(0.63 0.26 29.2))}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should fail since it doesn't provide an interpolation space but it passes, outputting: oklch(0.8967656043 0.1522729881 196.7205341356deg);

@@ -60,6 +60,8 @@ Error: $color1: To use color.mix() with non-legacy color lch(20% -20 0deg), you
<===> null_method/non_legacy/color2/options.yml
:ignore_for:
- libsass
:todo:
- dart-sass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have noted this in the wip dart sass PR. I marked this one as to do as well.

- dart-sass

<===> rectangular_space/input.scss
a {b: mix(red, blue, $method: oklab)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this expected to be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was mistakenly added as an error. I momentarily mixed up which interpolation spaces are valid here, thinking I was working on color.complement()


<===> specified/output.css
a {
b: rgb(145, 116, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this still has a non-negative blue channel. Should I just go ahead and update it with Dart Sass's output?

spec/core_functions/color/mix/named.hrx Outdated Show resolved Hide resolved

<===> contradiction/output.css
a {
b: rgba(0, 0, 0, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was changed back to rgba(0, 0, 0, 0). Should it still be transparent? @nex3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rgba(0, 0, 0, 0) matches the existing output here. If I recall, we chose to emit this instead of transparent because that keyword had some weird behavior in very old IE versions.

@dvdherron dvdherron requested a review from nex3 January 11, 2023 18:36
@nex3
Copy link
Contributor

nex3 commented Jan 18, 2023

I think we should be able to get these tests passing if we merge main into feature.color-4. In future, it's probably a good idea to avoid merging directly from main into one of these PRs, since the feature branch won't always be up-to-date.

@nex3 nex3 merged commit e6687a2 into sass:feature.color-4 Jan 31, 2023
@nex3 nex3 deleted the color-spaces branch January 31, 2023 20:34
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.

3 participants