Closed Bug 1369954 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 4 obsolete files)

XBL testcase
694 bytes, application/xhtml+xml
Details
Shadow DOM tetscase
802 bytes, text/html
Details
Bug 1369954 - Part 1: Rename Gecko_GetParentNode to Gecko_GetFlattenedTreeParentNode.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1369954 - Part 2: Remove unused Gecko_GetParentElement.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1369954 - Part 3: Test.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1369954 - Part 4: Stop running <meta viewport>-related tests on non-Android stylo.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Attached file 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.
Attached file Shadow DOM tetscase — 

    
    

    
  
(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

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
  

        Attachment #8875998 -
        Flags: review?(emilio+bugs)

        Attachment #8875999 -
        Flags: review?(emilio+bugs)

    
    

    
  
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 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 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 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 on attachment 8876000 [details]
Bug 1369954 - Part 3: Test.

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

        Attachment #8876000 -
        Flags: review?(emilio+bugs) → 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

> 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?)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  


  

    
    

    
      
  

    
  

        Attachment #8876014 -
        Flags: review?(emilio+bugs)

    
    

    
  
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 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+

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8875998 -
        Attachment is obsolete: true

    
  

        Attachment #8876014 -
        Attachment is obsolete: true

    
  

        Attachment #8876015 -
        Attachment is obsolete: true

    
  

        Attachment #8875999 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: