[css-pseudo] defaulting ‘color’ in :root highlights

[delan]

(was: compat risk for ::selection rules defaulting one highlight color from originating element)

Highlight inheritance marks a pretty big change for the already-widely-shipped ::selection. The current consensus as of #2474 is that the impls should change, and the value of making highlights behave intuitively outweighs the compat risk.

I don’t mean to relitigate this, but I recently found a concrete (albeit in WPT) example of breakage that I didn’t realise was possible. I don’t think this breakage should block highlight inheritance, but I just want to make sure everyone’s aware of it.

Before web-platform-tests/wpt#30813, css/css-pseudo/active-selection-012.html looked roughly like this (demo):

<style>
span { background-color: red; color: fuchsia; }
span::selection { background-color: green; }
</style>
<span>fuchsia on green</span>

The test expects ::selection to use a foreground color of fuchsia, default inheriting from the originating element, because the presence of background-color:green suppresses UA default highlight colors via paired cascade. Highlight inheritance broke the test: the ::selection foreground becomes initial, because applicable properties are never inherited from the originating element.

This means that highlight inheritance will break any content where ::selection relies on one highlight color (background-color or color) set explicitly, and the other implicitly inherited from the originating element. I previously believed we would only break content that somehow relies on descendants being reset to initial highlight styles (and doesn’t use universal rules).

[emilio]

I think ::selection { background-color: <something>; color: currentColor; } (that is, the test-case above) should not change the color of the element that is selected.

It's a common way to implement selections that work well on both light and dark mode for example, by providing a semi-transparent background color...

[delan]

I think ::selection { background-color: <something>; color: currentColor; } (that is, the test-case above) should not change the color of the element that is selected.

Under highlight inheritance, that would still be true — “currentColor on a highlight pseudo-element’s color property represents the color of the next highlight pseudo-element layer below” (#highlight-text). The test case in this issue is different, because there’s no color property in the ::selection rule.

[emilio]

Wait so color: currentcolor and color: inherit do two different things on highlights? That seems broken.

[delan]

Yeah, for originating elements and the other pseudos (at least tree-abiding ones), color:currentColor just means color:inherit, but for highlights, color:currentColor jumps across to the same place in the next (active?) highlight tree (or originating tree).

The way I’ve made sense of it is that in both cases, color:currentColor effectively means “don’t change the color”. One is “don’t change the color in this child”, the other is “don’t change the color in this highlight”.

[hober]

@megangardner

[delan]

(summary for meeting)

Highlights used to only inherit from the originating element, but now they inherit from the parent’s highlight styles, creating separate “inheritance trees” for each highlight.

We previously resolved that the compat risk was acceptable (#2474), because the old behaviour wasn’t really useful or intuitive anyway, but there’s a new breakage that I don’t think we were aware of, and I just want to check if we’re still happy with the risk.

[css-meeting-bot]

The CSS Working Group just discussed Compat risk for ::selection defaulting one color from originating element.

The full IRC log of that discussion

<fantasai_> Topic: Compat risk for ::selection defaulting one color from originating element
<fantasai_> github: https://github.com//issues/6774
<delan> https://github.com//issues/2474
<fantasai_> delan: The highlight inheritance and cascade in the spec is a pretty big change compared to processing model in implementations right now
<fantasai_> delan: previously our position was that we consider the compat risk of this big change to be acceptable
<fantasai_> delan: because the old model that exists in old browsers is kindof useless and broken
<fantasai_> delan: the styles don't inherit so you have the set them everywhere, unless writing a really awkward selector
<fantasai_> delan: while implementing in Blink, we found a WPT that broke as a result of turning on highlight inheritance
<fantasai_> delan: unclear if aware of that breakage, are we still happy with the compat risk?
<delan> span { background-color: red; color: fuchsia; }
<fantasai_> delan: what the test did is essentially it has the originating element with colors of fuchsia on red
<delan> span::selection { background-color: green; }
<fantasai_> delan: there's a selection rule that just sets a gree background color
<fantasai_> delan: paired cascade from earlier means that because one of the two highlight properties
<fantasai_> delan: there is no UA default for 'color', so we use whatever color we get normally
<fantasai_> delan: existing model in browsers has ::selection inherit styles from originating element
<fantasai_> delan: so test expects color to be fuchsia
<fantasai_> delan: but now the test fails
<fantasai_> delan: because under new rules, we don't inherit color, it's black
<Rossen_> q?
<fantasai_> fantasai: I think the test is correct, actually, and paired cascading is also correct
<fantasai_> fantasai: inheriting all the way up, should get currentColor, not black
<fantasai_> fantasai: though I'm not sure the spec is clear about that
<fantasai_> emilio: Goes back to currentColor discussion
<fantasai_> emilio: all implementations resolve 'color: currentColor' to an actual color
<fantasai_> emilio: so I think we need to solve the problem of 'initial' and 'currentColor' being different
<fantasai_> emilio: and we need a resolved color
<fantasai_> emilio: I think the computed value is wrong, and it uses a computed color
<fantasai_> emilio: nobody implements that, it has terrible perf implications
<fantasai_> florian: Delan, can you point out to which bit of the spec made you think that you would go to black rather than originating element?
<fantasai_> florian: wondering if it's ambiguous or wrong
<fantasai_> delan: ... value found by inheritance ... do we say what happens when we get to the top?
<fantasai_> delan: I don't think it says what to do if you get all the way to the root
<fantasai_> delan: was it wrong to assume it would work the same way as in the the normal tree?
<fantasai_> emilio: I think the spec is right
<fantasai_> emilio: but if everything worked by spec, color is initial which is currentColor and it would work
<fantasai_> emilio: but that's not how it works right now
<fantasai_> delan: so we'd change spec so that for highlights, we get to root, and have a value we get essentially what currentColor does now
<fantasai_> emilio: per spec should get currentColor as computed value
<fantasai_> emilio: so I think the spec for the color property is wrong
<fantasai_> delan: if we did that for all properties ...
<fantasai_> emilio: I think the only issue is with the color property
<fantasai_> emilio: all current impls store as computed color
<fantasai_> emilio: otherwise need to go figure it out every time resolving a color for a given style
<fantasai_> emilio: I think the spec doesn't reflect that
<fantasai_> emilio: didn't make a difference until now, that we want currentColor to behave specially
<fantasai_> Rossen: so do we need to move this discussion back to GH?
<fantasai_> delan: I suppose these questions are useful and interesting, but I'm not sure they necessarily change the original question, which was do we still want to move to paired cascade
<fantasai_> s/paired cascade/new cascading and inheritance model/
<fantasai_> emilio: I think we want the testcase to continue to pass
<fantasai_> emilio: Depending on how we resolve the issue
<fantasai_> delan: so can take to GH
<fantasai_> emilio: this seems complex enough discussion
<fantasai_> emilio: per spec everything works magically and it's great
<fantasai_> emilio: but I don't think that's reasonable
<fantasai_> emilio: so we should figure out the solution that works as we want
<fantasai_> delan: ok sounds good

[delan]

@emilio

emilio: I think the only issue is with the color property
emilio: all current impls store as computed color
emilio: otherwise need to go figure it out every time resolving a color for a given style
emilio: I think the spec doesn't reflect that

We’ve started implementing this in Blink, and we think we’ve found a workaround for this problem that doesn’t complicate things for non-highlight styles or slow down style resolution.

CL:3506351 leaves ‘color’ storage and resolved-color-returning functions like GetCurrentColor and VisitedDependentColor untouched, but adds an inherited flag for whether ‘color’ was ‘currentColor’. If the flag is true, the highlight painting code ignores the incorrectly-resolved ‘color’ in ComputedStyle and grabs it from the next layer below (currently originating, but soon this could be a highlight overlay). ‘currentColor’ is stored as itself for ‘background-color’, so for that, we check if the flag is set and the background is ‘currentColor’, and ignore accordingly.

gCS is not implemented yet, but we think it can be done similarly or even simpler, because we’re pretending that the given highlight is active for the whole element and all other highlights are inactive (#6818).

@fantasai

fantasai: I think the test is correct, actually, and paired cascading is also correct
fantasai: inheriting all the way up, should get currentColor, not black
fantasai: though I'm not sure the spec is clear about that

@frivoal

florian: Delan, can you point out to which bit of the spec made you think that you would go to black rather than originating element?
florian: wondering if it's ambiguous or wrong

I believed that ‘color’ would become ‘CanvasText’ (usually black) because of these spec texts (emphasis mine):

• #highlight-cascade: “When any supported property is not given a value by the cascade, its value is determined by inheritance from the corresponding highlight pseudo-element of its originating element’s parent element (regardless of whether that property is an inherited property).”
• css-cascade-4 #defaulting: “Inherited properties draw their defaults from [the corresponding highlight of] their parent element through inheritance [by analogy with the text above]; all other properties take their initial value.”
• css-cascade-4 #inheriting: “For [highlight pseudo-elements of] the root element [by analogy with the text above], which has no parent element, the inherited value is the initial value of the property.”
  • css-cascade-4 #inheriting also says “Pseudo-elements inherit according to the fictional tag sequence described for each pseudo-element.” but highlights have no fictional tag sequence, and I feel like those are non-normative anyway?
• css-color-4 #the-color-property: “Initial: CanvasText”

Have I missed something? If we want the specified value of a :root highlight’s ‘color’ to be ‘currentColor’, which I agree is reasonable, we should say so somehow. A few options come to mind:

1. For highlight pseudos, we redefine the initial value of ‘color’, from ‘CanvasText’ to ‘currentColor’. This would make ‘initial’ and ‘inherit’ and ‘unset’ also mean ‘currentColor’ at the root.
2. For highlight pseudos, we redefine the inherited value of ‘color’ at the root, from the initial value to ‘currentColor’. This would make ‘inherit’ and ‘unset’ also mean ‘currentColor’ at the root, though ‘initial’ would still mean ‘CanvasText’, just like how they work in pre-highlight inheritance impls (demo).
3. For highlight pseudos, we redefine defaulting for ‘color’ at the root, from using inheritance to ‘currentColor’. Under this option, ‘initial’ and ‘inherit’ and ‘unset’ would still mean ‘CanvasText’ at the root.
4. For highlight pseudos, we say that root highlights inherit all properties from the next highlight below. This would make ‘inherit’ and ‘unset’ also mean ‘currentColor’ at the root, though ‘initial’ would still mean ‘CanvasText’.

I don’t see how option 4 could work, because it would need to be the next active highlight below (making resolved styles entirely dependent on active highlights), and it would need to be the leaf node’s styles for that highlight (making the root styles dependent on descendant styles). Other than that, I don’t have any opinions on what’s best… maybe @mrego or @andruud can comment?

[emilio]

@emilio

emilio: I think the only issue is with the color property
emilio: all current impls store as computed color
emilio: otherwise need to go figure it out every time resolving a color for a given style
emilio: I think the spec doesn't reflect that

We’ve started implementing this in Blink, and we think we’ve found a workaround for this problem that doesn’t complicate things for non-highlight styles or slow down style resolution.

CL:3506351 leaves ‘color’ storage and resolved-color-returning functions like GetCurrentColor and VisitedDependentColor untouched, but adds an inherited flag for whether ‘color’ was ‘currentColor’. If the flag is true, the highlight painting code ignores the incorrectly-resolved ‘color’ in ComputedStyle and grabs it from the next layer below (currently originating, but soon this could be a highlight overlay). ‘currentColor’ is stored as itself for ‘background-color’, so for that, we check if the flag is set and the background is ‘currentColor’, and ignore accordingly.

Hmm, is that really enough? I'm not sure it can just be a flag, because currentColor can be interpolated with other colors, can't it? As in, what if I do color: color-mix(currentColor, ...) or other such things?

That is a bit unfortunate in general since it's a whack-a-mole and now all of the decoration code needs to be aware of this flag everywhere, right? So it can't really share much code with the rest of the painting code-paths.

[delan]

I'm not sure it can just be a flag, because currentColor can be interpolated with other colors, can't it? As in, what if I do color: color-mix(currentColor, ...) or other such things?

Blink doesn’t support any advanced color features yet, but I think in that case we would still set the flag, indicating that ‘color’ is dependent on currentColor. Once we know what currentColor will be — for now, that means during paint — we call whichever function we normally call to resolve (and possibly mix) color values.

‘text-shadow’ might prove tricky because it’s a list of shadows, each with a color value, but I think either the current approach (flag for the whole property) or moving the flag into Color (for more granularity) could work. The former is probably faster for us right now, because unlike WebKit, our Color is still just a newtype for rgba32.

That is a bit unfortunate in general since it's a whack-a-mole and now all of the decoration code needs to be aware of this flag everywhere, right? So it can't really share much code with the rest of the painting code-paths.

Not really, in Blink, as most of the paint code lets the caller pass intermediate structs for text colors (TextPaintStyle) and decoration details (TextDecorationInfo, AppliedTextDecoration). The text fragment painter or highlight overlay painter decides what colors to fill those structs with (using helpers like HighlightPaintingStyle), but from there the logic is the same.

[emilio]

Blink doesn’t support any advanced color features yet, but I think in that case we would still set the flag, indicating that ‘color’ is dependent on currentColor. Once we know what currentColor will be — for now, that means during paint — we call whichever function we normally call to resolve (and possibly mix) color values.

But that means that you need to preserve the unmixed color in the computed style, which is what you want to avoid to begin with right? Anyways I'm not opposed to that but seems a bit tricky off-hand.

[mrego]

Blink doesn’t support any advanced color features yet, but I think in that case we would still set the flag, indicating that ‘color’ is dependent on currentColor. Once we know what currentColor will be — for now, that means during paint — we call whichever function we normally call to resolve (and possibly mix) color values.

But that means that you need to preserve the unmixed color in the computed style, which is what you want to avoid to begin with right? Anyways I'm not opposed to that but seems a bit tricky off-hand.

As animations are not allowed on highlight pseudos probably the only thing special thing would be color-mix, that's not supported in Chromium yet so we didn't have to deal with that in the initial implementation.

I agree it's going to be more tricky than just storing a flag, but it's still something possible to implement.
Giving how highlights inheritance works, and that we need to do something special with currentcolor I believe we have to accept this complexity to move forward.

[delan]

Thanks @emilio! I think we should consider resolving with option 2 from my earlier comment:

2. For highlight pseudos, we redefine the inherited value of ‘color’ at the root, from the initial value to ‘currentColor’. This would make ‘inherit’ and ‘unset’ also mean ‘currentColor’ at the root, though ‘initial’ would still mean ‘CanvasText’, just like how they work in pre-highlight inheritance impls (demo).

[css-meeting-bot]

The CSS Working Group just discussed Default color in :root highlight, and agreed to the following:

• RESOLVED: Go with Delan's option 2.

The full IRC log of that discussion

<TabAtkins> Topic: Default color in :root highlight
<TabAtkins> github: https://github.com//issues/6774
<delan> https://github.com//issues/6774#issuecomment-1083055006
<TabAtkins> delan: For highlight pseudos, setting color:currentcolor means the color doesn't change when you highlight with that pseudo, compared to the original color underneath
<TabAtkins> delan: Editors agreed this is what should happen if color hasn't been set anywhere for a highlight
<TabAtkins> delan: I think the way this is achieved isn't actually specified. My best interp of Cascade is that we don't actually do that, and the spec says the default color of a highlight pseudo becomes black
<TabAtkins> delan: Three steps
<TabAtkins> delan: First, when you have an inherited property (all props are inherited for highlights), they do the defaulting by way of "inherited value"
<TabAtkins> delan: Second, inherited value is value from parent, unless you're at root, in which case it's initial value
<TabAtkins> delan: Third, initial value of color property is CanvasText
<TabAtkins> delan: Which is generally black (in light mode)
<Rossen_> q?
<TabAtkins> delan: So this raises the question of how to fix it
<TabAtkins> delan: Which step we add an exception for affects what happens when you use initial/inherit/unset
<TabAtkins> delan: One option is to say that for highlights, the initial value isn't CanvasText, it's currentcolor
<TabAtkins> delan: Here if you set color to initial/inherit/unset, they'll become currentcolor
<TabAtkins> delan: Second option is for highlights, the inherited value isn't the initial value at root, but instead currentcolor
<TabAtkins> delan: So when you set color to inherit/unset you get currentcolor, but initial means canvastext
<TabAtkins> delan: I like this the best
<TabAtkins> delan: Third option is to change defaulting for highlight pseudos and say that for root highlights, you don't inherit, we just set the value.
<TabAtkins> delan: So all the keywords would become canvastext
<TabAtkins> delan: Not sure my understanding is correct, but it's how I see things. What should we do?
<TabAtkins> fantasai: That was a great epxlanation of a complicated issue
<Rossen_> ack fantasai
<TabAtkins> fantasai: I think either first or second makes sense to me
<TabAtkins> fantasai: If no one has a reason to do something different your pref makes sense to me. I suspect your pref is the easiest to implement.
<TabAtkins> delan: I think all three are possible to implement. I preferred 2 over 1 because in option 2 you can say color:initial and get black, and I feel like that intuitively makes sense.
<Rossen_> ack emilio
<TabAtkins> emilio: Doesn't 2 change the - fix the weirdness around currentcolor in highlights?
<TabAtkins> emilio: If we change how it inherits doesn't it fix all the shenanigans about what currentcolor means in highlights?
<TabAtkins> emilio: Or is this orthogonal
<TabAtkins> delan: I don't think it does
<TabAtkins> delan: Are you talking about where we have the exception for currentcolor where it means this special thing for highlights?
<TabAtkins> emilio: yes
<TabAtkins> delan: Then no, this actually relies on that.
<TabAtkins> delan: Unless we don't literally use the word "currentcolor" in our fix and just say that it "keeps the same color"
<TabAtkins> delan: But as worded it relies on that currentcolor behavior
<TabAtkins> emilio: More generally, currentcolor refers to the computed value of the color property, how can you inherit it?
<TabAtkins> emilio: In impls the color property is special bc you don't want to resolve currentcolor by walking all the way to the root
<TabAtkins> emilio: and currentcolor disappears at computed value time, before inheritance
<TabAtkins> emilio: But if this is just an impl detail, eh, this just makes color more special, but given previous things we're past that point
<TabAtkins> Rossen_: So hearing some gravity towards options 1 and 2, particular 2 as delan's fave. Is this something we can resolve on?
<TabAtkins> delan: Restating option 2: For ::highlight pseudos, we redefine the "inherited value" of 'color' at the root, so instead of being the initial value (as normal) it is currentcolor.
<fantasai> currentColor does not disappear at computed value time... that's one of the important things about it
<TabAtkins> Rossen_: objections?
<TabAtkins> RESOLVED: Go with Delan's option 2.
<fantasai> WFM

[delan]

@emilio @fantasai re currentColor disappearing at computed value time, css-color-4 #resolving-other-colors says

The currentcolor keyword computes to itself.

which is why this rect is green, not red (demo)

<svg viewBox="0 0 300 150" xmlns="http://www.w3.org/2000/svg">
    <g><rect x="10" y="10" width="10" height="10" /></g>
    <style>
        g { color: red; fill: currentColor; }
        rect { color: green; fill: inherit; }
    </style>
</svg>

[emilio]

@delan yeah, my point is that that doesn't match how browsers work for the color property in particular.

I was going to point how even Chrome doesn't think the computed value is currentColor for the color property, but its computedStyleMap() is broken:

<span style="color: currentColor; background-color: currentColor"></span>

document.querySelector("span").computedStyleMap().get("color").toString() // rgb(0, 0, 0)
document.querySelector("span").computedStyleMap().get("background-color").toString() // rgb(0, 0, 0)

Per spec, both should be currentColor. In Firefox and pretty sure WebKit, the former would be black.

[emilio]

https://bugs.chromium.org/p/chromium/issues/detail?id=1338607 for the bug above.

[delan]

#6665 makes the edits for this question.