position:fixed items disappear due to wrong translation

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tatiana, Assigned: tatiana)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 607417
(Assignee)

Comment 1

6 years ago
Created attachment 573319 [details]
expected: image1
Assignee: nobody → tanya.meshkova
(Assignee)

Comment 2

6 years ago
Created attachment 573320 [details]
actual: image2
(Assignee)

Comment 3

6 years ago
Created attachment 573358 [details] [diff] [review]
patch: accumulate CSS transforms only
Attachment #573358 - Flags: review?(roc)
(Assignee)

Comment 4

6 years ago
Created attachment 573359 [details] [diff] [review]
reftest
Attachment #573318 - Attachment is obsolete: true
Attachment #573359 - Flags: review?(roc)
Can you explain this patch a bit more?
(Assignee)

Comment 6

6 years ago
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.
Attachment #573359 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 8

6 years ago
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.
Attachment #573358 - Attachment is obsolete: true
Attachment #573358 - Flags: review?(roc)
Attachment #573717 - Flags: review?(roc)
Comment on attachment 573717 [details] [diff] [review]
patch v2

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

OK great!
Attachment #573717 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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
https://tbpl.mozilla.org/php/getParsedLog.php?id=7473745&tree=Mozilla-Inbound#error0
(In reply to Oleg Romashin (:romaxa) from comment #10)
> will backout for now

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee570616da4c
Keywords: checkin-needed
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
Created attachment 576353 [details] [diff] [review]
reftest v2

https://tbpl.mozilla.org/php/getParsedLog.php?id=7534432&tree=Try&full=1
Attachment #573359 - Attachment is obsolete: true
Attachment #576353 - Flags: review?(roc)
(Assignee)

Comment 15

6 years ago
Created attachment 576354 [details] [diff] [review]
patch v3

moving by GetBounds().TopLeft()
Attachment #573717 - Attachment is obsolete: true
Attachment #576354 - Flags: review?(roc)
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?
(Assignee)

Comment 17

6 years ago
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).
(Assignee)

Comment 18

6 years ago
Created attachment 577491 [details] [diff] [review]
patch v4
Attachment #577491 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #576354 - Attachment is obsolete: true
Attachment #576354 - Flags: review?(roc)
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.
(Assignee)

Comment 20

6 years ago
Created attachment 577887 [details] [diff] [review]
patch v4.1
Attachment #577491 - Attachment is obsolete: true
Attachment #577491 - Flags: review?(roc)
Attachment #577887 - Flags: review?(roc)
Attachment #577887 - Flags: review?(roc) → review+
Attachment #576353 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/b87861e50640
https://hg.mozilla.org/mozilla-central/rev/8749089face4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.