1369954 - stylo: Selector matching should be performed on the DOM, not the flattened tree

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: XBL testcase

I'm a little disturbed if we have no tests for this...

Anyway, as pointed out in bug 1369867, stylo's selector matching is done on the flattened tree, which is wrong for both XBL and the version of Shadow DOM that we implement.  See attached XBL testcase (run it from a .xhtml file:// URL after setting the "dom.allow_XUL_XBL_for_file" preference to true) and upcoming Shadow DOM testcase.  In Gecko, the text that says it should be green is green. In stylo, for the XBL testcase, the second line is red.  Note that for the _first_ line we are in fact kinda matching on the "flattened tree", but in practice Gecko is just setting the parent of that anon content to the <div>, which is why it works.

I can't tell what stylo does with Shadow DOM, because it panics on the testcase I wrote.

Inheritance _does_ happen on the flattened tree for both XBL and Shadow DOM, so the flattened-tree traversals and PerLevelTraversalData and depth stuff should be OK.  Similarly, maintaining the bloom filter on the flattened tree is OK: It might include things it shouldn't, but will always include the things it _should_ have in it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Shadow DOM tetscase

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #0)
> Created attachment 8874076 [details]
> XBL testcase
> 
> I'm a little disturbed if we have no tests for this...

We might, they just might be marked as failing.

> 
> Anyway, as pointed out in bug 1369867, stylo's selector matching is done on
> the flattened tree, which is wrong for both XBL and the version of Shadow
> DOM that we implement.  See attached XBL testcase (run it from a .xhtml
> file:// URL after setting the "dom.allow_XUL_XBL_for_file" preference to
> true) and upcoming Shadow DOM testcase.  In Gecko, the text that says it
> should be green is green. In stylo, for the XBL testcase, the second line is
> red.  Note that for the _first_ line we are in fact kinda matching on the
> "flattened tree", but in practice Gecko is just setting the parent of that
> anon content to the <div>, which is why it works.
> 
> I can't tell what stylo does with Shadow DOM, because it panics on the
> testcase I wrote.

We're not supporting shadow DOM for the MVP of stylo (we have overholt's ok on that).

I guess we should introduce an explicit flattened_tree_parent, and use it in the appropriate places.

Over to heycam, who's done a lot of work on the stylo DOM parentage stuff before.

> 
> Inheritance _does_ happen on the flattened tree for both XBL and Shadow DOM,
> so the flattened-tree traversals and PerLevelTraversalData and depth stuff
> should be OK.  Similarly, maintaining the bloom filter on the flattened tree
> is OK: It might include things it shouldn't, but will always include the
> things it _should_ have in it.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1369954 - Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1369954 - Part 2: Remove unused Gecko_GetParentElement.

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: style: Distinguish between the tree structures used for traversal and selector matching.

This patch renames TNode::parent_element to traversal_parent, since it returns
the parent from the perspective of traversal (which in Gecko uses the
flattened tree).  It also renames TNode::children to traversal_children
for the saem reason.

We keep a parent_element function on TNode to use for selector matching,
which must be done on the real DOM tree structure.

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: style: Remove unused TElement::layout_parent_element.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1369954 - Part 3: Test.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875996 [details]
Bug 1369954 - Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode.

https://reviewboard.mozilla.org/r/147418/#review151644

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875997 [details]
Bug 1369954 - Part 2: Remove unused Gecko_GetParentElement.

https://reviewboard.mozilla.org/r/147420/#review151646

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875998 [details]
style: Distinguish between the tree structures used for traversal and selector matching.

https://reviewboard.mozilla.org/r/147422/#review151648

::: servo/components/style/dom.rs:124
(Diff revision 1)
> +    /// Get this node's parent element from the perspective of a restyle
> +    /// traversal.
> +    fn traversal_parent(&self) -> Option<Self::ConcreteElement>;
> +
> +    /// Get this node's children from the perspective of a restyle traversal.
> +    fn traversal_children(&self) -> LayoutIterator<Self::ConcreteChildrenIterator>;

Not super fond of these names, perhaps `flattened_tree_children`? Though it includes pseudos, so that isn't exactly it I guess... I guess traversal parent/children is fine for now.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875999 [details]
style: Remove unused TElement::layout_parent_element.

https://reviewboard.mozilla.org/r/147424/#review151650

::: servo/components/style/dom.rs
(Diff revision 1)
>          depth
>      }
>  
> -    /// While doing a reflow, the element at the root has no parent, as far as we're
> -    /// concerned. This method returns `None` at the reflow root.
> -    fn layout_parent_element(self, reflow_root: OpaqueNode) -> Option<Self> {

I thought this was used by Servo, but I guess got removed? shrug.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

https://reviewboard.mozilla.org/r/147426/#review151652

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8875998 [details]
style: Distinguish between the tree structures used for traversal and selector matching.

https://reviewboard.mozilla.org/r/147422/#review151648

> Not super fond of these names, perhaps `flattened_tree_children`? Though it includes pseudos, so that isn't exactly it I guess... I guess traversal parent/children is fine for now.

One thing I didn't like about "flattened tree" in these names is that it seems like a Gecko-specific concept.  (Although maybe that's not true once Servo supports Shadow DOM?)

Comment 14

[Cameron McCormack (:heycam)]

Attachment: fixup! style: Distinguish between the tree structures used for traversal and selector matching.

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

Comment 15

[Cameron McCormack (:heycam)]

Attachment: style: Add TNode::children_and_traversal_children_might_differ.

This will be used to optimize out traversing the real DOM children when
propagating restyle hints.

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

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8875996 [details]
Bug 1369954 - Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode.

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

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8875997 [details]
Bug 1369954 - Part 2: Remove unused Gecko_GetParentElement.

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

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8875998 [details]
style: Distinguish between the tree structures used for traversal and selector matching.

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

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8875999 [details]
style: Remove unused TElement::layout_parent_element.

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

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

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

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Bug 1369954 - Part 4: Stop running <meta viewport>-related tests on non-Android stylo.

Due to the confusing precedence of reftest manifest conditions,
we are running these tests when we shouldn't be.

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

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

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

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8876014 [details]
fixup! style: Distinguish between the tree structures used for traversal and selector matching.

https://reviewboard.mozilla.org/r/147456/#review151690

::: servo/components/script/layout_wrapper.rs:168
(Diff revision 1)
>      unsafe fn from_unsafe(n: &UnsafeNode) -> Self {
>          let (node, _) = *n;
>          transmute(node)
>      }
>  
> -    fn children(self) -> LayoutIterator<ServoChildrenIterator<'ln>> {
> +    fn parent_node(&self) -> Option<ServoLayoutNode<'ln>> {

nit: You could use `Option<Self>` here if you want I think

::: servo/components/script/layout_wrapper.rs:181
(Diff revision 1)
>              current: self.first_child(),
>          })
>      }
>  
> +    fn traversal_parent(&self) -> Option<ServoLayoutElement<'ln>> {
> +        self.parent_node().and_then(|n| n.as_element())

This can reuse the normal `parent_element` from selectors, right?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8876016 [details]
Bug 1369954 - Part 4: Stop running <meta viewport>-related tests on non-Android stylo.

https://reviewboard.mozilla.org/r/147460/#review151692

huh..

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

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

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8876016 [details]
Bug 1369954 - Part 4: Stop running <meta viewport>-related tests on non-Android stylo.

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

Comment 27

[Cameron McCormack (:heycam)]

https://github.com/servo/servo/pull/17252

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8875996 [details]
Bug 1369954 - Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode.

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

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8875997 [details]
Bug 1369954 - Part 2: Remove unused Gecko_GetParentElement.

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

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147426/diff/4-5/

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8876016 [details]
Bug 1369954 - Part 4: Stop running <meta viewport>-related tests on non-Android stylo.

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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cae7431057ac
Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8482bd081e6c
Part 2: Remove unused Gecko_GetParentElement. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f7ea798b7f9f
Part 3: Test. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f4ddc1f62425
Part 4: Stop running <meta viewport>-related tests on non-Android stylo. r=emilio

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbfc1396ac14
Part 5: Add FFI function for checking whether an element has XBL-created anonymous content. r=emilio

Comment 34

[Boris Zbarsky [:bzbarsky]]

> One thing I didn't like about "flattened tree" in these names is that it seems like a Gecko-specific concept.

That's the spec name in the shadow DOM spec for this thing... so if Servo plans to support shadow DOM at some point, we might as well use the name.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07edbac9bdee
Mark a few tests as failing awaiting investigation in bug 1371722. r=bustage

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cae7431057ac
https://hg.mozilla.org/mozilla-central/rev/8482bd081e6c
https://hg.mozilla.org/mozilla-central/rev/f7ea798b7f9f
https://hg.mozilla.org/mozilla-central/rev/f4ddc1f62425
https://hg.mozilla.org/mozilla-central/rev/dbfc1396ac14
https://hg.mozilla.org/mozilla-central/rev/07edbac9bdee