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

RESOLVED FIXED in Firefox 20

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: roc)

Tracking

({regression})

unspecified
mozilla21
x86
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ wontfix, firefox20+ fixed, firefox21 fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
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!
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
Created attachment 699019 [details]
simplified testcase

The yellow DIV should be at the top right of the black-border DIV, but it's not visible at all,
Assignee: nobody → roc
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

5 years ago
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
> What, why how?

Comment 0 covers that ground.
I finally have a patch for this. Just need to clean it up a bit and write tests. There were actually many bugs here...
Created attachment 699644 [details] [diff] [review]
fix

This fixes the positioning of the Starbase video element.
Attachment #699644 - Flags: review?(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?
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?
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.
Created attachment 700104 [details] [diff] [review]
v2

Added an assertion that catches the bug in my reftests, and fixed the bug.
Attachment #700104 - Flags: review?(bzbarsky)
Attachment #699644 - Attachment is obsolete: true
Attachment #699644 - Flags: review?(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.
Attachment #700104 - Flags: review?(bzbarsky) → review+

Comment 14

5 years ago
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
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/818ed8d0886d
https://hg.mozilla.org/mozilla-central/rev/818ed8d0886d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 830192
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
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 .
status-firefox19: --- → affected
status-firefox20: --- → affected
tracking-firefox19: ? → +
tracking-firefox20: ? → +
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.
Attachment #700104 - Flags: approval-mozilla-beta?
Attachment #700104 - Flags: approval-mozilla-aurora?
We need to fix bug 830192.

Comment 21

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

any chance of being able to look at #830192
It's on a need-to-know basis for now, until it's fixed...

Comment 23

5 years ago
(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.
It's already fixed in 21, so there's nothing to track.
Depends on: 830299

Updated

5 years ago
Depends on: 831335

Updated

5 years ago
status-firefox18: --- → wontfix
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.
Attachment #700104 - Flags: approval-mozilla-beta?
Attachment #700104 - Flags: approval-mozilla-beta+
Attachment #700104 - Flags: approval-mozilla-aurora?
Attachment #700104 - Flags: approval-mozilla-aurora+
Whiteboard: DO NOT LAND ON BRANCHES WITHOUT FIXING DEPENDENCIES
Depends on: 830138
status-firefox21: --- → fixed
Blocks: 834899

Updated

5 years ago
Keywords: regression

Updated

5 years ago
Depends on: 835056
No longer depends on: 835056

Updated

5 years ago
No longer depends on: 831335

Updated

5 years ago
No longer depends on: 830138
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.
Attachment #700104 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-

Updated

5 years ago
status-firefox19: affected → wontfix
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6269b3e8eef
status-firefox20: affected → fixed
Whiteboard: DO NOT LAND ON BRANCHES WITHOUT FIXING DEPENDENCIES
Depends on: 836990
(Reporter)

Updated

5 years ago
Depends on: 849996
You need to log in before you can comment on or make changes to this bug.