Closed
Bug 1303408
Opened 8 years ago
Closed 8 years ago
Scrolling over the position:absolute element incorrectly scrolls the scrollframe underneath
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: mstange, Assigned: rhunt)
References
Details
(Keywords: regression, Whiteboard: [gfx-noted])
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jcristau
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
See the testcase. Scrolling over the red box should not scroll the scroll frame with the gradient. Adding a z-index to the position:absolute element makes it work correctly.
Comment 1•8 years ago
|
||
Regression from APZ.
status-firefox48:
--- → wontfix
status-firefox49:
--- → wontfix
status-firefox50:
--- → fix-optional
Keywords: regression
Priority: -- → P2
Whiteboard: [gfx-noted]
Version: Trunk → 48 Branch
Updated•8 years ago
|
status-firefox52:
--- → affected
Comment 2•8 years ago
|
||
Assigning to Ryan as it's a good bug to poke around in APZ hit-testing code and learn that part of the codebase. The way hit-testing in APZ works is that when the main-thread pushes a layers update to the compositor, APZ builds a hit-testing tree. This happens in APZCTreeManager::UpdateHitTestingTree. There is some documentation at [1] on the HitTestingTreeNode data structure. That tree is then used for hit-testing when wheel events come in, at [2]. Oftentimes with bugs like these the problem is that the layout side of the code is generating a layer tree which doesn't match what APZ expects. This can be fixed either on the layout side (by changing what's generated) or on the APZ side (by changing what's expected). It's usually a matter of figuring out which is the more generic fix that will allow more types of web content to work correctly and not break existing things. [1] http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/gfx/layers/apz/src/HitTestingTreeNode.h#25 [2] http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/gfx/layers/apz/src/APZCTreeManager.cpp#798
Assignee: nobody → rhunt
Assignee | ||
Comment 3•8 years ago
|
||
I've run the test case with and without the z-index with layers.dump-decisions and layout.display-list.dump enabled. It looks like the overlay's LayerEventRegion is being merged into scrollable's LayerEventRegion. This causes there not to be a hit region for the frame metrics for that layer. Changing it so that we don't merge LayerEventRegion when we have a pseudoStackingContext fixes this issue. (Adding '|| pseudoStackingContext' here http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2848) Is this a possible regression from bug 1220466? I'm not familiar with this code so I don't know if this fix is sane in any way.
Flags: needinfo?(mstange)
Reporter | ||
Comment 4•8 years ago
|
||
Sounds plausible. Forwarding to Matt because he's more familiar with event regions, I think.
Flags: needinfo?(mstange) → needinfo?(matt.woodrow)
Comment 5•8 years ago
|
||
Definitely seems like a regression from bug 1220466. It's not obvious why the presence of a psuedoStackingContext would matter though. The idea is that an AnimatedGeometryRoot represents a frame that might move (along with its descendants), possibly asynchronously on the compositor. Each time we encounter one of these we want to start building a new set of EventRegions so that we have the regions separate on the compositor. It appears in this case we have multiple layers that don't move together (the scrollframe underneath moves independently of the overlay), but we combined the EventRegions as if they did. Did we not tag the frame as an AnimatedGeometryRoot even though it can be moved by the compositor? Or did something else go wrong that caused us to miss it.
Flags: needinfo?(matt.woodrow)
Updated•8 years ago
|
Assignee | ||
Comment 6•8 years ago
|
||
Talking with Matt on irc, it seems like the problem is in display list generation: 1. We merge the hit-region of the overlay's display item into the existing nsDisplayLayerEventRegion 2. An AGR is created for the scrollbox 3. The overlay's display item is sorted below the AGR This leaves the hit-region for the overlay div behind the scrollbox's hit-region. This problem occurs for both position: absolute elements and position: relative elements. And it also still occurs if we move the overlay to be after the scroll box in document order. This seems to show that we will merge with the existing nsDisplayLayerEventRegion even if there is an AGR in between. It seems like a solution to this is to always generate a new nsDisplayLayerEventRegion when we are on a positioned element. We can't rely on checking to see if there is an AGR between us and the nsDisplayLayerEventRegion we could merge with because we may get sorted in the end to after an AGR, like in this test case.
Assignee | ||
Comment 7•8 years ago
|
||
This causes us to always generate a new nsDisplayLayerEventRegion when we are building for a positioned element.
Attachment #8814966 -
Flags: review?(matt.woodrow)
Comment 8•8 years ago
|
||
Comment on attachment 8814966 [details] [diff] [review] positioned-layer-events.patch Review of attachment 8814966 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Can you please expand the comment for this branch to include why we need to do it for 'isPositioned'. Just something about about positioned elements being sorted on top of normal elements, so we need a LayerEventsRegion item to go at the new sorting position.
Attachment #8814966 -
Flags: review?(matt.woodrow) → review+
Comment 10•8 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/af5401b40b58 Create nsDisplayLayerEventRegions for positioned elements. r=mattwoodrow
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af5401b40b58
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 12•8 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(rhunt)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8815092 [details] [diff] [review] positioned-layer-events.patch Approval Request Comment [Feature/Bug causing the regression]: APZ + Bug 1220466 [User impact if declined]: When APZ is enabled a user's scroll will incorrectly apply to a scroll frame that is behind a positioned element. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Low [Why is the change risky/not risky?]: It won't affect layout in any way besides possibly doing more work [String changes made/needed]: None
Flags: needinfo?(rhunt)
Attachment #8815092 -
Flags: approval-mozilla-beta?
Attachment #8815092 -
Flags: approval-mozilla-aurora?
Comment 14•8 years ago
|
||
Comment on attachment 8815092 [details] [diff] [review] positioned-layer-events.patch apz-related scrolling fix for aurora52
Attachment #8815092 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4c67c7413cb
Comment 16•8 years ago
|
||
Comment on attachment 8815092 [details] [diff] [review] positioned-layer-events.patch Fix a scrolling regression related to APZ. Beta51+. Should be in 51 beta 7.
Attachment #8815092 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b54862bc3122
You need to log in
before you can comment on or make changes to this bug.
Description
•