-
Notifications
You must be signed in to change notification settings - Fork 191
lab() and lch() color previews added #306
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
Conversation
Thanks for the PR, it looks promising. I added comments.
|
Done! Didn't know about the formatter, I'll keep that in mind going forward. And I've replaced const when possible. Is the functionality required all covered by this? I'll add more test cases and the other two functions if so. Thank you. |
Thank you for this @GauravB159! I've gone over these changes and the conversions don't seem to be accurate. Good reference code can be found here : https://github.com/w3c/csswg-drafts/tree/main/css-color-4 We also have a bunch of tests : https://github.com/csstools/postcss-plugins/blob/main/plugins/postcss-lab-function/test/basic.display-p3-false.preserve-true.expect.css But best of all would be to use https://github.com/LeaVerou/color.js, if @aeschli is open to adding this as a dependency? One important aspect that doesn't seem to be covered here is out of gamut color handling. It is not possible to round trip |
Hi @romainmenke I used this webpage for a reference for the formulae. Is there somewhere I could see the correct values? I was using this online converter to verify my values. It does give values in decimals, but rgb requires whole numbers I guess, so I have rounded the values at the end. Maybe that is causing an issue?
Thanks for your help! I'll check out the code you linked, maybe there's some issue with the formula implementation. |
Thanks for the great info @romainmenke ! |
Rounding errors might also cause this, but those would be very small deviations. I think there are three options :
The "right" option depends on the function of this feature in VSCode :
Got it! The code linked from the CSSTools repo is a port to TypeScript from the reference implementation that exists in the csswg-drafts repo. It has the same W3C license because the only thing I did was add typings. |
@romainmenke Yeah, I understand what you're saying. Now that you mention it, I remember there were negative and valuss greater than 255 after converting to sRGB. I clamped those values to 0-255 which might be causing the loss. @aeschli Which option do you recommend we go with? We could store the negative values and clamp them just while displaying the color possibly. |
Conversion from a wide to a narrow gamut is naturally lossy, so that should not be avoided. In the case of the color picker, the users can be informed that conversion is lossy, like in Chrome's DevTools color picker: https://developer.chrome.com/docs/devtools/css/color#convert-colors This approach may require ditching the click-to-convert in favor of a dropdown. |
When displaying a color on a screen or in print, yes. But wide gamut colors can still be represented in narrow gamut notations by going beyond the value ranges that describe the boundaries of the narrower gamut.
There is no mathematical reason to make these lossy when converting from one notation to another. |
Interesting,
fits VSCode usecase the best, though I'm curious how would |
src/languageFacts/colors.ts
Outdated
z: 0, | ||
alpha: lab.alpha ?? 1 | ||
}; | ||
xyz["y"] = (lab.l + 16.0) / 116.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use xyz.y
instead of xyz["y"]
src/languageFacts/colors.ts
Outdated
@@ -14,7 +14,9 @@ export const colorFunctions = [ | |||
{ func: 'rgba($red, $green, $blue, $alpha)', desc: l10n.t('Creates a Color from red, green, blue, and alpha values.') }, | |||
{ func: 'hsl($hue, $saturation, $lightness)', desc: l10n.t('Creates a Color from hue, saturation, and lightness values.') }, | |||
{ func: 'hsla($hue, $saturation, $lightness, $alpha)', desc: l10n.t('Creates a Color from hue, saturation, lightness, and alpha values.') }, | |||
{ func: 'hwb($hue $white $black)', desc: l10n.t('Creates a Color from hue, white and black.') } | |||
{ func: 'hwb($hue $white $black)', desc: l10n.t('Creates a Color from hue, white and black.') }, | |||
{ func: 'lab($lightness $channel_a $channel_b $alpha)', desc: l10n.t('css.builtin.lab', 'Creates a Color from Lightness, Channel a, Channel b and alpha values.') }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l10n.t no longer uses a key as first argument
src/parser/cssScanner.ts
Outdated
@@ -440,12 +440,13 @@ export class Scanner { | |||
} | |||
|
|||
private _number(): boolean { | |||
let npeek = 0, ch: number; | |||
let npeek = 0, ch: number, next_ch: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a good change. In the scanner, numbers and the minus are separate tokens.
The parser then has the concept of unary operator, where a minus can be combined with a number literal
This happens in _parseTerm
@@ -128,6 +171,10 @@ suite('CSS - Language Facts', () => { | |||
assertColor(parser, '#main { color: hsla(240 100% 50% / .05) }', 'hsl', colorFrom256RGB(0, 0, 255, 0.05)); | |||
assertColor(parser, '#main { color: hwb(120 0% 0% / .05) }', 'hwb', colorFrom256RGB(0, 255, 0, 0.05)); | |||
assertColor(parser, '#main { color: hwb(36 33% 35%) }', 'hwb', colorFrom256RGB(166, 133, 84)); | |||
assertColor(parser, '#main { color: lab(90 100 100) }', 'lab', colorFrom256RGB(255, 112, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also test percentages
@aeschli When I was last working on this PR, oklch() and oklab() were still pending, I'm assuming they still are. The original issue does ask for them, should those be implemented in a separate PR? Haven't worked on this in a while but can look into it if needed. |
Yes, a new PR is best and help is welcome! |
result.push({ label: label, textEdit: TextEdit.replace(range, label) }); | ||
|
||
const lch = lchFromColor(color); | ||
if (lab.alpha === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be lch.alpha
?
…ctions __RESOLVES:__ _feature_ - *Add new CSS color functions* \[[microsoft#305](https://github.com/microsoft/vscode-css-languageservice/issues/305)\] __DESCRIPTION:__ CSS Color Module Level 4 adds support for `oklch` and `oklab` to browsers. They are becoming more popular, and I wanted to use them with color previews in my editor. This feature is built of of the original [microsoft#306](microsoft#306) which included steps for `lab` and `lch` but not the `ok` variants. Generally this code was designed using real world color [examples](https://www.oddcontrast.com/#hex__*f00__*4d216f80__srgb). __STEPS TO TEST:__
Issue #305
Main Repo Issue microsoft/vscode#165207