827577 - Fixed position content within a transform finds the wrong reference frame.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

When trying to find the reference frame for content, we walk up the frame tree looking for transformed or the root.

If the frame is within the 'FixedList' of the frame tree, but still appears within a transform in the display list, then we find the wrong reference frame.

STR: Load the link, and then click on one of the image to get a single playing video. Positioning is wrong compared to previous behaviour.

Regression from bug 788044.

Frame tree for the page: http://pastebin.mozilla.org/2044356
Display list: http://pastebin.mozilla.org/2044358

The important part is that the nsDisplayVideo is within the nsDisplayTransform(0x11d934ab8), yet still has ref=0x111bc8420 (the viewport frame).

Is there any better way to walk up the display list parent tree, instead of the frame tree since this appears to be incorrect?

We could make nsDisplayListBuilder keep a stack, or make nsDisplayItems track a parent pointer.

Comment 1

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

If the 0x11d934ab8 <section> element is transformed, and the <video> is a position:fixed child of that element, then the nsHTMLVideoFrame should be an abs-pos child of the <section>'s nsBlockFrame's absolute-children!

Comment 2

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

Yeah, so the problem in that frame tree is
-- frame 0x11d934ab8 is transformed
-- it contains abs-pos child frame 0x11d937b70
-- which contains child frame 0x11d93d808
-- which contains the placeholder for fixed-pos frame 0x11d93d9c8
-- but 0x11d93d9c8 has been placed on the fixed-pos-child list of the viewport, when it should be on the abs-pos list of 0x11d934ab8

Comment 3

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

Attachment: simplified testcase

The yellow DIV should be at the top right of the black-border DIV, but it's not visible at all,

Comment 4

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

Basically the bug here is that we handle position:fixed children of transformed elements by having GetFixedItems() return the abs-pos item list for the transformed element, depending on the value of the mFixedPosIsAbsPos flag. But that doesn't work when there's an abs-pos element between the fixed-pos element and the transformed element. In that case, mFixedPosIsAbsPos will be set to false when we PushAbsoluteContainingBlock for the intervening abs-pos element, so fixed-pos elements go straight to the viewport.

Comment 5

[Willie Meier]

This anomaly now exits in FF21.

What is mystifying is that it is not manifested in FF17.0.1 yet it is in FF versions that follow 18, 19, 20 and 21.

One can thus reasonably assume that this anomaly was introduced in FF18 and carried over to the  others.  What, why how?

Would not looking at FF17.0.1 and comparing it to FF18 shed any light to the anomaly.

The anomaly has far reaching affects

Comment 6

[Boris Zbarsky [:bzbarsky]]

> What, why how?

Comment 0 covers that ground.

Comment 7

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

I finally have a patch for this. Just need to clean it up a bit and write tests. There were actually many bugs here...

Comment 8

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

Attachment: fix

This fixes the positioning of the Starbase video element.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So with that setup, when we push an absolute containing block while inside a transformed element we:

1)  Save the state's mAbsoluteItems.
1)  Save the state's mFixedItems.
2)  Copy the current absolute items (for the transformed element) to the state's
    mFixedItems.

Then we go ahead and deal with the kid, all the while putting its absolute stuff in the new mAbsoluteItems and putting its fixed stuff in mFixedItems (which is now the list of fixed/absolute stuff in the transformed element).

Then we finish up with the kid, process its mAbsoluteItems, restore the old mAbsoluteItems, and restore the old mFixedItems.

What I'm confused about is what happens to the frames that got added to mFixedItems in the meantime.  Don't they get lost?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I feel like I must be missing something, because it looks like fixed pos inside abs pos inside transformed, with this patch, should just lose the fixed pos frame...  But clearly comment 8 says that isn't happening.  Why not?

Comment 11

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

Right, there is a bug here.

The Starbase testcase is working because the fixed-pos element is inserted dynamically and so we don't go through PushAbsoluteContainingBlock for its ancestors.

My reftest "1a" is working for a more subtle reason. mAbsoluteItems is non-empty when we call PushAbsoluteContainingBlock for the abs-pos child of the transformed element --- it contains the abs-pos child itself, at least. So we copy that to mFixedItems and later append the fixed-pos frame to the list. Later we restore mAbsoluteItems, which restores the first-child, last-child and containing block pointers. So starting from the first-child and traversing next-siblings we actually find the new fixed-pos frame that was appended. The last-child pointer of the mAbsoluteItems will be wrong though.

Comment 12

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

Attachment: v2

Added an assertion that catches the bug in my reftests, and fixed the bug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 700104 [details] [diff] [review]
v2

Yes, this I buy.  :)

Do we need debug-only Clear() calls like we have for mSavedItems?  I'd think we do...  r=me with that.

Comment 14

[Willie Meier]

Here is a 64,000$ question(s)

1. why did this not present itself in FF17 while present in FF18 --->FF21?

2. once the fix has been established, verified when will it be implemented?

Approx 2million+ users of the JWPlayer are affected by this anomaly
a maintenance release for FF18 or

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 1. why did this not present itself in FF17 while present in FF18 --->FF21?

Please read comment 0.  You already asked this once, and got an answer...

> 2. once the fix has been established, verified when will it be implemented?

I would guess Firefox 19.  This is certainly affecting other users of transforms, but it doesn't rise to the level at which we would usually do a chemspill so far.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/818ed8d0886d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/818ed8d0886d

Comment 18

[bhavana bajaj [:bajaj]]

Tracking for Fx19,Fx20 as this is a Fx18 regression . Please nominate for uplift at the earliest when ready to get good testing on aurora/beta .

Comment 19

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

Comment on attachment 700104 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 788044
User impact if declined: broken rendering of some sites
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): non-negligible risk, but not great risk. Anyway, it seems worth fixing.
String or UUID changes made by this patch: None.

Comment 20

[Boris Zbarsky [:bzbarsky]]

We need to fix bug 830192.

Comment 21

[Willie Meier]

(In reply to Boris Zbarsky (:bz) from comment #20)
> We need to fix bug 830192.

any chance of being able to look at #830192

Comment 22

[Boris Zbarsky [:bzbarsky]]

It's on a need-to-know basis for now, until it's fixed...

Comment 23

[Willie Meier]

(In reply to bhavana bajaj [:bajaj] from comment #18)
> Tracking for Fx19,Fx20 as this is a Fx18 regression . Please nominate for
> uplift at the earliest when ready to get good testing on aurora/beta .

FF21 is/was also affected thus is tracking also needed.

Comment 24

[Boris Zbarsky [:bzbarsky]]

It's already fixed in 21, so there's nothing to track.

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 700104 [details] [diff] [review]
v2

This is FF18 cleanup impacting sites in the wild, and will make it into beta 3 when landed this week. Approving for branches.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 700104 [details] [diff] [review]
v2

Since this code ended up being regression prone, we'll wontfix for FF19 and instead uplift to FF20 with associated fixes.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6269b3e8eef