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

lab, lch, oklab and oklch syntax lowering does not support numeric values for L component #445

Closed
fpapado opened this issue Mar 17, 2023 · 7 comments

Comments

@fpapado
Copy link

fpapado commented Mar 17, 2023

The lab(), lch(), oklab() and oklch() functional notations support a numeric value for the L component. However, lightningcss only supports percentage values for those components, and bails out if a number is used.

In other words, these two declarations should be equivalent and lowered to background: #777:

.lch-percentage {
  background: lch(50% 0 223.97);
}
/* This is a valid representation as well (L [0,100] for lch()) */
.lch-number {
  background: lch(50 0 223.97);
}

But this is the output instead:

.lch-percentage {
  background: #777;
  background: lch(50% 0 223.97);
}

/* Stays intact, instead of being lowered */
.lch-number {
  background: lch(50 0 223.97);
}

Link to the playground, showing the lack of lowering for the numeric L component

Expected behaviour

  • lch() should allow numeric values of [0,100] for the L component
  • lab() should allow numeric values of [0,100] for the L component
  • okch() should allow numeric values of [0,1] for the L component
  • oklab() should allow numeric values of [0,1] for the L component

Note the difference in the numeric range: lch() and lab() define them as [0,100] corresponding to 0% and 100%, while oklch and oklab define them as [0,1.0] corresponding to 0% and 100%.

Reference for lch() and lab() numeric range
Reference for oklch() and oklab() numeric range

Please let me know if there is any other information that I can provide, or whether I missed something! I would be happy to contribute a PR for this :)

@fpapado
Copy link
Author

fpapado commented Mar 21, 2023

I've been poking at this over the past few days, out of curiosity. The underlying issue about parsing the functional notation, parsing the L component as either a number of a percentage.

With that change, there is a follow-up issue for the relative component parsing, which is currently giving me trouble. In relative color syntax, the channel keywords now resolve to numbers (e.g. lch(from peru calc(l * 0.8) c h) would treat l/c/h as number). I'm digging through the code trying to understand if it handles those as percentages, and how to change it. Most of the tests are passing, except the relative syntax math.

On a related note, the WPT tests have also been updated recently. They use the numeric syntax and also show some of the calculations. I updated the tests to this latest version, for completeness.

I noticed while going through the specs, that the serialisation of the lab/lch/oklab/oklch component values has also changed since lightningcss' implementation. The components now are serialised as numbers base 10, instead of percentages. Serialised, L is [0,1] for oklab/oklch and [0,100] for lab/lch. So it makes sense to support the number or percentage parsing for round-tripping. Updating the serialisation can be done in another PR, it might need updates to the tests iirc.

(It's possible I am missing something again 😅 )

@devongovett
Copy link
Member

Thanks for all your research on this! Should be fixed by above commit. For now I've left the serialization alone. Safari 15 implemented the old spec as well, and does not support numeric values for l or percentages for the other components, so serializing the old way is the most compatible.

@devongovett
Copy link
Member

So it turns out that the way relative colors work also changed, which will break existing code using lightningcss. This is kinda unfortunate and I'm not sure what to do about it. w3c/csswg-drafts#7876 (comment)

@fpapado
Copy link
Author

fpapado commented Apr 11, 2023

Ah, I had a similar fear when I read the latest spec and was making code changes 😓 The lch(from indianred calc(l + 10%) c h) test was failing. I think some kind of breaking change might be inevitable, because even if you kept the current behaviour, native css to lightningcss won't be a direct conversion as browsers ship the syntax (if I understand correctly).

I did a quick check on Safari 16.4, and I believe it passes the channels as numbers:
Screenshot 2023-04-11 at 17 41 55
Screenshot 2023-04-11 at 17 42 31

How are changes in draft proposals (e.g. nesting, custom media queries) handled in lightningcss? Is it accepted by the users that there might be breaking changes? I can imagine something more structured (e.g. introducing a flag for the new relative color syntax behaviour, but would people move over?), or emitting a warning in the logic that tries Number/Percentage calculations. I briefly tried modifying that last one, but got a bit lost along the way.

It's also a bit unfortunate that there are not many WPT cases covering this (last time I checked).
I've been working with this set for minimal reproductions, if it's any help:

  // Test that relative syntax color functions round-trip to their single color function equivalent
  test("lab(from lab(50 0 0) l a b)", "lab(50 0 0)");
  test("lch(from lch(50 0 0) l c h)", "lch(50 0 0)");
  test("oklab(from oklab(50 0 0) l a b)", "oklab(50 0 0)");
  test("oklch(from oklch(50 0 0) l c h)", "oklch(50 0 0)");

  // Test that relative syntax color functions round-trip to their single color function equivalent, when using calc to substitute
  test("lab(from lab(50 0 0) calc(l) calc(a) calc(b))", "lab(50 0 0)");
  test("lch(from lch(50 0 0) calc(l) calc(c) calc(h))", "lch(50 0 0)");
  test("oklab(from oklab(50 0 0) calc(l) calc(a) calc(b))", "oklab(50 0 0)");
  test("oklch(from oklch(50 0 0) calc(l) calc(c) calc(h))", "oklch(50 0 0)");

  // Relative syntax calculations
  // In lab and lch, l is [0,100], and calc() reflects that
  test("lab(from indianred calc(l + 10) a b)", "lab(63.9252 45.7516 23.1557)");
  test("lch(from indianred calc(l + 10) c h)", "lch(63.9252 51.2776 26.8448)");

  // NOTE: This is a breaking change from the previous version that treated channels as percentages
  // This would not work by itself
  // test("lch(from indianred calc(l + 10%) c h)", "lch(63.9252 51.2776 26.8448)");

  test("lab(from indianred calc(l * 1.1) a b)", "lab(59.3178 45.7516 23.1557)");
  test("lch(from indianred calc(l * 1.1) c h)", "lch(59.3178 51.2776 26.8448)");

  // In oklab and oklch, l is [0,1], and calc() reflects that
  test(
    "oklab(from indianred calc(l + 0.1) a b)",
    "oklab(0.715441 .133439 .0545326)",
  );
  test(
    "oklch(from indianred calc(l + 0.1) c h)",
    "oklch(0.715441 .144152 22.2284)",
  );
  test(
    "oklab(from indianred calc(l * 1.1) a b)",
    "oklab(0.6769851 .133439 .0545326)",
  );
  test(
    "oklch(from indianred calc(l * 1.1) c h)",
    "oklch(0.6769851 .144152 22.2284)",
  );

@devongovett
Copy link
Member

How are changes in draft proposals (e.g. nesting, custom media queries) handled in lightningcss?

Yeah for nesting and custom media queries they are under an opt in flag (eg drafts.nesting), but I did not implement relative colors under a flag because my impression was that the spec was already stable. 😕

@devongovett
Copy link
Member

I opened a PR #465 to implement the latest spec changes for relative colors. Haven't decided how to release it yet since it is a breaking change. Wasn't really planning on doing Lightning CSS 2.0 yet, but I guess we could. Otherwise we could hold this change until then, or try to investigate backward compatibility but not sure if that's a good idea. If you have any ideas let me know.

@fpapado
Copy link
Author

fpapado commented Apr 16, 2023

Thank you for your work on this 🙇 Looking at the PR clarifies some of the questions when I tried implementing this, and also how far-reaching the changes are (specifically changing the channel types for each colorspace, and carrying the types over during parsing).

My main idea to delay a v2 release, would be to put this new implementation under a flag, such as draft.relative_color_number_channels or maybe v2.relative_color_number_channels (to clarify that it will change). Then we could split the codepaths, offer a migration guide to the new flag and so on, until making it the only option in v2.

Looking at the code, it seems like large parts of src/values/colors.rs would diverge. To me, this sounds relatively ok, as long as there are no extra changes on top (which would have to apply to both codepaths). Earlier I was wondering why people would switch to a flag, but having compatibility with native CSS seems like a good incentive to start with.

Another thought I had, was offering a custom transform (or would a codemod be better?), to rewrite relevant calc expressions in relative color syntax. This option could work alongside the v2 flag, or the v2 release itself. I'm not sure about the complexity of implementing it.

It would be nice to have some metrics about the usage of the relative syntax in lightningcss, to understand how wide the breakage would be.

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

No branches or pull requests

2 participants