1141884 - scroll snapping doesn't work with wheel events and apzc

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Paul Rouget [:paul]]

The attached example snaps properly without APZC. Doesn't snap with APZC enabled.

Comment 1

[Paul Rouget [:paul]]

Attachment: snap.html

Comment 2

[Paul Rouget [:paul]]

Kip, any idea what's going on?

Comment 3

[:kip (Kearwood Gilbert)]

IIRC, Scroll Snapping should be working on B2G; however, other refactoring work is changing how input events for desktop, particularly scroll wheel events, are being processed for APZC.  These events will need to trigger scroll snapping in the same manner as the main thread counterparts.

Comment 4

[Paul Rouget [:paul]]

(In reply to :kip (Kearwood Gilbert) from comment #3)
> IIRC, Scroll Snapping should be working on B2G; however, other refactoring
> work is changing how input events for desktop, particularly scroll wheel
> events, are being processed for APZC.  These events will need to trigger
> scroll snapping in the same manner as the main thread counterparts.

Can you share the bug numbers?

Comment 5

[Paul Rouget [:paul]]

Kip, can you point me to the relevant bugs?

Comment 6

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #5)
> Kip, can you point me to the relevant bugs?
Please see Bug 1142866 for the mouse wheel refactoring work.

Comment 7

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #5)
> Kip, can you point me to the relevant bugs?
Also, see Bug 969250 for details on how scroll snapping will be triggered by scroll bars.

Comment 8

[Paul Rouget [:paul]]

Kip, I think APZC/wheel code has changed significantly lately, but this bug is still reproducible.

Anything I can do to help fix this?

Comment 9

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #8)
> Kip, I think APZC/wheel code has changed significantly lately, but this bug
> is still reproducible.
> 
> Anything I can do to help fix this?
Hi Paul, I am currently working on issues related to CSS transforms, so won't be back onto this for some time.  Please feel free to take this on if you will be able to start earlier.  I'd be glad to answer any questions you might have on how the snapping works.

Comment 10

[Paul Rouget [:paul]]

(In reply to :kip (Kearwood Gilbert) from comment #9)
> (In reply to Paul Rouget [:paul] from comment #8)
> > Kip, I think APZC/wheel code has changed significantly lately, but this bug
> > is still reproducible.
> > 
> > Anything I can do to help fix this?
> Hi Paul, I am currently working on issues related to CSS transforms, so
> won't be back onto this for some time.  Please feel free to take this on if
> you will be able to start earlier.  I'd be glad to answer any questions you
> might have on how the snapping works.

I really don't know this code base, but I can give it a try.

Here are some questions:
- does scroll snapping works with touch events?
- is scrolling with the scrollbar handled by the apz controller?
- should/can the snapping be async too? And do we have code that does that already?
- if so, does the snapping need to happen as part of the current transaction?
- what should trigger the snapping? The end of the transaction or the dom event?

Comment 11

[:kip (Kearwood Gilbert)]

> Here are some questions:
> - does scroll snapping works with touch events?
Yes, on B2G scroll snapping will trigger any time the last finger is lifted from the touchscreen.  Additionally, the momentum of the scroll when the user releases their finger is used when selecting the scroll snapping destination and to initialize the scroll snapping animation.
> - is scrolling with the scrollbar handled by the apz controller?
I believe so.
> - should/can the snapping be async too? And do we have code that does that
> already?
Technically, a regular fling animation is played until the main thread responds with the snapping destination, then the scroll snapping animation takes over and replaces the fling. To the end user, this usually happens so quickly that it is not visibly two separate events.  If the main thread is slow to respond (i.e. due to Javascript), this system attempts to cover up any jankiness without having to send too much information back to the APZC from the main thread.
> - if so, does the snapping need to happen as part of the current transaction?
The scroll snapping animation is backed by smooth scrolls, so it doesn't necessarily have to happen immediately when the event is fired.  It may happen on a subsequent frame.
> - what should trigger the snapping? The end of the transaction or the dom
> event?
The snapping should trigger when the user is releasing control back to the browser and has finished their scrolling gesture.  For example, when the last finger is released from a trackpad, when the finger is released from the touch screen, when the mouse button is released when dragging a scroll-bar thumb, and when the key is released in keyboard scrolling.

I hope this helps, please let me know if you need more info.

Comment 12

[Paul Rouget [:paul]]

In cocoa code, when the user release his fingers, we go through this code:

1) phase test happens here: http://hg.mozilla.org/mozilla-central/file/b9424d63fe35/widget/cocoa/nsChildView.mm#l4865
2) DispatchAPZAwareEvent called here: http://hg.mozilla.org/mozilla-central/file/b9424d63fe35/widget/cocoa/nsChildView.mm#l4819
3) in APZCTreeManager::ReceiveInputEvent, we receive the SCROLLWHEEL_INPUT event: http://hg.mozilla.org/mozilla-central/file/b9424d63fe35/gfx/layers/apz/src/APZCTreeManager.cpp#l581

I don't know if scrollSnapping should be triggered here or later (in nsGfxScrollFrame.cpp?).

At this level, I don't think we make any difference between a scroll event that comes from finger moving and momentum scroll events (but maybe it's too early to do so).

We could do a couple of things:
1) send a PANGESTURE_CANCELLED when the user remove his fingers
2) or, test in SCROLLWHEEL_INPUT if event->isMomentum is true. isMomentum is set in convertCocoaMouseWheelEvent, but I don't know how to reach this property from the InputData argument we have in APZCTreeManager::ReceiveInputEvent.

Now, that being said, maybe we can check isMomentum later, where scrollsnap is usually called, in nsGfxScrollFrame.cpp. But I'm having trouble figuring out, in this file, what is called when the inputEvent triggered in nsChildView end up being consumed, if it ever happens.

How does it work for touch events? Where is scrollSnap called when the last finger is relased?

Comment 13

[Paul Rouget [:paul]]

David, you worked on the wheel code for apzc, maybe you can help me here too.

Comment 14

[:kip (Kearwood Gilbert)]

(In reply to Paul Rouget [:paul] from comment #12)
> In cocoa code, when the user release his fingers, we go through this code:
> 
> 1) phase test happens here:
> http://hg.mozilla.org/mozilla-central/file/b9424d63fe35/widget/cocoa/
> nsChildView.mm#l4865
> 2) DispatchAPZAwareEvent called here:
> http://hg.mozilla.org/mozilla-central/file/b9424d63fe35/widget/cocoa/
> nsChildView.mm#l4819
> 3) in APZCTreeManager::ReceiveInputEvent, we receive the SCROLLWHEEL_INPUT
> event:
> http://hg.mozilla.org/mozilla-central/file/b9424d63fe35/gfx/layers/apz/src/
> APZCTreeManager.cpp#l581
> 
> I don't know if scrollSnapping should be triggered here or later (in
> nsGfxScrollFrame.cpp?).
If it is a simpler solution, I don't see any problem with triggering it here.

> 
> At this level, I don't think we make any difference between a scroll event
> that comes from finger moving and momentum scroll events (but maybe it's too
> early to do so).
Yes, no difference as far as scroll-snapping is concerned.  For OSX momentum scrolling, it should trigger when the synthesized momentum scroll events start and the user is no longer in control.  If some frames of momentum scrolling pass before the main thread figures out where to snap, that is okay.  Scroll snapping code accommodates this and will blend the animations together.
> 
> We could do a couple of things:
> 1) send a PANGESTURE_CANCELLED when the user remove his fingers
> 2) or, test in SCROLLWHEEL_INPUT if event->isMomentum is true. isMomentum is
> set in convertCocoaMouseWheelEvent, but I don't know how to reach this
> property from the InputData argument we have in
> APZCTreeManager::ReceiveInputEvent.
> 
> Now, that being said, maybe we can check isMomentum later, where scrollsnap
> is usually called, in nsGfxScrollFrame.cpp. But I'm having trouble figuring
> out, in this file, what is called when the inputEvent triggered in
> nsChildView end up being consumed, if it ever happens.
Perhaps we need to be careful with this..  Would it be possible for the end-user to turn off momentum scrolling in OSX preferences and cause us to miss the snapping event?
> 
> How does it work for touch events? Where is scrollSnap called when the last
> finger is relased?
IIRC, this call is hit when the last finger is released from the multitouch trackpad:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#3386

Comment 15

[Chris Lord [:cwiiis]]

