1235994 - Scroll snapping and scroll momentum interact poorly

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Steps to reproduce:

1. Load https://webkit.org/demos/scroll-snap/index.html#scroll-normal
2. Use mac touchpad to "flick" the numbers sideways. I.e. give the scrollable
   area a little bit of scroll speed and then quickly remove fingers from
   touchpad.
3. Notice the numbers keeps scrolling for a bit after fingers are removed.
4. Load https://webkit.org/demos/scroll-snap/index.html#scroll-snap-1
5. Use mac touchpad to "flick" the numbers sideways.

Actual behavior:
The scrolling numbers abruptly change scroll speed when the fingers are removed from the touchpad.

It feels like the scrollable area is immediately halted as soon as the fingers are removed, and then slowly animated to get to the closest scroll snapping position. No regard is made for the speed that the area had when it was "let go".

Expected behavior:
The scrollable area should have the same "feel" of momentum. When the fingers are removed from the touchpad in step 5 above, we should never abruptly change the scroll speed.

It should even be possible to flick the scrollable area past multiple scroll-snap positions. I.e. if I start scrolling at the 1, but give it a good flick, it should be possible for the scrollable area to scroll all the way over to the 4 before it comes to a stop. Even if I removed my fingers before we had scrolled much pasted the 1.

Or if I give it a more gentle flick, it should be possible to get to the 3.


In general, it seems like there are other bad interactions between scroll snapping and momentum scrolling. Unless I make sure that there is no momentum when I release the fingers off the touchpad, we often don't end up scrolling exactly to a scroll-snap position, but end up a little bit off of it.

I'm not sure if this problem is specific to OSX and touchpads, or if it also happens on touchscreens like Android and B2G.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The "In general..." part seems to be bug 1233413.

Comment 2

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

So when I tried reproducing this I saw even worse problems, where the scrollable can get stuck between snap points. That seems to be a regression from bug 1229039. The way it works is the main-thread will preventDefault wheel events that are targetted at scrollframes with snapping, and do the snapping itself. However the changes in bug 1229039 cause subsequent pan gesture events to start new input blocks, which cancel the smooth scroll animation triggered by the main thread.

If I undo that change then I see the thing Jonas filed this bug about, where the momentum-based destination is not used, only the touch-end destination is used for fling snapping purposes.

Comment 3

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

I think the right fix here is to special-case OS X trackpad-based wheel events so that they don't do main-thread snapping but use the same code that we use for touch in APZC.

Comment 4

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

Attachment: MozReview Request: Bug 1235994 - Extract a helper function to request a snap to the predicted destination. r?botond

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

Comment 5

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

Attachment: MozReview Request: Bug 1235994 - Add a flag to wheel events to track if they may have momentum following them. r?mstange

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

Comment 6

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

Attachment: MozReview Request: Bug 1235994 - For wheel events which may have momentum following them, handle scroll snapping in APZ. r?mstange

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

Comment 7

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

Try push at [1]. I also verified this doesn't regress bug 1141884.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=349067717188

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8703195 [details]
MozReview Request: Bug 1235994 - Extract a helper function to request a snap to the predicted destination. r?botond

https://reviewboard.mozilla.org/r/29311/#review26089

Comment 9

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

No idea why the try push in comment 7 was so orange, it doesn't seem related to my changes. I did a new try push rebased to latest m-c at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fe140591228

Comment 10

[Markus Stange [:mstange]]

Huh, I thought I had submitted my reviews in this bug already. Sorry for the delay.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8703197 [details]
MozReview Request: Bug 1235994 - For wheel events which may have momentum following them, handle scroll snapping in APZ. r?mstange

https://reviewboard.mozilla.org/r/29315/#review26679

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8703196 [details]
MozReview Request: Bug 1235994 - Add a flag to wheel events to track if they may have momentum following them. r?mstange

https://reviewboard.mozilla.org/r/29313/#review26677

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c18d14659cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdda106011de
https://hg.mozilla.org/integration/mozilla-inbound/rev/d476ed635a0a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c18d14659cb
https://hg.mozilla.org/mozilla-central/rev/cdda106011de
https://hg.mozilla.org/mozilla-central/rev/d476ed635a0a

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Works great! Thanks!!