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

CSSColorValue as input for CanvasRenderingContext2D fill and stroke styles #6609

Closed
wants to merge 10 commits into from

Conversation

mysteryDate
Copy link
Contributor

@mysteryDate mysteryDate commented Apr 22, 2021

Adding CSSColorValue objects as an input to stroke and fill styles for CanvasRenderingContext2D. Originally proposed in this issue:
#5616


/canvas.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would appreciate any review help from @tabatkins since I'm not sure I fully understand CSS typed OM...

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<p>When set to a <code>CanvasPattern</code> or <code>CanvasGradient</code> object, the assignment
is <span>live</span>, meaning that changes made to the object after the assignment do affect
subsequent stroking or filling of shapes.</p>

<p>When set to a <a href="https://drafts.css-houdini.org/css-typed-om-1/#colorvalue-objects">CSSColorValue</a> object, the assignment is not live and instead
the object is converted to a color up assignment.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "converted to a color up assignment" mean? Does the fillStyle property stop returning a CSSColorValue object and start returning ... "a color"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! That's weird, I think some auto-formatting mangled my words.
I meant converted to a color. Basically:

ctx.fillstyle = new CSSRGB(1, 0, 1);
console.log(ctx.fillstyle):
> "#ff00ff"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great, converted to a color string. That helps with some of the mutability problems discussed in the other thread.

Make sure to define the algorithm for that, instead of using the "objects must be assigned themselves" clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't know what I was thinking when I wrote that. 🤦‍♀️

So right now each CSSColorValue type has a ToColor() function that returns an RGB color. Should I define the colorspace -> RGB conversions for each colorspace? Where should I do this?

Copy link
Contributor

@tabatkins tabatkins Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I haven't written the serializers for the color classes yet (there's a TODO issue there in the spec), but you should be able to assume they exist if necessary.

You should be able to just say "convert to an sRGB color", tho; all the built-in spaces know how to be converted to sRGB (see https://drafts.css-houdini.org/css-typed-om/#dom-csscolorvalue-torgb for example). The internal value the spec works with is an abstract CSS color; you don't need to pretend you're working with JS objects.

So right now each CSSColorValue type has a ToColor() function that returns an RGB color. Should I define the colorspace -> RGB conversions for each colorspace? Where should I do this?

No need there, that function is specifically to turn it into a CSSColor object (similar to toRGB() turning it into CSSRGB).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a link to https://drafts.css-houdini.org/css-typed-om/#dom-csscolorvalue-torgb and explicitly mentioned sRGB.

source Outdated
<p>When set to a <code>CanvasPattern</code> or <code>CanvasGradient</code> object, the assignment
is <span>live</span>, meaning that changes made to the object after the assignment do affect
subsequent stroking or filling of shapes.</p>

<p>When set to a <a href="https://drafts.css-houdini.org/css-typed-om-1/#colorvalue-objects">CSSColorValue</a> object, the assignment is not live and instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would it mean for the assignment to be live? Aren't CSSColorValue objects immutable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not immutable, but they are "dead". It's intended that you can reuse TypedOM values to set a property multiple times, tweaking the values on the same object rather than constantly creating new objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't quite understand the distinction. How would you mutate a CSSColorValue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By just setting the attributes? They're not readonly.

The point is that if you, say, get the value of the 'color' property and get a CSSColorValue out of it, you can fiddle with the values and it doesn't do anything to the page's style. You have to set it back to the proeprty for it to have an effect.

(And then you can do animations more cheaply by just reusing that color object and setting styles with it in each frame, rather than getting a fresh object on each frame and adjusting them.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find any attributes on either CSSColorValue or CSSStyleValue...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what's happening now. Inputting CSSColorValue is the same as inputting the equivalent color string.

What do you mean by "processing model work"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood the intent, because you said "objects must be assigned themselves", which implies to me that the property must have its value set to the object.

If you want to convert it into a string, then you need to define or reference the algorithm for converting a CSSColorValue into a string, and make sure that is run upon setting.

Partially this is annoying because the existing infrastructure is not using proper setter/getter steps and associated values. I might be able to work on an editorial PR to make that easier for you...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each color space needs have it's own conversion: https://drafts.css-houdini.org/css-typed-om-1/#dom-csscolorvalue-torgb
Is it enough to reference the houdini, or do they need to be defined explicitly here?

Partially this is annoying because the existing infrastructure is not using proper setter/getter steps and associated values. I might be able to work on an editorial PR to make that easier for you...

I'm not sure I understand. What is the "existing infrastructure"?

Copy link
Member

@domenic domenic Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "existing infrastructure" I mean the existing spec text. See my message below at #6609 (comment).

After the refactoring in #6614, the data model will be that the fill/stroke style are either a CSS color (not a string), a CanvasPattern, or a CanvasGradient. Note that on getting the CSS color is serialized into a string using the HTML spec's existing "serialization of a color" algorithm, which always produces either #123456 or rgba(, , ,) format values.

After this PR, what would you like the getter to return? I can see a few options:

  • In the setter. store the CSS color in RGBA format using the same algorithm that Houdini's toRGB() uses. (Houdini basically just says "convert this into an sRGB color"; I'm not sure if that's enough for an interoperable implementation.) Then, use the HTML spec's existing "serialization of a color" algorithm on the result in the getter.

  • In the setter, store the CSS color in some sort of unconverted original format. Then in the setter, serialize it using some sort of CSS serialization of that color, defined somewhere.

I think the main difference here is in cases where your CSSColorValue is outside the sRGB gamut. If you take the first option I mention, you'd convert into sRGB and lose fidelity, but fit very nicely into the existing infrastructure. Whereas if you take the second one, you get more power, but we have to do more work.

Concretely, consider:

const cssColor = CSSColorValue.parse("lch(59% 50 224)");
context.fillStyle = cssColor;

console.log(context.fillStyle);

should this log lch(59% 50 224) or #009cbc? (They are different colors.) And which should be used for drawing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Houdini basically just says "convert this into an sRGB color"; I'm not sure if that's enough for an interoperable implementation.)

It's enough; all the algorithms are well-defined and precise, and Color 4 references them more explicitly to help implementors. (They're just from the literature, rather than being reproduced in our spec.)


AFAIK, canvas is still sRGB; there's no way to get access to values outside that space yet, right? So it should eagerly convert to an internal RGB color (and then use the existing serialization logic to always produce a hex string).

source Outdated
parsing the value results in failure, then it must be ignored, and the attribute must retain its
previous value. If the new value is a <code>CanvasPattern</code> object that is marked as <span
data-x="concept-canvas-pattern-not-origin-clean">not origin-clean</span>, then the
<code>CanvasGradient</code>, <code>CanvasPattern</code> and <a href="https://drafts.css-houdini.org/css-typed-om-1/#colorvalue-objects">CSSColorValue</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What CSS context are the color values evaluated in? E.g. if the color value references a variable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't, luckily - that would make it a CSSUnparsedValue, rather than a CSSColorValue. With the exception of CSSColor and CSSCMYK (which can reference a user-defined color space), all the CSSColorValue subclasses are self-contained and eagerly evaluatable into whatever color format you want.

That does still mean we need to specify how those two deal with user-defined color spaces, tho. Easy route is to say "it can't" and you're stuck with just the predefined color spaces (and the naive CMYK space). That's probably how we want to do it for now; making it better would be a bit difficult with shadows and such, and we shouldn't hang this on that issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about currentcolor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a CSSKeywordValue, so similarly you don't need to worry about that here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also jumping back a sec, I'm reviewing Typed OM and see that, currently, it doesn't try to apply user-defined colorspaces in its own color conversion anyway yet. So if you have a new CSSColor("--foo", [0,1,0]), calling .toRGB() just throws currently. So doing the same thing here (eagerly checking for a custom colorspace and rejecting such CSSColor objects) is totally consistent with current practice, not just the easy way out. 👍

I anticipate we'll add a way to ref such custom colorspaces in the future, I'm just punting for now. When that happens it should end up working here as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about CSSMathValue coordinates? Not all math can be eagerly evaluatable, I think. E.g. what about things that represent colors like lch(0 calc(100vw / 10em) 50%)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That precise concern already exists, since you can provide that exact value as a string today. I haven't checked the spec to see if it actually addresses it (or just assumes that you're only passing simple values), but regardless, it's either undefined or already-solved for both cases, so the Typed OM object doesn't introduce anything new.

@domenic
Copy link
Member

domenic commented Apr 23, 2021

Hi @mysteryDate, I put up a PR at #6614 which refactors the current definition of fillStyle and strokeStyle to have clear getter/setter steps. That should make it much easier for you to slot in by just defining appropriate setter steps which convert the result to a CSS color and store that in the internal "fill style" and "stroke style" concepts.

You could try doing some git-fu to pull down my branch and rebase on top of it, or you could sit tight until that gets merged and rebased; hopefully Monday or so when @annevk gets back. I'm sorry about how that work delays your PR, but I do think it should make your PR easier to write.

@mysteryDate
Copy link
Contributor Author

Hi @mysteryDate, I put up a PR at #6614 which refactors the current definition of fillStyle and strokeStyle to have clear getter/setter steps. That should make it much easier for you to slot in by just defining appropriate setter steps which convert the result to a CSS color and store that in the internal "fill style" and "stroke style" concepts.

You could try doing some git-fu to pull down my branch and rebase on top of it, or you could sit tight until that gets merged and rebased; hopefully Monday or so when @annevk gets back. I'm sorry about how that work delays your PR, but I do think it should make your PR easier to write.

Thanks! There's no rush, I'll wait.

source Outdated Show resolved Hide resolved
source Outdated
parsing the value results in failure, then it must be ignored, and the attribute must retain its
previous value. If the new value is a <code>CanvasPattern</code> object that is marked as <span
data-x="concept-canvas-pattern-not-origin-clean">not origin-clean</span>, then the
<code>CanvasGradient</code>, <code>CanvasPattern</code> and <a href="https://drafts.css-houdini.org/css-typed-om-1/#colorvalue-objects">CSSColorValue</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about currentcolor?

@mysteryDate
Copy link
Contributor Author

@annevk For some reason github isn't letting me comment on your last comment:

What about currentcolor?

How would this get passed in? I've never seen currentcolor used in javascript.

@annevk
Copy link
Member

annevk commented Apr 27, 2021

See #6609 (comment); it's a resolved issue though.

@domenic domenic added addition/proposal New features or enhancements topic: canvas labels May 4, 2021
@domenic
Copy link
Member

domenic commented May 4, 2021

OK, #6614 is merged now, so it's probably best to recreate this PR on top of the current main branch. (You can do that either by force-pushing to your CSSColorValue-input branch, or closing this PR and opening a new one; either work fine.)

I think it'll be much easier now as you can add explicit steps in the fillStyle/strokeStyle setter steps that roughly look like:

  1. If the given value is a CSSColorValue, then:
    1. If the given value has a custom colorspace (not sure how to write this in CSS spec-ese), throw some appropriate exception
    2. Set this's fill style to the result of converting the CSSColorValue into an sRGB CSS color. (Not sure how to write this in CSS spec-ese.)

Although maybe now that we have custom color space support it should not be an sRGB color necessarily, but instead should be one that matches the this's color space? I'm not sure what the intended integration is there.

@mysteryDate
Copy link
Contributor Author

@domenic Done with the merge and the setter steps! I just linked to the toRGB that @tabatkins mentioned.
Shall I go ahead and file implementation bugs?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high-level this seems fine, but I have a bunch of nits and some questions about the color space parts.

@whatwg/canvas this could use your input as well.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
<li><p>If the given value has a custom colorspace, throw a <code>TypeError</code></p></li>
<li><p>Set <span>this</span>'s <span data-x="concept-CanvasFillStrokeStyles-fill-style">fill
style</span> to the result of converting the given value into an sRGB CSS color using its
<dfn data-x-href="https://drafts.css-houdini.org/css-typed-om/#dom-csscolorvalue-torgb">toRGB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to reference a low-level primitive. You cannot reference a public method here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why does it always convert to sRGB? Is that not lossy if the input is a display-p3 color and the canvas is also using that color space?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Canvas is still limited to sRGB internally. We're working on wide-gamut support, but it has not yet launched.

What kind of "low-level primitive" are you referring to? I'm going based on what @tabatkins said here on this PR:
#6609 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's limited to 8 bits per color, but you can use display-p3, right? Isn't there a difference?

As for the primitive, some kind of operation that does the conversion that doesn't involve invoking a JavaScript method. Presumably whatever the JavaScript method ends up invoking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now there's no way to input anything but sRGB colors to canvas fill and stroke styles. I can see that we don't want to enshrine this in the spec though.

Could it be something like:
"Convert the given value into this' colorspace and set this' fillStyle to the result."

Currently the "underlying function" is the virtual toColor function of CSSColorValue:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/cssom/css_color_value.h;l=31;bpv=1;bpt=1?q=csscolorvalue&sq=package:chromium&ct=os

This has a different implementation for every subclass of CSSColorValue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there's https://drafts.css-houdini.org/css-typed-om/#dom-csscolorvalue-tocolor, but it doesn't seem like there's an underlying algorithm we could hook into. We'd really only need step 3. @tabatkins @svgeesus could that be created?

Perhaps I see what you're saying now though in that we can only serialize to sRGB so perhaps we should always invoke this algorithm with sRGB for now until we want to make further changes here.

Copy link

@svgeesus svgeesus May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now there's no way to input anything but sRGB colors to canvas fill and stroke styles.

There is a proposal for that which adds both Wide Gamut and HDR to Canvas.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't seem like there's an underlying algorithm we could hook into. We'd really only need step 3. @tabatkins @svgeesus could that be created?

I suggest Converting between predefined RGB color spaces which outlines the steps. There is also a link to sample JS that does the conversion (but the same conversion can be done in native code of course)

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall; one question.

