Closed Bug 1371130 Opened 7 years ago Closed 7 years ago

stylo: descendant restyle hints not handled correctly in the presence of XBL

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: heycam, Assigned: emilio)

References

Details

Attachments

(7 files, 6 obsolete files)

test
600 bytes, application/xhtml+xml
Details
Bug 1371130: Expose AllChildrenIterator::AppendNativeAnonymousChildren.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1371130: expose methods to get ::before, ::after, and the other NAC pseudos.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1371130: Invalidate style using the DOM tree, and scan pseudo-elements and NAC separately.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1371130: Get restyle hints right in presence of XBL.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1371130: Test.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1371130: Remove a few unused FFI functions.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Bug 1369954 made me realize that the https://github.com/servo/servo/pull/16906 changes, which added restyle hints to handle sequences of child combinators by counting down each traversal level of the tree, are wrong in the face of XBL, because we traverse down the flattened tree, but need to be counting the traversal levels of the regular DOM tree.
Attached file test — 

    
    

    
  
The simple solution I guess is to detect when traverse into the <xbl:children>-inserted content, and convert any restyle hints with specific descendant levels into a whole-subtree restyle hint.

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #2)
> The simple solution I guess is to detect when traverse into the
> <xbl:children>-inserted content, and convert any restyle hints with specific
> descendant levels into a whole-subtree restyle hint.

That won't work, since we might consumed all of the specific descendant levels by the time we get to the <xbl:children>-inserted content.  Instead I think we need to do something when pre-processing the children of the bound element.

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #3)
> (In reply to Cameron McCormack (:heycam) from comment #2)
> > The simple solution I guess is to detect when traverse into the
> > <xbl:children>-inserted content, and convert any restyle hints with specific
> > descendant levels into a whole-subtree restyle hint.
> 
> That won't work, since we might consumed all of the specific descendant
> levels by the time we get to the <xbl:children>-inserted content.  Instead I
> think we need to do something when pre-processing the children of the bound
> element.

Note that I'm rewriting a bunch of this also in bug 1368240, and I think I want a dom_children iterator for style invalidation, which afaict would fix this.

This is also a problem for LaterSiblings hints, for pretty much the same reason.

    
    

    
  
OK, great.  I have some patches to clear up the distinction between flattened tree and the real DOM tree for parent and children iterators, which I will get finished today.

I wondered about LaterSiblings too, although I suspect there is something already odd about how sibling combinators are handled in the presence of XBL in Gecko.

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
  

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

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

    
    

    
  
Comment on attachment 8876001 [details]
Bug 1371130 - Part 1: Add FFI function for checking whether an element has XBL-created anonymous content.

https://reviewboard.mozilla.org/r/147430/#review151654

::: layout/style/ServoBindings.cpp:246
(Diff revision 1)
>  }
>  
> +bool
> +Gecko_NodeHasBindingWithAnonymousContent(RawGeckoNodeBorrowed aNode)
> +{
> +  if (!aNode->IsElement() ||

We can check this without FFI.

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

    
    

    
  
Comment on attachment 8876001 [details]
Bug 1371130 - Part 1: Add FFI function for checking whether an element has XBL-created anonymous content.

https://reviewboard.mozilla.org/r/147430/#review151654

> We can check this without FFI.

That's true.  Though I think we shouldn't actually call this with a non-Element node...

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8876002 -
        Attachment is obsolete: true

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

    
  

        Attachment #8876005 -
        Attachment is obsolete: true

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

    
    

    
  
I'm landing part 1 as part of bug 1369954.  (I forgot it was actually a pre-requisite of that bug.)
Assignee: nobody → cam
Priority: -- → P2

    
    

    
  
Cam, mind if I take this one? I think I have an idea for doing that with the invalidation stuff elegantly.
Flags: needinfo?(cam)

    
    

    
  
Yes, go ahead. :-)
Assignee: cam → emilio+bugs
Flags: needinfo?(cam)

    
  

        Attachment #8876001 -
        Attachment is obsolete: true

    
  

        Attachment #8876003 -
        Attachment is obsolete: true

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

    
  

        Attachment #8876004 -
        Attachment is obsolete: true

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

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8879145 -
        Attachment is obsolete: true

        Attachment #8879145 -
        Flags: review?(cam)

    
    

    
  
Comment on attachment 8879144 [details]
Bug 1371130: Test.

https://reviewboard.mozilla.org/r/150462/#review155176

This test is the one cam submitted (I've kept the author information in the patch), and it's r=me

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

    
    

    
  
Comment on attachment 8879140 [details]
Bug 1371130: Expose AllChildrenIterator::AppendNativeAnonymousChildren.

https://reviewboard.mozilla.org/r/150454/#review155464

::: dom/base/ChildIterator.h:231
(Diff revision 1)
> +  static void AppendNativeAnonymousChildren(const nsIContent* aContent,
> +                                            nsTArray<nsIContent*>& aKids,
> +                                            uint32_t aFlags);

If we're exposing it as a utility function, then I think it would be better on nsContentUtils.  r=me with moving it (and its private helper) there.

        Attachment #8879140 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8879141 [details]
Bug 1371130: expose methods to get ::before, ::after, and the other NAC pseudos.

https://reviewboard.mozilla.org/r/150456/#review155488

r=me with that.

::: servo/components/style/gecko/wrapper.rs:719
(Diff revision 1)
> +    /// Execute `f` for each anonymous content pseudo-element (apart from
> +    /// ::before and ::after) whose originating element is `self`.
> +    fn each_anonymous_content_pseudo<F>(&self, mut f: F)
> +    where
> +        F: FnMut(Self),
> +    {

The changes in the following patch to this function should be folded in to this patch.

        Attachment #8879141 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8879142 [details]
Bug 1371130: Invalidate style using the DOM tree, and scan pseudo-elements and NAC separately.

https://reviewboard.mozilla.org/r/150458/#review155468

::: servo/components/style/invalidation/element/invalidator.rs:318
(Diff revision 1)
> +
> +    fn invalidate_pseudo_elements_and_nac(
> +        &mut self,
> +        invalidations: &InvalidationVector
> +    ) -> bool {
> +        let mut any_pseudo = false;

Since we invalidate non-pseudo NAC children too, maybe rename this to "any_children" or "any_pseudo_or_nac" or something.

::: servo/components/style/invalidation/element/invalidator.rs:331
(Diff revision 1)
> +            any_pseudo |=
> +                self.invalidate_pseudo_element_or_nac(after, invalidations);
> +        }
> +
> +        let element = self.element;
> +        element.each_anonymous_content_child(|pseudo| {

Rename "pseudo" to "child", for the same reason?

        Attachment #8879142 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8879143 [details]
Bug 1371130: Get restyle hints right in presence of XBL.

https://reviewboard.mozilla.org/r/150460/#review155502

::: servo/components/style/dom.rs:345
(Diff revision 2)
> -    /// Execute `f` for each anonymous content pseudo-element (apart from
> -    /// ::before and ::after) whose originating element is `self`.
> +    /// Execute `f` for each anonymous content child (apart from ::before and
> +    /// ::after) whose originating element is `self`.

Please fold this change in to part 2, too. :-)

::: servo/components/style/invalidation/element/invalidator.rs:296
(Diff revision 2)
> -            );
> -
> -        invalidated || invalidated_children
>      }
>  
> -    fn invalidate_pseudo_elements_and_nac(
> +    fn invalidate_descendant(

invalidate_descendant sounds a bit like it's only going to invalidate one descendant element.  I guess we call this function for a single "child", and to invalidate it and its descendants.  Would a name like "invalidate_child_and_its_descendants" be better?  Well, that's a bit long, but something to that effect?

Maybe just "invalidate_child", if you're happy for that to imply that we recurse and invalidate its descendants too.

::: servo/components/style/invalidation/element/invalidator.rs:351
(Diff revision 2)
> +    fn invalidate_nac(
> +        &mut self,
> +        invalidations: &InvalidationVector,
> +    ) -> bool {

It would be good to preserve the comment about not needing to care about sibling invalidations for NAC.  Maybe in this function?

        Attachment #8879143 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8879146 [details]
Bug 1371130: Remove a few unused FFI functions.

https://reviewboard.mozilla.org/r/150474/#review155510

        Attachment #8879146 -
        Flags: review?(cam) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/24b1e6fdb935
Move AllChildrenIterator::AppendNativeAnonymousChildren to nsContentUtils. r=heycam
https://hg.mozilla.org/integration/autoland/rev/09568de198cf
expose methods to get ::before, ::after, and the other NAC pseudos. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f67242fcc692
Remove a few unused FFI functions. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9c95f557d70a
Test. r=emilio

    
    

    
  
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f6c09baa86b1
Whitelist AppendOwnedAnonBoxes to please the hazard analysis. r=me

    
    

    
  
It would really be nice if the hazard analysis bit explained why the annotation is needed and safe in either code comments or commit message...

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: