fix: hsl parsing not being spec-compliant #650
Conversation
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
41617f7 to
3c04af8
Compare
Add the `<number>` type to the saturation and lightness coordinates (i.e. changing them from `<percentage>` to `<percentage> | <number>`) for the HSL color space to match the specification (see https://www.w3.org/TR/css-color-4/#funcdef-hsl). Make parsing independent from the `format.alpha` configuration of color spaces and instead rely on the parser to identify whether there is a "last alpha" element in a color string using functional notation. This way, colors that are being parsed with a format that sets `format.alpha` to `true` (for serialization purposes) can still omit the alpha value which is valid in both legacy and modern syntax for hsl/hsla colors. Closes color-js#428, color-js#648.
3c04af8 to
085a113
Compare
|
@svgeesus @facelessuser @lloydk @MysteryBlokHed Any objections to merging this? Hearing none, I’ve approved it and will merge in the next couple days. |
| // If there's a slash here, it's modern syntax | ||
| $0.startsWith("/") | ||
| // If there's still elements to process after there's already 3 in `args` (and the we're not dealing with "color()"), it's likely to be a legacy color like "hsl(0, 0%, 0%, 0.5)" | ||
| || (name !== "color" && args.length === 3) |
There was a problem hiding this comment.
I would note that if you ever implement a color space with more than 3 color channels, this will be a problem. It is true that most color spaces are usually 3 channels, and it may be that there is no intention of implementing color spaces with more than 3 channels, but I feel I need to at least point it out.
There was a problem hiding this comment.
Yup, that was my reservation too, but I figured we can cross that bridge when we get to it. Though nothing prevents folks from having custom color spaces with more coordinates, so now I wonder if it may be better to localize this to the few formats it affects (I believe only hsl(), hsla(), rgb(), rgba()?). E.g. what if alpha in the format could also be a function instead of a static boolean? Then we could pass the current parsing state, instead of encoding the logic here.
Or, alternatively, we can expose a higher level property specifically for this (e.g. legacy: true).
There was a problem hiding this comment.
If moving forward in the current state is preferred, I would consider at least going ahead and generating a technical debt issue describing the issue to be solved in the future. I agree though that it might not be an immediate concern.
There was a problem hiding this comment.
Yes, that's an obvious flaw in my approach. It's an assumption that currently holds true, but that can break with time.
The first thing I thought was to store the number of possible channels on the relevant format. This also opens up the possibility for users to hook into individual parsing rules per channel for custom formats should that be something you're interested in.
Since in parseFunction, we don't have knowledge of the Format instance just yet, that would be a bit more heavy-handed to change. I can create a ticket for this as it would at least be good to have an answer for the project's intention with this.
There was a problem hiding this comment.
Draft for such a follow up issue should this get merged:
Remove parser dependance on all CSS colors in functional notation (except color()) having 4 elements (3 color coordinates; 1 alpha)
Currently, the colorjs.io parsing logic has a dependance on color strings in functional notation (except color()) having 4 elements: 3 color coordinates and 1 alpha. This was introduced in #650 and discussed in #650 (comment).
This dependance would lead to errors if support for color strings in functional notation with a different number of elements is introduced.
One such case is color() which has 5 elements (https://www.w3.org/TR/css-color-4/#predefined). This also illustrates that it's plausible that this becomes a problem as the elements in between the parentheses aren't constrained to be just color coordinates and an alpha value. For color(), the first element is the color space parameter and effectively shifts the color and alpha elements "to the right".
To remove this dependance, each color space format could hold a definition of each element in the corresponding functional notation. The introduction of a new format and/or color space would then have to only take care of providing an accurate definition and not need to mess with the general parsing logic.
There was a problem hiding this comment.
One thing to note is that even though the official color spaces don't use more than 3 channels yet, an author-defined color space can use any number of channels.
I’m not sure storing the number of channels on the format is the right way to go, since it could be variable.
It definitely seems like the right API shape is not obvious, so in the interest of progress, I'm still leaning towards merging this and filing the issue above.
Perhaps you could contain the scope of the change somewhat by adding a check for commas.
There was a problem hiding this comment.
There are a lot of places in the types where we assume only 3 coordinates, the most notable being the Coords type.
export type Coords = [number | null, number | null, number | null];
So we'll need to make a number of changes to the types to support an arbitrary number of coordinates.
There was a problem hiding this comment.
I’m not sure storing the number of channels on the format is the right way to go, since it could be variable.
I was thinking more along the lines of capturing the same information as the specs show for the syntax of colors: https://www.w3.org/TR/css-color-4/#funcdef-hsl
This would allow you to accurately describe where which kind of element can be expected and also whether its optional. In any case, that'll be part of the discussion in the follow up issue.
Perhaps you could contain the scope of the change somewhat by adding a check for commas.
Would you mind clarifying that for me? Checking parts[2] for commas like name !== "color" && args.length === 3 && parts[2].includes(",")?
There was a problem hiding this comment.
Opened the technical debt tracking issue for the dependance on number of channels here: #651
Just saw this. Typically, expressions are logged in the collapsed tree of the test, to avoid noise (i.e. you need to expand them). Is that the case or are they not logged in at all? I’m thinking that perhaps this should have been opt-in rather than just wrapping all console methods. 🤔 cc @DmitrySharabin (not urgent) |
Having now tried this on a macOS computer, I can confirm that logs happen. I originally worked on this using Windows where I'm fairly sure I tried this and it didn't work. I think what didn't work was expanding the leave nodes in the interactive report. So the timeout-based logging allowed me to "break" out of the reported nodes, so to speak. For what it's worth, I didn't thoroughly look into htest's options for whether perhaps either the "console log wrapping" or interactive reporter can be disabled. I reckon either would've "revealed" the logs to me. |
Oh, that is possibly an issue we are aware of and have already addressed, but it has not been released yet. It's been quite a while since our last release, and some good changes are still pending. It's probably about time to release them, @LeaVerou? |
| // If there's a slash here, it's modern syntax | ||
| $0.startsWith("/") | ||
| // If there's still elements to process after there's already 3 in `args` (and the we're not dealing with "color()"), it's likely to be a legacy color like "hsl(0, 0%, 0%, 0.5)" | ||
| || (name !== "color" && args.length === 3) |
There was a problem hiding this comment.
Yeah not super keen on that assumption, but the nature of legacy functional syntax makes it hard to avoid.
Having read the discussion I am in favor of merging it and logging the issue, as suggested.
There was a problem hiding this comment.
Tracking how to improve this in #651
| @@ -565,6 +565,60 @@ const tests = { | |||
| args: "hsl(230.6 179.7% 37.56% / 1)", | |||
| expect: { spaceId: "hsl", coords: [230.6, 179.7, 37.56], alpha: 1 }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Thanks both for the tests and the notes on missing/invalid tests!
|
This may also break if we ever pick up although note that RCS is only valid on modern syntax, so perhaps not. |
Changes
Add the
<number>type to the saturation and lightness coordinates (i.e. changing them from<percentage>to<percentage> | <number>) for the HSL color space to match the specification (see https://www.w3.org/TR/css-color-4/#funcdef-hsl).Make parsing independent from the
format.alphaconfiguration of color spaces and instead rely on the parser to identify whether there is a "last alpha" element in a color string using functional notation. This way, colors that are being parsed with a format that setsformat.alphatotrue(for serialization purposes) can still omit the alpha value which is valid in both legacy and modern syntax for hsl/hsla colors.Closes #428, #648.
Notes
Important: Currently (both before and with this PR's changes), HSL colors in the legacy functional format parse fine with
<number>values for saturation/lightness. Two skipped tests are added for this.This pull request contains a significant change to the parsing logic that needs careful review. Specifically, there's one assumption I'm making about the structure of color strings in functional notation: I assume that all color strings in functional notation except
color()that have four "elements" contain the alpha channel in the last element. Now I know this is certainly not true for relative CSS colors (e.g.lch(from blue calc(l + 20) c h)), but I'm not sure there's ever a chance for the current parsing logic to handle such colors. In other words, I assume that the parsing logic I changed deals exclusively with<color-function>excluding specificallycolor().The exclusion of
color()is possibly something that can be addressed, but I wanted to keep the scope of this PR clear. From a quick look at https://www.w3.org/TR/css-color-4/#predefined, a similar assumption as above could be made forcolor(): Perhaps one can assume that allcolor()strings that have five "elements" contain the alpha channel in the last element.I used the currently failing examples from the issues Ensure HSL is spec compliant #428 and parse.js throws unnecessary error for hls() #648 in the tests, feel free to suggest more cases that are useful to test.
Only related on a meta level: I found working with the test suite a little tricky because logging expressions was difficult. For one, logging synchronously would often lead to nothing being logged. I eventually resorted to using the following timeout-based technique which isn't ideal:
Furthermore, I found myself commenting out a lot of the tests to reduce the noise while logging. htest now has support for
skipwhich is useful, but while working on code, I regularly useonlyflags for only running tests within a test suite flagged as such without the need to disable them otherwise.