(In reply to :kip (Kearwood Gilbert) from comment #3)
> IIRC, Scroll Snapping should be working on B2G; however, other refactoring
> work is changing how input events for desktop, particularly scroll wheel
> events, are being processed for APZC.  These events will need to trigger
> scroll snapping in the same manner as the main thread counterparts.

I may well be misinterpreting how scroll snapping is intended to work, but it would appear that it doesn't work on B2G at the moment. It seems to snap to whatever scroll position you lift your finger at, regardless of what snap points you have setup, or to the top of the container if you try to use scroll-snap-coordinate. It also seems to break the overscroll animation.

Comment 16

[Paul Rouget [:paul]]

(In reply to Chris Lord [:cwiiis] from comment #15)
> I may well be misinterpreting how scroll snapping is intended to work, but
> it would appear that it doesn't work on B2G at the moment. It seems to snap
> to whatever scroll position you lift your finger at, regardless of what snap
> points you have setup, or to the top of the container if you try to use
> scroll-snap-coordinate. It also seems to break the overscroll animation.

Does attachment 8575722 [details] work with touch events?

Comment 17

[Wilson Page [:wilsonpage]]

We're looking to use this in new Gaia homescreen and media apps. Who can we poke to get this fixed?

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Paul Rouget [:paul] from comment #16)
> (In reply to Chris Lord [:cwiiis] from comment #15)
> > I may well be misinterpreting how scroll snapping is intended to work, but
> > it would appear that it doesn't work on B2G at the moment. It seems to snap
> > to whatever scroll position you lift your finger at, regardless of what snap
> > points you have setup, or to the top of the container if you try to use
> > scroll-snap-coordinate. It also seems to break the overscroll animation.
> 
> Does attachment 8575722 [details] work with touch events?

Attachment 8575722 [details] does work, but I expect its simplicity isn't really exercising the code too much. I'll see if I can attach a more complex example.

Comment 19

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

(In reply to :kip (Kearwood Gilbert) from comment #11)
> > - is scrolling with the scrollbar handled by the apz controller?
> I believe so.

No, scrolling by dragging the scrollbar is not handled by APZ. The test case attached to this bug snaps fine if you drag the scrollbar because it is handled by the main thread.

> > - what should trigger the snapping? The end of the transaction or the dom
> > event?
> The snapping should trigger when the user is releasing control back to the
> browser and has finished their scrolling gesture.  For example, when the
> last finger is released from a trackpad, when the finger is released from
> the touch screen, when the mouse button is released when dragging a
> scroll-bar thumb, and when the key is released in keyboard scrolling.

What about when the mousewheel is used for scrolling? With the mousewheel you generally get a series of discrete events, and it's not always clear what the "last" event is at the time you receive an event. Should we wait for a few hundred milliseconds after an event, and if no further events are received, consider that done? If we attempt a snap after every single wheel event it may block the user from actually scrolling at all.

Comment 20

[:kip (Kearwood Gilbert)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> (In reply to :kip (Kearwood Gilbert) from comment #11)
> > > - is scrolling with the scrollbar handled by the apz controller?
> > I believe so.
> 
> No, scrolling by dragging the scrollbar is not handled by APZ. The test case
> attached to this bug snaps fine if you drag the scrollbar because it is
> handled by the main thread.
> 
> > > - what should trigger the snapping? The end of the transaction or the dom
> > > event?
> > The snapping should trigger when the user is releasing control back to the
> > browser and has finished their scrolling gesture.  For example, when the
> > last finger is released from a trackpad, when the finger is released from
> > the touch screen, when the mouse button is released when dragging a
> > scroll-bar thumb, and when the key is released in keyboard scrolling.
> 
> What about when the mousewheel is used for scrolling? With the mousewheel
> you generally get a series of discrete events, and it's not always clear
> what the "last" event is at the time you receive an event. Should we wait
> for a few hundred milliseconds after an event, and if no further events are
> received, consider that done? If we attempt a snap after every single wheel
> event it may block the user from actually scrolling at all.

Mouse wheel scrolling with snapping is handled in a platform specific way.  I'd recommend you try it on both Mac, Windows, and Linux to see how it is working (with Paz disabled).  Regular mouse wheels advance immediately to the next and prior snap points like the up and down keyboard keys work.  There is an exception for OS X, which has momentum simulated in the OS and a discrete touch ended event for trackpads and Magic Mouse.

Also, note that the smooth scrolls are interruptible by cssom-view Dom methods and user input, so the scroll bar always responds immediately to user's input, even if in. Smooth scroll or snapping animation.

Comment 21

[:kip (Kearwood Gilbert)]

If your mouse wheel has a "clicky" feel, then you should advance to the next snap point in the direction of scroll.  Otherwise, you should see the snap point in the direction of scroll selected, which is at least the scrolled distance away, unless there are no snap points past that distance, in which case a closer snap point is selected.  You should not see scroll snapping cause a reversal in direction when using a normal (non-Apple) mouse scroll.

Comment 22

[Chris Lord [:cwiiis]]

A note, my case that wasn't working before appears to work perfectly now... Maybe I was just doing something wrong...

Comment 23

[Markus Stange [:mstange]]

I'm currently working on this. The right place to trigger the scroll snap is on a PANGESTURE_END event.
I have patches that make nsChildView submit PanGestureInput events to APZ instead of WidgetWheelEvents. They need to land before this.

Comment 24

[Paul Rouget [:paul]]

> I have patches that make nsChildView submit PanGestureInput events to APZ instead of WidgetWheelEvents. They need to land before this.

Is this happening here or in a different bug?

Comment 25

[Markus Stange [:mstange]]

Different bug. I'll file it as soon as my patches are ready (hopefully tomorrow) and make it block this bug.

Comment 26

[Paul Rouget [:paul]]

Now that bug 1193062 has landed, what's the next step?

Comment 27

[Markus Stange [:mstange]]

I have a fix for this somewhere in my local queue, I just need to separate it out.

Comment 28

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

Snapping doesn't work on Linux with a "clicky" scroll wheel either; this isn't limited to the OS X trackpad.

Comment 29

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

Stealing. I think i might just disable apz handling of wheel events for scrollable frames with snap points for now. We can add support for this later since it's a bit involved.

Comment 30

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

Attachment: Simple PoC

Here's a simple hacky proof-of-concept that makes snapping work again with APZ. It's not perfect though; for example on Linux with a clicky scroll wheel on the test page attached I can still scroll to the very bottom of the subframe when I shouldn't be allowed to. What's worse is that the extra scroll is only in the compositor so the scrollTop value is wrong. I'm still thinking about this.

Comment 31

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

Attachment: Patch

The previous PoC works surprisingly well, just needed to add a preventDefault so APZ wouldn't double-handle the event.

Comment 32

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

Comment on attachment 8679567 [details] [diff] [review]
Patch

David reminded me that I need to make sure scroll-snapping frames get added to the dispatch-to-content region. I'll fix that but also need to investigate why the patch worked without that in some scenarios where it shouldn't have. Dropping review for now.

Comment 33

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

Attachment: Part 1 - Don't request a snap when wheel blocks are prevented

Comment 34

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

Attachment: Part 2 - Have the main thread handle wheel events for snapping scrollframes

Comment 35

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

Attachment: Part 3 - Use APZ to actually do the scrolling to the snap point

This part is not strictly needed. In the scenario we care about (i.e. wheel-scrolling a scrollframe with snap points) it allows the main thread to use APZ smooth scrolling rather than the "async scrolling" code in nsGfxScrollFrame. However I'm not sure if it breaks other scenarios.

Comment 36

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

Comment on attachment 8680206 [details] [diff] [review]
Part 1 - Don't request a snap when wheel blocks are prevented

As far as I can tell ResetInputState is really only relevant for touch events. It was getting run for other types of events also which until wasn't a problem but it interferes with the code in part 2 when it runs for wheel events.

Comment 37

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

Comment on attachment 8680210 [details] [diff] [review]
Part 2 - Have the main thread handle wheel events for snapping scrollframes

Basically this patch special-cases scrollframes with snap points. The main thread steals the wheel events from the APZ for such scrollframes.

Markus, I'd like to get your opinion on this as well - as far as I can tell it doesn't break the swiping/pangesture stuff on OS X but I may not have been testing it properly. Do you foresee any issues with this?

Comment 38

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

Comment on attachment 8680212 [details] [diff] [review]
Part 3 - Use APZ to actually do the scrolling to the snap point

Goes with part 2 but is optional. It causes the wheel events on snap-pointed scrollframes to go down the path at [1] so that it ends up getting handled by the APZ rather than having the main thread run the animation and keep sending incremental steps to the compositor.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=da38bd42a863#2124

Comment 39

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8680212 [details] [diff] [review]
Part 3 - Use APZ to actually do the scrolling to the snap point

Review of attachment 8680212 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not as familiar with what this could impact, switching r? to :kip if that's okay.

Comment 40

[Markus Stange [:mstange]]

Comment on attachment 8680210 [details] [diff] [review]
Part 2 - Have the main thread handle wheel events for snapping scrollframes

Review of attachment 8680210 [details] [diff] [review]:
-----------------------------------------------------------------

This won't interfere with swiping. Content scripts that call preventDefault() prevent swiping because PostHandleEvent isn't reached, so mViewPortIsOverscrolled is never set to true, so TriggersSwipe() returns false. But what you're doing here doesn't prevent the following code from setting mViewPortIsOverscrolled, so TriggersSwipe() will still return true in the right cases.

Comment 41

[Markus Stange [:mstange]]

Comment on attachment 8680212 [details] [diff] [review]
Part 3 - Use APZ to actually do the scrolling to the snap point

Review of attachment 8680212 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, why not.

Comment 42

[:kip (Kearwood Gilbert)]

Comment on attachment 8680212 [details] [diff] [review]
Part 3 - Use APZ to actually do the scrolling to the snap point

Review of attachment 8680212 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 43

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21ac71c3b933
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8559a54694
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f973ba21bd6d

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21ac71c3b933
https://hg.mozilla.org/mozilla-central/rev/eb8559a54694
https://hg.mozilla.org/mozilla-central/rev/f973ba21bd6d

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/21ac71c3b933
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/eb8559a54694
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f973ba21bd6d

Comment 46

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion