Quirks and standardization in Colors
Closed this issue · 24 comments
What is the problem or limitation you are having?
This is a mix of improvement and arguably some bugfixes.
First, there are a number of odd behaviors in the color() string-parsing function.
- Trailing commas throw an error:
color("rgb(1, 2, 3,)") # doesn't work
- It doesn't matter what the last character (which should be a
)) is:color("rgb(1, 2, 3q") # works
- The docstring says HSL takes
"hsl(0, 0%, 0%)". That's true, but it interprets S and L as percentages (divided by a hundred) even when no%is present. This seems odd considering that callinghsl(h, s, l)directly expects them to be floats in the range [0.0 – 1.0].
Describe the solution you'd like
The first two are simply parsing glitches, but my preferred solution to the third case would be to make the actual constructors more versatile in what they accept. Why not convert percentages (and for that matter hex values) from strings, so that rgba("FF", "FF", "FF", "100%") (or for that matter rgba("FF", "FF", "FF", "FF") too) becomes equivalent to rgba(255, 255, 255, 1.0) ?
On the opposite end of things, the color classes aren't careful about what data type they store as attributes. For instance, regardless of what exactly you supply to rgba(), as long as it can be coerced, you ideally should wind up with three ints and a float. However, as it stands, rgba(255.0, 255.0, 255.0, 1) gets stored exactly as supplied.
Describe alternatives you've considered
If one were writing this from a blank page, I'm not even positive how much benefit the string-parsing really brings, especially since we already import all the color constructors into the toga.__ namespace. But I suppose having it around does make things more parallel with how uses can supply values to other style properties either as the named constant or a string literal.
It might make sense for the rgb and rgba constructors to be able to take a single string of hex digits as well (e.g. rgb("FF00FF")), though I'm not sure how much additional usefulness that would provide.
Additional context
I have a half-finished conversion of color to use structural pattern-matching; it may or may not make sense to worry much about tackling any of this till 3.9 is gone in a few months.
The first two are simply parsing glitches, but my preferred solution to the third case would be to make the actual constructors more versatile in what they accept. Why not convert percentages (and for that matter hex values) from strings, so that
rgba("FF", "FF", "FF", "100%")(or for that matterrgba("FF", "FF", "FF", "FF")too) becomes equivalent torgba(255, 255, 255, 1.0)?
Although that introduces a different ambiguity. Does color("rgb(10, 10, 10)") interpret those as base 10 or base 16... : /
I think this syntax was based on CSS, so we should do whatever CSS does.
My two cents: rgb("FF", "FF", "00") simply does nothing in CSS, and the correct way is #FFFF00 (also works in lowercase). rgb() accepts integer values only from my testing in CSS.
So this is what I get for not doing research... I've specified plenty of colors in CSS with the various #FFF et al syntax, but had it in my head the hsl() and rgb() versions were just mimicking our own constructors.
There are still some bugs, but I'll look over the CSS standard before making any more recommendations for or against the intentional parts.
Well, not research exactly, just testing in a browser, and my third cent is that don't feel bad about this, sometimes it's hard to remember all the guidelines (such as imitating CSS etc)...
From reading the docs and playing around in Firefox, I've learned some things.
rgb()andrgba()are supposed to be identical (and accept optional alpha); same forhsl()andhsla(). The shorter forms are preferred, but the others are maintained as aliases.- The
red, green, blue[, alpha]syntax is similarly supported for backwards compatibility, but the preferred modern syntax isred green blue[ / alpha]. And the equivalent for HSL. - Alpha values are treated as a percentage if they have a
%; otherwise1is fully opaque. i.e.50% == .5 - On the other hand, saturation and lightness are always interpreted as a percentage out of 100, whether a
%is present or not. - Hue can optionally include
degafter the number. - Hues below
0and above360are accepted, and wrap around. i.e.100 == 460 == -260
Obviously the string-parsing function should match CSS as closely as possible; how exactly the individual classes and their constructors can best align with that, whether and how much they'd need to change, and if so how best to maintain backwards compatibility, may require some thought.
I'm not sure if @anaelisaramos still had any plans for #3635. If not, I wouldn't mind giving this yak a trim.
One other note: in keeping with the general web browser tendency to try and make things work, Firefox, at least, seems perfectly happy to take values out of the allowable range for R, G, B, S, L, and A, simply clamping them to the minimum or maximum. (Hue, as I mentioned above, is a little different, since rather than clamping, it wraps around.)
It seems to me that it would make sense to follow suit, at least for the string-parsing. Should the explicit constructors too? Or should they maintain their current behavior of throwing an error when an input is out of range?
To clarify, when I said "do whatever CSS does", I was referring to the names of the functions and the interpretation of their arguments, but not necessarily the surface syntax. I don't think we want to support non-Pythonic syntax like separating arguments with spaces, passing floats as strings with percentage signs, or allowing a "deg" suffix.
To clarify, when I said "do whatever CSS does", I was referring to the names of the functions and the interpretation of their arguments, but not necessarily the surface syntax. I don't think we want to support non-Pythonic syntax like separating arguments with spaces, passing floats as strings with percentage signs, or allowing a "deg" suffix.
Even in the string parser? I'm not talking about passing things to the constructors without commas, and despite my initial suggestion, I do agree those shouldn't try to parse string arguments (except perhaps as a side effect of int(arg) validation). We're already accepting % suffixes in the parser, I don't see how that's different than deg.
OK, I wasn't aware of that parser, and the range of formats it accepts (#3568). I guess the rgb and hsl formats wrapped inside strings were intended to help with using Travertino to actually implement CSS. But unlike the font shorthand syntax, they aren't of any use in a Toga app, which should always use the rgb and hsl functions directly.
So I don't think it's worth the effort of extending them to match the current version of CSS. In fact, since they were never documented, I'd be inclined to deprecate them immediately and save the work of maintaining them. @freakboy3742: what do you think?
But unlike the font shorthand syntax, they aren't of any use in a Toga app, which should always use the
rgbandhslfunctions directly.
Oh, I didn't realize this wasn't intended usage. Guess that's one more reason documenting it is important! Does make sense, though.
I'd argue that users might still find it convenient for us to support the #rrggbb string syntax as a shorthand, though.
Yes, definitely, I've got no problem with the #rrggbb syntax.
Okay, so entirely ditch parsing of "rgb(...)" / "hsl(...)" strings.
Outside of string-parsing, in order to make things more consistent with how CSS constructs colors, we could:
- Rename the
rgbaclass torgb, make the alpha argument optional, and assignrgbaas a direct alias. Do the same forhslandhsla. - When given values outside their acceptable range, clip them to the max/min (or in case of hue, modulo by 360) rather than throwing an error.
- It would be more consistent with CSS to accept (and store?) saturation and brightness as integers from 0 to 100 rather than floats from 0 to 1. But that feels out of place to me in Python, and there'd be no way to handle backwards compatibility without some ambiguity.
So I don't think it's worth the effort of extending them to match the current version of CSS. In fact, since they were never documented, I'd be inclined to deprecate them immediately and save the work of maintaining them. @freakboy3742: what do you think?
Yeah - I think I agree. In the fullness of time (assuming Colosseum ever gains momentum), we'll need some sort of "fully compliant" parser; but in the interim, it seems better to lean on rgb()/hsl() as functions, rather than string-parsed representations.
Outside of string-parsing, in order to make things more consistent with how CSS constructs colors, we could:
- Rename the
rgbaclass torgb, make the alpha argument optional, and assignrgbaas a direct alias. Do the same forhslandhsla.
That makes sense to me. I wasn't aware rgba was no more than an alias for rgb.
- When given values outside their acceptable range, clip them to the max/min (or in case of hue, modulo by 360) rather than throwing an error.
If that's what CSS does, then yes - we should do the same.
- It would be more consistent with CSS to accept (and store?) saturation and brightness as integers from 0 to 100 rather than floats from 0 to 1. But that feels out of place to me in Python, and there'd be no way to handle backwards compatibility without some ambiguity.
What are you basing the "integer would be more consistent" statement upon? My read is that the S and L channels in the hsl() spec are defined as <percentage>, which is a <number> followed by %; and <number> can contain a decimal (as opposed to <integer>).
What are you basing the "integer would be more consistent" statement upon? My read is that the S and L channels in the
hsl()spec are defined as<percentage>, which is a<number>followed by %; and<number>can contain a decimal (as opposed to<integer>).
Okay, forget the integer part — but a number from 0–100 rather than 0–1. In CSS, for alpha, if you specify a number with a percent sign, it's treated as a percentage (0–100), while if you leave it off, it's interpreted as (and clipped to) 0–1. So 50% is half opaque, but 50 is fully opaque (because it's >= 1).
However, saturation and lightness, at least in my testing on Firefox, seem to always be interpreted as percentages, even with no percent sign. So hsl(..., 1, 1) looks to CSS like nearly black (saturation and lightness at 1%), while Travertino interprets it as fully white (saturation and lightness at 100%).
Here's what hsl(120, 1, 1) looks like in Toga Cocoa:
And here's what it looks like when run on web (since the HSL value is output to CSS as-is):
One other issue: currently, validation is performed on the arguments when a new RGB or HSL color is created... but there's nothing to prevent them being subsequently set to impossible values. It would probably be a good idea to either:
- Make the attributes read-only properties
- Take the validation out of
__init__and put it in simple self-validating descriptors
I'm not sure how useful editable colors are, but I'm also not sure there's a good reason they need to be read-only. (Although if they were, then the conversion properties — rgb.hsl and hsl.rgb — could be cached.)
Okay, forget the integer part — but a number from 0–100 rather than 0–1. In CSS, for alpha, if you specify a number with a percent sign, it's treated as a percentage (0–100), while if you leave it off, it's interpreted as (and clipped to) 0–1. So
50%is half opaque, but50is fully opaque (because it's >= 1).
It looks to me like that might be a quirk of Firefox, rather than something standardized. Safari and Chrome both reject and ignore hsl(120, 1, 1) as an invalid CSS color definition. You have to specify hsl(120, 1%, 1%) for it to be parsed (and, for the record, hsl(120, 0.01, 0.01) is also rejected).
I'm not sure how useful editable colors are, but I'm also not sure there's a good reason they need to be read-only. (Although if they were, then the conversion properties —
rgb.hslandhsl.rgb— could be cached.)
The only way I can think that it might be useful is in a true MVC-style app, where you define a "color" property, connect one channel of the color to a slider, and the slider as the background of a piece of text. Move the slider, the "data" changes, and so does the color - without needing to set up an explicit "on change modify color" handler.
However, to make that work, you'd need to have the cooperation of everything using the color - so for the short term at least, I'd be happy to make the channels on a color read only.
It looks to me like that might be a quirk of Firefox, rather than something standardized. Safari and Chrome both reject and ignore
hsl(120, 1, 1)as an invalid CSS color definition. You have to specifyhsl(120, 1%, 1%)for it to be parsed (and, for the record,hsl(120, 0.01, 0.01)is also rejected).
Aha, that's what I get for ever guessing something would be consistent across browsers.
I think this means two things:
- We should keep storing saturation and lightness in the [0–1] range.
- We need to divorce the colors' CSS representation from their Python code. I recommend we keep
__repr__as copy-pasteable Python, and make__str__valid CSS.
[...] so for the short term at least, I'd be happy to make the channels on a color read only.
👍
With that all more or less settled, and having not heard from Ana, I'll get cooking on this.
I think this means two things:
- We should keep storing saturation and lightness in the [0–1] range.
- We need to divorce the colors' CSS representation from their Python code. I recommend we keep
__repr__as copy-pasteable Python, and make__str__valid CSS.
+1 - these both seem like good outcomes.
(also - it looks like GitHub agrees about hsl() interpretations. Valid hsl gets a color preview:
invalid hsl does not:
)
One more thing I just noticed is that R, G, B, and H can all be numbers too, not just integers. Looks like it'll be floats all the way down.
(R, G, B and be can even be percentages, but there's no way to make that unambiguous at the Python level.)
One more thing I just noticed is that R, G, B, and H can all be numbers too, not just integers. Looks like it'll be floats all the way down.
(R, G, B and be can even be percentages, but there's no way to make that unambiguous at the Python level.)
RGB as numbers may be legal CSS, but at least some color implementations (Android, Winforms) are going to have a problem with that, as they expect integers in the 0-255 range. Using floats will also complicate rendering colors as #RRGGBB sequences - I think I'd be comfortable with the loss of fidelity of continuing to store colors as 0-255 integers.
What about hue? The current tests show some ambiguity — for instance, it expects the __repr__ of hsl(10, 0.2, 0.3, 0.5) to read 10, not 10.0; but the HSL / RGB conversion tests use floating-point values for it (and for exactly one of them, it makes a big enough difference to fail the comparison as written).
Given that hue is an angle (expressed in degrees if there's no explicit unit), I think it makes sense to be a float. There's no simple # expression analog for HSL colors, so the fact that the repr comes out as hsl(10.0, 0.2, 0.3, 0.5) seems fine to me.
As for the test failure - is that failure caused by the RGB integer simplification? If that's the case, maybe retaining float as an internal RGB representation, but externally rounding to int for external representations that require it might be the way to go...
Summarized from a discussion with @freakboy3742 on Discord: Storing red/green/blue and/or hue as floats is theoretically more precise, and they could still be displayed as integers for __repr__ and/or output to CSS. However:
- The extra precision is unlikely to be beneficial in practice, at least not unless someone's performing a whole bunch of conversions in a row.
- Floating-point math is slower.
- Equivalence testing is also slower (and fuzzier) when determining if a property has changed or not.
- Storing as floats but displaying as integers could cause some odd corner cases where two colors that display the same as integers could convert to colors in the opposite format that display differently.
So R/G/B/H will be stored as integers. And some quick testing indicates that round isn't meaningfully slower than int, so we'll use that for converting.