1349134 - stylo: TraversalRootBehavior::UnstyledChildrenOnly doesn't work when <xbl:children> distributes the new children further down the tree

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Cameron McCormack (:heycam)]

dom/xbl/crashtests/368276-1.xhtml trips the assertions in bug 1345695 about resolving style when lazy style resolution wasn't requested.  This happens because we have an XBL binding that distributes real DOM children of the bound element to places deeper in the shadow tree.

For example:

  <binding id="x">
    <content>
      <span>
        <children/>
      </span>
    </content>
  </binding>

  <div style="-moz-binding: url(#x)"></div>

  <script>
    onload = function() {
      var e = document.createElement("b");
      e.textContent = "hello";
      document.querySelector("div").appendChild(e);
   };
  </script>

At the time the appendChild call runs, the <div> and its anonymous child <span> both have up-to-date styles.  nsCSSFrameConstructor::ContentAppended decides to eagerly style the new subtree using ServoStyleSet::StyleNewChildren.  Servo, in its TraversalRootBehavior::UnstyledChildrenOnly mode, uses a Gecko StyleChildrenIterator to find the children of the <div>, but this only returns the <span>.  Since the <span> already has styles, we skip it.

I guess we really want to call StyleNewChildren on aFirstNewContent's flattened tree parent, but we'd need to do that on all of aFirstNewContent's later siblings too, since they could be distributed by different <xul:children> elements.  Which means really we should call StyleNewSubtree on each of those newly appended elements.

Bobby, do you think falling back to this strategy when aContainer has a binding would be acceptable?  Should we instead try to coalesce calls to StyleNewChildren that we detect have the same flattened tree parent?

Comment 1

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Like this.  A couple of questions though:

1. Do I need to worry about text nodes that are appended?  I'm not sure how they behave when a binding is in effect.  Maybe I need to call StyleNewChildren on the real DOM parent if I find one?

2. Is NODE_MAY_BE_IN_BINDING_MNGR the right thing to check for?  Does that work for Web Components too?

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8849483 [details] [diff] [review]
WIP v0.1

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

We don't need to do any styling for text nodes, since we only style elements. As noted below, I think StyleNewChildrenIndividually should call StyleNewSubtree on each element child (and assert that there's no servo data while we're at it).

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7393,5 @@
> +      Element* parent = child->AsElement()->GetFlattenedTreeParentElement();
> +      MOZ_ASSERT(parent);
> +      mPresShell->StyleSet()->AsServo()->StyleNewChildren(parent);
> +    }
> +  }

Hm, why do this and not just call StyleNewSubtree on each one?

