Closed Bug 701190 Opened 13 years ago Closed 13 years ago

position:fixed items disappear due to wrong translation

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: tatiana, Assigned: tatiana)

References

Details

Attachments

(4 files, 6 obsolete files)

expected: image1
(deleted), text/plain
Details
actual: image2
(deleted), text/plain
Details
reftest v2
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
patch v4.1
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Attached patch reftest (obsolete) (deleted) — — Splinter Review
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
Blocks: 607417
Attached file expected: image1 (deleted) —
Assignee: nobody → tanya.meshkova
Attached file actual: image2 (deleted) —
Attached patch patch: accumulate CSS transforms only (obsolete) (deleted) — — Splinter Review
Attachment #573358 - Flags: review?(roc)
Attached patch reftest (obsolete) (deleted) — — Splinter Review
Attachment #573318 - Attachment is obsolete: true
Attachment #573359 - Flags: review?(roc)
Can you explain this patch a bit more?
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.
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.
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review
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+
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
(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
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.
Attached patch patch v3 (obsolete) (deleted) — — Splinter Review
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?
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).
Attached patch patch v4 (obsolete) (deleted) — — Splinter Review
Attachment #577491 - Flags: review?(roc)
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.
Attached patch patch v4.1 (deleted) — — Splinter Review
Attachment #577491 - Attachment is obsolete: true
Attachment #577491 - Flags: review?(roc)
Attachment #577887 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: