Skip to content

[Color 4] Update tests for color.mix() and color.complement()#1843

Merged
nex3 merged 34 commits intosass:feature.color-4from
oddbird:color-spaces
Jan 31, 2023
Merged

[Color 4] Update tests for color.mix() and color.complement()#1843
nex3 merged 34 commits intosass:feature.color-4from
oddbird:color-spaces

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
@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
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.


<===> 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.


<===> 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.


<===> 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.

- 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);

: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?


<===> 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