stylo: Incremental restyle for anonymous boxes

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
11 months ago
3 days ago

People

(Reporter: bholley, Assigned: bz)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Reporter)

Updated

11 months ago
Blocks: 1288278
No longer blocks: 1243581
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
Created attachment 8838834 [details]
Partial analysis of our current anon boxes

There's a few more to analyze, but I need to be more awake for that.
Created attachment 8839240 [details]
Completed analysis of our anon boxes
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
I created https://public.etherpad-mozilla.org/p/anon-box-stylo to track this stuff.
Priority: -- → P1
Depends on: 1347411
Blocks: 1243581
Depends on: 1369321
Depends on: 1374761
You need to log in before you can comment on or make changes to this bug.