Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. #439

jgerigmeyer · 2024-02-14T20:15:50Z

Partial solution for #433, as an alternative to #435.

This PR makes a few changes, and I'm not tied strongly to any of them -- this is a starting point for discussion.

Parses both <dashed-ident> and <ident> versions of color() spaces, warning if e.g. color(rec2100-hlg) is used instead of color(--rec2100-hlg), and similarly if color(--display-p3) is used instead of color(display-p3).
Simplifies color space options, where now every color gets formats.color.id defined if it's missing -- defaults to cssId if provided, otherwise id.
Adds missing cssId option to color spaces, using dashed-ident where appropriate, and removes unnecessary formats.color definitions.
Adds parsing tests for all missing color spaces.

This PR does not attempt to auto-generate coordGrammer for color spaces.

This shouldn't be a breaking change in terms of parsing colors (and it does fix #433), but it is a breaking change in the sense that now e.g. color(acescc) will be serialized as color(--acescc).

…ents to color spaces.

netlify · 2024-02-14T20:15:54Z

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	`da4dcb3`
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/65cfc009e20ade0008cac2c5
😎 Deploy Preview	https://deploy-preview-439--colorjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

src/spaces/prophoto-linear.js

lloydk

Should the HDR color spaces in CSS Color HDR Module Level 1 have a <dashed-ident>?

Will we remove the <dashed-ident> cssId as a breaking change if/when the HDR spaces are implemented in browsers?

I think we should go with unprefixed ids for the HDR spaces as they'll most likely be made a standard part of CSS.

Overall I prefer this PR to #435.

src/spaces/prophoto-linear.js

src/parse.js

src/spaces/a98rgb.js

jgerigmeyer · 2024-02-15T16:56:24Z

Should the HDR color spaces in CSS Color HDR Module Level 1 have a ?

I think we should go with unprefixed ids for the HDR spaces as they'll most likely be made a standard part of CSS.

I went with <dashed-ident> for color spaces that aren't currently part of the spec even if they will be added, but I'm fine with whatever @LeaVerou prefers here.

Will we remove the cssId as a breaking change if/when the HDR spaces are implemented in browsers?

Yes, my assumption is that as colors are added to the spec and implemented in browsers, we will remove the dashed-ident from their cssId. It's not entirely a breaking change in that we will still parse those colors with or without the prefix, but it would mean that they would be serialized without the dashed-ident going forward.

Overall I prefer this PR to #435.

🎉

LeaVerou · 2024-02-16T15:36:16Z

src/parse.js

@@ -102,17 +103,24 @@ export default function parse (str, {meta} = {}) {
 							Object.assign(meta, {formatId: "color", types});
 						}

+						if (colorSpec.id.startsWith("--") && !id.startsWith("--")) {
+							console.warn(`Use prefixed color(${colorSpec.id}) instead of color(${id}).`);


We need to clarify why this distinction exists, otherwise the warning makes it sound like Color.js is annoying for no reason. E.g. something like:

Suggested change

console.warn(`Use prefixed color(${colorSpec.id}) instead of color(${id}).`);

console.warn(`${space.name} is a non-standard space, so you should use color(${colorSpec.id}) instead of color(${id}).`);

@LeaVerou Adjusted in b1cf95a -- what do you think?

src/parse.js

LeaVerou · 2024-02-16T15:40:42Z

src/spaces/p3-linear.js

@@ -14,6 +14,7 @@ const fromXYZ_M = [

 export default new RGBColorSpace({
 	id: "p3-linear",
+	cssId: "--display-p3-linear",


Oof. I understand why it's as awkward as it is, but it makes me sad.

I wonder if it may make sense to allow providing multiple dashed ids, so we could also provide --p3-linear but that's a out of scope for this PR…

we should not have called it just "p3" because that is ambiguous. Cinema uses DCI-P3, video uses P3-D65 and CSS (among other things) uses Display-P3.

src/spaces/p3.js

LeaVerou

This is fantastic work @jgerigmeyer, thank you so much for working on this!!

My only substantive comment at this point is that I'd prefer to keep the trailing commas to keep the diff small, but even that I wouldn't elevate it to a blocker.

lloydk

LGTM

* main: Enforce trailing commas unless `]` or `}` is on the same line. (#440) Update index.html Update color-swatch.css lint [apps/gamut-mapping] Support multiple colors [apps/gamut-mapping] Show favicon and title immediately, not just when color changes [apps/gamut-mapping] Prepare HTML & CSS for supporting multiple colors

svgeesus · 2024-02-16T19:37:39Z

I like the idea of parsing both the dashed and undashed forms.

I went with <dashed-ident> for color spaces that aren't currently part of the spec even if they will be added, but I'm fine with whatever @LeaVerou prefers here.

Note that CSS Color HDR is now an official CSSWG spec, building on CSS Color 4 which remains SDR-only. So we should not have dashed idents for things already part of that spec.

See also:

Support rec2100-linear? #443

src/spaces/ictcp.js

src/spaces/jzazbz.js

src/spaces/jzczhz.js

svgeesus · 2024-02-16T19:41:44Z

src/spaces/p3-linear.js

@@ -14,6 +14,7 @@ const fromXYZ_M = [

 export default new RGBColorSpace({
 	id: "p3-linear",
+	cssId: "--display-p3-linear",


we should not have called it just "p3" because that is ambiguous. Cinema uses DCI-P3, video uses P3-D65 and CSS (among other things) uses Display-P3.

src/spaces/rec2100-hlg.js

src/spaces/rec2100-pq.js

jgerigmeyer · 2024-02-16T20:09:23Z

Note that CSS Color HDR is now an official CSSWG spec, building on CSS Color 4 which remains SDR-only. So we should not have dashed idents for things already part of that spec.

@svgeesus Adjusted in da4dcb3

svgeesus

LGTM

* main: [ci] Update lint rules for PR's (#444) Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. (#439) [types] Fix formatting in `toGamut.d.ts` (#445) [types] Add JSDoc based on website docs (#436)

…ents to color spaces. (color-js#439) * Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. * Warn when using incorrect dashed-ident/ident * refactor * Update warnings * Add trailing commas and make warning more descriptive. * Remove dashed-ident prefix for HDR colors.

jgerigmeyer added 2 commits February 14, 2024 14:44

Parse both dashed-ident and ident versions, and add missing dashed-id…

b732d33

…ents to color spaces.

Warn when using incorrect dashed-ident/ident

4921123

refactor

01d9953

jgerigmeyer commented Feb 14, 2024

View reviewed changes

src/spaces/prophoto-linear.js Show resolved Hide resolved

jgerigmeyer requested review from LeaVerou and lloydk February 14, 2024 20:20

jgerigmeyer mentioned this pull request Feb 14, 2024

(Some) nonstandard color spaces error when round-tripping #433

Closed

lloydk reviewed Feb 15, 2024

View reviewed changes

src/spaces/prophoto-linear.js Show resolved Hide resolved

src/parse.js Show resolved Hide resolved

LeaVerou reviewed Feb 15, 2024

View reviewed changes

src/spaces/a98rgb.js Show resolved Hide resolved

Update warnings

43f01f8

jgerigmeyer mentioned this pull request Feb 15, 2024

Preparing for v0.5.0 release #403

Closed

LeaVerou reviewed Feb 16, 2024

View reviewed changes

src/parse.js Show resolved Hide resolved

LeaVerou reviewed Feb 16, 2024

View reviewed changes

src/spaces/p3.js Outdated Show resolved Hide resolved

LeaVerou approved these changes Feb 16, 2024

View reviewed changes

lloydk approved these changes Feb 16, 2024

View reviewed changes

Add trailing commas and make warning more descriptive.

b1cf95a

jgerigmeyer force-pushed the missing-space-formats branch from 92e0915 to b1cf95a Compare February 16, 2024 17:03

jgerigmeyer mentioned this pull request Feb 16, 2024

Skip warnings in test contexts. #441

Merged

jgerigmeyer requested a review from LeaVerou February 16, 2024 17:14

svgeesus requested changes Feb 16, 2024

View reviewed changes

Remove dashed-ident prefix for HDR colors.

da4dcb3

jgerigmeyer requested a review from svgeesus February 16, 2024 20:09

svgeesus approved these changes Feb 19, 2024

View reviewed changes

jgerigmeyer merged commit d6f6836 into main Feb 19, 2024
5 checks passed

jgerigmeyer deleted the missing-space-formats branch February 19, 2024 16:12

jgerigmeyer mentioned this pull request Feb 20, 2024

Add missing formats to color spaces #435

Closed

apiiro-snyk mentioned this pull request Jul 3, 2024

[Snyk] Upgrade colorjs.io from 0.4.5 to 0.5.0 apiiro-snyk/halo#3

Open

Carolinewly mentioned this pull request Aug 4, 2024

[Snyk] Upgrade colorjs.io from 0.4.5 to 0.5.2 Carolinewly/halo#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. #439

Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. #439

jgerigmeyer commented Feb 14, 2024 •

edited

Loading

netlify bot commented Feb 14, 2024 •

edited

Loading

lloydk left a comment

jgerigmeyer commented Feb 15, 2024

LeaVerou Feb 16, 2024

jgerigmeyer Feb 16, 2024

LeaVerou Feb 16, 2024

svgeesus Feb 16, 2024 •

edited

Loading

LeaVerou left a comment

lloydk left a comment

svgeesus commented Feb 16, 2024

svgeesus Feb 16, 2024 •

edited

Loading

jgerigmeyer commented Feb 16, 2024

svgeesus left a comment

	console.warn(`Use prefixed color(${colorSpec.id}) instead of color(${id}).`);
	console.warn(`${space.name} is a non-standard space, so you should use color(${colorSpec.id}) instead of color(${id}).`);

Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. #439

Parse both dashed-ident and ident versions, and add missing dashed-idents to color spaces. #439

Conversation

jgerigmeyer commented Feb 14, 2024 • edited Loading

netlify bot commented Feb 14, 2024 • edited Loading

✅ Deploy Preview for colorjs ready!

lloydk left a comment

Choose a reason for hiding this comment

jgerigmeyer commented Feb 15, 2024

LeaVerou Feb 16, 2024

Choose a reason for hiding this comment

jgerigmeyer Feb 16, 2024

Choose a reason for hiding this comment

LeaVerou Feb 16, 2024

Choose a reason for hiding this comment

svgeesus Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

LeaVerou left a comment

Choose a reason for hiding this comment

lloydk left a comment

Choose a reason for hiding this comment

svgeesus commented Feb 16, 2024

svgeesus Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

jgerigmeyer commented Feb 16, 2024

svgeesus left a comment

Choose a reason for hiding this comment

jgerigmeyer commented Feb 14, 2024 •

edited

Loading

netlify bot commented Feb 14, 2024 •

edited

Loading

svgeesus Feb 16, 2024 •

edited

Loading

svgeesus Feb 16, 2024 •

edited

Loading