Optimize position:static <--> position:relative changes to avoid jank from massive frame reconstruction

NEW
Unassigned

Status

()

Core
Layout
2 years ago
a month ago

People

(Reporter: dholbert, Unassigned)

Tracking

(Blocks: 2 bugs, {perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: [qf:p3])

Attachments

(1 obsolete attachment)

(Reporter)

Description

2 years ago
Bug 1256706 describes a scenario where twitter dynamically adds the following style to a given tweet in your tweet-stream:
  z-index:3;
  position:relative;

The point is just to make this one tweet rise above the others. (in their case, so that a "halo" box-shadow effect will show up & not get hidden behind later DOM elements).

Right now, this style-change triggers frame reconstruction, which is annoying because it destroys some a11y state that we had hanging off of the old frame.

I suspect we don't actually need frame reconstruction here, though -- we only need it if the ownership of some abspos descendants will change as a result.

I think we could probably get away with adding a new style change hint for position:static<-->relative changes, which would normally trigger reflow, but it would trigger frame-reconstruction instead if it detected either of the following scenarios:
 (1) We're going from position:relative to position:static, and our frame has a nonempty absolutely-positioned-child list. (So, we need to reparent these abspos kids.)

 (2) We're going from position:static to position:relative, and a traversal of our frame subtree reveals that we've got some descendants that are abspos with respect to one of our ancestors.

Scenario (2) would be a bit expensive (requiring a tree traversal, down to our nearest positioned descendants at least), but it's probably no more expensive than frame-reconstruction. It could be more expensive if we got a bunch of these changes at once (which frame-reconstruction could coalesce into a single operation but which would require N traversals with this new strategy), but we could also optimize that with a new frame state bit that we set on any frame when an abspos descendant is positioned with respect to its ancestor... or something like that. (And we could just leave it set forever, assuming the worst from then on.)

Does this sound sane?
(Reporter)

Updated

2 years ago
Summary: Don't always do frame-reconstruction for position:static <--> position:relative changes → Only do frame-reconstruction for position:static <--> position:relative changes if an abspos descendant might be affected
Note that we already do something like this for transform changes.  See the nsChangeHint_UpdateContainingBlock change hint.  In particular, nsStyleDisplay::CalcDifference has:

    NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdateContainingBlock,
                          NS_CombineHint(nsChangeHint_UpdateOverflow,
                                         nsChangeHint_RepaintFrame)));

That seems like an appropriate change hint for changes from static to relative position to me (with reflow probably added in, as you note).

That said, we may need a new changehint with similar semantics, because NeedToReframeForAddingOrRemovingTransform checks aFrame->IsRelativelyPositioned() and that's something that could be changing in our case, right?
(Reporter)

Comment 2

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)
> That said, we may need a new changehint with similar semantics, because
> NeedToReframeForAddingOrRemovingTransform checks
> aFrame->IsRelativelyPositioned() and that's something that could be changing
> in our case, right?

Yup, that's precisely what would be changing in this case.

Thanks for the feedback!
Note that for inlines that has an ib-split sibling we use a different
pseudo for the anon block depending on position:relative/static,
which might complicate things.

There are probably some frame bits that needs to be updated (e.g. see
MarkAs[Not]AbsoluteContainingBlock).
And maybe some frame properties like NormalPositionProperty.

We may want to always reconstruct some table cases since they have
special behavior attached to position:relative:
https://drafts.csswg.org/css-position-3/#valdef-position-relative
(dunno if we implement those currently though)

> Scenario (2) would be a bit expensive

It might be cheaper to check the abs.pos. list on the nearest abs.pos.
container (ideally it's empty) and see if any placeholder's ancestors
is one of our continuations.  Or perhaps just give up and reconstruct
if the list is non-empty?

We could also query nsFrameManager's placeholder map - if it's empty
then we can't possibly have any positioned children.

Updated

2 years ago
Blocks: 1256706

Updated

2 years ago
Blocks: 1277201
Hi Daniel, just curious, what is the plan here? Is there a good way forward?
Flags: needinfo?(dholbert)
(Reporter)

Comment 5

2 years ago
There is an (approximate) good way forward, described in comments above.  Just needs some spare cycles from a layout hacker to fix it.  (I'm backlogged at the moment, so I'm not diving in just yet.)
Flags: needinfo?(dholbert)
(Reporter)

Comment 6

2 years ago
(If you're asking from the perspective of mitigating bug 1256706: for the short-term, it might be best to proceed over there with something along the lines of bug 1256706 comment 7.)

Updated

2 years ago
No longer blocks: 1277201
(Reporter)

Updated

a year ago
See Also: → bug 1344377
We could also potentially (for both the transform change case handled in nsChangeHint_UpdateContainingBlock and for this case) reframe only the descendants that need it, rather than the entire frame.  There are probably some fun edge cases to work through, and it would probably deserve another bug separate from this one.

Updated

a year ago
Blocks: 1341750

Updated

a year ago
Whiteboard: [qf]
> we use a different pseudo for the anon block depending on position:relative/static

Looking at the rules in ua.css, the two share a bunch of styles and in addition ::-moz-anonymous-positioned-block has:

  position: inherit;
  top: inherit;
  left: inherit;
  bottom: inherit;
  right: inherit;
  z-index: inherit;
  clip: inherit;  

and ::-moz-anonymous-block has:

  position: static ! important;

The thing being inherited from is the inline.  So we could certainly style both as "position: inherit".  Further, the top/left/bottom/right/z-index properties only apply when position is not static.  "clip" doesn't apply to non-absolutely-positioned things at all, so I suspect it's pointless here in the first place.

So we could just switch to using a single pseudo for both.
Comment hidden (mozreview-request)
Attachment #8851851 - Attachment is obsolete: true
Attachment #8851851 - Flags: review?(dbaron)

Updated

11 months ago
Assignee: nobody → dholbert
Whiteboard: [qf] → [qf:p1]
Sounds like dholbert has done some analysis that suggests the basic optimization won't actually fix the twitter performance issue (although the stronger optimization in comment 7 may well help), but there are some easy ways that twitter could fix that issue on their side.  Probably worth both (a) contacting twitter and (b) working on both halves of the optimization in comment 7.
Flags: needinfo?(dholbert)
See Also: → bug 1342220
(Reporter)

Comment 12

9 months ago
I'm going to try to address Bug 1342220 by suggesting that Twitter change their CSS, as noted in Bug 1342220 comment 21, but if that doesn't pan out, we'll probably want to implement something like what's described in comment 7 here.
Whiteboard: [qf:p1] → [qf:p3]
(Reporter)

Updated

9 months ago
Blocks: 1351016
(Reporter)

Comment 13

9 months ago
[generalizing summary slightly, per the suggested alternative in comment 7, and also because the original optimization I was thinking of won't actually help in the real-world case that prompted this bug, as dbaron noted in comment 11]
Summary: Only do frame-reconstruction for position:static <--> position:relative changes if an abspos descendant might be affected → Optimize position:static <--> position:relative changes to avoid jank from massive frame reconstruction
(Reporter)

Comment 14

9 months ago
(In reply to Daniel Holbert [:dholbert] from comment #12)
> I'm going to [...] suggest that Twitter change their CSS,
> [...] but if that doesn't pan out, we'll probably want to
> implement something like what's described in comment 7 here.

Good news -- Twitter was receptive to my suggestion (which is tracked in bug 1368070).

So for now, I'm going to unassign this, since it's not clear that it'll make a difference on any real-world sites once Twitter is fixed. (Hopefully it's pretty rare that a site tweaks "position" on the root of a large tree.)
Assignee: dholbert → nobody
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.