Closed Bug 1352863 Opened 7 years ago Closed 7 years ago

Support slider.snapMultiplier with async scrollbar dragging

Categories

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

Product:
Core
Component:
Panning and Zooming
Version:
55 Branch
Platform:
All
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: jack, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(8 files)

Bug 1352863 - Add a CoordOf metafunction that maps point and rect types to their coordinate type.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Move the GetAxis*() functions from AsyncPanZoomController.cpp into a new DirectionUtils.h header.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Add a GetPerpendicularDirection() function to DirectionUtils.h.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Add a DistanceTo() method to BaseRect.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Propagate the visible region from Layer to HitTestingTreeNode.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Record the position of the scrollbar thumb at the start of a scrollbar drag.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Rename a couple of local variables for clarity.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1352863 - Implement support for slider.snapMultiplier during APZ dragging.
59 bytes, text/x-review-board-request
kats
: review+
Details 
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, async-scrollbar-drag
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
(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
(That is, blocks 1211610.)
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
(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.
(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.
(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 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+
(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().
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
Depends on: 1368315
No longer depends on: 1368315
Depends on: 1371299
Depends on: 1424591
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: