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] Clean up the definition of color.same() #3852

Merged
merged 1 commit into from May 2, 2024
Merged

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Apr 25, 2024

This was still written with the expectation that unknown color spaces
existed. It also makes missing channels equivalent to 0s even if they
survive the conversion to xyz.

This was still written with the expectation that unknown color spaces
existed. It also makes missing channels equivalent to 0s even if they
survive the conversion to `xyz`.
Copy link
Contributor

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

The conversion makes sense. Not sure about none === 0 for this function.


* Let `color1` and `color2` be the result of [converting] `$color1` and
`$color2` into `xyz` color space, respectively.
* Replace any [missing] components in `color1` and `color2` with 0.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind this? Just that they would result in the same display value?

Copy link
Member

Choose a reason for hiding this comment

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

First I thought it was probably OK since missing components are treated as zero according to the spec above. However, after skimming through the CSS spec, I now agree that for this function zero shouldn't be treated as equal to none 🤔, or maybe there has to be a note clarifying the scope of what same() is comparing to.

For example, IIUC from the CSS color spec example 33:

$color1: oklch(78.3% 0.108 326.5);
$color2: oklch(39.2% 0.4 none);
$color3: oklch(39.2% 0.4 0);

$with-none: color-mix(in oklch, $color1, $color2);
$with-zero: color-mix(in oklch, $color1, $color3);

This results in $with-none and $with-zero having different values, right?

And if so, I think it would be confusing in some situations for color.same($color2, $color3) to return true. But I guess it's fine if the function is meant to compare that two colors will display the same even if they don't behave the same in other situations like interpolating colors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's my reasoning:

  1. Missing channels are defined by spec as being treated as 0 everywhere other than interpolation. Since this is not an interpolation context, I think it fits the conceptual model to treat them as 0 here as well.

  2. Missing channels are only well-defined within a given color space, and same() is trying to provide uniform behavior across color spaces. If we treat missing channels as distinct here, then there exists no Oklch color that's same() as rgb(none 150 255).

  3. Relatedly, the previous definition already didn't consider missing channels in polar spaces (the place where they're by far the most meaningful and likely to arise naturally) as meaningful. Because it converts both colors into xyz, a missing hue is always treated identically to an undefined hue—which I think it good, because we want color.same(oklch(50% 0% 0deg), oklch(50% 0% 180deg)) to return true since they're both visually gray.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nex3 Did you see the recent discussions around preserving none in calculations whenever possible? w3c/csswg-drafts#10211 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! I hadn't seen that. It does weaken point 1 somewhat, although I think in general the other points stand. I think making the distinction "== compares every little detail, same() compares whether the colors will look the same if you use them directly" makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Then I'm OK with it, but I think a comment should be added to make it clear that this same() is meant for colors that are displayed the same


* Let `color1` and `color2` be the result of [converting] `$color1` and
`$color2` into `xyz` color space, respectively.
* Replace any [missing] components in `color1` and `color2` with 0.
Copy link
Member

Choose a reason for hiding this comment

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

First I thought it was probably OK since missing components are treated as zero according to the spec above. However, after skimming through the CSS spec, I now agree that for this function zero shouldn't be treated as equal to none 🤔, or maybe there has to be a note clarifying the scope of what same() is comparing to.

For example, IIUC from the CSS color spec example 33:

$color1: oklch(78.3% 0.108 326.5);
$color2: oklch(39.2% 0.4 none);
$color3: oklch(39.2% 0.4 0);

$with-none: color-mix(in oklch, $color1, $color2);
$with-zero: color-mix(in oklch, $color1, $color3);

This results in $with-none and $with-zero having different values, right?

And if so, I think it would be confusing in some situations for color.same($color2, $color3) to return true. But I guess it's fine if the function is meant to compare that two colors will display the same even if they don't behave the same in other situations like interpolating colors?

@nex3 nex3 requested a review from mirisuzanne April 30, 2024 21:25
Copy link
Contributor

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

works for me

@nex3 nex3 merged commit d659781 into feature.color-4 May 2, 2024
9 checks passed
@nex3 nex3 deleted the same branch May 2, 2024 20:30
nex3 added a commit to sass/dart-sass that referenced this pull request May 3, 2024
nex3 added a commit to sass/dart-sass that referenced this pull request May 9, 2024
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

3 participants