Closed Bug 1550510 Opened 6 years ago Closed 6 years ago

Cannot drag vertical scroll bar in nested popup page

Categories

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

68 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: rontilby, Assigned: kats)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(1 file)

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.

Component: Untriaged → Layout: Scrolling and Overflow
Product: Firefox → Core
Component: Layout: Scrolling and Overflow → Panning and Zooming
Priority: -- → P3
Regressed by: 1527182

Thanks, I can reproduce and will investigate.

Assignee: nobody → kats
Status: UNCONFIRMED → NEW
Ever confirmed: true

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.

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.

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.

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.

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 :(

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.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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".

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.

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c0a07a2c5d02 Stop hoisting scrollinfo items inside filters when WR is enabled. r=botond
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
QA Whiteboard: [qa-68b-p2]
Flags: in-testsuite+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: