Last Comment Bug 827577 - Fixed position content within a transform finds the wrong reference frame.
: Fixed position content within a transform finds the wrong reference frame.
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla21
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
http://www.starbase-alpha.com/modesF....
Depends on: 830192 830299 836990 849996
Blocks: 788044 834899
  Show dependency treegraph
 
Reported: 2013-01-07 15:47 PST by Matt Woodrow (:mattwoodrow)
Modified: 2013-03-14 17:18 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
wontfix
+
fixed
fixed


Attachments
simplified testcase (272 bytes, text/html)
2013-01-07 23:48 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
fix (13.21 KB, patch)
2013-01-09 02:18 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
v2 (13.43 KB, patch)
2013-01-09 15:33 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2013-01-07 15:47:54 PST
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) 2013-01-07 19:37:04 PST
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) 2013-01-07 19:42:44 PST
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) 2013-01-07 23:48:13 PST
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,
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-01-08 03:32:14 PST
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 2013-01-08 21:10:23 PST
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 [:bz] (still a bit busy) 2013-01-08 21:13:02 PST
> What, why how?

Comment 0 covers that ground.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-01-08 22:28:24 PST
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) 2013-01-09 02:18:58 PST
Created attachment 699644 [details] [diff] [review]
fix

This fixes the positioning of the Starbase video element.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2013-01-09 07:47:23 PST
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 [:bz] (still a bit busy) 2013-01-09 07:48:34 PST
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) 2013-01-09 15:19:25 PST
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) 2013-01-09 15:33:35 PST
Created attachment 700104 [details] [diff] [review]
v2

Added an assertion that catches the bug in my reftests, and fixed the bug.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2013-01-09 18:46:43 PST
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 2013-01-10 03:33:33 PST
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 [:bz] (still a bit busy) 2013-01-10 05:14:57 PST
> 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) 2013-01-11 05:12:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/818ed8d0886d
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-01-12 12:55:45 PST
https://hg.mozilla.org/mozilla-central/rev/818ed8d0886d
Comment 18 bhavana bajaj [:bajaj] 2013-01-14 15:35:13 PST
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) 2013-01-14 17:20:00 PST
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 [:bz] (still a bit busy) 2013-01-14 19:18:07 PST
We need to fix bug 830192.
Comment 21 Willie Meier 2013-01-14 20:14:39 PST
(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 [:bz] (still a bit busy) 2013-01-14 20:21:37 PST
It's on a need-to-know basis for now, until it's fixed...
Comment 23 Willie Meier 2013-01-14 20:28:45 PST
(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 [:bz] (still a bit busy) 2013-01-14 20:37:35 PST
It's already fixed in 21, so there's nothing to track.
Comment 25 Alex Keybl [:akeybl] 2013-01-16 10:31:37 PST
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] 2013-01-29 08:47:19 PST
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] 2013-01-30 15:08:33 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6269b3e8eef

Note You need to log in before you can comment on or make changes to this bug.