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

Wrong serialization result for rgba/hsla spaces, fixes #296 #297

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kajkal
Copy link

@kajkal kajkal commented Mar 28, 2023

#296 fix

const c = new Color("rgba(255,0,0,1)");

// before:
c.toString({format: "rgba"}); // rgba(100%, 0%, 0%)
// after:
c.toString({format: "rgba"}); // rgba(100%, 0%, 0%, 1)

@LeaVerou
Copy link
Member

Hi there,

Thank you for contributing and sorry it took so long to review this!

Could you elaborate a bit on this new withAlpha parameter that replaced the two previous noAlpha and lastAlpha params? It seems like a single boolean param cannot represent the three different states of syntax:

  • Mandatory alpha, e.g. rgba(0, 0, 0, 0)
  • Optional alpha, e.g. rgb(0 0 0 / 0)
  • Disallowed alpha, e.g. rgb(0, 0, 0)

OTOH if we hardcode that commas cannot have optional alpha, I suppose we could use a single param for this.

@kajkal
Copy link
Author

kajkal commented May 29, 2023

Hi,
a single optional boolean param can represent 3 states, at least in my mind.
I described my reasoning in more detail in #296.

@kajkal
Copy link
Author

kajkal commented May 29, 2023

By the way, I was reading the specification (Candidate Recommendation Draft) recently and from the way I understand it rgb() and rgba() are equivalent to each other, i.e. that rgba(0,0,0) or rgb(0,0,0,0) or rgba(0 0 0 / 0) are according to my interpretation of the specification valid. What is your opinion on the matter? Should rgba() force uses to define alpha after , or force uses to set alpha in rgb() only after /?
From what I checked color.js does not currently support the rgba(0,0,0) or rgba(0 0 0) and I was wondering if this is intentional or not.

@LeaVerou
Copy link
Member

Oh that's a good point, I had actually forgotten about that.

I do think that most people using format: "rgba" expect "old-style" rgba() though, with mandatory alpha. Now whether we cater to that or not is a separate discussion. Unfortunately often people expect what they're familiar with, not what is allowed or supported. We even had comments from people complaining that we return percentages by default, because they thought only numbers 0-255 were valid.

@kajkal
Copy link
Author

kajkal commented May 29, 2023

I looked more closely at the code some time ago and don't remember much anymore, so my comments may not be quite accurate.

From the output format perspective:
Currently there are 3 formats that work very similarly: "rgb_number", "rgba" and "rgba_number".
I would suggest replacing "rgba" and "rgba_number" with a single "legacy" or "css3" that would return color in a format compliant with CSS color 3 - so commas and 0..255 for r,g,b, rgb when no alpha and rgba when with alpha - or always rgba.
And as for the "rgb_number" format, I would give the option to include alpha.

So to summarize the 3 output formats beyond keyword and hex:
default: rgb(100% 80% 60% / 0.3) 0%..100%
rgb_number: rgb(255 204 153 / 0.3) 0..255
legacy: rgba(255,204,153,0.3) 0..255

From the input format perspective:
rgb() and rgba() are interchangeable so maybe they can be replaced by one format? Or two if you would like to distinguish rgb(0..255 x3) from rgb(0%..100% x3). Speaking of which I don't know what to think about "rgb(255 0 50%)" with which color.js doesn't see any problems but is not compliant with the specification. I solved this exact problem quite recently in a custom parser this way - maybe you could introduce similar concept in your library by adding separatorGrammar alongside coordGrammar?

So to summarize the 3 input formats beyond keyword and hex:
default: rgba?(100% 80% 60% / 0.3) or rgba?(100%,80%,60%,0.3) or mix with <number> and discard rgb_number
rgb_number: rgba?(255 204 153 / 0.3) or rgba?(255,204,153,0.3)
legacy: served by both above

Four additional, loosely related comments:

  1. this regexp allows for #fffffff which according to the specification is not allowed
  2. this regexp does not allow for values like +0.0 or 12e1 which I think should be supported.
  3. new Color('rgb(0,0,128,0.7)') + '' throws error while new Color('rgba(0,0,128,0.7)') + '' doesn't throw
  4. new Color('rgba(0,0,128,0.7)').toJSON().alpha === 0.7 // false. This is a problematic behavior when you want to use some library for testing, for example jest, and expect.toEqual does not work properly because alpha is an object Number and not a primitive. This also applies to parse from "colorjs.io/fn"

These are my thoughts, I don't know if they interest you at all but for my own sake I leave them here.

@svgeesus
Copy link
Member

svgeesus commented Jul 6, 2023

I would suggest replacing "rgba" and "rgba_number" with a single "legacy" or "css3" that would return color in a format compliant with CSS color 3 - so commas and 0..255 for r,g,b,

Note that there are two legacy formats, the one in CSS Color 3 which uses <integer> and the one from CSS Color 4 which uses <number>.

@kajkal
Copy link
Author

kajkal commented Jul 6, 2023

Thanks for your reply. Yes you are right. The theoretical legacy1/css3 (CSS Color 3 compliant) format should use integers (not real) 0..255, and alpha as real 0..1 not %.

My initial motivation for creating this PR was to provide a reliable way to get a valid CSS Color 3 compliant rgb()/rgba() color value. I don't know if the change proposed here is desirable or if a larger revision of the rgb formats is planned to better match the current state of the CSS Color 4 specification.
In other words: should I implement @LeaVerou suggestion and change the type from withAlpha?: boolean to alpha?: "always" | "never" | "auto" or should I close this PR?

@svgeesus svgeesus marked this pull request as draft January 24, 2024 15:31
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.

3 participants