scroll snapping doesn't work with wheel events and apzc

RESOLVED FIXED in Firefox 45

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: paul, Assigned: kats)

Tracking

unspecified
mozilla45
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [browserhtml])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
The attached example snaps properly without APZC. Doesn't snap with APZC enabled.
(Reporter)

Comment 1

3 years ago
Created attachment 8575722 [details]
snap.html
(Reporter)

Comment 2

3 years ago
Kip, any idea what's going on?
Flags: needinfo?(kgilbert)
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.
Flags: needinfo?(kgilbert)
(Reporter)

Comment 4

3 years ago
(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?
(Reporter)

Comment 5

3 years ago
Kip, can you point me to the relevant bugs?
Flags: needinfo?(kgilbert)
(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.
Flags: needinfo?(kgilbert)
(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.
(Reporter)

Comment 8

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

Anything I can do to help fix this?
(Reporter)

Updated

3 years ago
Flags: needinfo?(kgilbert)
(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.
Flags: needinfo?(kgilbert)
(Reporter)

Comment 10

3 years ago
(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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(kgilbert)
> 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.
Flags: needinfo?(kgilbert)
(Reporter)

Comment 12

3 years ago
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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(kgilbert)
(Reporter)

Comment 13

3 years ago
David, you worked on the wheel code for apzc, maybe you can help me here too.
Flags: needinfo?(dvander)
(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
Flags: needinfo?(kgilbert)
(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.
(Reporter)

Comment 16

3 years ago
(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?
We're looking to use this in new Gaia homescreen and media apps. Who can we poke to get this fixed?
(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.
Blocks: 1157746
Component: Layout → Panning and Zooming
(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.
Flags: needinfo?(dvander)
Flags: needinfo?(kgilbert)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(kgilbert)
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.
Blocks: 1178298
No longer blocks: 1157746
Whiteboard: [browserhtml]
A note, my case that wasn't working before appears to work perfectly now... Maybe I was just doing something wrong...
(Reporter)

Updated

2 years ago
Summary: scroll snapping doesn't work with APZC enabled → scroll snapping doesn't work with mouse events and apzc
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
(Reporter)

Comment 24

2 years ago
> 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?
Different bug. I'll file it as soon as my patches are ready (hopefully tomorrow) and make it block this bug.
Depends on: 1193062
(Reporter)

Comment 26

2 years ago
Now that bug 1193062 has landed, what's the next step?
Flags: needinfo?(mstange)
I have a fix for this somewhere in my local queue, I just need to separate it out.
Flags: needinfo?(mstange)
Snapping doesn't work on Linux with a "clicky" scroll wheel either; this isn't limited to the OS X trackpad.
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.
Assignee: mstange → bugmail.mozilla
OS: Mac OS X → All
Summary: scroll snapping doesn't work with mouse events and apzc → scroll snapping doesn't work with wheel events and apzc
Created attachment 8678929 [details] [diff] [review]
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.
Created attachment 8679567 [details] [diff] [review]
Patch

The previous PoC works surprisingly well, just needed to add a preventDefault so APZ wouldn't double-handle the event.
Attachment #8678929 - Attachment is obsolete: true
Attachment #8679567 - Flags: review?(dvander)
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.
Attachment #8679567 - Flags: review?(dvander)
Depends on: 1219296
Created attachment 8680206 [details] [diff] [review]
Part 1 - Don't request a snap when wheel blocks are prevented
Created attachment 8680210 [details] [diff] [review]
Part 2 - Have the main thread handle wheel events for snapping scrollframes
Created attachment 8680212 [details] [diff] [review]
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.
Attachment #8679567 - Attachment is obsolete: true
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.
Attachment #8680206 - Flags: review?(botond)
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?
Attachment #8680210 - Flags: review?(mstange)
Attachment #8680210 - Flags: review?(dvander)
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
Attachment #8680212 - Flags: review?(mstange)
Attachment #8680212 - Flags: review?(dvander)

Updated

2 years ago
Attachment #8680206 - Flags: review?(botond) → review+
Attachment #8680210 - Flags: review?(dvander) → review+
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.
Attachment #8680212 - Flags: review?(dvander) → review?(kgilbert)
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.
Attachment #8680210 - Flags: review?(mstange) → review+
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.
Attachment #8680212 - Flags: review?(mstange) → review+
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!
Attachment #8680212 - Flags: review?(kgilbert) → review+
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21ac71c3b933
https://hg.mozilla.org/mozilla-central/rev/eb8559a54694
https://hg.mozilla.org/mozilla-central/rev/f973ba21bd6d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 45

2 years ago
bugherderuplift
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
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
No longer depends on: 1219296
Depends on: 1249040
You need to log in before you can comment on or make changes to this bug.