701190 - position:fixed items disappear due to wrong translation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tatiana Meshkova (:tatiana)]

Attachment: 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)]

Attachment: expected: image1

Comment 2

[Tatiana Meshkova (:tatiana)]

Attachment: actual: image2

Comment 3

[Tatiana Meshkova (:tatiana)]

Attachment: patch: accumulate CSS transforms only

Comment 4

[Tatiana Meshkova (:tatiana)]

Attachment: reftest

Comment 5

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

Can you explain this patch a bit more?

Comment 6

[Tatiana Meshkova (:tatiana)]

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) (email my personal email if necessary)]

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)]

Attachment: 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) (email my personal email if necessary)]

Comment on attachment 573717 [details] [diff] [review]
patch v2

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

OK great!

Comment 10

[Oleg Romashin (:romaxa)]

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 11

[Oleg Romashin (:romaxa)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=7473745&tree=Mozilla-Inbound#error0

Comment 12

[Ed Morley [:emorley]]

(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)]

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 14

[Tatiana Meshkova (:tatiana)]

Attachment: reftest v2

https://tbpl.mozilla.org/php/getParsedLog.php?id=7534432&tree=Try&full=1

Comment 15

[Tatiana Meshkova (:tatiana)]

Attachment: patch v3

moving by GetBounds().TopLeft()

Comment 16

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

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)]

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)]

Attachment: patch v4

Comment 19

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

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)]

Attachment: patch v4.1

Comment 21

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/mozilla-central/rev/b87861e50640
https://hg.mozilla.org/mozilla-central/rev/8749089face4