Closed Bug 1355374 Opened 3 years ago Closed 3 years ago

Scrollbar doesn't follow mouse cursor if scrollable area is partially off-screen

Categories

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

55 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- verified

People

(Reporter: 684sigma, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

I found a problem in Nightly 55. It doesn't happen in Beta 53.
Sometimes when scrolling a scrollable area up, scrollbar moves slower than mouse cursor. Here's how to reproduce it:

1. Open attached page
2. Click "Append"
3. Start dragging the scrollbar starting from its upper point, move mouse to the lower edge of the screen and back

Result: Scrollbar moves slower than mouse cursor once it [scrollbar] touches the lower side of the window
Expected: Scrollbar should move at the same speed as mouse cursor
Has STR: --- → yes
Keywords: regression
Mozregression-gui generated this regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4cbc57e29e25e8abe783b16915bd74325a040b02&tochange=d7518eef6b8924b4ddad6cdeeae4eed1c6129b26
->
1324117 – Enable APZ scrollbar dragging on Nightly
https://bugzilla.mozilla.org/show_bug.cgi?id=1324117
Blocks: 1324117
Has Regression Range: --- → yes
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Too late for 53, not a release blocker.
I think Ovidiu got a little trigger happy here. This doesn't affect anything except 55, and is a bug in the async scrollbar dragging feature which is currently Nightly-only. I can reproduce on Windows, it only happens with apz.drag.enabled set to true.
OS: Unspecified → All
Hardware: Unspecified → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [gfx-noted]
Sorry for that, but I followed what regression range indicated.
I can reproduce this. Will investigate.
Assignee: nobody → botond
(marking as fix-optional for 55 since this is a nightly-only feature and blocks 1352312, which is the bug to ship by default)
(In reply to Mike Taylor [:miketaylr] from comment #6)
> (marking as fix-optional for 55 since this is a nightly-only feature and
> blocks 1352312, which is the bug to ship by default)

(I think you meant bug 1211610.)
(Ah crud, copypasta fail. :()
The issue here is that LayerMetricsWrapper::GetScrollThumbLength() is using the size of the visible region of the scroll thumb's layer to determine the length of the scroll thumb.

When the thumb is partially offscreen, it's visible region can be smaller than the entire thumb, so the calculation goes astray.

[1] http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/gfx/layers/LayerMetricsWrapper.h#417
Comment on attachment 8860528 [details]
Bug 1355374 - Use the AsyncDragMetrics to communicate the scroll thumb length to APZ.

https://reviewboard.mozilla.org/r/132540/#review135398
Attachment #8860528 - Flags: review?(mstange) → review+
Comment on attachment 8860529 [details]
Bug 1355374 - Remove the no-longer-used LayerMetricsWrapper::GetScrollThumbLength() and HitTestingTreeNode::mScrollThumbLength.

https://reviewboard.mozilla.org/r/132542/#review135428
Attachment #8860529 - Flags: review?(bugmail) → review+
Comment on attachment 8860530 [details]
Bug 1355374 - Mochitest for dragging partially offscreen scroll thumb.

https://reviewboard.mozilla.org/r/132544/#review135430

Looks good, thanks!
Attachment #8860530 - Flags: review?(bugmail) → review+
And, of course, the test, which was passed for me locally, is failing in automation :/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d81acfc9453f2883ac672228f730c1bc58356a
(In reply to Botond Ballo [:botond] from comment #16)
> And, of course, the test, which was passed for me locally, is failing in
> automation :/

It's passing for me locally even when running the entire directory, so I'll have to push some logging to automation.
(In reply to Botond Ballo [:botond] from comment #17)
> It's passing for me locally even when running the entire directory, so I'll
> have to push some logging to automation.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c813ea3d82a3481e8c7641a5bbf733528e3df2
(In reply to Botond Ballo [:botond] from comment #18)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=04c813ea3d82a3481e8c7641a5bbf733528e3df2

Trying again, this time with the logging code actually compiling (or whatever you call your variable names not having typos in them in JS-land):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b485e9acd5a9f953a1f7f5ee2b2202b719aba7f0
The issue is that, in the test, I try to calculate a ratio of how many pixels of scrolling should occur per pixel of mouse movement during scrollbar dragging as follows:

  (height of scrolled content) / (height of scroll container)

This is wrong; the correct ratio is:

    (height of scrolled content - height of scroll container)
  / (height of scroll container - length of scrollbar thumb)

(Additionally, if there are scrollbar buttons present, their heights need to be subtracted from the denominator as well.)

Since I got both the numerator and denominator wrong, by sheer luck on my local machine the numbers worked out such that the resulting ratio was close enough to the correct ratio to make the test pass; not so on the test machines.

Unfortunately, web content is not able to determine the length of the scrollbar thumb, so I believe the correct ratio cannot be computed using standard web APIs. We'd have to use something like the APZ test data mechanism to expose the length of the thumb to the test code.

I'm not convinced that this is worth doing; I'm just going to land the fix and abandon the test, though if someone feels strongly that we need this test, or has suggestions for how to test this more easily, I'm open to it.
Attachment #8860530 - Attachment is obsolete: true
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 623d72b7b35d -d 49fbd8da1b0e: rebasing 391515:623d72b7b35d "Bug 1355374 - Use the AsyncDragMetrics to communicate the scroll thumb length to APZ. r=mstange"
merging layout/xul/nsSliderFrame.cpp
warning: conflicts while merging layout/xul/nsSliderFrame.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/687a8e437cda
Use the AsyncDragMetrics to communicate the scroll thumb length to APZ. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ef85518ce6ba
Remove the no-longer-used LayerMetricsWrapper::GetScrollThumbLength() and HitTestingTreeNode::mScrollThumbLength. r=kats
https://hg.mozilla.org/mozilla-central/rev/687a8e437cda
https://hg.mozilla.org/mozilla-central/rev/ef85518ce6ba
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached video Recording #28.mp4
I verified this issue on Mac OS X 10.10 with FF Nightly 55.0a1(2017-04-25) and I can reproduce this issue. Please see the attached video. I also found a new issue that is reproducible if you don't click on "Append", only grab the scrollbar and move it up and down, this bug it's also recorded in the second part of the video.
See Also: → 1359868
(In reply to ovidiu boca[:Ovidiu] from comment #26)
> I verified this issue on Mac OS X 10.10 with FF Nightly 55.0a1(2017-04-25)
> and I can reproduce this issue. Please see the attached video. I also found
> a new issue that is reproducible if you don't click on "Append", only grab
> the scrollbar and move it up and down, this bug it's also recorded in the
> second part of the video.

Thanks for finding this! I filed bug 1359868 for it.
I verified this issue on Mac OS X 10.12, Ubuntu 16.04 x64 and Windows 10 x64 with FF 56.0a1(2017-07-03) and I can confirm the fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.