Support slider.snapMultiplier with async scrollbar dragging

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: jack, Assigned: botond)

Tracking

({regression})

55 Branch
mozilla55
All
Windows
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170402030202

Steps to reproduce:

In a long blog entry, I wanted to read the author's name at the top of the page. In 99% of windows programs including previous versions of firefox, the position is remembered until you release the mouse button, while over the scrollbar, so I clicked the position marker on the scroll bar, dragged to the top of the page; I read what I needed to read, and as I have been doing for years, moved the mouse far to the left of the scroll bar, expecting it to snap back into position, so I could release the mouse. 


Actual results:

It never snapped back to position. I lost my place and needed to spend 5-10 seconds trying to find my place again.


Expected results:

It should have snapped back to position, as if I  had never dragged the scroll bar. This is the behavior on every other program I'm familiar with, even those with custom scrollbars, except on macs.
Thank you for the report. This happens only when the preference apz.drag.enabled is set to true which applies only to NIGHTLY_BUILD.
Blocks: 1324117, 1211610
Status: UNCONFIRMED → NEW
Component: Untriaged → Panning and Zooming
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Summary: Accidental feature removal? - dragging away from scrollbar to snap back to previous vertical position no longer works → dragging away from scrollbar to snap back to previous vertical position no longer works
OS: Unspecified → Windows
Priority: -- → P2
Hardware: Unspecified → All
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
(marking as fix-optional for 55 since this is a nightly-only feature and blocks 1352312, which is the bug to ship by default)
status-firefox55: affected → fix-optional
I never knew the scrollbar had this feature. TIL!
It looks like this behaviour is controlled by the slider.snapMultiplier pref [1]. If the pref is set to zero, the behaviour is disabled. If the pref is 1 or greater, it represents a multipler of the thumb's width (i.e. extent in the direction perpendicular to scrolling) such that if the mouse gets farther away from the thumb than that, the thumb's position snaps back to the position at the beginning of the scroll.

The pref is set to 0 by default, but overridden to be 6 on Windows only [2], which is why we're seeing this behaviour on Windows only (I guess to match the platform convention).

[1] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/modules/libpref/init/all.js#1082
[2] http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/modules/libpref/init/all.js#3661
Assignee: nobody → botond
Summary: dragging away from scrollbar to snap back to previous vertical position no longer works → Support slider.snapMultiplier with async scrollbar dragging

Comment 6

a year ago
(In reply to Jack Eidsness from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0)
> Gecko/20100101 Firefox/55.0
> Build ID: 20170402030202
> 
> Steps to reproduce:
> 
> In a long blog entry, I wanted to read the author's name at the top of the
> page. In 99% of windows programs including previous versions of firefox, the
> position is remembered until you release the mouse button, while over the
> scrollbar, so I clicked the position marker on the scroll bar, dragged to
> the top of the page; I read what I needed to read, and as I have been doing
> for years, moved the mouse far to the left of the scroll bar, expecting it
> to snap back into position, so I could release the mouse. 

