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

Support color spaces #2832

Merged
merged 21 commits into from
Sep 21, 2022
Merged

Support color spaces #2832

merged 21 commits into from
Sep 21, 2022

Conversation

mirisuzanne
Copy link
Contributor

proposal/color-4-new-formats.md Outdated Show resolved Hide resolved
proposal/color-4-new-formats.md Outdated Show resolved Hide resolved
proposal/color-4-new-formats.md Outdated Show resolved Hide resolved
@mirisuzanne mirisuzanne marked this pull request as ready for review August 26, 2022 23:27
@mirisuzanne
Copy link
Contributor Author

The summary section still needs fleshing out, and I'm sure I have some typos and broken links in there - but I think the overall proposal is ready for a round of review. It's a big one! 😅

@stof
Copy link
Contributor

stof commented Aug 27, 2022

The commit history is totally broken. I suspect that at some point, master was rebased on top of the branch instead of the opposite

@mirisuzanne
Copy link
Contributor Author

I can close and reopen without all the history, if that's helpful.

$brighter-green: color(display-p3 0 1 0);
```

<!-- flesh this out more -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's particularly worth giving an overview here of the "rules of thumb" for how colors will now work in Sass. IIRC:

  • Colors defined in one space stay in that space (and are emitted in that space) unless explicitly changed.
  • You can always convert a color to widely-compatible sRGB format using ....
  • Functions that work different ways in different spaces use the color's space by default but also have an explicit $space parameter.
  • "Legacy colors" come from ... and are always emitted in a compatible format.

Also, list the new functions with brief documentation for each of them, list the functions that are being deprecated, and other such changes to the core modules.

Comment on lines 148 to 149
or allow out-of-gamut colors in output. Browsers will also provide improved
gamut mapping, based on the user device capabilities.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would emphasize here that we don't clamp colors because browsers do better gamut-mapping than we possibly can.


1. Channel values are no longer clamped to the gamut of a given color space.
Authors can use the provided `to-gamut()` function to map colors into gamut,
or allow out-of-gamut colors in output. Browsers will also provide improved
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "or allow" is a little confusing here, because it sounds like it's an explicit choice the author is making when (I believe) it's just the default. Maybe instead: "but by default Sass will emit out-of-gamut colors".

Authors can use the provided `to-gamut()` function to map colors into gamut,
or allow out-of-gamut colors in output. Browsers will also provide improved
gamut mapping, based on the user device capabilities.
2. Channel values are no longer rounded to the nearest integer, since the spec
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd explicitly say "RGB channel values", since we never rounded HSL channel values.

Comment on lines 154 to 155
While we are not attempting to support all of [CSS Color Level 5][color-5] at
this point, we have used it as a reference while updating color manipulation
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth mentioning that we aren't support Color 5 because it's not in browsers yet.

Comment on lines 1245 to 1246
* Return the result of calling `color.adjust()` with `$color`, a $hue of
`180deg`, and a $space of `space`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Return the result of calling `color.adjust()` with `$color`, a $hue of
`180deg`, and a $space of `space`.
* Return the result of calling `color.adjust($color, $hue: 180deg, $space: space)`.

You don't have to use prose when it gets in the way :-). Similarly elswhere.

Comment on lines 1239 to 1241
> This currently allows `hsl`, `hwb`, `lch`, and `oklch`. We could also
> map some cubic spaces with matching gamuts e.g. (`oklab` -> `oklch`),
> but that adds implicit complexity, while still excluding color spaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we do do this mapping for color.invert()—either way we should be consistent.

### `color.invert()`

```
invert($color, $space)
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing color.invert() function also supports a $weight parameter (which it just tosses into $mix at the end).


* If `$space` is undefined:

* If `$color` is a legacy color, let `space` be `hsl`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be rgb to match the current behavior?

proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved

* If `$space` is not a [color space][], throw an error.

* Return the result of [converting][] the `origin-color`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this misses an error for the case where $space or origin-space are unknown spaces, because the current conversion algorithm only supports known spaces.

* If `$space` is null:

* Let `color` be the value of `color`, and let `space` be the result of
calling `space($color)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
calling `space($color)`.
calling `color.space($color)`.

proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
* Let `target-space` be the value of `$space` if specified, or the value of
`origin-space` otherwise.

* If `target-space` is not a valid [color space][], throw an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

is valid color space here meant to mean known color space ?

```

> This overload exists to support Microsoft's proprietary [`alpha()`
> function][].
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we deprecate that overload in the namespaced function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I've removed this entire alpha() section based on Natalie's comments above, since it doesn't make any changes. Not sure if we want to add a special mention of this overload in the deprecations section?


* If `$space` is 'lab', let `space` be `lch`.

* Otherwise, if `$space` is 'oklab', let `space` be `oklch`.
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a mix between quotes and backticks around color space names here.


* Otherwise, append the last element of `input` to `components`

* If `components` is undefined or an empty list, throw an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

it indeed seems always assigned

[CSS Color Level 4][color-4]. Since the CIE color space defines the entire
gamut of visible color, much larger than the target sRGB gamut, out-of-range
color definitions will be clipped using a *relative-colorimetric* approach that
leaves in-gamut colors un-affected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
leaves in-gamut colors un-affected.
leaves in-gamut colors unaffected.

element of `split-last` to `components`.

> This solves for a legacy handling of `/` in Sass that would produce an
> unquoted string when the alpha value is a css function such as `var()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> unquoted string when the alpha value is a css function such as `var()`
> unquoted string when the alpha value is a CSS function such as `var()`

* Let `alpha` be the number after the slash, and append the number before
the slash to `components`.

* Otherwise, append the last element of `input` to `components`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Otherwise, append the last element of `input` to `components`
* Otherwise, append the last element of `input` to `components`.

$method: null)
```

* If `$method` undefined:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If `$method` undefined:
* If `$method` is undefined:


* If either `color1` or `color2` is not a color, throw an error.

* If `weight` is undefined, set `weight` to `50%`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If `weight` is undefined, set `weight` to `50%`.
* If `weight` is null, set `weight` to `50%`.


* If `$color` is not a color, throw an error.

* If `$space` is undefined:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If `$space` is undefined:
* If `$space` is null:


* If `input` is a [special variable string][], return `null`.

* Let `include-space` be true if `space` is undefined, and false otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Let `include-space` be true if `space` is undefined, and false otherwise.
* Let `include-space` be true if `space` is null, and false otherwise.

* Let `gray` be the result of `whiteness + blackness`.

* Return the result of calling `color.change()` with `color`, a $hue of `hue`,
$whiteness of `gray - whiteness`, and $blackness of `gray - blackness`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$whiteness of `gray - whiteness`, and $blackness of `gray - blackness`.
$whiteness of `gray - whiteness`, and `$blackness` of `gray - blackness`.


* If `components` is not an unbracketed space-separated list, throw an error.

* If `space` is undefined, let `space` be the first element in `components`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If `space` is undefined, let `space` be the first element in `components`,
* If `space` is null, let `space` be the first element in `components`,

@mirisuzanne
Copy link
Contributor Author

mirisuzanne commented Sep 5, 2022

Thank you for the very helpful comments @nex3, @stof, and @dvdherron - it's a big proposal, so easy to get lost & miss things. :) I think I've addressed most of the comments, and would be happy for another look when you're able.

I added a number of additional global color functions for deprecation, but haven't yet removed/changed any notes referencing them (e.g. notes like This function is also available as a global function named grayscale()). I wasn't sure 1) if we want all those functions deprecated, or 2) how to handle those notes if they are deprecated - but I assume we may still need some changes there.

shown significant benefits by providing a more *perceptually uniform*
distribution of colors, so that similar mathematical adjustments achieve
visually similar results.
- A *color space* is the result of projecting a color model into a coordinate
Copy link
Contributor

Choose a reason for hiding this comment

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

Those definitions look weird to me. What you call a "color model" here is exactly what the CSS spec calls a "color space". Color spaces are sRGB, CIE Lab and OKLab, not RGB or HSL. What you call a color space here is called a coordinate system of the color space in the CSS spec.
I think we should align the terminology to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading more in details, I think there is a confusion in the CSS spec between the terminology defined at the beginning of the spec and the the <color-space> production in the defined syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can try to make some clarifications here, but I think we're technically aligned with the spec already.

To some extent it's just a problem of confusing and overlapping terms. OKLab & RGB are both color models, and also color spaces (which means they are also coordinate systems). The CSS spec doesn't define color model as a term, they only reference it in the color space term definition. I think the CSS spec is also a bit inconsistent about when they say 'color space' and when they say 'coordinate system'.

Both of us are also blurring 'color space' with 'color function' a bit, since they're mostly one-to-one, and defining them that way is easier to explain to authors. But that gets a bit tangled, since rgb and srgb are technically the same space, just using legacy and non-legacy functions. There's no reason for CSS to provide both as part of a <color-space> production for 'color interpolation method', but it is useful for us to distinguish between them as a way of tracking legacy vs non-legacy colors. By keeping rgb as a distinct space, we no longer need the is-legacy boolean.

The other confusion here is that we used non-overlapping examples for an overlapping concept. In my definition, I used legacy function examples for color spaces (rgb and hsl). In their definition examples, they used only color spaces in the color() function. Both are accurate. Both srgb and hsl are color spaces.

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.

Some things that aren't covered here yet:

  • Throwing errors for NaN/infinity.
  • Serializing non-legacy colors.

I wasn't sure 1) if we want all those functions deprecated, or 2) how to handle those notes if they are deprecated - but I assume we may still need some changes there.

Generally when a proposal makes backwards-incompatible changes to behavior, the main body of the proposal just says what the eventual new behavior will be (in this case, this would mean "these functions always throw errors"). Then in a Deprecation Process section at the bottom of the spec, it describes how existing implementations should deprecate their current behavior in preparation for becoming spec-compliant.

The general idea here is that, if a new implementation were to come along, it should just implement the primary spec—it wouldn't be obligated to match Dart Sass deprecation-for-deprecation.

Comment on lines 94 to 97
a color function. The 'RGB' color space is identical to the 'sRGB' space, and
both describe the 'sRGB' gamut. But we have both `rgb()` and `color(srgb)`
syntax, in order to distinguish legacy from non-legacy variations. Some new
Copy link
Contributor

Choose a reason for hiding this comment

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

Might also be worth mentioning that RGB and sRGB have different coordinate systems, since RGB goes from 0-255 and sRGB goes from 0-1.


There are several rules of thumb for working with color spaces in Sass:

- The `rgb`, `hsl`, and `hwb` spaces are considered 'legacy spaces', and will
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would be more readable in plain-text form with empty lines between the list elements.

Comment on lines 142 to 144
- The `srgb` color space is equivalent to `rgb`, except that one is a legacy
space, and the other is not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also the coordinate systems!

