1303408 - Scrolling over the position:absolute element incorrectly scrolls the scrollframe underneath

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: testcase

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

[Kartikaya Gupta (email:[e-mail])]

Regression from APZ.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

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.

Comment 4

[Markus Stange [:mstange]]

Sounds plausible. Forwarding to Matt because he's more familiar with event regions, I think.

Comment 5

[Matt Woodrow (:mattwoodrow)]

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.

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

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.

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: positioned-layer-events.patch

This causes us to always generate a new nsDisplayLayerEventRegion when we are building for a positioned element.

Comment 8

[Matt Woodrow (:mattwoodrow)]

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.

Comment 9

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: positioned-layer-events.patch

With comment

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af5401b40b58
Create nsDisplayLayerEventRegions for positioned elements. r=mattwoodrow

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af5401b40b58

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 13

[Ryan Hunt [:rhunt] (on leave until early May)]

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 14

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8815092 [details] [diff] [review]
positioned-layer-events.patch

apz-related scrolling fix for aurora52

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4c67c7413cb

Comment 16

[Gerry Chang [:gchang]]

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.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b54862bc3122

Comment 18

[Andrei Vaida [:avaida]]

Setting qe-verify- per Comment 13.