Closed Bug 1292609 Opened 8 years ago Closed 7 years ago

stylo: Incremental restyle for anonymous boxes

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: bholley, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

Completed analysis of our anon boxes
(deleted), text/plain
Details
Bug 1292609. Restyle ::-moz-xul-anonymous-block as needed with stylo.
(deleted), text/x-review-board-request
Details 
Emilio and I had a discussion with Boris about this on Monday.

The basic issue is that Stylo traversal is driven by the DOM rather than the frame tree. This means that it's not exactly easy to find and updateanonymous boxes during incremental restyle. But Boris thinks that we might not actually need to update them much, because:
* the UA rules that specify them never change.
* nothing ever inherits style from them.
* they rarely inherit style from other things.

So the idea is to add an assertion in nsStyleContext that inherit structs are never resolved for anonymous boxes, and see if that trips anything on try, potentially building up a small list of anonymous box selectors that trip the assertion, whitelisting and iterating until green. We'd then combine that with the list of all anonymous selectors that have UA rules with values of 'inherit'. Finally, we figure out which DOM elements these anonymous boxes correspond to, and just do an explicit switch() statement on the node type to check for them when post-traversing the content tree.
Blocks: stylo-incremental
No longer blocks: stylo
Per today's stylo meeting, we can also probably avoid restyling placeholder frames (which use mozOtherNonElement), since those generally just need the initial values for reset structs and nothing else.
Depends on: 1340277
Depends on: 1324661
Depends on: 1340404
Depends on: 1340405
Depends on: 1340723
Attached file Partial analysis of our current anon boxes (obsolete) (deleted) — 
There's a few more to analyze, but I need to be more awake for that.
Attached file Completed analysis of our anon boxes (deleted) — 

        Attachment #8838834 -
        Attachment is obsolete: true

    
    

    
  
OK, so summary of discussion with bholley:

1) For the "attached to a specific kind of frame" case, we will aim to have a virtual function on nsIFrame that can be called to update things for that frame type.

2) For the "hang out somewhere in the tree" case, we will have to do a walk from the parent looking for them or something.  We can optimize based on an nsIFrame flag.  Some of these maybe we can avoid updating at all (see next comment in this bug).

3) For the stuff at document root, we can either update it in a simple pass or not update at all if we can get away with the latter.

    
    

    
  
One other thought we had: For things we suspect or prove don't ever depend on their inherited styles (e.g. placeholders, ::-moz-pagebreak, whatever else in the list looks like it doesn't care), can we just give them either a null parent style context, or a fixed "root" one that never changes if having too many "root" style contexts is a problem?  This is a change we could make in Gecko too, so we catch it if we're wrong about them not depending on inherited styles.

David, any objections to doing this?
Flags: needinfo?(dbaron)

    
    

    
  
One other note to self: The various root frame stuff _does_ need to inherit "direction" from the root.  But changing "direction" triggers a reframe, so we just reconstruct all those frames anyway if "direction" changes on the root...  So this testcase:

  <html style="border: 1px solid green; width: 100px;"
        onclick="document.documentElement.style.direction = 'rtl'">
    Click me to flip direction
  </html>

still passes with stylo...

    
    

    
  
For the specific case of ::-moz-pagebreak we only have it in paginated cases, and I'm not even sure we do dynamic restyles in those cases... Do we?

    
    

    
  
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> One other thought we had: For things we suspect or prove don't ever depend
> on their inherited styles (e.g. placeholders, ::-moz-pagebreak, whatever
> else in the list looks like it doesn't care), can we just give them either a
> null parent style context, or a fixed "root" one that never changes if
> having too many "root" style contexts is a problem?  This is a change we
> could make in Gecko too, so we catch it if we're wrong about them not
> depending on inherited styles.
> 
> David, any objections to doing this?

It sounds reasonable to me.  I don't think we track roots anywhere any longer, so I don't think number of roots is an issue.

It's worth being careful about whether there's *really* no dependency on inherited styles, though.
Flags: needinfo?(dbaron)

    
    

    
  
> It's worth being careful about whether there's *really* no dependency on inherited styles, though.

Absolutely.  I'm hoping that try will catch the cases when there is something, but some of it will probably need a bit of thought to, for things that are less well-tested than placeholders, for example.

    
    

    
  
(times UTC-8)
19:43:56 <bz> _should_ I worry about the lack of sharing in that case?
19:44:07 <dbaron> hmmm
19:44:10 <dbaron> that's a good point
19:44:19 <bz> I just realized it could be an issue.  At least for Gecko
19:44:26 <dbaron> then again, you could do the sharing manually -- just have one!
19:44:30 <bz> (not sure what the story is with stylo)
19:44:34 <bz> Good point.
19:44:41 <bz> Alright, I can do that.
19:44:55 <bz> well, I need one per pseudo or something.
19:45:04 <bz> But I can do that too.
19:45:07 <dbaron> yeah, one per pseudo is what I meant

    
  
Assignee: emilio+bugs → bzbarsky

    
  
Depends on: 1343078

    
  
Depends on: 1343771

    
  
Depends on: 1345362

    
  
Priority: -- → P1

    
  
Depends on: 1347411

    
  
Depends on: 1369321

    
  
Depends on: 1374761

    
  
Priority: P1 → --

    
  
Priority: -- → P1

    
    

    
  
status-firefox56=affected and blocking bug 1381147 because we probably want to uplift this fix before starting our Stylo experiment on Beta 56.
Blocks: stylo-pref-study

          status-firefox55:
          --- → wontfix

          status-firefox56:
          --- → affected

          status-firefox57:
          --- → affected

          status-firefox-esr52:
          --- → unaffected

    
  
Depends on: 1388625

    
  
Depends on: 1388626

    
    

    
      
  


  

    
  

        Attachment #8895529 -
        Attachment is obsolete: true

        Attachment #8895529 -
        Flags: review?(cam)

    
  

        Attachment #8895529 -
        Attachment is obsolete: false

    
    

    
  
These have all landed.  Fixed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED

    
    

    
  
Too late for 56. Mass won't fix for 56.

          status-firefox56:
          affected → wontfix

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: