-
Notifications
You must be signed in to change notification settings - Fork 143
[css-typed-om] Do we need separate classes per color space? #1034
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
Comments
The unit types don't need separate classes because they're all the exact same thing - a single number paired with a single short string for the unit. There's nothing about their functionality that requires different behavior between them, either - all you want for them is math, and they all act identically there. Colors are pretty strongly different, imo. Each color function has a different shape; they're generally all three/four-argument, but the arguments represent different things, and have different types. I don't think treating them identically as a list of args helps authors - in This feels similar to trying to unify the transform functions into a single class with a list of args, which would similarly be bad for authors with dubious benefit, if any, to specs and impls.
I think this is a code smell that suggests the merging wouldn't be a good thing. ^_^
It does, but is this a bad thing? (I'm also not quite sure what you mean by "on every CSSColorValue" - each new color class would add one method to the superclass; instances would see it by inheritance, as usual. Is there some multiplication of methods I'm missing?)
I don't understand this bit - what about the design makes it harder? Custom color spaces is just through |
In terms of Typed OM, they are all
That's a strawman, I wasn't suggesting Also note that this design does not allow for named getters for custom color spaces even if they are named in the In Color.js we actually support things like
I could argue the same for
Several reasons: a) It's currently very difficult to find out the color space of a color: let space = color instanceof CSSColor? color.colorspace : Object.prototype.toString.call(color).match(/\[object CSS(\w+)\]/?.[0]; and that's assuming these subclasses are reflected in these objects' You could add a property to reflect the color space, but then you'd have multiple sources of truth. b) It makes it very clunky to make manipulations of colors without changing a color's color space: // Find color space
let space = color instanceof CSSColor? color.colorspace : Object.prototype.toString.call(color).match(/\[object CSS(\w+)\]/?.[0];
color = color.toLCH();
color.l *= 1.2; // actual manipulation
// Back to original color space
if (("to" + space) in color) {
color = color["to" + space]();
}
else {
color = color.toColor(space);
} This should really be one line, and is one line in literally every color manipulation library out there. Also, converting to an arbitrary color space is really clunky too, as we can see from the example above. c) It makes it harder for authors to polyfill Typed OM for future color functions since they cannot create objects that have a custom d) It privileges the color functions over |
Yeah, I meant CSS types. (Sorry, the terminology gets confusing since there's not a 1:1 relationship there.)
Its not a strawman, it's literally what you suggested, according to your sample IDL. ^_^ If you meant to imply that there'd also be some sort of maplike interface to interact with the channels by name, it didn't come thru in your message, sorry.
I'm not sure I understand the concern here. What sort of code is being written that wants to interact with arbitrary color objects and manipulate their channels without any knowledge of what the color space is? I don't think there's any math you can meaningfully perform using an arbitrary list of channels, which might even be a mix of percentage and angle values. Could you elaborate?
Yeah, a maplike interface giving a way to address the coords of a I could see us potentially moving that method to the superclass, I suppose. I think it'd need a little more justification.
Ah, that's reasonable to fix. I already do this for CSSNumericValue - the operation subclasses are separate subclasses, but also expose their corresponding function name in a property, to make it easier to respond to. I can see moving ".colorspace" to the superclass as a readonly property, and making it writable in I just filed #1036 to remind me to take care of this. (Maybe we should do this for the transform subclasses too...)
I don't think it does - the color() function is inherently more arbitrary and less well-defined in its author-exposed interface, because it has to be generic over all possible color space definitions. That said, you're right that CSSColor is somewhat anti-privileged - it should really be using indexed properties, rather than a separate
In general, the Typed OM isn't currently being designed to be author-extensible. You can't add new units, etc. either. Nobody seemed to like my efforts to design an author-extensible set of color classes back in Color 4, either. :( That said, there's definitely room to improve here. I think color functions definitely are a good excuse to be author-extensible. I wonder if we can get away with something quite simple, actually - I filed #1038 to explore this topic.
I explored this in my earlier attempt in Color 4, and thought that implicitly invoking a bunch of colorspace conversions wouldn't be a great idea. And since this is a CSS API, which color function you "really" are is important to preserve.
Ooh, that's fair. If I fix #1036 as I want, then we can change the conversion method to a generic If we do that, then your code collapses down to: let space = color.colorspace;
color = color.to("lch");
color.l.value *= 1.2;
color = color.to(space); which seems straightforward and simple while maintaining the important invariants. Is this sort of "temporarily switch to another colorspace for modification, then switch back" a common thing in your experience? If so, we can make it easier - say: color.modifyIn("lch", ({l})=>{return {l:l*1.2}}); That is, specify what space you're going to do the modifications in, then give a callback function that takes the color in that destination space, and returns an object with the properties you wanted to modify. (Unspecified props are left alone.) It then converts back to its original color space. I'd want at least a little evidence that this is something common enough to be worth the additional API surface area, tho. |
Obviously it can be done as you do above, where you create a second object, fiddle with it, then put the results back into the first object. It is nicer for the creation and deletion to happen out of sight though. Places where these implicit colorspace conversions happen (not necessarily mutating the original):
In all cases The calculation should not need to special case if c1 or c2 happen to have been specified be in a legacy sRGB format (HSL, hex, etc) |
Thanks! If you're not mutating the original colors, I think all of these cases would involve calling a conversion function to get a fresh object, right? So that feels similar to what we already have, especially if we switch to a generic I suppose if you were just reading the color in another colorspace, and doing so in a sufficiently simple way, then a more direct "give me this channel in this colorspace" API (like Color.js's I don't think that in any of these cases you're switching back to the original color space, right? Well, in all of your examples there are two colors, and as you say they can be in different color spaces anyway, so I'm not even sure what "back" would mean there. |
Given that the entire set of numeric types is represented by a single
CSSUnitValue
class, I was surprised to see the class hierarchy ofCSSColorValue
. Is this complexity and rigidity needed? Note that this requires adding new classes and new methods on everyCSSColorValue
for every single color space supported (and it makes it harder to support custom color spaces).Why not just
Though I'm not sure how to represent the difference between e.g.
color(srgb...)
andrgb(...)
. Perhaps an additionaltype
attribute.The text was updated successfully, but these errors were encountered: