textual icon indicating copy to clipboard operation
textual copied to clipboard

[colors] Add a "auto" color

Open olivierphi opened this issue 3 years ago • 14 comments

When text has color: auto [percentage] it will be displayed with the contrasting colour of its background

As a result we can have widgets that have color: auto; in their CSS, and see their text colour automatically adapted to their background: Screenshot from 2022-06-29 14-33-56

olivierphi avatar Jun 29 '22 13:06 olivierphi

@willmcgugan the implementation is twice more verbose than the "sentinel 'auto' Color" one, and I'm not a big fan of the fact that we now offload a characteristic of a "color" property to another property of the stylesheet. The whole thing is more complicated now imho. But here we go... This new implementation works now, with the tests of the previous one still passing as is :slightly_smiling_face:

olivierphi avatar Jun 30 '22 16:06 olivierphi

More complicated maybe, but I think this a better abstraction. If you consider that an object should have one reason to exist, a Color class should only ever abstract a color (and "auto" is not a color). "auto" to my mind is more of a way of enabling functionality on the Style object.

willmcgugan avatar Jun 30 '22 19:06 willmcgugan

@willmcgugan Feedback (hopefully) addressed! :slightly_smiling_face: 28edacf

olivierphi avatar Jul 01 '22 09:07 olivierphi

I have to say I'm really not a fan of the color_auto - it's so tightly coupled to color and having 2 properties interfering with each other like that is just confusing. If we have to guess what the behaviour is for something that is conceptually so simple, I think we're already going to the wrong route.

I liked the idea of a sentinel value but agree the previous approach broke the "color" abstraction.

IMO it'd be nice to have the color property value have a type Color | Auto (i.e. a union where Auto is some container or sentinel to mark the special case), rather than having a separate flag. The approach as it stands feels like "using a flag to simulate a union type".

darrenburns avatar Jul 01 '22 12:07 darrenburns

So self.styles.color could return a Color or an Auto?

What kind of interface would the Auto support? Can I write self.styles.color.r or would I have to do is isinstance(self.styes.color) beforehand?

willmcgugan avatar Jul 01 '22 13:07 willmcgugan

So self.styles.color could return a Color or an Auto? What kind of interface would the Auto support? Can I write self.styles.color.r or would I have to do is isinstance(self.styes.color) beforehand?

Maybe the problem is that we're trying to fit something that is not a colour (and that doesn't exists in "browser's CSS", so we cannot take inspiration from there) into a property named "color"? :thinking:

Shouldn't we rather follow the "principle of least surprise", and say that if it's definitely not a colour it shouldn't be set via a property that has this name? Maybe it could go under something like a standalone color-auto property, which value could either be:

  • set (I guess it could be a synonym for the value 95%, which is the default value for the get_contrast_text method?)
  • unset (similarly to border: unset for example in browsers' CSS, so one can cancel the property if it's been set somewhere else)
  • an alpha? (in either percentage or fractional units?)

olivierphi avatar Jul 01 '22 13:07 olivierphi

I did not mean to do that!

willmcgugan avatar Jul 01 '22 13:07 willmcgugan

While we're at it I'm putting this here - mostly for myself, for future reference :smile:

List of the "universal" values we can apply to any CSS property in browsers' CSS:

  • initial: to set a property to its initial value.
  • inherit: to make an element's property the same as its parent.
  • unset: to set a property to its inherited value if it inherits or to its initial value if not - combination of the initial and inherit keywords
  • revert: to reset a property to the value established by the user-agent stylesheet (or by user styles, if any exist).
  • revert-layer: to reset a property to the value established in a previous cascade layer.

olivierphi avatar Jul 01 '22 13:07 olivierphi

I agree that styles.color should probably not be anything other than a Color. It would surprise me if that wasn't the case..

For the Python interface, I think we will need color_auto as a boolean, but also color_auto_alpha as a float to capture the alpha color. I think that would avoid the edge case I mention above.

@DrBenton I see where you are going with another CSS property to capture the auto color state, but find the declaration of "color: auto 90%" to be quite elegant. And another rule might confuse, especially if there is a "color" and a "color-auto".

willmcgugan avatar Jul 01 '22 13:07 willmcgugan

For the Python interface, I think we will need color_auto as a boolean, but also color_auto_alpha as a float to capture the alpha color. I think that would avoid the edge case I mention above.

Alright, let's do this! *cracks his knuckles

olivierphi avatar Jul 01 '22 14:07 olivierphi

Just for the record. After a 3 way call I think we've agreed on the following:

  • The Styles.color property setter can accept "auto" with an optional alpha, e.g. "auto 90%"
  • The Styles.color property getter returns the calculated contrasting color.
  • We add a Styles.color_auto boolean (getter only) that exposes if "auto" is in effect.

I think this alleviates any edge cases, and is reasonably easy to wrap ones head around.

Thumbs up if you're in favor...

willmcgugan avatar Jul 01 '22 14:07 willmcgugan

As I was implementing this I realised that there is a slight issue :sweat_smile: :

  • In order to make ColorProperty return a conytrasting colour on the fly when we request its value, like we said, we would need it to compute that value by traversing the Node's ancestors tree.
  • The problem with this is that we already do exactly this in the DOMNode's colors getter, which not only returns this colour but also the base_background, base_color and background ones.

--> so we would do a calculation of the colour that needs to traverse all the Node's ancestors in this colors getter, but if there is a color: auto in the mix each ancestor would also calculate its colour the fly using its own ancestors in the ColorProperty __get__ method- which gives us a loop-over-ancestors wrapped in another loop-over-ancestors :grimacing:

However, I realised that we can solve the original issue (being able to do widget.styles.color_auto = False) pretty simply, by following this "private property / public getter" principle we were mentioning: if the implementation is still the same but _color_auto is now just a private property of the StylesBase class, the issue is actually solved! :heavy_check_mark: :smile:

--> 94ba806

olivierphi avatar Jul 01 '22 15:07 olivierphi

@DrBenton I'm not sure I follow. Let's have a chat about it on Tuesday.

willmcgugan avatar Jul 01 '22 16:07 willmcgugan

@willmcgugan here you go! 4439ca1

olivierphi avatar Jul 05 '22 13:07 olivierphi