1550510 - Cannot drag vertical scroll bar in nested popup page

Status: RESOLVED FIXED

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

Description

[Ronald Tilby]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Login to Familysearch.org (all accounts are free)
Load this url:
https://www.familysearch.org/tree/person/details/L6QQ-NHF
Click the "Edit" field to the right of the "Birth" label
On the "Edit Birth" popup screen click the down arrow to open up the Standardized Event Place selection scroll box.
Try to grab and slide the scroll bar in the Standardized Event places list box.

Mozregression points to Bug 1527182 - With WR enabled, we can async scroll even inside SVG filters. r=botond

Actual results:

Scroll bar won't drag and scroll the box contents.

Expected results:

Scroll bar should drag and scroll the box contents.

Comment 1

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

Thanks, I can reproduce and will investigate.

Comment 2

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

Initial investigation indicates that the position of thumb is not where one would expect, and so we end up in the "mouse is away from thumb, will snap" codepath.

This also explains why on Linux I'm able to drag the scrollbar but it jumps away from the mouse.

Comment 3

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

I spent a bunch of time chasing this down. It looks like it's a problem in the WebRenderScrollData representation. In particular how the transform is represented in the "ancestor transform" vs "transform" and what happens when APZC queries it to populate the APZC's ancestor transform.

If there's a WebRenderLayerScrollData instance with an ancestor transform and a single scrollable metrics, the ancestor transform goes onto the HitTestingTreeNode as intended, but doesn't end up as part of the ancestor transform of the APZC for that metrics. The semantics don't quite line up correctly.

Comment 4

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

I have two different potential fixes for this issue. Both work locally. One modified the WebRenderCommandBuilder.cpp logic for handling deferred transforms. It is simpler conceptually but will result in more IPC data as we will be creating extra WebRenderLayerScrollData instances. The other fix is entirely in WebRenderScrollDataWrapper.h and basically introduces an extra wrapper instance for the ancestor transform while walking around the tree in the compositor. Both are basically functionally identical; one changes the actual data structure built and sent over while the other just pretends that the data structure was built differently.

I kicked off try pushes with both fixes for verification that they don't regress anything else.
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=ad740f7ae1744b6d27c079aaaa503d30717fac08
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=e727a8ffc81d4dfb5b17dff5fb9d3cac954ced3f

I'm inclined to use the first fix since it's less hacky and I think in practice it won't increase IPC data that much.

I also need to write a test.

Comment 5

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

The bad news is that one of the fixes causes assertion failures and therefore the two fixes are not equivalent. The good news is that the fix I wanted to go with doesn't cause assertion failures, so I'll just use that.

Comment 6

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

In attempting to write a test for this I discovered that the conditions for this bug to exist are even more narrow than I originally thought. There needs to be a ScrollInfoLayer generated inside a transform. Which made me look at the code generating the ScrollInfoLayer, which is a result of this function. There's even a comment there that says it needs to be kept in sync with the code I changed in the regressing bug. But sadly the comment was not mirrored at the other location :(

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=07563c5dd432100eb568e4b53d9d729a0a74dbc5

Comment 8

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

Attachment: Bug 1550510 - Stop hoisting scrollinfo items inside filters when WR is enabled. r?botond

In bug 1527182 we made it so that APZ can directly drag-scroll scrollframes
that are inside SVG effects, because that's possible with WR on the compositor.
However the code changed in that bug was meant to be kept in sync with
a second piece of code. The second piece of code controls the generation
of ScrollInfo items for scrollframes inside SVG effects - since we can
APZ-scroll them with WR, we don't need the scrollinfo item anymore.
Producing the scrollinfo item was changing the structure of the APZ tree
in terms of where the transform ended up, and was causing badness with
untransforming the drag mouse events.

This patch adds a test that covers the scenario and also corrects the defect
by bringing the two bits of code back in sync.

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)

In attempting to write a test for this I discovered that the conditions for this bug to exist are even more narrow than I originally thought. There needs to be a ScrollInfoLayer generated inside a transform. Which made me look at the code generating the ScrollInfoLayer, which is a result of this function. There's even a comment there that says it needs to be kept in sync with the code I changed in the regressing bug. But sadly the comment was not mirrored at the other location :(

Oops :( I guess at the time I thought the display-list building code was the interesting place where any change would be originated, and the slider frame code would just want to "follow along".

Comment 10

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

Ah well.

Try push is showing a couple of assertion failures firing on the new test in non-WebRender configurations, but the assertion failures are not related to the code I'm touching (they are the same as bug 1232856). I'll just add a SimpleTest.expectAssertions to make those not fail the test, and leave a comment in bug 1232856.

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c6fc13b032a70c17d794edd2149b893f74d07c52

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0a07a2c5d02
Stop hoisting scrollinfo items inside filters when WR is enabled. r=botond

Comment 13

[Dorel Luca [:dluca]]

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