The default bug view has changed. See this FAQ.

stylo: Teach servo how to restyle pseudo-implementing NAC

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
2 months ago
2 days ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 10 bugs)

Firefox Tracking Flags

(Not tracked)

Details

See the discussion in bug 1323693. I'll write up a bit more here.
My plan here is as follows:

First, we encode the pseudo-element for all pseudo-element-implementing NAC directly into the DOM element. This is bug 1331045.

Then, we teach Servo how to recognize pseudo-implementing NAC. We can make this check pretty cheap because it can only happen if NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE (in which case we then check the property table). If this happens, we skip the normal style computation, and instead copy the appropriate pseudo style hanging off the parent's ElementData into the primary style of the child. I think we'll probably want to just copy the pseudo map verbatim from the parent to the child, so that we can support nested pseudo-element implementations in nsNumberControlFrame::CreateAnonymousContent. Boris, is there any spec that actually has nested pseudo-elements and says whether the pseudos should inherit from each other in that way, as opposed to all the pseudos inheriting style from the host element? Or do nested PEs only exist for our internal stylesheets?

Finally, we simplify StyleChildrenIterator to get rid of the guesswork surrounding pseudo-element-implementing-NAC, and just traverse everything. We can then remove special handling for these pseudos in ServoRestyleManager::RecreateStyleContexts, and just rely on the normal content tree traversal to find everything. One tricky bit is the generation of ReconstructFrame change hints, which is illegal in NAC subtrees (see bug 1297857). I guess we'll need to forward any ReconstructFrame hints to the root of the NAC subtree?

We'll also want an optimization to avoid style traversal in servo for NAC subtrees once we get ReconstructFrame, since the NAC will get blown away anyway.

We'll still need special handling for ::first-line and ::first-letter, which aren't NAC-backed. Are there any others?
Flags: needinfo?(bzbarsky)
I need to think through this a bit, but my first question is this: are there cases in which we can't figure out where the pseudo-element should be inheriting its style from simply based on its position in the (anonymous) DOM?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> I need to think through this a bit, but my first question is this: are there
> cases in which we can't figure out where the pseudo-element should be
> inheriting its style from simply based on its position in the (anonymous)
> DOM?

The only case that I'm aware of where an nsIAnonymousContentCreator creates trees of depth > 1 is in the number frame stuff [1]. That stuff sets things up so that the styles inherit from the DOM parent (rather than from the root).

However, heycam's patch over in bug 1330843 causes us to inherit from the host element in all cases.

From a complexity standpoint, it's probably marginally better to just inherit from the parent, because then we don't need to walk up the tree and find the host.

From an efficiency standpoint, it's probably marginally better to have all the pseudo-styles inherit from the root, because then we don't need to copy the pseudo styles to the ElementStyles of each pseudo-implementing NAC element.

It's probably not a big deal either way, we just need to pick something and stick with it.

[1] http://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_nsIAnonymousContentCreator%3A%3AContentInfo%3E_2&redirect=false
Looks like this code at least expects to punch through some number of pseudos up to the host: http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/forms/nsTextControlFrame.cpp#355

So maybe we should always inherit from the host, and change the behavior for the NumberControlFrame stuff?

Curious what jwatt thinks about all this, given that he wrote a bunch of the code in question.
Flags: needinfo?(jwatt)
Though I guess that _also_ raises the question of whether we want to go to the root of the NAC, or all the way up to the first non-NAC element. It looks like the text control frame is explicitly trying to do the latter, unless I'm misunderstanding something. Are there situations where we want the former?
Bug 1330843 comment 11 reveals some implementation difficulties with the walk-us-out-of-the-nac-subtree approach. These difficulties are non-issue with the stylo backend, but we probably need to settle on some mutually compatible behavior between the two backends, at least for the interim.

It would be really helpful to understand if there's a reason why the NumberControlFrame stuff does the inheritance like it does, and if there's a reason why the TextControlFrame wants to punch through the NAC subtree.
I haven't paged all this back in yet, but note that what the pseudo-element inherits from is totally observable by styling it with "inherit" values for properties that don't normally inherit, and hence should be defined in the relevant spec that defines the pseudo-element.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> I haven't paged all this back in yet, but note that what the pseudo-element
> inherits from is totally observable by styling it with "inherit" values for
> properties that don't normally inherit, and hence should be defined in the
> relevant spec that defines the pseudo-element.

As far as I can tell, none of the Web-exposed pseudo-elements have have any nesting, nor any of the internal pseudo-elements in Blink [1]. So I think we're kind of on our own here.

