-
Notifications
You must be signed in to change notification settings - Fork 692
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
[css-pseudo] Allow custom Properties in ::selection pseudos (revisited) #11317
Comments
I should note that implementing the proposed behavior is likely straight-forward and efficient (it's a 2 line change in Chromium). Implementing dual inheritance paths, through both originating and highlight trees, is likely very bad for performance, |
I am a bit concerned about making |
I'm totally fine with making it apply to all highlight pseudos. So the proposed resolution would be:
|
Indeed. I believe all issues with |
Final call for objections to this proposed resolution. I'll get a spec PR up within the next week where folks will also be able to comment. |
It was resolved in w3c#11317, CSS Highlight Pseudos should allow custom property definitions within the rule itself, but take any missing custom properties from the originating element. This PR updates the spec to re-list custom properties as supported on highlights. Futhermore, it fixes the description of custom property inheritance that was wrong even for the prior version of the spec.
Having a brief look at the PR, what is the model we're going for here? Custom properties apply to the pseudo (thus affecting what the computed value is), yet they "do not inherit"? Does this mean that a pseudo-element element style does not inherit computed values from its parent style, but instead some other kind of value? This would be very annoying indeed, since nothing (else) would need to hold on to such alternate-reality-computed-values, as far as I can tell. We'd need to store/manage those values alongside the actual computed values, just for this. Why can we not just let them inherit, again? Sidenote: I do think it's intended that the label I also don't think this is a great candidate for an async resolution---at least not in its current state. |
The correct proposed language is "All custom properties inherit from the originating element" combined with re-allowing custom properties on the highlight itself. Much of the confusion here results from my relative inexperience writing spec PRs. The proposal is the former resolution, Issue #9909, plus re-allowing custom properties on the highlight itself. The former resolution both banned custom properties on the highlight itself and said inherit from the originating element. The current spec draft is in fact wrong in that it says both "custom properties are inherited from the originating element" and that they inherit through the highlight cascade. I just updated the PR to fix this. #11528 In practice this means custom properties use the long-standing back-compatible browser behavior, while other properties move to the new model. This is done because so much existing selection styling depends on custom properties, and without this custom-property behavior we can never launch the improved highlight inheritance model. We've tried to launch 3 times in Chromium and this issue has tripped us up every time. The current proposal addresses all of the known breaking issues around custom properties (by basically reverting to the old behavior). For those following along:
Hence this issue and proposed language, which addresses all three failure modes. From an implementation perspective, the highlight gets a shared copy of all the originating element's variables and then adds in any that the highlight defines. So basically the same as regular custom property inheritance. If for consistency we really want to have custom properties inherit through the highlight cascade we would need to split selection out specifically, which is argued against above, or support both methods until sites can migrate (i.e. for years). Supporting both requires two sets of custom properties in pseudo highlights and some method for resolving conflicts. That really is something I do not want to do. |
If we do want to support the back-compat behavior and the new highlight inheritance model at the same time, I think we would need a zig-zag or zipper inheritance model. Say we have
then
This sort-of supports both authors trying to use the new model and authors trying to use the old model for selection property inheritance. In particular, it will be back-compatible with custom properties inheriting from the originating element, because that's the first place to look for a property (second if you count the pseudo itself). And anything that works with previous browsers must have any properties used for selection inheriting up through the element tree. It is possible to support both old and new behavior at the same time in one page if you (a) use property names specific to the highlights e.g. I believe we can
I am leaning toward option 2 because it supports back-compat while giving a viable transition to the new highlight inheritance model. I'm totally fine with option 1. |
Right, OK. Stated that way, it's not too bad. Got confused earlier.
SGTM
Too complicated IMO. The resolution in #9909, "On highlight pseudos, the var() function takes from the originating element.", makes it sound like this is a special behavior for the var() lookup rather than actually affecting the computed values of the highlight style (the difference is visible in e.g. CSSOM). The IRC log does not show a discussion leading up to this conclusion; it kind of looks like we actually intended to affect the computed value? In order to not make this more "special" than it needs to be, I do think we should affect the computed value rather than special-casing var() lookup. Especially if custom properties will actually be allowed on highlight pseudos again. Either way, it would be good to clarify this point. |
The CSS Working Group just discussed
The full IRC log of that discussion<kbabbitt> schenney: on our 3rd attempt for highlight inheritance in chrome<kbabbitt> ... TabAtkins and lea pointed out that we want custom props on highlights <kbabbitt> ... previous resoution was no custom props on highlights, inherit from originating element <kbabbitt> ... think it's a no brainer we should allow custom props on pseudo highlights <kbabbitt> ... then there's a question of how we inherit <kbabbitt> ... 2 possibilities <kbabbitt> fantasai: let's do in one go <kbabbitt> schenney: the simplest way to implement is to use existing model for inheritance <kbabbitt> ... in highlight pseudoes <kbabbitt> ... inherit through originating element chain <kbabbitt> ... look first on highlight then on originating element <kbabbitt> ... which inherits through remaining element cain <kbabbitt> ... inherit custom properties for highlights through originating element chain <kbabbitt> fantasai: but not regular propertes <kbabbitt> schenney: yes <kbabbitt> ... simple to implement, matches previous behavior <kbabbitt> ...downside is that custom props have diff inheritance model from every other property allowed on highlights <kbabbitt> ... alternative method allows inherit both through highlight chain and originating chain <kbabbitt> ... inherit from highlight chain, and on top of that from originating element <kbabbitt> ... and then any properties on highlight itself <kbabbitt> .. downside is hard to state, pain to debug <kbabbitt> ... not all browser can implement efficiently] <kbabbitt> ... upside is that it allows authors to put custom props on highlight without knowing they're special <kbabbitt> ... whereas inherit from origin requires additional explanation <kbabbitt> ... from that perspective I like the fact we can transition to both methods existing at same time <kbabbitt> ... but ack that it's messy <kbabbitt> ... should we go for simplicity but annoyance to developers vs complexity with both methods <TabAtkins> q+ <astearns> ack fantasai <kbabbitt> fantasai: say I set color: customcolor <kbabbitt> ... and then customcolor is set to blue ... on a highlight pseudo <fantasai> span::selection { color: var(--customcolor); } <fantasai> p::selection { --customcolor: blue; } <fantasai> span { --customcolor: red; } <kbabbitt> fantasai: now the question is what color is the selection/ <kbabbitt> ... from author perspective both red and blue are reasonable <kbabbitt> ... we've set custom-color close to where we're using it <kbabbitt> ... we've also set it differently on selection element <kbabbitt> ... you're proposing a priority between these two, will that make sense to everyone? <kbabbitt> ... even when there's an ancestor in between? <kbabbitt> schenney: yes. so... <kbabbitt> ... people who have been working before the originating element will have priority <kbabbitt> ... people coiming new, prent will have priority <kbabbitt> ... agree each is equally valid <kbabbitt> ... inherit from parent, then originating, then span, so color will be red <kbabbitt> ... reason for this is it supports back compat use case <kbabbitt> ... if there's any ambiguity <kbabbitt> ... you've hit the footgun where it's unclear when we inherit from both <kbabbitt> ... I'd expet people to use one or other <fantasai> p::selection { --customcolor: blue; } <kizu> q+ <kbabbitt> ... if they use both, colors should match up <fantasai> p { --customcolor: red; } <kbabbitt> fantasai: in first example [in irc] you'd get red? <dbaron> (It seems a bit weird if --customcolor inherits differently from color.) <kbabbitt> schenney: yes <kbabbitt> fantasai: second example also red? <kbabbitt> schenney: no blue\ <kbabbitt> [crosstalk] <kbabbitt> fantasai: no dispute that text not inside span is blue <kbabbitt> ... part inside span is red or blue? <kbabbitt> .. because span's value for custom color is read <kbabbitt> schenney: yes <kbabbitt> ... it would be read <kbabbitt> s/read/red/ <kbabbitt> ... originating will override inheritance <kbabbitt> ... from a back compat perspective we want to inherit from originating element chain and be done <kbabbitt> ... these questions don't arise which is good <kbabbitt> ... we'd have to give advice to authors <kbabbitt> ... custom properties in highlight are distinct from other custom props <kbabbitt> ... pseudos need to have same values as elements <kbabbitt> ... andruud is for just inherit through originating element chain <kbabbitt> ...because it avoids complexity <kbabbitt> ... which is why I implemented it that way in Chromium <florian> q+ <kbabbitt> ... purely the feature I'm thinking about <astearns> ack TabAtkins <kbabbitt> TabAtkins: with andruud on just use originating element <florian> q- <kbabbitt> ... zipper concept you're describing is not possible under current def of inheritance <kbabbitt> ... a prop always has an inherited value from a particular spot <kbabbitt> ... we'd have to invent a new concept which is possible <kbabbitt> ... but sticking with current model is favorable if it can be avoided <astearns> ack kizu <kbabbitt> kizu: don't think it will matter in practice <kbabbitt> ...maybe author will expect things to behave the same <kbabbitt> ... the way you inhert through highlight elements makes sense since you don't want color to match backgruond <kbabbitt> ... but I don't think this will matter in practice <kbabbitt> ... what will be simpler should be okay <kbabbitt> ... not a point of confusion because not a common use case <astearns> ack fantasai <kbabbitt> fantasai: it could make sense to allow a custom prop to be set on highlight and then use directly on same highlight pseudo <kbabbitt> ... but this double inheritance is weird and will cause problems <kbabbitt> ... won't match what people expet <kbabbitt> schenney: that's option 1 <kbabbitt> ...only resolution needed there is allow custom props on highlight pseudos <kbabbitt> ... which I'm totally fine wityh <kbabbitt> Proposed: allow custom properties on highlight pseudos <kbabbitt> RESOLVED: allow custom properties on highlight pseudos <fantasai> PROPOSED: Allow custom properties on highlight pseudoes; custom properties continue to inherit from the originating element <fantasai> RESOLVED: Allow custom properties on highlight pseudoes; custom properties continue to inherit from the originating element |
It was resolved in #11317, CSS Highlight Pseudos should allow custom property definitions within the rule itself, but take any missing custom properties from the originating element. This PR updates the spec to re-list custom properties as supported on highlights. Furthermore, it fixes the description of custom property inheritance that was wrong even for the prior version of the spec.
Chromium continues to try to ship the Highlight Inheritance model for selection. The third attempt has failed because CSS pre-processing and generation tools create
::selection
rules like this:In #6264 we allowed custom properties on highlight pseudos.
Then in #6641 we also said that all custom properties should be copied from the root for the
root::selection
, and then start adding those in the::selection
inheritance path.This proved insufficient when launched in Chromium because many sites rely on
::selection
inheriting custom properties from their originating elements. So we discussed disallowing custom properties in highlights and inheriting from the originating elements, and resolved to do that in #9909.@tabatkins commented in #6641 that the example above must be allowed for their support. @LeaVerou also raised the same concern in one of the issues above. I did not pay enough attention to that. So the change from #9909 broke many many major sites.
To make it possible to ship highlight inheritance for
::selection
, and bring::selection
implementations into line with other highlight pseudos, I believe we need this ...::selection
pseudo elements inherit custom properties from theiroriginating-element
and then apply custom properties from the::selection
rule itself. Custom properties from::selection
rules apply only within that rule, and are not inherited by descendants.My current thinking is that we limit this behavior to
::selection
and not other highlight pseudos, as a back-compatibility requirement. It's not a strongly held position.Proposing async resolution but feel free to add to the agenda. 4 week time limit given holidays.
@delan @fantasai @frivoal
The text was updated successfully, but these errors were encountered: