Skip to content

Conversation

kleinfreund
Copy link
Contributor

@kleinfreund kleinfreund commented Jun 18, 2025

For code review, commit 3 fix: parser not being able to distinguish between legacy and modern formats is the core of what the PR title suggests. Commits 1 and 2 are related to the code this work touches, but themselves not essential for the commit 3. As such, I'd be completely fine with a request to drop them or split them off into a separate PR.

Changes

refactor: correct parseArgument types

Avoid the /** @type {ArgumentMeta} */ (meta) type assertion and use /** @satisfies {ArgumentMeta} */ (meta) instead to ensure meta conforms to the ArgumentMeta type.

chore: add comma check to legacy syntax parsing check to be more robust

Make the parser check identifying whether a parameter is the alpha channel more robust by checking for the presence of commas in the color string when checking for legacy syntaxes. This also allows for the removal of the name !== "color" check as modern color syntaxes can't contain commas.

fix: parser not being able to distinguish between legacy and modern formats

Validate the parsed CSS types for elements in color strings that are using functional syntax against the coordinate grammar defined in the corresponding color space. If a disallowed type is found in the parsed color, a TypeError is thrown.

To actually distinguish legacy and modern syntaxes, the parser was updated to look for a named format with a _legacy suffix first (e.g. parsing an hsl color will look for hsl_legacy first and then for hsl). This is not a good system as it's not flexible and because it relies on somewhat strangely named formats to be defined in the library. Ideally, color spaces define their grammar kind of like the specification does. The parser would identify the color space and then check if the parsed color matches any of the space's grammar(s) (e.g. for HSL one of legacy hsl/hsla or modern hsl/hsla). I also think it would be useful to distinguish between grammars (parsing) and formats (serialization). However, any further structural changes to the parsing logic would blow up the scope of this change which intends to fix the incorrect parsing of legacy HSL colors.

Closes #648.

Notes

As stated in the commit messages, I'm not happy with the approach of the _legacy-suffixed formats. I'd prefer a comprehensive grammar definition separately from formats to be used to try and match a parsed color string.

The grammars for the HSL color space could be defined like this (based on https://www.w3.org/TR/css-color-4/#funcdef-hsl):

const hsl = {
  grammars: [
    {
      name: "hsl",
      label: "<modern-hsl-syntax>",
      parameters: [
        {
          types: ["<hue>", "none"],
        },
        {
          types: ["<percentage>", "<number>", "none"],
        },
        {
          types: ["<percentage>", "<number>", "none"],
        },
        {
          types: ["<percentage>", "<number>", "none"],
          optional: true,
        },
      ],
    },
    {
      name: "hsla",
      label: "<modern-hsla-syntax>",
      parameters: [
        {
          types: ["<hue>", "none"],
        },
        {
          types: ["<percentage>", "<number>", "none"],
        },
        {
          types: ["<percentage>", "<number>", "none"],
        },
        {
          types: ["<percentage>", "<number>", "none"],
          optional: true,
        },
      ],
    },
    {
      name: "hsl",
      label: "<legacy-hsl-syntax>",
      legacy: true,
      parameters: [
        {
          types: ["<hue>"],
        },
        {
          types: ["<percentage>"],
        },
        {
          types: ["<percentage>"],
        },
        {
          types: ["<percentage>", "<number>"],
          optional: true,
        },
      ],
    },
    {
      name: "hsla",
      label: "<legacy-hsla-syntax>",
      legacy: true,
      parameters: [
        {
          types: ["<hue>"],
        },
        {
          types: ["<percentage>"],
        },
        {
          types: ["<percentage>"],
        },
        {
          types: ["<percentage>", "<number>"],
          optional: true,
        },
      ],
    },
  ],
}

I haven't worked on anything using it so I can't say how ergonomic this is, but it should have all the information needed to accurately parse all specified color variants. The optional legacy property (false by default) on a grammar definition would be used to indicate the necessity of commas in the parameter list. The optional property (false by default) on a parameter definition would be used to mark a parameter as optional. Lastly, types lists the allowed CSS types per parameter. This schema also allows for such specifics like legacy RGB colors only allowing <percentage> or <number> and not a mix of them.

Since introducing anything like this would affect all color space definitions, this would be well out of scope for what I set out to fix in this change which is to fix the parsing of legacy HSL colors.

Avoid the `/** @type {ArgumentMeta} */ (meta)` type assertion and use `/** @Satisfies {ArgumentMeta} */ (meta)` instead to ensure `meta` conforms to the `ArgumentMeta` type.
Make the parser check identifying whether a parameter is the alpha channel more robust by checking for the presence of commas in the color string when checking for legacy syntaxes. This also allows for the removal of the `name !== "color"` check as modern color syntaxes can't contain commas.
…ormats

Validate the parsed CSS types for elements in color strings that are using functional syntax against the coordinate grammar defined in the corresponding color space. If a disallowed type is found in the parsed color, a `TypeError` is thrown.

To actually distinguish legacy and modern syntaxes, the parser was updated to look for a named format with a `_legacy` suffix first (e.g. parsing an `hsl` color will look for `hsl_legacy` first and then for `hsl`). This is not a good system as it's not flexible and because it relies on somewhat strangely named formats to be defined in the library. Ideally, color spaces define their grammar kind of like the specification does. The parser would identify the color space and then check if the parsed color matches any of the space's grammar(s) (e.g. for HSL one of legacy hsl/hsla or modern hsl/hsla). I also think it would be useful to distinguish between grammars (parsing) and formats (serialization). However, any further structural changes to the parsing logic would blow up the scope of this change which intends to fix the incorrect parsing of legacy HSL colors.

Closes color-js#648.
Copy link

netlify bot commented Jun 18, 2025

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 6991f5c
🔍 Latest deploy log https://app.netlify.com/projects/colorjs/deploys/6852e5a76855e400087afcc7
😎 Deploy Preview https://deploy-preview-653--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@kleinfreund
Copy link
Contributor Author

I'm putting this up for review now despite ultimately not being super happy with the approach (as mentioned in the PR description). Feel free to discard this PR or re-use parts of it in a different approach.

Also, as I added that bit later, from the top of the PR description:

For code review, commit 3 fix: parser not being able to distinguish between legacy and modern formats is the core of what the PR title suggests. Commits 1 and 2 are related to the code this work touches, but themselves not essential for the commit 3. As such, I'd be completely fine with a request to drop them or split them off into a separate PR.

@kleinfreund kleinfreund marked this pull request as ready for review July 29, 2025 17:11
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.

parse.js throws unnecessary error for hls()

1 participant