The two cases in Gecko when we get nesting are:
(1) When we explicitly set up nesting in the ContentInfo, which only happens in nsNumberControlFrame [2].
(2) When a pseudo-element creates an element that has its own NAC (i.e. the <input> element within nsNumberControlFrame's NAC).

None of the pseudos in (1) are web-exposed, so the difference is only observable internally, and only insofar as any properties from the two container divs might be inherited. A quick scan of [3] and [4] suggests that the only inherited property specified in either div is |writing-mode: horizontal-tb| on the ::-moz-number-spin-box. I'm not sure whether that has any observable effect on the ::-moz-number-spin-{down,up}, but we could certainly just copy it over in pinch.

As for (2), the main interesting case I've seen is bug 1004130, where we added a special case to allow the placeholder pseudo on an <input type="number"> to apply to the NAC within the <input> field. The code added for that bug is pretty specific, but my general sense is that developers expect content-addressable pseudo-elements to apply to things within compound elements, and don't really care much about the structure of the NAC.

So in the general case for a given piece of NAC, I think we want to apply pseudo rules from the originating element (not just from the root of the NAC subtree, given the nested NAC case described above). As for what we _inherit_ from, it probably doesn't matter so much - but the least surprising thing is to inherit from the DOM parent in the NAC subtree, because that avoids totally changing the inheritance behavior when giving a piece a NAC a (possibly-internal) pseudo.

The upshot here is that this _may_ solve the implementation issue in bug 1330843 comment 11, because we can just inherit from the passed-in style context (which should correspond to the DOM parent), assuming ResolvePseudoElementStyle knows how to deal with that.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/ComputedStyleConstants.h?sq=package:chromium&dr&rcl=1484526055&l=58
[2] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/forms/nsNumberControlFrame.cpp#367
[3] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/style/res/forms.css#1087
[4] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/layout/style/res/forms.css#1116
I just realized that we still need to pass the originating element to ResolvePseudoElementStyle (which is what bug 1004130 was all about), so we still have the implementation problem described in comment 6.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> I just realized that we still need to pass the originating element to
> ResolvePseudoElementStyle (which is what bug 1004130 was all about), so we
> still have the implementation problem described in comment 6.

Or wait! We don't, because the problem there is that we can't find the _frame_. We can still walk up the DOM tree to find the originating element.

(Sorry, it's a bit late).
Heycam pointed out that the inheritance is in fact observable with:

input[type=number]::placeholder { color: inherit }

Which screws up my whole proposal on how we can just use normal DOM inheritance within NAC trees.

We discussed this for a bit, and concluded the following:
* The originating element for pseudo elements should always been the first non-NAC ancestor
* The styles of every element in a NAC tree (no matter how deep and nested) should always inherit from the style of the originating element

This brings us back to the implementation issue in comment 6. We decided that we can probably solve it by adding the desire content/stylecontext pair to the frame constructor state at the top of ProcessChildren. I'm going to work on that in bug 1331322.
I have a question, which is probably kind of dumb, but I'll drop it here anyway in case it isn't...

I guess that some parts of these can be implemented with Shadow DOM? If so, given that the only complex case is the number frame, should we focus in properly implementing Shadow DOM for Stylo, and potentially rewriting <input type="number"> using it?

It's probably a bit more complex than that, but I'm worried about adding a super-uncommon complex path to the style system and then another similar path for Shadow DOM. Ideally we would be able to use Shadow DOM for most if not all the NAC we have.

WDYT, Bobby?
Flags: needinfo?(bobbyholley)
On our call, Boris pointed out that another advantage of inheriting all NAC (including non-pseudo-element-implementing NAC) from the originating element is that it would fix this discrepancy:

data:text/html,<input type="text" style="text-decoration: underline" value="1234">
data:text/html,<input type="number" style="text-decoration: underline" value="1234">

(need to run for a bit, will respond to emilio's comment when I get back)
So a summary of my thoughts.  

1)  https://drafts.csswg.org/css-pseudo-4/ doesn't say much about how pseudo-elements should inherit, nor do the specs that define them.  I filed https://github.com/w3c/csswg-drafts/issues/953 but assuming that inheritance happens from the originating element unless specified otherwise is a good idea.  And yes, this is totally observable.

2)  Specs really are pretty free to define pseudo-elements as inheriting from anything they please.  ::before/::after/::first-line/::first-letter already have all sorts of inheritance weirdness (and note that the first two of those _are_ NAC-based, but still weird because of the interactions with ::first-line).  nothing stops a spec in the future from defining something other than "inherit from the originating element".  So ideally we would not bake that _too_ hard into our design.  But at the moment, I'm not obviously aware of pseudo-elements that inherit from something else.

3)  I haven't looked into whatever the ::cue stuff and related pieces do in terms of inheritance.

4)  As discussed with Bobby, we can solve the problem mentioned in comment 6 by simply passing in a frame, when we have one, to the innermost ResolveStyleContext function in nsCSSFC.  In the NAC-backed pseudo-element case, we always have one.  We can even make it be the "right" one (walked out of NAC, etc) in the NAC-specific code.  At least assuming none of the relevant frames can end up doing display:contents on us....

5)  We do NOT want to gate this on Shadow DOM.  We just don't.  Especially not the unspecified thing that is "native" or "default" or whatever you want to call it Shadow DOM.  The complexity of dealing with that is pretty much guaranteed to be larger than Bobby's proposal above, plus what we discussed on the call today in terms of passing in frames to get the thing we're inheriting from.

6)  You'll probably need to fix nsPlaceholderFrame::GetParentStyleContext and nsFrame::DoGetParentStyleContext to have the new behavior, so when stylo is off we get the same behavior for dynamic (gecko-side) restyling and initial frame construction.
Flags: needinfo?(bzbarsky)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> I guess that some parts of these can be implemented with Shadow DOM?

As bz said, I think builtin shadow DOM is still pretty far off. In general, the first target for replacement for shadow DOM will probably be the stuff we implement with in-content XBL (like video controls and marquee). Replacing NAC is trickier.

> If so,
> given that the only complex case is the number frame

Well, not entirely. Number frame is the only thing where we get these nested pseudos right now. But we still have tons of other pseudo-implementing NAC that we need to crawl. Right now we don't handle most of that with incremental restyle, and the nice thing about treating all NAC the same is that we mostly get it for free.

> , should we focus in
> properly implementing Shadow DOM for Stylo, and potentially rewriting <input
> type="number"> using it?
> 
> It's probably a bit more complex than that, but I'm worried about adding a
> super-uncommon complex path to the style system and then another similar
> path for Shadow DOM.

There's actually not all that much complexity, since StyleChildrenIterator handles most of the nastiness (and it will get simpler when we stop distinguishing regular vs pseudo-implementing NAC). We just need a few lines of code to check the NAC bit and crawl the parent chain when selecting the parent ElementData.
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> 2)  Specs really are pretty free to define pseudo-elements as inheriting
> from anything they please.  ::before/::after/::first-line/::first-letter
> already have all sorts of inheritance weirdness (and note that the first two
> of those _are_ NAC-based, but still weird because of the interactions with
> ::first-line).

What is this interaction?

>  nothing stops a spec in the future from defining something
> other than "inherit from the originating element".  So ideally we would not
> bake that _too_ hard into our design.

I don't think it'll be baked too hard. We can always just special-case the pseudo when crawling the parent chain.

> 3)  I haven't looked into whatever the ::cue stuff and related pieces do in
> terms of inheritance.

https://www.w3.org/TR/2011/WD-html5-20110525/rendering.html#the-::cue-pseudo-element doesn't say anything about inheritance, so it may be undefined. I think Cameron is aware of what's going on in that spec, and he's on board with this plan.

> 4)  As discussed with Bobby, we can solve the problem mentioned in comment 6
> by simply passing in a frame, when we have one, to the innermost
> ResolveStyleContext function in nsCSSFC.  In the NAC-backed pseudo-element
> case, we always have one.  We can even make it be the "right" one (walked
> out of NAC, etc) in the NAC-specific code.  At least assuming none of the
> relevant frames can end up doing display:contents on us....

I don't think I follow this hazard. Can you explain it more?

> 6)  You'll probably need to fix nsPlaceholderFrame::GetParentStyleContext
> and nsFrame::DoGetParentStyleContext to have the new behavior, so when stylo
> is off we get the same behavior for dynamic (gecko-side) restyling and
> initial frame construction.

That's a good point. I'll do that.
Flags: needinfo?(jwatt) → needinfo?(bzbarsky)
> What is this interaction?

The part of ::before/::after that falls on the first line inherits from ::first-line, and the rest inherits from the originating element.  This is not special to ::before, of course; that's just how ::first-line behaves.

> doesn't say anything about inheritance, so it may be undefined.

Sort of, but it does basically say that the whole thing should be treated as follows: "The document tree against which the selectors are matched is the tree of WebVTT Node Objects rooted at the list of WebVTT Node Objects for the cue."

I would expect that this spec expects inheritance in this tree to be inheritance along its tree structure; the "::cue pseudo-element" stuff is just a nasty hack to effectively inject a scoped stylesheet into this tree, a far as I can tell.  So I guess if we don't actually plan to represent these as pseudo-elements in any real sense they're not a problem.

> I don't think I follow this hazard. Can you explain it more?

The hazard would be if we could somehow construct some NAC for an element, then not construct an actual frame for it, but construct frames for the NAC, because the element is display:contents.

Luckily, construction of NAC stuff goes via the frame, so if the element is display:contents the NAC doesn't get constructed at all, more or less.  To a first approximation.  Insert muttering about <svg:use> here.  ;)
Flags: needinfo?(bzbarsky)
Depends on: 1338382
Depends on: 1331322
No longer depends on: 1331045
Blocks: 1340009
Depends on: 1340022
Blocks: 1324348
Sorry for the delay here. I have partial patches, just buried with email and reviews.
Blocks: 1341815
Had some time to work on this yesterday - getting close but still a few failures on try.
Depends on: 1338921
Blocks: 1341973
Duplicate of this bug: 1331441
Blocks: 1345699
Blocks: 1338761
Blocks: 1342106
Priority: -- → P1
Blocks: 1346408
Blocks: 1344104
You need to log in before you can comment on or make changes to this bug.