1292662 - stylo: Handle style for Anonymous XBL and NAC

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Right now, I don't believe we handle anonymous subtrees at all. Servo doesn't know about them (and so can't traverse them), and trying to get the style from servo for those nodes will just trigger the bailout case and hand out the default computed values:

https://github.com/servo/servo/blob/f0f39904a2361e1d249a4fe359660d9c6516afe8/ports/geckolib/glue.rs#L269

Comment 1

[Bobby Holley (:bholley)]

Ideally, Servo would learn about anonymous subtrees, and would be able to style them during its parallel traversal like everything else. A few questions:

* How does our handling of XBL subtrees and Shadow DOM subtrees differ? Is there a reasonable abstraction we could use to describe them both to Servo so that Servo could just think it's dealing with Shadow DOM?
* What are the CSS inheritance semantics for XBL subtrees and Shadow DOM subtrees? Do both of them experience normal inheritance and selector matching across the anonymous gap (modulo the special XBL <stylesheet> stuff)?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Also, we need to take into account scoping of stylesheets in shadow trees[1], so servo will need to learn about different style scopes, which might be tricky.

[1]: https://w3c.github.io/webcomponents/spec/shadow/#h-html-elements-in-shadow-trees

Comment 3

[Bobby Holley (:bholley)]

I split the shadow DOM pieces into bug 1293844. This bug can focus on XBL AC and NAC, which are more important for getting our test suite mostly green.

Comment 4

[Bobby Holley (:bholley)]

I think we'll generally want a mechanism to expose an additional list of anonymous children to Servo's parallel traversal.

Both AC and NAC have the following properties:
* Style is inherited from the parent
* Selectors match normally

AC and NAC differ in the following ways:
* AC matches all rules, NAC matches UA rules (this is bug 1293809)
* AC can define additional stylesheets that apply in that subtree.
* Some NAC items correspond to PEs. We'll want to avoid passing these to the traversal to avoid cascading them twice.

We'll also need bug 1293812, which is about making sure that we fix up the style contexts for anonymous content during post-traversal.

Comment 5

[Bobby Holley (:bholley)]

One slightly-odd discrepancy that falls out of the rules as I understand them is as follows:
* A piece of NAC that does not implement a PE gets normal selector matching: i.e. if the root of the anonymous subtree is a <div> and the parent is an <input>, it will match rules of the form |input > div|.
* A piece of NAC that _does_ implement a PE gets the style for that PE, ignoring the actual element type of the NAC: i.e. if the root of the anonymous subtree is a <div> that implements ::-moz-number-spin-box, it will match rules of the form |input::-moz-number-spin-box|, but _not_ |input > div|.

David, do I have that right?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> One slightly-odd discrepancy that falls out of the rules as I understand
> them is as follows:
> * A piece of NAC that does not implement a PE gets normal selector matching:
> i.e. if the root of the anonymous subtree is a <div> and the parent is an
> <input>, it will match rules of the form |input > div|.
> * A piece of NAC that _does_ implement a PE gets the style for that PE,
> ignoring the actual element type of the NAC: i.e. if the root of the
> anonymous subtree is a <div> that implements ::-moz-number-spin-box, it will
> match rules of the form |input::-moz-number-spin-box|, but _not_ |input >
> div|.
> 
> David, do I have that right?


So assuming:
  NAC == native-anonymous content
  PE == pseudo-element

I think the underlying reason things happen this way is that nsIAnonymousContentCreator, which is the main way we create native-anonymous content, has a mechanism for customizing the style of that content.  So, say, nsNumberControlFrame::CreateAnonymousContent has a helper function called MakeAnonymousElement that customizes the style to use the input element itself (mContent->AsElement()) along with the given pseudo-element (or, in some cases, anonymous box).

When it's an anonymous box, there's (I believe) no associated element (unlike for pseudo-elements).

I'm not sure if you can describe this as a general rule since CreateAnonymousContent methods have a decent amount of flexibility.  Though I hope they all do what you describe (modulo the anonymous boxes, which can't be styled from content, not having an associated element at all).

Comment 7

