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

Status: RESOLVED DUPLICATE of bug 1611462

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Daniel Holbert [:dholbert]]

(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!

Comment 3

[Mats Palmgren (inactive)]

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.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Daniel, just curious, what is the plan here? Is there a good way forward?

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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 9

[Boris Zbarsky [:bzbarsky]]

Filed bug 1351139 on comment 8.  Patch coming up there.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1261484. Use only one kind of anonymous box for the block wrappers we create for a block-inside-inline split.

Review commit: https://reviewboard.mozilla.org/r/124054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124054/

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[Daniel Holbert [:dholbert]]

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.

Comment 13

[Daniel Holbert [:dholbert]]

[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]

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

We do this, see bug 1611462.