1424591 - SAP: Scroll bar cannot be dragged while holding left mouse button

Status: RESOLVED FIXED

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

Description

[Malkiel Hadari]

Attachment: HTML_and_CSS_only_Sample.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

1.	Open the attached file HTML_and_CSS_only_Sample.html
2.	Using the mouse only, try to drag the scrollbar to scroll the page 


Note: if you use the mouse wheel instead to scroll, then the scrollbar IS moved and the page does scroll.

Issue found on desktop Firefox browser (no such issue in Chrome or IE).



Actual results:

Actual result: The scrollbar is freezes (cannot be dragged) and the page does not scroll


Expected results:

Expected result: The scrollbar is dragged and the page scrolls

Comment 1

[YF (Yang)]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e0709edc613808cd1073632634e004c10ce16e47&tochange=58f1f2ef3d07643330ea732adcafa2611cb2f92b

Comment 2

[Botond Ballo [:botond]]

I'm not able to reproduce on Linux, even with slider.snapMultiplier set to 6 (as it is on Windows).

I'll try on a Windows machine next week.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> I'm not able to reproduce on Linux, even with slider.snapMultiplier set to 6
> (as it is on Windows).

Oh, never mind, slider.snapMultiplier is a "Once" pref (requires a restart to take effect). I *can* reproduce on Linux with slider.snapMultiplier set to 6.

Comment 4

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Further reduced the testcase a bit. Seems to be related to perspective / transform. Somehow that causes us to compute the distance between the mouse and the scrollbar thumb incorrectly, causing the "mouse is far away from the scrollbar thumb" codepath to misfire.

Comment 5

[Botond Ballo [:botond]]

Note: if you need a quick workaround, setting the pref "slider.snapMultiplier" to 0 should fix it.

Comment 6

[Botond Ballo [:botond]]

Backing out the second patch from bug 1168263 ("Exclude perspective transforms from the transform used to convert from screen coordinates to an APZC's coordinate space") fixes the problem.

Comment 7

[Botond Ballo [:botond]]

So in the change mentioned in the previous comment, we modified the code that computes the transform from an APZC up to its parent to exclude the CSS transforms of layers which represented perspective transforms.

The reason this is a problem, is that this ancestor transform is used to convert between Screen coordinates and an APZC's ParentLayer coordinates. If we exclude a component of this transform, and use the resulting transform to convert form Screen to ParentLayer coordinates, the result will not *really* be in ParentLayer coordinates. If that result is then compared to something that really is in ParentLayer coordinates (such as quantities stored on the layer itself), that comparison is bogus.

The motivation for excluding perspective transforms is to address scenarios where we have multiple layers that scroll together, and some of them are covered by a perspective transform and some are not. In such cases, including the perspective transform leads to the different layers having different ancestor transforms, which violates APZ constraint (the APZC needs to have a single ancestor transform).

The page in this bug is not like that: here, the APZC in question only has one layer, and the perspective transform is on an ancestor layer. In such cases, excluding the perspective transform is not necessary.

So, the problem can be resolved by limiting the exclusion to perspective transforms which are on the path from layers that scroll together to their common ancestor.

Comment 8

[Botond Ballo [:botond]]

I have an implementation of the solution approach described in comment 7, but it's failing layout/generic/test/test_bug1198135.html:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=feccfcf83df70b34e44d3624a5cfe3cb1189879f&selectedJob=151714632

Comment 9

[Botond Ballo [:botond]]

Curiously, I can't reproduce that test failure locally.

Comment 10

[Botond Ballo [:botond]]

I managed to reproduce the test failure locally by just opening the test page in a browser rather than actually running the test harness, and fix it with a simple tweak to the original patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b289978ffb22086eb612c8a38794af9cfc6755

Comment 11

[Botond Ballo [:botond]]

This time with a Try push that actually includes the change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7387c7212a765013792804086658aa16796e8a5&selectedJob=151840450

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1424591 - Only exclude a perspective transform from an APZC ancestor transform if it's on the path from layers that scroll together to their common ancestor.

Review commit: https://reviewboard.mozilla.org/r/207800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207800/

Comment 13

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

Comment on attachment 8937094 [details]
Bug 1424591 - Only exclude a perspective transform from an APZC ancestor transform if it's on the path from layers that scroll together to their common ancestor.

https://reviewboard.mozilla.org/r/207800/#review213688

LGTM, thanks! I like the introduction of the AncestorTransform struct, it makes things overall more readable :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26b24e77688e
Only exclude a perspective transform from an APZC ancestor transform if it's on the path from layers that scroll together to their common ancestor. r=kats

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/26b24e77688e