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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | wontfix |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
People
(Reporter: bholley, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 1 obsolete file)
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•8 years ago
|
Reporter | ||
Comment 1•8 years ago
|
||
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.
![]() |
Assignee | |
Comment 2•8 years ago
|
||
There's a few more to analyze, but I need to be more awake for that.
![]() |
Assignee | |
Comment 3•8 years ago
|
||
Attachment #8838834 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 4•8 years ago
|
||
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.
![]() |
Assignee | |
Comment 5•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 6•8 years ago
|
||
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...
![]() |
Assignee | |
Comment 7•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 9•8 years ago
|
||
> 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 | |
Updated•8 years ago
|
Assignee: emilio+bugs → bzbarsky
![]() |
Assignee | |
Comment 11•8 years ago
|
||
I created https://public.etherpad-mozilla.org/p/anon-box-stylo to track this stuff.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P1
Reporter | ||
Updated•7 years ago
|
Priority: P1 → --
Reporter | ||
Updated•7 years ago
|
Priority: -- → P1
Comment 12•7 years ago
|
||
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
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8895529 -
Attachment is obsolete: true
Attachment #8895529 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8895529 -
Attachment is obsolete: false
![]() |
Assignee | |
Comment 14•7 years ago
|
||
These have all landed. Fixed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Comment 15•7 years ago
|
||
Too late for 56. Mass won't fix for 56.
You need to log in
before you can comment on or make changes to this bug.
Description
•