Is this an actual feature? Or just a bug that some started to use as a feature?
I always hated it. Often it when I drag on something long for a while the cursor strays somewhat to the left and the whatever I view jumps to the original position. Or in worse cases when I'm at about the limit jumps back and forth. Extremely disorienting, and frustrating.
I'd rather escape could just cancel the scroll. It works for any other kind of mouse dragging, but not this...
(In reply to avada from comment #6)
> Is this an actual feature? Or just a bug that some started to use as a
> feature?

Given that - as described in comment 5 - there is a pref to control it, it's definitely a deliberate feature. The code change that introduced it predates our migration from CVS to Mercurial in 2007, so I can't easily find the associated bug report, but it was probably added to match the scrollbar behaviour of other Windows programs.

> I always hated it. Often it when I drag on something long for a while the
> cursor strays somewhat to the left and the whatever I view jumps to the
> original position. Or in worse cases when I'm at about the limit jumps back
> and forth. Extremely disorienting, and frustrating.

As also mentioned in comment 5, you can disable the behaviour by setting the slider.snapMultiplier pref to 0.

Comment 8

a year ago
(In reply to Botond Ballo [:botond] from comment #7)

> Given that - as described in comment 5 - there is a pref to control it, it's
> definitely a deliberate feature. The code change that introduced it predates
> our migration from CVS to Mercurial in 2007, so I can't easily find the
> associated bug report, but it was probably added to match the scrollbar
> behaviour of other Windows programs.
 
I see. Though I was thinking, It might have been a bug originally in Windows.
 
> As also mentioned in comment 5, you can disable the behaviour by setting the
> slider.snapMultiplier pref to 0.

This is good to know, thanks.
(In reply to Botond Ballo [:botond] from comment #7)
> Given that - as described in comment 5 - there is a pref to control it, it's
> definitely a deliberate feature. The code change that introduced it predates
> our migration from CVS to Mercurial in 2007, so I can't easily find the
> associated bug report, but it was probably added to match the scrollbar
> behaviour of other Windows programs.

I ended up digging up the bug report: bug 22175. Feel free to read the discussion there (dates back to 2000 :)) for additional background.

Comment 10

a year ago
(In reply to Botond Ballo [:botond] from comment #9)
> I ended up digging up the bug report: bug 22175. Feel free to read the
> discussion there (dates back to 2000 :)) for additional background.

:) I guess those people are long gone. Too bad they decided against escape for cancellation.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8869637 [details]
Bug 1352863 - Add a CoordOf metafunction that maps point and rect types to their coordinate type.

https://reviewboard.mozilla.org/r/141220/#review144914
Attachment #8869637 - Flags: review?(bugmail) → review+
Comment on attachment 8869638 [details]
Bug 1352863 - Move the GetAxis*() functions from AsyncPanZoomController.cpp into a new DirectionUtils.h header.

https://reviewboard.mozilla.org/r/141222/#review144916

I don't think we have a policy on this but I'd prefer to put the new header in the EXPORTS.mozilla.layers section so that you have to include it as mozilla/layers/DirectionUtils.h. it seems layers-specific enough that we probably shouldn't pollute the global header namespace with this
Attachment #8869638 - Flags: review?(bugmail) → review+
Comment on attachment 8869639 [details]
Bug 1352863 - Add a GetPerpendicularDirection() function to DirectionUtils.h.

https://reviewboard.mozilla.org/r/141224/#review144918
Attachment #8869639 - Flags: review?(bugmail) → review+
Comment on attachment 8869640 [details]
Bug 1352863 - Add a DistanceTo() method to BaseRect.

https://reviewboard.mozilla.org/r/141226/#review144920
Attachment #8869640 - Flags: review?(bugmail) → review+
Comment on attachment 8869641 [details]
Bug 1352863 - Propagate the visible region from Layer to HitTestingTreeNode.

https://reviewboard.mozilla.org/r/141228/#review144922

::: gfx/layers/LayerMetricsWrapper.h:339
(Diff revision 1)
>      return EventRegions();
>    }
>  
> +  const LayerIntRegion& GetVisibleRegion() const
> +  {
> +    MOZ_ASSERT(IsValid());

Not so sure this is correct. Shouldn't the visible region be transformed by the layer transform for !AtBottomLayer() instances? i'd have to check some layer dumps to see what happens in cases with transforms.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> > +  const LayerIntRegion& GetVisibleRegion() const
> > +  {
> > +    MOZ_ASSERT(IsValid());
> 
> Not so sure this is correct. Shouldn't the visible region be transformed by
> the layer transform for !AtBottomLayer() instances?

Oops, you're right, it should be transformed for !AtBottomLayer() instances.

(I don't think this scenario comes up for scroll thumb layers, which are the only layers whose visible region we are using in this patch series, but of course the function should be written in a way that's correct for all layers.)
Comment on attachment 8869642 [details]
Bug 1352863 - Record the position of the scrollbar thumb at the start of a scrollbar drag.

https://reviewboard.mozilla.org/r/141230/#review145492
Attachment #8869642 - Flags: review?(bugmail) → review+
Comment on attachment 8869643 [details]
Bug 1352863 - Rename a couple of local variables for clarity.

https://reviewboard.mozilla.org/r/141232/#review145496
Attachment #8869643 - Flags: review?(bugmail) → review+
Comment on attachment 8869644 [details]
Bug 1352863 - Implement support for slider.snapMultiplier during APZ dragging.

https://reviewboard.mozilla.org/r/141234/#review145498
Attachment #8869644 - Flags: review?(bugmail) → review+
Comment on attachment 8869641 [details]
Bug 1352863 - Propagate the visible region from Layer to HitTestingTreeNode.

https://reviewboard.mozilla.org/r/141228/#review145500

r+ on this with the LayerMetricsWrapper impl fixed.
Attachment #8869641 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> I don't think we have a policy on this but I'd prefer to put the new header
> in the EXPORTS.mozilla.layers section so that you have to include it as
> mozilla/layers/DirectionUtils.h. it seems layers-specific enough that we
> probably shouldn't pollute the global header namespace with this

Done.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> r+ on this with the LayerMetricsWrapper impl fixed.

Fixed. I also made a corresponding fix to the WebRenderScrollDataWrapper implementation. The latter required adding a WebRenderLayerScrollData::GetTransformTyped() function, to match Layer::GetTransformTyped().

Comment 39

a year ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd956e57f014
Add a CoordOf metafunction that maps point and rect types to their coordinate type. r=kats
https://hg.mozilla.org/integration/autoland/rev/e6848cb27508
Move the GetAxis*() functions from AsyncPanZoomController.cpp into a new DirectionUtils.h header. r=kats
https://hg.mozilla.org/integration/autoland/rev/eb635a15cfa5
Add a GetPerpendicularDirection() function to DirectionUtils.h. r=kats
https://hg.mozilla.org/integration/autoland/rev/07bc82c0e6e0
Add a DistanceTo() method to BaseRect. r=kats
https://hg.mozilla.org/integration/autoland/rev/f7d1e6a56779
Propagate the visible region from Layer to HitTestingTreeNode. r=kats
https://hg.mozilla.org/integration/autoland/rev/f3a6eadd0a97
Record the position of the scrollbar thumb at the start of a scrollbar drag. r=kats
https://hg.mozilla.org/integration/autoland/rev/a1a7093513d0
Rename a couple of local variables for clarity. r=kats
https://hg.mozilla.org/integration/autoland/rev/58f1f2ef3d07
Implement support for slider.snapMultiplier during APZ dragging. r=kats
status-firefox-esr52: --- → unaffected
Depends on: 1368315
No longer depends on: 1368315
Depends on: 1371299

Updated

8 months ago
Depends on: 1424591
You need to log in before you can comment on or make changes to this bug.