[Bobby Holley (:bholley)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #6) 
> I'm not sure if you can describe this as a general rule since
> CreateAnonymousContent methods have a decent amount of flexibility.

It seems like there's still some need for a convention here so that restyling knows what to do, right? That is to say, the behavior of ResolvePseudoElementStyle and ResolveStyleFor differ in terms of which selectors they match, and the restyle manager invokes one or the other depending on whether the style context is marked as corresponding to a pseudo-element or not.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an API for the Servo style system to traverse anonymous children. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Style XBL anonymous content after binding explicit children to insertion points. v1

We're doing it too soon, which means that the subtree isn't full set up.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use GetFlattenedTreeParent in Servo parent hooks where appropriate. v1

Otherwise explicit children bound to insertion points will appear to have a
placeholder XBL children element as their parent, which will break during style
traversal because we never traverse or style such nodes.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Use StyleChildrenIterator in ServoRestyleManger. v1

Most importantly, this causes us to traverse NAC during style context fixup.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8781779 [details] [diff] [review]
Part 1 - Add an API for the Servo style system to traverse anonymous children. v1

Review of attachment 8781779 [details] [diff] [review]:
-----------------------------------------------------------------

f+, though we should be really careful to see if we need to use the NAC tree to resolve sibling selectors, and to get the parent. From a quick look at [1], and not knowing a lot about NAC:

> As far as CSS is concerned, anonymous content nodes are children (or descendants) of the bound element, they are ancestors of explicit content, and they are siblings of the explicit content. Style rules using the child, descendant, or sibling selectors transparently cross bind scopes and operate on the altered and original content models.

So it seems we need to take them into account for sibling selectors?

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Anonymous_Content

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8781782 [details] [diff] [review]
Part 4 - Use StyleChildrenIterator in ServoRestyleManger. v1

Review of attachment 8781782 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/ServoRestyleManager.cpp
@@ +148,5 @@
>    if (aContent->HasDirtyDescendantsForServo()) {
>      MOZ_ASSERT(primaryFrame,
>                 "Frame construction should be scheduled, and it takes the "
>                 "correct style for the children, so no need to be here.");
> +    StyleChildrenIterator it(aContent);

So with this iterator we step over, e.g., :before and :after, right? (but we still compute them[1]).

If that's the case, when this and my pseudo-element restyle is merged, we'd do the lookup twice if the node is dirty. It's probably not a huge perf impact, but maybe leaving a note about that (or in general, specializing this iterator) in the StyleChildrenIterator declaration?

I agree that right now it's worth to reuse the AlChildrenIterator code, but if for some reason it ends up being slow, we'd rather not forget about it.

[1]: http://searchfox.org/mozilla-central/source/dom/base/ChildIterator.cpp#385

Comment 14

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> I agree that right now it's worth to reuse the AlChildrenIterator code, but
> if for some reason it ends up being slow, we'd rather not forget about it.

Good thought - I filed bug 1296065 about this.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> So it seems we need to take them into account for sibling selectors?

Looking into this a bit, I don't think we do:

http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/style/nsCSSRuleProcessor.cpp#2394

Also note bug 962628, which made things work for top-level anonymous content subtrees, but doesn't do any handling across insertion points.

This seems to be the state of the world in Gecko at least. It's possible that shadow DOM will eventually require something more complex here, but if so Gecko doesn't implement such complexity at present.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8781779 [details] [diff] [review]
Part 1 - Add an API for the Servo style system to traverse anonymous children. v1

Review of attachment 8781779 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ChildIterator.cpp
@@ +535,5 @@
> +  if (pseudoType == CSSPseudoElementType::AnonBox) {
> +    MOZ_ASSERT_IF(aContent->IsNodeOfType(nsINode::eTEXT),
> +                  f->StyleContext()->GetPseudo() == nsCSSAnonBoxes::mozText);
> +    MOZ_ASSERT_IF(aContent->IsHTMLElement(nsGkAtoms::table),
> +                  f->StyleContext()->GetPseudo() == nsCSSAnonBoxes::tableWrapper);

Is this true for <table> elements that are not display:table?  I guess in that case we won't get into here because it won't be an AnonBox.  And what about non-<table> elements that are display:table?  I guess I'm not sure what these assertions are for, other than documenting invariants on text nodes and <table> elements, but which don't impact our decision to return false here.

@@ +577,5 @@
> +      // Skip any native-anonymous children that are used to implement pseudo-
> +      // elements. These match pseudo-element selectors instead of being
> +      // considered a child of their host, and thus the style system needs to
> +      // handle them separately.
> +      MOZ_ASSERT(child->IsRootOfNativeAnonymousSubtree());

Might make more sense to do this assertion inside IsNativeAnonymousImplementationOfPseudoElement, just before we return true, as it's the logic in that function we're checking.

But should we be using IsInNativeAnonymousSubtree() instead?  What if the NAC root has a style context without a pseudo, but its child has a style context with a pseudo?  Or does that never happen?  If so, can we assert to say that things will need adjustment if we ever create such NAC subtrees?

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8781780 [details] [diff] [review]
Part 2 - Style XBL anonymous content after binding explicit children to insertion points. v1

Review of attachment 8781780 [details] [diff] [review]:
-----------------------------------------------------------------

s/full/fully/ in the commit message.

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8781781 [details] [diff] [review]
Part 3 - Use GetFlattenedTreeParent in Servo parent hooks where appropriate. v1

Review of attachment 8781781 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if there's a good reason not to care about Web Components for now (which should probably be mentioned in a comment).

::: layout/style/ServoBindings.cpp
@@ +45,5 @@
>  
>  RawGeckoNode*
>  Gecko_GetParentNode(RawGeckoNode* aNode)
>  {
> +  if (MOZ_UNLIKELY(aNode->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR))) {

Why do we only call GetFlattenedTreeParent when this flag is set?  Is it that we don't care about the first half of what nsIContent::GetFlattenedTreeParent does (which I think handles Web Components insertion points)?

@@ +82,5 @@
>  {
> +  if (MOZ_UNLIKELY(aElement->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR))) {
> +    nsIContent* parent = aElement->GetFlattenedTreeParent();
> +    if (!parent->IsElement()) {
> +      // This probably can't happen, but the type system doesn't prevent it.

Will we never set this NODE_MAY_BE_IN_BINDING_MNGR flag on the root element?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #18)
> Comment on attachment 8781781 [details] [diff] [review]
> Part 3 - Use GetFlattenedTreeParent in Servo parent hooks where appropriate.
> v1
> 
> Review of attachment 8781781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if there's a good reason not to care about Web Components for now
> (which should probably be mentioned in a comment).

Yeah, I was initially planning to just punt on Web Components for now, but I think you're right that it's worth handling while I'm here. I spent some time optimizing the call in bug 1296509 and now it should be fast (and hopefully correct) in the common case.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #16)
> Comment on attachment 8781779 [details] [diff] [review]
> Part 1 - Add an API for the Servo style system to traverse anonymous
> children. v1
> 
> Review of attachment 8781779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/ChildIterator.cpp
> @@ +535,5 @@
> > +  if (pseudoType == CSSPseudoElementType::AnonBox) {
> > +    MOZ_ASSERT_IF(aContent->IsNodeOfType(nsINode::eTEXT),
> > +                  f->StyleContext()->GetPseudo() == nsCSSAnonBoxes::mozText);
> > +    MOZ_ASSERT_IF(aContent->IsHTMLElement(nsGkAtoms::table),
> > +                  f->StyleContext()->GetPseudo() == nsCSSAnonBoxes::tableWrapper);
> 
> Is this true for <table> elements that are not display:table?  I guess in
> that case we won't get into here because it won't be an AnonBox.  And what
> about non-<table> elements that are display:table?  I guess I'm not sure
> what these assertions are for, other than documenting invariants on text
> nodes and <table> elements, but which don't impact our decision to return
> false here.

I've switched this to:

MOZ_ASSERT(f->StyleContext()->GetPseudo() == nsCSSAnonBoxes::mozText ||
           f->StyleContext()->GetPseudo() == nsCSSAnonBoxes::tableWrapper);


> @@ +577,5 @@
> > +      // Skip any native-anonymous children that are used to implement pseudo-
> > +      // elements. These match pseudo-element selectors instead of being
> > +      // considered a child of their host, and thus the style system needs to
> > +      // handle them separately.
> > +      MOZ_ASSERT(child->IsRootOfNativeAnonymousSubtree());
> 
> Might make more sense to do this assertion inside
> IsNativeAnonymousImplementationOfPseudoElement, just before we return true,
> as it's the logic in that function we're checking.

Done.

> But should we be using IsInNativeAnonymousSubtree() instead?  What if the
> NAC root has a style context without a pseudo, but its child has a style
> context with a pseudo?  Or does that never happen?

I don't think it does. If it did, this detection algorithm would be broken, since it's based on GetPrimaryFrame.

> If so, can we assert to
> say that things will need adjustment if we ever create such NAC subtrees?

I don't know where we would assert that unfortunately, since ResolvePseudoElementStyle doesn't have enough information AFAICT, and the callers are the scattered NAC creation routines.

Comment 21

[Cameron McCormack (:heycam)]

> I don't know where we would assert that unfortunately, since ResolvePseudoElementStyle
> doesn't have enough information AFAICT, and the callers are the scattered NAC creation
> routines.

We could do it in nsCSSFrameConstructor where we call CreateAnonymousContent, I guess, if you think it's worth it.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #21)
> > I don't know where we would assert that unfortunately, since ResolvePseudoElementStyle
> > doesn't have enough information AFAICT, and the callers are the scattered NAC creation
> > routines.
> 
> We could do it in nsCSSFrameConstructor where we call
> CreateAnonymousContent, I guess, if you think it's worth it.

But we'd basically need to traverse the entire subtree to look for the case of nonroot-with-PE-SC, right?

In fact, I think this case can't happen at all because the AC creator returns pairs of root nodes with optional custom style contexts. The custom style contexts are the only way we end up with a PE, and the API can only express them as a root.

Comment 23

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> But we'd basically need to traverse the entire subtree to look for the case
> of nonroot-with-PE-SC, right?

Yes, so perhaps it's not worth it.

> In fact, I think this case can't happen at all because the AC creator
> returns pairs of root nodes with optional custom style contexts. The custom
> style contexts are the only way we end up with a PE, and the API can only
> express them as a root.

An AC creator can provide an explicit style context for the root without a pseudo, and an explicit style context for an element in ContentInfo::mChildren with a pseudo, can't it?

Comment 24

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #23)
> An AC creator can provide an explicit style context for the root without a
> pseudo, and an explicit style context for an element in
> ContentInfo::mChildren with a pseudo, can't it?

Oh, you're right - I thought the children were implicitly expressed, but I didn't see that API.

And in fact, the one consumer of it appears to be nsNumberControlFrame::CreateAnonymousContent, which appears to have a whole tree of pseudo-elements. But the root is also a pseudo-element, so I guess it doesn't affect our invariants here.

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 5 - Modify StyleChildIterator::IsNeeded to handle the root scroll frame. v1

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 6 - Temporarily switch off frame reconstruction hints for native anonymous nodes. v1

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 7 - Use accessors to unset stylo dirty bits. v1

I often find myself wanting to hook them for logging etc, and this makes the
API symmetric with the setters.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8784631 [details] [diff] [review]
Part 7 - Use accessors to unset stylo dirty bits. v1

Review of attachment 8784631 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsINode.h
@@ +1039,5 @@
> +  inline void UnsetIsDirtyAndHasDirtyDescendantsForServo() {
> +    MOZ_ASSERT(IsStyledByServo());
> +    UnsetFlags(NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO | NODE_IS_DIRTY_FOR_SERVO);
> +  }
> +

These methods don't really need "inline" on them since that's implied by the function body being here.

Comment 29

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b0f2c56f633

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f13817111ef
Add an API for the Servo style system to traverse anonymous children. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba090154f48
Style XBL anonymous content after binding explicit children to insertion points. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1748ac77ef
Use GetFlattenedTreeParent in Servo parent hooks where appropriate. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa02a5285a8e
Use StyleChildrenIterator in ServoRestyleManger and ServoStyleSet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bf16ffab2f
Modify StyleChildIterator::IsNeeded to handle the root scroll frame. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/98ab62940ffc
Temporarily switch off frame reconstruction hints for native anonymous nodes. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d015eed45f
Use accessors to unset stylo dirty bits. r=heycam

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f13817111ef
https://hg.mozilla.org/mozilla-central/rev/1ba090154f48
https://hg.mozilla.org/mozilla-central/rev/1f1748ac77ef
https://hg.mozilla.org/mozilla-central/rev/aa02a5285a8e
https://hg.mozilla.org/mozilla-central/rev/e1bf16ffab2f
https://hg.mozilla.org/mozilla-central/rev/98ab62940ffc
https://hg.mozilla.org/mozilla-central/rev/45d015eed45f