Last Comment Bug 701190 - position:fixed items disappear due to wrong translation
: position:fixed items disappear due to wrong translation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Tatiana Meshkova (:tatiana)
:
Mentors:
Depends on:
Blocks: 607417
  Show dependency treegraph
 
Reported: 2011-11-09 14:09 PST by Tatiana Meshkova (:tatiana)
Modified: 2011-12-27 22:31 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reftest (2.25 KB, patch)
2011-11-09 14:09 PST, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Review
expected: image1 (7.71 KB, text/plain)
2011-11-09 14:15 PST, Tatiana Meshkova (:tatiana)
no flags Details
actual: image2 (7.67 KB, text/plain)
2011-11-09 14:16 PST, Tatiana Meshkova (:tatiana)
no flags Details
patch: accumulate CSS transforms only (5.92 KB, patch)
2011-11-09 16:07 PST, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Review
reftest (2.47 KB, patch)
2011-11-09 16:09 PST, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Review
patch v2 (5.04 KB, patch)
2011-11-10 18:14 PST, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Review
reftest v2 (2.29 KB, patch)
2011-11-22 17:03 PST, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Review
patch v3 (5.08 KB, patch)
2011-11-22 17:05 PST, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Review
patch v4 (5.18 KB, patch)
2011-11-28 21:59 PST, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Review
patch v4.1 (5.51 KB, patch)
2011-11-30 00:39 PST, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Review

Description Tatiana Meshkova (:tatiana) 2011-11-09 14:09:12 PST
Created attachment 573318 [details] [diff] [review]
reftest

PRECONDITIONS: 
export MOZ_ENABLE_FIXED_POSITION_LAYERS=1

STEPS LEADING TO PROBLEM: 
1. apply reftest patch and run test-pos-fixed-transform

EXPECTED OUTCOME:
pass

ACTUAL OUTCOME:
fail
Comment 1 Tatiana Meshkova (:tatiana) 2011-11-09 14:15:56 PST
Created attachment 573319 [details]
expected: image1
Comment 2 Tatiana Meshkova (:tatiana) 2011-11-09 14:16:50 PST
Created attachment 573320 [details]
actual: image2
Comment 3 Tatiana Meshkova (:tatiana) 2011-11-09 16:07:33 PST
Created attachment 573358 [details] [diff] [review]
patch: accumulate CSS transforms only
Comment 4 Tatiana Meshkova (:tatiana) 2011-11-09 16:09:16 PST
Created attachment 573359 [details] [diff] [review]
reftest
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-09 17:38:40 PST
Can you explain this patch a bit more?
Comment 6 Tatiana Meshkova (:tatiana) 2011-11-09 17:58:02 PST
Currently TransformRectToBoundsInAncestor() uses nsIFrame::GetTransformMatrix(), that includes frame offset translation. This translation moves display port far away from fixed item bounds.
In order to have item's bounds and displayport in the same coordinate system, we need to apply CSS transforms only.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-09 18:22:53 PST
OK, that makes sense.

I don't know why you've removed the fixed-background path. Your "if (!frame)" check is not going to catch all fixed backgrounds, in fact, it's not going to catch any since fixed-backgrounds are attached to nsBlockFrames etc, not the nsViewportFrame.

Seems to me the simplest fix would be to use TransformRectToBoundsInAncestor (terribly named, actually), but recognize that it returns a rectangle relative to 'frame', to add ToReferenceFrame() to the result.
Comment 8 Tatiana Meshkova (:tatiana) 2011-11-10 18:14:15 PST
Created attachment 573717 [details] [diff] [review]
patch v2

There is no need to handle fixed backgrounds exceptionally.
TransformRectToBoundsInAncestor + ToReferenceFrame correction always gives an identity transform for them.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-10 18:22:20 PST
Comment on attachment 573717 [details] [diff] [review]
patch v2

Review of attachment 573717 [details] [diff] [review]:
-----------------------------------------------------------------

OK great!
Comment 10 Oleg Romashin (:romaxa) 2011-11-18 13:46:45 PST
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/reftest-sanity/test-pos-fixed-transform.html | image comparison (==) 
will backout for now
Comment 12 Ed Morley [:emorley] 2011-11-18 14:00:51 PST
(In reply to Oleg Romashin (:romaxa) from comment #10)
> will backout for now

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee570616da4c
Comment 13 Tatiana Meshkova (:tatiana) 2011-11-22 17:00:52 PST
Comment on attachment 573717 [details] [diff] [review]
patch v2

>+  nsRect result = nsLayoutUtils::TransformRectToBoundsInAncestor(
>+                    frame,
>+                    nsRect(0, 0, displayport->width, displayport->height),
>+                    aBuilder->ReferenceFrame());
>+  result.MoveBy(aBuilder->ToReferenceFrame(frame));

oh, actually ToReferenceFrame is not good enough for nsDisplayTransform items.
Comment 15 Tatiana Meshkova (:tatiana) 2011-11-22 17:05:02 PST
Created attachment 576354 [details] [diff] [review]
patch v3

moving by GetBounds().TopLeft()
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-22 18:23:00 PST
Comment on attachment 576354 [details] [diff] [review]
patch v3

Review of attachment 576354 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +455,5 @@
> +  nsRect result = nsLayoutUtils::TransformRectToBoundsInAncestor(
> +                    frame,
> +                    nsRect(0, 0, displayport->width, displayport->height),
> +                    aBuilder->ReferenceFrame());
> +  result.MoveBy(aItem->GetBounds(aBuilder).TopLeft());

This can't be right. The top-left of the display item bounds isn't related to any coordinate system.

Can you explain what the problem was with the previous patch?
Comment 17 Tatiana Meshkova (:tatiana) 2011-11-28 21:54:18 PST
OK, I see.
yes, I can explain.

Let's say we have.
<body>
<div style="position: fixed; background: lightblue; top: 0; left: 0; width:100px; height:100px; -moz-transform: translate(360px,0px);"/>
</body>

We'll get the following display list:

CanvasBackground 0x92c1d70(Canvas(html)(-1)) (0,0,48000,18000)(0,0,48000,18000) opaque uniform
nsDisplayTransform 0x9273e48(Block(div)(1)) (21600,0,6000,6000)(21600,0,4800,6000) opaque
    Background 0x9273e48(Block(div)(1)) (0,0,6000,6000)(0,0,6000,6000) opaque uniform

Let's check how do we calculate mVisibleRect for nsDisplayTransform with v2 patch.
GetBounds() for nsDisplayTransform is (x=21600,y=0,w=6000,h=6000).
ToReferenceFrame() for nsDisplayTransform is (x=0, y=0).
If displayPort is (x=0, y=0, w=48000, h=18000), then v2 of GetDisplayPortBounds(nsDisplayTransform) returns (x=-21600, y=0, w=48000, h=18000),
that is wrong. It should be (x=0, y=0, w=48000, h=18000).
So, mVisibleRect is (x=21600, y=0, w=4800, h=6000) instead of (x=21600, y=0, w=6000, h=6000).
Comment 18 Tatiana Meshkova (:tatiana) 2011-11-28 21:59:05 PST
Created attachment 577491 [details] [diff] [review]
patch v4
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-29 15:18:15 PST
I see. The problem is that the visible rect for the nsDisplayTransform needs to not take that display item's transform into account, but the visible rect for nsLayoutUtils::TransformRectToBoundsInAncestor does take the transform into account. Please add a comment about that.
Comment 20 Tatiana Meshkova (:tatiana) 2011-11-30 00:39:54 PST
Created attachment 577887 [details] [diff] [review]
patch v4.1

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