Smooth scrolling occurs regardless of pref with APZ keyboard scrolling enabled

VERIFIED FIXED in Firefox 57

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: kalle, Assigned: rhunt)

Tracking

({regression, reproducible})

56 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 verified)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

Reporter

Description

2 years ago
STR:

1. Disable smooth scrolling.
2. Open a long page such as about:support.
3. Use the keyboard to scroll (either by line or by page).

Results:

The smooth scrolling algorithm is applied even though the feature is turned off.

Interestingly, this does not happen on every page. It seems as though the longer the page, the more reliably the bug occurs. Sometimes scrolling starts out normally but soon reverts to smooth scroll.

The bug has appeared in Nightly somewhere after 20170721.
Severity: normal → major
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Component: General → Panning and Zooming
Product: Firefox → Core

Comment 3

2 years ago
Two remarks from #1387858:

1. Globally (but not for a single tab) disabling e10s fixes this
2. Disabling apz.keyboard.enabled has no effect.
Ryan, can you look into this? I guess if the general.smoothscroll pref is set to false we need to do instantaneous scrolls with keyboard APZ.
Severity: major → normal
Flags: needinfo?(rhunt)
Priority: -- → P3
Summary: Smooth scrolling occurs regardless of pref → Smooth scrolling occurs regardless of pref with APZ keyboard scrolling enabled
Whiteboard: [gfx-noted]

Comment 5

2 years ago
> 2. Disabling apz.keyboard.enabled has no effect.

Actually, it works after disabling that preference and restarting the browser.
Assignee

Comment 6

2 years ago
I have a patch for this.
Assignee: nobody → rhunt
Flags: needinfo?(rhunt)
Comment hidden (mozreview-request)
Assignee

Comment 8

2 years ago
Note, I couldn't figure out a great name for the scrolling function, I'm open to a better name.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8895049 [details]
Bug 1387551 - Handle "general.smoothScroll" with async keyboard scrolling.

https://reviewboard.mozilla.org/r/166170/#review171456

::: commit-message-a921b:4
(Diff revision 1)
> +Bug 1387551 - Handle "general.smoothScroll" with async keyboard scrolling. r?kats
> +
> +This commit modifies AsyncPanZoomController to scroll immediately for any keyboard
> +scrolls when the pref, "general.smoothScroll" is enabled.

s/enabled/disabled/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1766
(Diff revision 1)
> +
> +    ParentLayerPoint displacement =
> +      (destination - mFrameMetrics.GetScrollOffset()) * mFrameMetrics.GetZoom();
> +
> +    CancelAnimation();
> +    Scroll(displacement);

Instead of adding and calling this new Scroll function, can you re-use CallDispatchScroll()? It seems like the Scroll function you added does a subset of AttemptScroll, and the parts that we don't (handoff/overscroll) can be disabled easily. CallDispatchScroll basically just delegates to AttemptScroll but is more general to properly handle scroll handoff chains if the input type supports handoff. I think to call CallDispatchScroll you will need:

ParentLayerPoint startPoint = mFrameMetrics.GetScrollOffset() * mFrameMetrics.GetZoom();
ParentLayerPoint endPoint = destination * mFrameMetrics.GetZoom();
OverscrollHandoffState - same as http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/gfx/layers/apz/src/AsyncPanZoomController.cpp#1996 but use GetCurrentKeyboardBlock and add ScrollSource::Keyboard.

The other thing you would need to do is update InputQueue::AllowScrollHandoff to also check the keyboard block, and add KeyboardBlockState::AllowScrollHandoff which always returns false.

This is probably going to end up with more code than the Scroll() function, but I think overall it will be better to wire this up through existing codepaths rather than create an entirely new function that might get mis-used down the road.
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
I updated the patch to use CallDispatchScroll.

Additionally I factored FindSnapPointNear out into a separate function to help reuse between the two code paths, and also to prevent deadlock with mMonitor as FindSnapPointNear needs the lock to be held, but CallDispatchScroll needs it not to be held.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8895049 [details]
Bug 1387551 - Handle "general.smoothScroll" with async keyboard scrolling.

https://reviewboard.mozilla.org/r/166170/#review172478

This looks good, thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1777
(Diff revision 2)
> +    SetState(NOTHING);
> +
> +    // The calls above handle their own locking; moreover,
> +    // ToScreenCoordinates() and CallDispatchScroll() can grab the tree lock.
> +    ReentrantMonitorAutoEnter lock(mMonitor);
> +    RequestContentRepaint();

I think this RequestContentRepaint call is redundant since it should get done as part of http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/gfx/layers/apz/src/AsyncPanZoomController.cpp#2651 but calling it twice is harmless so this is fine too.

::: gfx/layers/apz/src/InputQueue.cpp:544
(Diff revision 2)
>    if (GetCurrentPanGestureBlock()) {
>      return GetCurrentPanGestureBlock()->AllowScrollHandoff();
>    }
> +  if (GetCurrentKeyboardBlock()) {
> +    return GetCurrentKeyboardBlock()->AllowScrollHandoff();
> +    }

nit: fix indent
Attachment #8895049 - Flags: review?(bugmail) → review+
Assignee

Comment 13

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> Comment on attachment 8895049 [details]
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1777
> (Diff revision 2)
> > +    SetState(NOTHING);
> > +
> > +    // The calls above handle their own locking; moreover,
> > +    // ToScreenCoordinates() and CallDispatchScroll() can grab the tree lock.
> > +    ReentrantMonitorAutoEnter lock(mMonitor);
> > +    RequestContentRepaint();
> 
> I think this RequestContentRepaint call is redundant since it should get
> done as part of
> http://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/gfx/layers/apz/src/
> AsyncPanZoomController.cpp#2651 but calling it twice is harmless so this is
> fine too.
> 

Hmm, I got that from OnScrollWheel [1]. I initially had it removed because I had inlined most of CallDispatchScroll and saw it was redundant, but for some reason I was getting missing repaints in testing so I added it back. I can't reproduce anymore it so it must have been something else. Maybe deadlock with mMonitor.

http://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2009

Comment 14

2 years ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3265cc2501b6
Handle "general.smoothScroll" with async keyboard scrolling. r=kats

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3265cc2501b6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-08-12), so I'm marking this bug as VERIFIED. Thanks.
Status: RESOLVED → VERIFIED
QA Contact: Virtual
You need to log in before you can comment on or make changes to this bug.