The default bug view has changed. See this FAQ.

only reframe fixed-positioned descendants when whether an element has a transform changes

RESOLVED FIXED in mozilla19

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dbaron, Assigned: roc)

Tracking

(Blocks: 2 bugs, {perf})

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
OS: Linux → All
Priority: -- → P3
Hardware: x86_64 → All
We'd need to still reframe if the element is an ib split, or do more fixup there.
(Reporter)

Updated

6 years ago
Keywords: perf
(Reporter)

Updated

5 years ago
Duplicate of this bug: 730771
Also, we need to handle abs-pos descendants too, not just fixed-pos.
Mats was working on this in bug 730771 :-).
Assignee: nobody → matspal
Blocks: 776190
Assignee: matspal → roc
Created attachment 649593 [details] [diff] [review]
fix
Attachment #649593 - Flags: review?(dbaron)
https://tbpl.mozilla.org/?tree=Try&rev=a9bc37af8fe5
https://tbpl.mozilla.org/?tree=Try&rev=81710d9005fd
(Reporter)

Comment 8

5 years ago
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?
(Reporter)

Comment 9

5 years ago
Er, I guess Boris said the opposite in comment 3, but I don't know why.
(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.
(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.
(Reporter)

Comment 12

5 years ago
(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.
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.
And also, thank you very much for picking this up!
(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.
Created attachment 649864 [details] [diff] [review]
fix v2

I sent that comment to www-style.
Attachment #649593 - Attachment is obsolete: true
Attachment #649593 - Flags: review?(dbaron)
Attachment #649864 - Flags: review?(dbaron)
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.
Attachment #649864 - Attachment is obsolete: true
Attachment #649864 - Flags: review?(dbaron)
Attachment #649931 - Flags: review?(dbaron)
(Reporter)

Comment 18

5 years ago
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
Attachment #649931 - Flags: review?(dbaron) → review+
(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.
(Reporter)

Comment 20

5 years ago
(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?
Ah, of course you are correct.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89374c446c2
Backed out by:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d93599ccd6b

Please bear https://groups.google.com/d/topic/mozilla.dev.platform/nrbJE1ixkIs/discussion in mind when using inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e247827bab
https://hg.mozilla.org/mozilla-central/rev/f4e247827bab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
One of the bugs in this push caused bug 782196:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b753e1dce89f&tochange=94e4dbce3b94

Updated

5 years ago
Depends on: 804323

Comment 27

5 years ago
Backout from 17.0 Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/f1e54c50517a
Target Milestone: mozilla17 → mozilla18

Comment 28

4 years ago
Backout from Aurora 18:
http://hg.mozilla.org/releases/mozilla-aurora/rev/eab0a3d65b1e
Target Milestone: mozilla18 → mozilla19
Depends on: 816458
You need to log in before you can comment on or make changes to this bug.