Last Comment Bug 691651 - only reframe fixed-positioned descendants when whether an element has a transform changes
: only reframe fixed-positioned descendants when whether an element has a trans...
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 normal with 2 votes (vote)
: mozilla19
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
: 730771 (view as bug list)
Depends on: 804323 816458
Blocks: 641340 776190
  Show dependency treegraph
 
Reported: 2011-10-03 20:21 PDT by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2012-11-29 10:02 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (11.07 KB, patch)
2012-08-07 03:59 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v2 (11.33 KB, patch)
2012-08-07 16:27 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v3 (11.44 KB, patch)
2012-08-07 19:17 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-10-03 20:21:32 PDT
When an element changes between having a transform and not having a transform, we currently reframe the element because we need to reparent any fixed-positioned descendants of the element.

As I suggested in bug 641340 comment 21, we should instead search for fixed-positioned descendants (or maybe check ancestor fixed-pos lists first, though probably not) and just reframe those, since there usually aren't any.  This will avoid the need to reframe.

(Bug 524925 will also avoid the need to reflow.)

This requires changing nsStyleDisplay::CalcDifference, adding a new change hint to nsChangeHint.h, and changing nsCSSFrameConstructor::ProcessRestyledFrames to process that change hint.

Before doing this, however, we should double-check that there isn't anything else that needs to be changed when HasTransform() changes.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-10-03 20:31:53 PDT
We'd need to still reframe if the element is an ib split, or do more fixup there.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-02-27 07:15:14 PST
*** Bug 730771 has been marked as a duplicate of this bug. ***
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-02-27 07:22:14 PST
Also, we need to handle abs-pos descendants too, not just fixed-pos.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-28 06:07:01 PST
Mats was working on this in bug 730771 :-).
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 03:59:41 PDT
Created attachment 649593 [details] [diff] [review]
fix
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 04:01:22 PDT
https://tbpl.mozilla.org/?tree=Try&rev=a9bc37af8fe5
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 15:09:23 PDT
https://tbpl.mozilla.org/?tree=Try&rev=81710d9005fd
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-08-07 15:21:02 PDT
Why is this a non-inherited hint?  Since it involves a walk over descendants, it seems like any hint on an ancestor subsumes the same hint on a descendant.

(Also, you want the version where FrameHasAbsPosPlaceholderDescendants calls itself rather than a nonexistent function with similar name.)

Also, why doesn't this restrict the search to just position:fixed descendants, which are likely to be a lot rarer than anything that's absolutely positioned?
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-08-07 15:23:45 PDT
Er, I guess Boris said the opposite in comment 3, but I don't know why.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 15:46:04 PDT
(In reply to David Baron [:dbaron] from comment #8)
> Why is this a non-inherited hint?  Since it involves a walk over
> descendants, it seems like any hint on an ancestor subsumes the same hint on
> a descendant.

The FRAME_IS_SPECIAL check means that we might decide that an ancestor with nsChangeHint_AddOrRemoveTransform doesn't need to be reframed, while a descendant has FRAME_IS_SPECIAL and would need to be reframed, so we'd have a bug if the descendant's hint was thrown away.

Also, if we allow a descendant's hint to be thrown away, that frame won't have NS_FRAME_MAY_BE_TRANSFORMED set on it.

> (Also, you want the version where FrameHasAbsPosPlaceholderDescendants calls
> itself rather than a nonexistent function with similar name.)

Yes, fixed in the latest try push :-).

> Also, why doesn't this restrict the search to just position:fixed
> descendants, which are likely to be a lot rarer than anything that's
> absolutely positioned?

Because setting a 'transform' makes the frame a container for abs-pos children as well as rel-pos, so we might have to reframe to collect abs-pos children.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 15:46:41 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Because setting a 'transform' makes the frame a container for abs-pos
> children as well as rel-pos,

I meant "as well as fixed-pos", of course.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-08-07 15:55:25 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> The FRAME_IS_SPECIAL check means that we might decide that an ancestor with
> nsChangeHint_AddOrRemoveTransform doesn't need to be reframed, while a
> descendant has FRAME_IS_SPECIAL and would need to be reframed, so we'd have
> a bug if the descendant's hint was thrown away.

Why do we need to reframe for NS_FRAME_IS_SPECIAL?  Is it just because there might be frames to look for in the special siblings, or is it for some other reason?  If the former, why wouldn't the search on an ancestor be sufficient?  (I guess this is mostly irrelevant due to the next point.)

> Also, if we allow a descendant's hint to be thrown away, that frame won't
> have NS_FRAME_MAY_BE_TRANSFORMED set on it.

Ah, true.  Ok, so it does need to be non-inherited.

> > Also, why doesn't this restrict the search to just position:fixed
> > descendants, which are likely to be a lot rarer than anything that's
> > absolutely positioned?
> 
> Because setting a 'transform' makes the frame a container for abs-pos
> children as well as rel-pos, so we might have to reframe to collect abs-pos
> children.

If this is the case, shouldn't we send a comment on the spec saying that the spec should say so.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-08-07 16:02:21 PDT
We might be able to optimize out looking for abs-pos kids inside a frame that is itself an absolute containing block.  We'd still need to look for fixed-pos in there, though, so we may not care too much about this case.

What we can _definitely_ do is not walk into any transformed descendants.  Not sure whether it matters.

I'm not sure I follow the reasons for the FRAME_IS_SPECIAL check.  Why is that needed?

Probably worth documenting that we set the bit because normally frame construction would do it but we're not planning to reframe.

Also worth checking that we don't have nsChangeHint_ReconstructFrame already before we dive into FrameHasAbsPosPlaceholderDescendants.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-08-07 16:04:31 PDT
And also, thank you very much for picking this up!
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 16:09:42 PDT
(In reply to David Baron [:dbaron] from comment #12)
> Why do we need to reframe for NS_FRAME_IS_SPECIAL?  Is it just because there
> might be frames to look for in the special siblings, or is it for some other
> reason?  If the former, why wouldn't the search on an ancestor be
> sufficient?  (I guess this is mostly irrelevant due to the next point.)

I put it in because of comment #1 and because if the frame has special siblings, we would need to search them to see if they need to become abs-pos containing blocks. Since transforms don't apply to inlines any more, I guess it's not needed.

> > Because setting a 'transform' makes the frame a container for abs-pos
> > children as well as rel-pos, so we might have to reframe to collect abs-pos
> > children.
> 
> If this is the case, shouldn't we send a comment on the spec saying that the
> spec should say so.

Yes.

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #13)
> We might be able to optimize out looking for abs-pos kids inside a frame
> that is itself an absolute containing block.  We'd still need to look for
> fixed-pos in there, though, so we may not care too much about this case.
> 
> What we can _definitely_ do is not walk into any transformed descendants. 
> Not sure whether it matters.

Right, I have no evidence that making this check any more complex actually matters.

> Probably worth documenting that we set the bit because normally frame
> construction would do it but we're not planning to reframe.

OK.

> Also worth checking that we don't have nsChangeHint_ReconstructFrame already
> before we dive into FrameHasAbsPosPlaceholderDescendants.

Sure.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 16:27:11 PDT
Created attachment 649864 [details] [diff] [review]
fix v2

I sent that comment to www-style.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-07 19:17:31 PDT
Created attachment 649931 [details] [diff] [review]
fix v3

One assertion happened in reftests --- passing nsChangeHint_UpdateTransformLayer for a frame that is (no longer) transformed. Fixed by not applying nsChangeHint_UpdateTransformLayer when switching between HasTransform() and !HasTransform(). There's no need since nsChangeHint_RepaintFrame does everything nsChangeHint_UpdateTransformLayer would do here.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-08-09 12:10:21 PDT
Comment on attachment 649931 [details] [diff] [review]
fix v3

>+    nsFrameList::Enumerator childFrames(lists.CurrentList());
>+    for (; !childFrames.AtEnd(); childFrames.Next()) {

Declare childFrames inside the for?

>+      // Look through placeholder frames to their out-of-flows

This comment is misleading, since you're looking at the out-of-flow, but you're not traversing its descendants.  I think this is ok since inlines aren't transformable, and they lead to the only case where you'd need to walk through out-of-flows (inline containing a float containing a fixed-pos).

(Or, alternatively, you could walk through placeholders and skip walking into out-of-flows, but I don't *think* there's a need to.)

>+    if ((hint & nsChangeHint_AddOrRemoveTransform) &&
>+        !(hint & nsChangeHint_ReconstructFrame)) {
>+      if (!frame || FrameHasAbsPosPlaceholderDescendants(frame)) {
>+        NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
>+      } else {
>+        // We can just add this state bit unconditionally, since it's
>+        // conservative. Normally frame construction would set this if needed,
>+        // but we're not going to reconstruct the frame so we need to set it.
>+        frame->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
>+      }
>+    }

Why not just move the null-check of frame to the outer if (as && frame)?

Also, maybe point out above the AddStateBits that the AddStateBits is one of the reasons the hint needs to be non-inherited?

r=dbaron
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-09 17:20:32 PDT
(In reply to David Baron [:dbaron] from comment #18)
> (Or, alternatively, you could walk through placeholders and skip walking
> into out-of-flows, but I don't *think* there's a need to.)

I actually think that's simpler, doesn't rely on complex logic for correctness. I'll do it.

> >+    if ((hint & nsChangeHint_AddOrRemoveTransform) &&
> >+        !(hint & nsChangeHint_ReconstructFrame)) {
> >+      if (!frame || FrameHasAbsPosPlaceholderDescendants(frame)) {
> >+        NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
> >+      } else {
> >+        // We can just add this state bit unconditionally, since it's
> >+        // conservative. Normally frame construction would set this if needed,
> >+        // but we're not going to reconstruct the frame so we need to set it.
> >+        frame->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
> >+      }
> >+    }
> 
> Why not just move the null-check of frame to the outer if (as && frame)?

We need to add nsChangeHint_ReconstructFrame if frame is null, so we can't skip the whole thing if frame is null.

> Also, maybe point out above the AddStateBits that the AddStateBits is one of
> the reasons the hint needs to be non-inherited?

Sure.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-08-09 17:32:57 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> We need to add nsChangeHint_ReconstructFrame if frame is null, so we can't
> skip the whole thing if frame is null.

Why?  If we need to construct a frame and there isn't one now, wouldn't something else have led to an nsChangeHint_ReconstructFrame hint?
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-09 17:51:35 PDT
Ah, of course you are correct.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-10 04:23:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89374c446c2
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-12 22:21:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e247827bab
Comment 25 Ed Morley [:emorley] 2012-08-13 11:11:33 PDT
https://hg.mozilla.org/mozilla-central/rev/f4e247827bab
Comment 27 Scoobidiver (away) 2012-10-30 05:15:57 PDT
Backout from 17.0 Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/f1e54c50517a
Comment 28 Scoobidiver (away) 2012-11-08 01:12:57 PST
Backout from Aurora 18:
http://hg.mozilla.org/releases/mozilla-aurora/rev/eab0a3d65b1e

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