@@ -63301,6 +63309,18 @@ try {
<span>this</span>'s <span data-x="concept-canvas-origin-clean">origin-clean</span> flag to
false.</p></li>

<li><p>If the given value is a <code>CSSColorValue</code>, then:</p>
<ol>
<li><p>If the given value has a custom color space, throw a <code>TypeError</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below: is there any way to know whether the CSSColorValue has a custom color space ahead of time? Is this done with instanceof checks against CSSColor? Does CSSColorValue.parse() produce a CSSColor instance if the incoming strings match the custom color space syntax?

@annevk
Copy link
Member

annevk commented May 12, 2021

As far as I can tell from w3c/css-houdini-drafts#1014 there's no agreement on this API on the CSS side.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label May 12, 2021
@mysteryDate
Copy link
Contributor Author

As far as I can tell from w3c/css-houdini-drafts#1014 there's no agreement on this API on the CSS side.

Yeah, I see all that now. From what I gather it seems like consensus is leaning more towards a generic Color class than using CSSColorValue in this context. From the perspective of canvas, we don't have a strong preference. We obviously want to align with CSS as much as possible, but really we're just looking for more expressive color input.
The issue for is that the spec for this Color class looks like it's a long way off. Perhaps I could pitch in there?

@annevk
Copy link
Member

annevk commented May 12, 2021

Yeah, you can comment there. 😊 (It seems to me that there is disagreement about CSSColorValue too. I think for canvas accepting both (if get both) wouldn't be too bad, though I'm not sure why we need both yet.)

@tabatkins
Copy link
Contributor

There's only a vague idea of what an independent Color class would be, and I don't think we'll end up needing it. In any case, it's still far away from reality.

@mysteryDate
Copy link
Contributor Author

@tabatkins Given that, is there a good reason to not support CSSColorValue as an input now? It seems like the reluctance is mostly along the lines of "we don't want CSSOM to become something it's not designed to do." But given as it works as a canvas input without any redesign, Color is a long way off, it's significantly better than strings, what's the downside?

@tabatkins
Copy link
Contributor

As an input, no, there's no reason not to. There's some risk if we expose CSSColorValue as an output, but that's not what's being asked here.

Even in the ideal future where we had a bells-and-whistles generic Color class hierarchy, it would still be a good idea to accept the Typed OM classes as well, since it's easy to obtain those from the CSS APIs.

@svgeesus
Copy link

I just linked to the toRGB

The name may have confused you - that is not a method for all RGB colorspaces. It is only for sRGB, and only if you want a legacy sRGB object.

@svgeesus
Copy link

There's only a vague idea of what an independent Color class would be, and I don't think we'll end up needing it. In any case, it's still far away from reality.

Compared to the vague and handwavy conversions which are woefully underspecified in Typed OM, the ideas for a Color class were presented to CSS WG in July 2020 and have been incubated and refined since then. The algorithms are pretty solid at this point.

Since the ideas are backed by running code, calling it "far from reality" is a stretch.

The API design is also coming together, you should expect to see a proposal soon.

Please, stop with "bells and whistles" scaremongering. You were not able to state what any of those "bells" might be, when challenged. Lets keep the discussion on a technical level.

@tabatkins
Copy link
Contributor

Chris, I'm not trying to be hostile, and I'd appreciate you not being so either.

Please, stop with "bells and whistles" scaremongering.

Wasn't meant to be - that was a complimentary "bells and whistles". My point, again, is that on input, it doesn't matter if we have an extremely cool and good JS Color API, we'd still want to accept TypedOM color values as well. We're not worrying about output right now; at the moment, that's still CSS strings.


I'm ignoring the rest of your comment, as it's not suitable to be addressed here; let's leave CSSWG discussion in CSSWG threads.

@svgeesus
Copy link

on input, it doesn't matter if we have an extremely cool and good JS Color API, we'd still want to accept TypedOM color values as well

Agreed.

@mysteryDate
Copy link
Contributor Author

on input, it doesn't matter if we have an extremely cool and good JS Color API, we'd still want to accept TypedOM color values as well

Agreed.

So it sounds like we can move forward with this PR then? Aside from mozilla and webkit implementation bugs, is there anything left to clear up?

@annevk
Copy link
Member

annevk commented May 17, 2021

Well, I have two remaining concerns:

  1. How much agreement is there on CSSColorValue?
  2. If CSSColorValue is meant to stringify (as stated in the document), do we really need this? (See Array color input for Canvas2D #5616 (comment).)

@domenic
Copy link
Member

domenic commented Aug 17, 2021

Per https://groups.google.com/a/chromium.org/g/blink-dev/c/bEgFQrTsDso/m/6cNS6GBhAQAJ it looks like this is being abandoned in favor of the stringifying approach. I'll close this then. Let me know if that's wrong and we can reopen and continue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements do not merge yet Pull request must not be merged per rationale in comment topic: canvas
Development

Successfully merging this pull request may close these issues.

7 participants