provided for manipulation, the resulting color will still be returned in the
same space as the origin color. For `color.mix()`, the first color parameter
is considered the origin color.
- All legacy and RGB spaces represent bounded gamuts of color. When converting (
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can find a better term than "RGB spaces", since a naive reader will probably assume it means either "the rgb color space" or "the rgb or srgb color spaces". Maybe "RGB-style spaces"?

provided for manipulation, the resulting color will still be returned in the
same space as the origin color. For `color.mix()`, the first color parameter
is considered the origin color.
- All legacy and RGB spaces represent bounded gamuts of color. When converting (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- All legacy and RGB spaces represent bounded gamuts of color. When converting (
- All legacy and RGB spaces represent bounded gamuts of color. When converting

Comment on lines 294 to 297
are considered _out of gamut_ for the given color space. If the channel is
bounded, or has a percentage mapping with a lower-boundary of zero, then the
channel is considered _scalable_.
Copy link
Contributor

Choose a reason for hiding this comment

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

This term "scalable" isn't actually referenced anywhere. Is it supposed to be referenced in color.scale()?


* Let `gray` be the result of `whiteness + blackness`.

* Let `white` be `gray - whiteness`, and let `black` be `gray - blackness`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always just set white to blackness and black to whiteness.


* Let `new` be `(channel + 180deg) % 360deg`.

* Otherwise, if `channel` represents either chroma or saturation:
Copy link
Contributor

Choose a reason for hiding this comment

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

"represents" is somewhat underspecified here. Maybe just say "if channel's name is either chroma or saturation"


* Otherwise:

* Let `origin` be `$color`'s [known color space].
Copy link
Contributor

Choose a reason for hiding this comment

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

What if its color space isn't known?


* If `$color` is not a color, throw an error.

* Let `rgb` be the result of [converting] `$color` to `rgb`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should gamut-map too, since there's no way to represent out-of-gamut colors in the 8-digit hex format.

@mirisuzanne
Copy link
Contributor Author

mirisuzanne commented Sep 7, 2022

Still a few ToDo items here:

  • re-organize deprecated functions
  • serialization for non-legacy colors
  • make a small "Look Up a Known Color Space" procedure that maps a SassScript argument to a color space?

proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
proposal/color-4-new-spaces.md Outdated Show resolved Hide resolved
Co-authored-by: david herron <41588129+dvdherron@users.noreply.github.com>
Co-authored-by: david herron <41588129+dvdherron@users.noreply.github.com>
@mirisuzanne
Copy link
Contributor Author

I think all the outstanding issues here have been addressed. Ready for another round of review.

@nex3 nex3 self-requested a review September 20, 2022 01:48
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.

One more round of review—we should be good to go after this!

Comment on lines 174 to 177
* Mapping colors into a gamut is a lossy process. Whenever possible, it should
be left to the browser, which can map colors based on a given user's display
capabilities. However, authors can perform explicit gamut mapping with the
`color.to-gamut()` function.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems mostly redundant with the last bullet point, other than mentioning color.to-gamut(). Maybe the two should be merged?


[known color space]: #known-color-space

#### Serialization of Non-Legacy Colors
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be in a separate "Serialization" section.


* Emit "color(", followed by `color-space`, " ", `components`, and then ")".

* Otherwise, emit `color-space` followed by "(", `components`, and then ")".
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this will emit unknown color spaces as their own Sass functions, which I think is incorrect. We should probably explicitly list the color spaces that have their own function names, and emit the color() function as the default.


To serialize a non-legacy color `color`:

* Let `components` be a space-separated list of `color`'s channel values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Channel values aren't intrinsically SassScript values, so this should explain how to convert from the one to the other. In particular:

  • Which units (if any) do which channels have?
  • What SassScript value should missing channels have?

Comment on lines 592 to 594
with a matching name. It throws an error if `name` is not a valid color space
name, and either returns the known color space, or `name` if no color space is
matched.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of confusing that this can return either a color space or the name of a color space—it means that any spec invoking this procedure could end up working with two different "types". Maybe it should just return null if the space isn't known?


* For each keyword `key` and value `new` in `channel-args`:

* If `new` is not a number or the special value `none`, throw an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above: we need to be explicit about how to recognize a SassScript value that indicates none.


* If the keyword argument `$alpha` is specified in `$args`:

* If `alpha == none`, throw an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not treat it as zero? Or potentially just keep it as none?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think CSS Colors 4 defines a valid usage of none for the alpha value. IIRC, it only has none for color space channels, not for the alpha.

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 current spec does allow alpha components to be set to none - see eg https://drafts.csswg.org/css-color-4/#missing:

All color functions allow any of their components to be specified as none.

and https://drafts.csswg.org/css-color-4/#funcdef-rgb for explicit mention in a color function.

But yes, I think we can treat none as 0 in this case. Making that change.

Comment on lines 1471 to 1472
* Set the `channel` with name or index of `key` in `channels` to
`channel + adjust`, treating values of `none` as `0`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since none indicates that the channel value is irrelevant, should we just preserve it as-is even if an adjustment is made to it?

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 would expect adjust() and scale() to have the same behavior here. Likely choosing between:

  • Maintain none as an irrelevant channel. This strikes me as potentially confusing for authors, since it results in no change and no notification?
  • Adjust/scale from 0.
  • Throw an error.

CSS color 5 has a similar situation, where color channels can be adjusted in a relative color syntax. I don't see it clearly defined in that spec what should happen when missing channels are adjusted. I think it would be great to follow their lead if we're able? I've reached out to Chris, and can also open an issue in CSS. My expectation would be either adjusting-from-0, or considering it an invalid operation.


* Let `max` be the upper boundary of `channel` in `space`.

* If `channel > max` or `channel < 0`, throw an error.
Copy link
Contributor

@nex3