@@ +7492,5 @@
>    }
>  
>    // We couldn't construct lazily. Make Servo eagerly traverse the subtree.
>    if (isNewlyAddedContentForServo) {
> +    if (aContainer->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {

Hm, I'm not entirely sure this is right. Do insertion points themselves have NODE_MAY_BE_IN_BINDING_MNGR, or is it just the rebound children?

I think it might make the most sense to just call FlattenedTreeParentIsParent on aFirstNewContent and base it on that.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> > +      Element* parent = child->AsElement()->GetFlattenedTreeParentElement();
> > +      MOZ_ASSERT(parent);
> > +      mPresShell->StyleSet()->AsServo()->StyleNewChildren(parent);
> > +    }
> > +  }
> 
> Hm, why do this and not just call StyleNewSubtree on each one?

It lets us parallelize the styling of the child elements that have the same flattened tree parent.  (Which I guess might be a common pattern, having a <children> element that selects all explicit children of the bound element.)

> @@ +7492,5 @@
> Hm, I'm not entirely sure this is right. Do insertion points themselves have
> NODE_MAY_BE_IN_BINDING_MNGR, or is it just the rebound children?

I don't know. :(  (The comment above the flag definition is not so helpful.)

> I think it might make the most sense to just call
> FlattenedTreeParentIsParent on aFirstNewContent and base it on that.

What about a binding like this:

  <binding ...>
    <content>
      <children includes="b"/>
      <span>
        <children includes="i"/>
      </span>
    </contents>
  </binding>

  <div style="-moz-binding: ..."><b>x</b><i>x</i></div>

The <b>'s flattened tree parent will be the <div>.  So we'd need to check all of the children to see if they are not FlattenedTreeParentIsParent.

What about |aContainer->HasFlag(NODE_MAY_BE_IN_BINDING_MGR) && aContainer->GetXBLBinding()|?

(But again is this condition true for elements with Web Components bindings?)

Comment 4

[Bobby Holley (:bholley)]

Ok. So. For correctness' sake, I think the general algorithm should do the following:

(1) Get the flattened tree parent of the first child and store it in |flattened|.
(2) StyleNewChildren(flattened).
(2) Iterate next siblings until we either hit the end or hit an element which returns a different result for GetFlattenedTreeParent than |flattened|. In the latter case, update |flattened| and go back to (1).

That basically boils down to what we have now for the non-weird cases, but _does_ cost some extra GetFlattenedTreeParent calls. The remaining question then is whether we can reliably detect the existence of remapped children by checking bits on the parent. I think that is probably a question for bz.

Comment 5

[Cameron McCormack (:heycam)]

Boris, is there a cheap, reliable way to detect whether a given element has flattened tree children that are not explicit children?  Checking NODE_MAY_BE_IN_BINDING_MGR on the parent seems like it might be a prerequisite for this, but is there something more accurate, without using say a FlattenedChildIterator to actually look for them?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm not aware of anything faster than what FlattenedChildIterator::Init does if you actually want accurate.

And even then that doesn't catch the shadow DOM cases, I think...  I'm not quite sure what would offhand.  Maybe wchen remembers?

Comment 7

[Cameron McCormack (:heycam)]

Ah, it turns out (as I found in bug 1349487) that we handle insertions of nodes into elements with a ShadowRoot separately.  So I guess just handling XBL-distributed children is fine here.

> I'm not aware of anything faster than what FlattenedChildIterator::Init does if you actually want accurate.

OK.  Actually I want something that just has a good balance of accuracy to speed.  For now, I'll go for |aContainer->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) && aContainer->GetXBLBinding()|, unless there is any other flag/state I can check that indicates whether the binding has an <xbl:children> element somewhere not at the top level of the <content>.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1349134 - stylo: Style newly appended children of an element with a binding through their flattened tree parents.

Review commit: https://reviewboard.mozilla.org/r/123074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123074/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Isn't that insufficient?  An insertion point parent has flattened tree children that are not explicit children, right?  Or am I misremembering what "explicit children" means?

Comment 10

[Cameron McCormack (:heycam)]

Can you give an example of the case you're thinking about?

Comment 11

[Boris Zbarsky [:bzbarsky]]

  <div>
    text
    <children/>
    <p>stuff</p>
  </div>

Comment 12

[Cameron McCormack (:heycam)]

Per IRC discussion, the overhead of taking the "slow" path even when there is no XBL / Web Components / etc. weirdness might be insubstantial, in which case we should just do that, as it'll be more robust against flattened tree weirdness that we might not think of.  I'll test.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8850471 [details]
Bug 1349134 - stylo: Style newly appended children of an element with a binding through their flattened tree parents.

Ok, canceling review on this patch then. I think bz is right that the overhead of the slow path is pretty insubstantial, since it would just be a single GetFlattenedTreeParent call, which is probably fine.

IMO it's not worth spending the time to measure it.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: test

I did test, just to make myself happy, and I got pretty noisy numbers, but both the slow and fast paths were around 50ms.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8850471 [details]
Bug 1349134 - stylo: Style newly appended children of an element with a binding through their flattened tree parents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123074/diff/1-2/

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8850471 [details]
Bug 1349134 - stylo: Style newly appended children of an element with a binding through their flattened tree parents.

https://reviewboard.mozilla.org/r/123074/#review125746

r=me with those things fixed.

::: layout/base/nsCSSFrameConstructor.h:161
(Diff revision 2)
> +   * on their flattened tree parents.  This is used to respond to elements
> +   * inserted under an element with an XBL binding.  Only used when we are
> +   * styled by Servo.

This comment is misleading, since we use this for everything now. It should say something about handling xbl insertion point rebinding if applicable.

::: layout/base/nsCSSFrameConstructor.h:165
(Diff revision 2)
> +  void StyleNewChildrenIndividually(nsIContent* aStartChild,
> +                                    nsIContent* aEndChild);

I think we should call this "StyleNewChildRange", given that there isn't really an alternative path anymore.

::: layout/base/nsCSSFrameConstructor.cpp:7391
(Diff revision 2)
> +
> +  for (nsIContent* child = aStartChild; child != aEndChild;
> +       child = child->GetNextSibling()) {
> +    // Calling StyleNewChildren on one child will end up styling another child,
> +    // if they share the same flattened tree parent.  So we check HasServoData()
> +    // to avoid wastefully calling StyleNewChildren on the same parent twice.

Please alter this to note that it also avoids the GetFlattenedTreeParentElement call as well. And also say that, in the common case, this boils down to a single call on the parent, modulo walking the siblings list (which is very fast).

::: layout/base/nsCSSFrameConstructor.cpp:7495
(Diff revision 2)
> -    mPresShell->StyleSet()->AsServo()->StyleNewChildren(aContainer->AsElement());
> +    // If the element has an XBL binding, then the newly inserted explicit
> +    // children might be distributed elsewhere in the shadow tree, so we need
> +    // to style each of the new children's flattened tree parents separately.
> +    StyleNewChildrenIndividually(aFirstNewContent, nullptr);

Along the same lines as the discussion above, I think this comment (and the one below) can go. I think a call to StyleNewChildRange() is self-explanatory, and the gory details of insertion points can be documented in that implementation of that function.

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8850471 [details]
Bug 1349134 - stylo: Style newly appended children of an element with a binding through their flattened tree parents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123074/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff4b66de4c9c
stylo: Style newly appended children of an element with a binding through their flattened tree parents. r=bholley

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff4b66de4c9c