Closed Bug 1303408 Opened 4 years ago Closed 4 years ago
Scrolling over the position:absolute element incorrectly scrolls the scrollframe underneath
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.
Regression from APZ.
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  on the HitTestingTreeNode data structure. That tree is then used for hit-testing when wheel events come in, at . 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.  http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/gfx/layers/apz/src/HitTestingTreeNode.h#25  http://searchfox.org/mozilla-central/rev/8cf1367dd89cc36ef8f025dfc6af6d5c086838a7/gfx/layers/apz/src/APZCTreeManager.cpp#798
Assignee: nobody → rhunt
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.
Sounds plausible. Forwarding to Matt because he's more familiar with event regions, I think.
Flags: needinfo?(mstange) → needinfo?(matt.woodrow)
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.
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.
This causes us to always generate a new nsDisplayLayerEventRegion when we are building for a positioned element.
Attachment #8814966 - Flags: review?(matt.woodrow)
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+
Attachment #8814966 - Attachment is obsolete: true
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/af5401b40b58 Create nsDisplayLayerEventRegions for positioned elements. r=mattwoodrow
Please request Aurora/Beta approval on this when you get a chance.
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
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 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+
Setting qe-verify- per Comment 13.
You need to log in before you can comment on or make changes to this bug.