1203405 - Ignore the order of abs/fixed-pos frames when building the frame lists

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Per discussion in IRC and bug 1126230, it seems the order of abs/fixed-pos frames in their frame list is not important. The z-order of these frames is ensured by the order of their in-flow placeholders.

It seems to me that, the only place requires the correct order of those frames is nsCSSFrameConstructor::ReplicateFixedFrames(), which generates a list of placeholder frames from the fixed frame list. However, that function takes a single nsPageContentFrame* parameter which indicates this is only for printing. In that case, we should always reconstruct all frames, and the content wouldn't change dynamically. Thus that is not affected by ignoring the order.

Also, it seems we have already failed to maintain that order if we dynamically insert interleaved fixed-pos and abs-pos items to a fixed-pos containing block (those with transform/filter set).

Given these, as well as that it costs non-trivial time to choose the correct insertion point, I think we should just ignore the order of the abs/fixed-pos frame list.

Try run shows this change works fine with existing tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67f8672fa168

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1203405 - Ignore order of abs/fixed-pos frames in the frame list.

Bug 1203405 - Ignore order of abs/fixed-pos frames in the frame list.

Comment 2

[Mats Palmgren (inactive)]

Hmm, doesn't our BuildDisplayList code make assumptions about items being in
document order in various places? (as a side-effect of processing frames
in frame list order which is assumed to be document order)
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=e637f3ff6f3e#2068

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #2)
> Hmm, doesn't our BuildDisplayList code make assumptions about items being in
> document order in various places? (as a side-effect of processing frames
> in frame list order which is assumed to be document order)
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.
> cpp?rev=e637f3ff6f3e#2068

Yes, it should have already been in the document order at this point.

But the display lists of abs/fixed-pos frames are built when we visit their in-flow placeholders. We do not traverse the abs/fixed frame list for building the display list.

See https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/generic/nsFrame.cpp#2364

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659062 [details]
MozReview Request: Bug 1203405 - Ignore order of abs/fixed-pos frames in the frame list.

Robert is probably a better reviewer here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

But some things worth testing or checking on are:

1)  Tabbing through things.
2)  Accessibility behavior.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

It seems to me that the frame iterator traverses the frame in order of in-flow placeholder as well. See https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/nsFrameTraversal.cpp#380

This impl may have issue if it starts at an out-of-flow frame, but set FollowOOFs to false. But it isn't the case for tabbing at least: https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/dom/base/nsFocusManager.cpp#2888

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8659062 [details]
MozReview Request: Bug 1203405 - Ignore order of abs/fixed-pos frames in the frame list.

https://reviewboard.mozilla.org/r/18743/#review16865

The code looks OK but it's a scary change.

I pretty sure this will change tabbing behavior (see nsFocusManager::GetNextTabbableContent). The exact tabbing order doesn't matter too much, but it is important that the order be consistent and independent of the history of style changes, and I suspect we're not going to have that here.

I can't see any other problems right now, though.

If there's no urgency to ship top-layer I suggest we land this change at the start of the next release cycle to maximise the time we have to detect regressions.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 8659062 [details]
> MozReview Request: Bug 1203405 - Ignore order of abs/fixed-pos frames in the
> frame list.
> 
> https://reviewboard.mozilla.org/r/18743/#review16865
> 
> The code looks OK but it's a scary change.
> 
> I pretty sure this will change tabbing behavior (see
> nsFocusManager::GetNextTabbableContent). The exact tabbing order doesn't
> matter too much, but it is important that the order be consistent and
> independent of the history of style changes, and I suspect we're not going
> to have that here.

It won't. See comment 6.

> I can't see any other problems right now, though.
> 
> If there's no urgency to ship top-layer I suggest we land this change at the
> start of the next release cycle to maximise the time we have to detect
> regressions.

It's probably urgency. If we do not ship top-layer at this version, we probably need to also backout bug 1179288 in this version for bug 1201078 (which already has a duplicate now).

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

It could affect other usage of FrameTraversal, like Selection and TypeAheadFind. They set FollowOOFs to false, which could be problematic. It would need further investigation.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Well, but using the frame iterator without FollowOOFs starting from an out-of-flow frame should already have a weird behavior, and thus that usage should be avoided anyway.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

BTW, I don't really think this should block shipping top-layer, though. Since the order may have already been broken, I don't believe making top-layer frames even more out-of-order could be a significant regression.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8659062 [details]
MozReview Request: Bug 1203405 - Ignore order of abs/fixed-pos frames in the frame list.

So the version has been 44 now, may I land it now?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/18743/#review18001

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8daf4b09b89ca4b59b803f39a9ad18f6e42f6557
Bug 1203405 - Ignore order of abs/fixed-pos frames in the frame list. r=roc

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8daf4b09b89c