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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: bz, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 4 obsolete attachments)

694 bytes, application/xhtml+xml
Details
802 bytes, text/html
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
Created attachment 8874076 [details]
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.
(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.
Assignee: nobody → cam
Priority: -- → P1
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8875998 - Flags: review?(emilio+bugs)
Attachment #8875999 - Flags: review?(emilio+bugs)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8875996 [details]
Bug 1369954 - Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode.

https://reviewboard.mozilla.org/r/147418/#review151644
Attachment #8875996 - Flags: review?(emilio+bugs) → review+

Comment 9

8 months ago
mozreview-review
Comment on attachment 8875997 [details]
Bug 1369954 - Part 2: Remove unused Gecko_GetParentElement.

https://reviewboard.mozilla.org/r/147420/#review151646
Attachment #8875997 - Flags: review?(emilio+bugs) → review+

Comment 10

8 months ago
mozreview-review
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.
Attachment #8875998 - Flags: review?(emilio+bugs) → review+

Comment 11

8 months ago
mozreview-review
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.
Attachment #8875999 - Flags: review?(emilio+bugs) → review+

Comment 12

8 months ago
mozreview-review
Comment on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

https://reviewboard.mozilla.org/r/147426/#review151652
Attachment #8876000 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 13

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8876014 - Flags: review?(emilio+bugs)

Comment 23

8 months ago
mozreview-review
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?
Attachment #8876014 - Flags: review?(emilio+bugs) → review+

Comment 24

8 months ago
mozreview-review
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..
Attachment #8876016 - Flags: review?(emilio+bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8875998 - Attachment is obsolete: true
Attachment #8876014 - Attachment is obsolete: true
Attachment #8876015 - Attachment is obsolete: true
Attachment #8875999 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

8 months ago
Pushed by cmccormack@mozilla.com:
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

8 months ago
Pushed by cmccormack@mozilla.com:
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
> 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.
Depends on: 1371722

Comment 35

8 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07edbac9bdee
Mark a few tests as failing awaiting investigation in bug 1371722. r=bustage
You need to log in before you can comment on or make changes to this bug.