1387551 - Smooth scrolling occurs regardless of pref with APZ keyboard scrolling enabled

Status: VERIFIED FIXED

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

Description

[bintoro]

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.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Probably it's due bug #1377280.

Comment 3

[Laurențiu Nicola]

Two remarks from #1387858:

1. Globally (but not for a single tab) disabling e10s fixes this
2. Disabling apz.keyboard.enabled has no effect.

Comment 4

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

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.

Comment 5

[Laurențiu Nicola]

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

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

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

I have a patch for this.

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Bug 1387551 - Handle "general.smoothScroll" with async keyboard scrolling.

This commit modifies AsyncPanZoomController to scroll immediately for any keyboard
scrolls when the pref, "general.smoothScroll" is enabled.

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

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

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

Comment 9

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

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 10

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8895049 [details]
Bug 1387551 - Handle "general.smoothScroll" with async keyboard scrolling.

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

Comment 11

[Ryan Hunt [:rhunt] (on leave until early May)]

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

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

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

Comment 13

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3265cc2501b6
Handle "general.smoothScroll" with async keyboard scrolling. r=kats

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33140d5eb28bac5ddd189469edbc2c83d5e92b5c

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3265cc2501b6

Comment 17

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

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.