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.

We'd need to still reframe if the element is an ib split, or do more fixup there.

Also, we need to handle abs-pos descendants too, not just fixed-pos.

Mats was working on this in bug 730771 :-).

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?

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.

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

I sent that comment to www-style.

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

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

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

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.

One of the bugs in this push caused bug 782196: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b753e1dce89f&tochange=94e4dbce3b94

Backout from 17.0 Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/f1e54c50517a

Backout from Aurora 18: http://hg.mozilla.org/releases/mozilla-aurora/rev/eab0a3d65b1e