1450099 - Mac scroll bar scrolls on click even when not shown

Status: RESOLVED FIXED

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

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

STEPS TO REPRODUCE:
1. Navigate to a page long enough to scroll vertically
2. Position the mouse cursor in the far right, where the scrollbar would appear
3. Click

EXPECTED RESULTS:
The click passes through to the page.

ACTUAL RESULTS:
The scrollbar appears and the page scrolls. This makes it impossible to click targets within the scrollbar area.

Seems to be regressed by bug 1422070, regression range is:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=aaaa51a8bf64cde9a2a47647902518502d3dd88e&tochange=0cb270dda57624ae29346c6da60292280589f728

Tested on a MacBook Pro (Retina, 13-inch, Early 2015) running High Sierra 10.13.3 (17D102)

Comment 1

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

Regression in Fx 59.

Comment 2

[Botond Ballo [:botond]]

The problem is that APZ is entering the SCROLLBAR_DRAG state too eagerly: it's entered as soon as you click anywhere on the scrollbar track, when it should only be entered when you click on the thumb.

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1450099 - Add a fast-path to FindScrollThumbNode() for when the drag metrics has not been initialized.

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

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1450099 - Only enter the SCROLLBAR_DRAG state if the scrollbar thumb is clicked.

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

Comment 5

[Botond Ballo [:botond]]

(The purpose of the first patch is to mitigate the fact that the second patch causes FindScrollThumbNode() to be called slightly more often.)

Comment 6

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

Comment on attachment 8965087 [details]
Bug 1450099 - Add a fast-path to FindScrollThumbNode() for when the drag metrics has not been initialized.

https://reviewboard.mozilla.org/r/233784/#review239440

::: gfx/layers/apz/src/APZCTreeManager.cpp:2558
(Diff revision 1)
>  RefPtr<HitTestingTreeNode>
>  APZCTreeManager::FindScrollThumbNode(const AsyncDragMetrics& aDragMetrics)
>  {
>    RecursiveMutexAutoLock lock(mTreeLock);
>  
> +  if (!aDragMetrics.mDirection) {

Should be safe to move this above the lock acquisition.

Comment 7

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

Comment on attachment 8965088 [details]
Bug 1450099 - Only enter the SCROLLBAR_DRAG state if the scrollbar thumb is clicked.

https://reviewboard.mozilla.org/r/233786/#review239442

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8965087 [details]
Bug 1450099 - Add a fast-path to FindScrollThumbNode() for when the drag metrics has not been initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233784/diff/1-2/

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8965088 [details]
Bug 1450099 - Only enter the SCROLLBAR_DRAG state if the scrollbar thumb is clicked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233786/diff/1-2/

Comment 10

[Botond Ballo [:botond]]

Markus tried the patches locally and confirmed that they fix the problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=574eddb0d54d1ba69fd7d779f57ff19543375d96

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16bec207dc41
Add a fast-path to FindScrollThumbNode() for when the drag metrics has not been initialized. r=kats
https://hg.mozilla.org/integration/autoland/rev/2dc5471ebd4a
Only enter the SCROLLBAR_DRAG state if the scrollbar thumb is clicked. r=kats

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/16bec207dc41
https://hg.mozilla.org/mozilla-central/rev/2dc5471ebd4a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Looks like we missed our chance to uplift this to Beta for Fx60 still. Is this something we should keep on the radar for possible ESR60 inclusion still?

Comment 14

[Botond Ballo [:botond]]

I would lean towards no, as this seems like a minor edge case, but let's ask Adam as well: do you think this bug has a significant impact on user experience?

Comment 15

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Botond Ballo [:botond] from comment #14)
> I would lean towards no, as this seems like a minor edge case, but let's ask
> Adam as well: do you think this bug has a significant impact on user
> experience?

It's literally an edge case, I wouldn't worry too much about it. I only noticed it on one Microsoft doc site that had a button partially overlapping the scrollbar, so I could work around it easily.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Sounds like it can ride the trains then. Thanks for the extra info.