STR:

1. Using a MBP with macOS Monterey and a trackpad, visit https://snap.glitch.me/carousel.html in Firefox
2. On the trackpad, horizontally swipe the carousel

ER:

Center-scroll snapping should ensure that the scrollable area should snap to the center of the purple rectangles.

AR:

We don't appear to snap - at least, not until we pass a certain threshold of the purple rectangles.

This only started appearing for me after I upgraded to Monterey. I didn't experience this with the same builds on Big Sur.

Hi, Mike!

I can reproduce this on a MacBook Pro (16,1) (running macOS 12.1 build 21C52) with a trackpad and a Magic Mouse (the top of which functions like a trackpad). But I can't reproduce it in a VMware VM (with the Magic Mouse) running build 21C52 on a host running macOS 10.15.7 build 19H1615.

I suppose this means Apple has changed (at a very low level) the events it sends, and/or their properties, when doing horizontal scrolling.

In principle, this is actually something one can check using HookCase's System events example hook library. Though you might have to change the code to log mouse-moved events.

Hi smichaud! Great to hear from you - I hope you're keeping well!

I suppose this means Apple has changed (at a very low level) the events it sends, and/or their properties, when doing horizontal scrolling.

Hm, in that case, I wonder if this actually fits under Widget :: Cocoa. What do you think, spohl?

But I can't reproduce it in a VMware VM (with the Magic Mouse) running build 21C52 on a host running macOS 10.15.7 build 19H1615.

Or in another VMware VM running 21C52 on my MacBook Pro (16,1) running macOS 11.6.2 build 20G314 (using the host's trackpad).

Yes, Widget::Cocoa is a good starting place.

(Following up comment #1 and comment #3)

I finally managed to run a copy of my macOS 12 VM on macOS 12, and I'm still not able to reproduce the bug in it. So my failures on other host versions of macOS may be due to the peculiarities of VMware Fusion (versions 12.1.2 and 12.2.1), rather than anything Apple has done.

If I ever dig further into this bug, though, I'll still start with my System events example hook library.

I can reproduce it. When the finger is lifted without any momentum, we snap properly. But if the finger is lifted with some momentum ("fling"), we get stuck.

It looks like panEvent.mFollowedByMomentum is now always false. This means we start the scroll snap, but then we receive unexpected momentum events, and don't react properly.

The mFollowedByMomentum flag is set here: https://searchfox.org/mozilla-central/rev/435a77f1a1aaf1a78d30a2aaa81c6158a2f83dba/widget/cocoa/nsChildView.mm#3250-3259

It's based on an assumption about event loop contents at the time at which the finger is lifted. This was always rather sketchy and is now coming back to bite us. I introduced this assumption in bug 1254275 and it was probably a mistake.

I think we need to remove all uses of mFollowedByMomentum and find a better solution.

Scroll snapping uses an MSD smooth scroll to animate to the target position.
During the scroll snap animation, any synthesized momentum events
from the OS should be ignored; the scroll snap animation has taken over.

It is possible that this patch has unfortunate consequences when mixing
keyboard and touchpad scrolling. For example, pressing space to scroll down
by a page and then doing a fling with the touchpad might now have different
results.

ScrollSnapUtils::GetSnapPointForDestination has different behavior based on
the scroll unit. More specifically, the difference is in CalcSnapPoints::AddEdge:
Unless the unit is DEVICE_PIXELS, this function will only look for snap
points in the scroll direction. For example, if we are currently at position 40,
our destination is 50, and we have snap points at 30 and 100, then we'll only
consider snap point 100, even though snap point 30 would be closer to the destination.
Furthermore, if the destination is at the same position as the current position,
then we will not snap at all.
This behavior makes some sense for discrete-tick wheel scrolling, but I don't think
it is appropriate for touchpad scrolling. ScrollSnapToDestination is called from
OnPanMomentumStart and from AttemptFling; in both these contexts I think it makes
more sense to take all snap points into account.

With this change, it becomes possible to call ScrollSnapToDestination even when
our current velocity is zero. Without this change, we wouldn't scroll snap at
all in that case.

Depends on D135920

This patch makes us use the same code path for scroll snapping of
zero-velocity OnPanEnd and followed-by-momentum OnPanEnd.
This is preferable because mFollowedByMomentum is unreliable.

Depends on D135921

Depends on D135922

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:mstange, could you have a look please?
For more information, please visit auto_nag documentation.

The second patch (which contains the interesting functional change) is awaiting a test.

I'm also fine with deferring the test to a follow-up bug if Markus prefers.

I uploaded two gtests as D139561. One gtest is to test the exact same case described in the commit message in D135922. It passes. The other one is a similar to the first one, that is that there are two snap points, 30 and 100. But there are pan momentum events after pan end event at 50 so the final destination position is close to 100, thus the last snap point should be 100, but in fact with those changes for this bug the last snap position is also 30. I suppose D135922 regressed this case. I am not sure this behavior change is intentional or not. (I guess it's unintentional)

Also note that with these changes we'd also need to tweak this velocity check in TestSnappingOnMomentum.cpp since the check fails because the velocity at that moment is now zero.

For doc writers: this is being noted by mdn/browser-compat-data#15104. When this gets fixed, the note ought to be updated to reflect that.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)

The other one is a similar to the first one, that is that there are two snap points, 30 and 100. But there are pan momentum events after pan end event at 50 so the final destination position is close to 100, thus the last snap point should be 100,

I'm not sure I understand - why would the pan momentum events make a difference for scroll snapping? They occur after the fingers have been lifted. The scroll snap point is decided at the point of lifting the fingers, and is computed based on the velocity and position at that point. So any events that happen after that point should be ignored for the purposes of scroll snapping, by design.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)

Also note that with these changes we'd also need to tweak this velocity check in TestSnappingOnMomentum.cpp since the check fails because the velocity at that moment is now zero.

Oh, that is unfortunate - it would be better if the velocity was correct.

(In reply to Markus Stange [:mstange] from comment #18)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)

The other one is a similar to the first one, that is that there are two snap points, 30 and 100. But there are pan momentum events after pan end event at 50 so the final destination position is close to 100, thus the last snap point should be 100,

I'm not sure I understand - why would the pan momentum events make a difference for scroll snapping? They occur after the fingers have been lifted. The scroll snap point is decided at the point of lifting the fingers, and is computed based on the velocity and position at that point. So any events that happen after that point should be ignored for the purposes of scroll snapping, by design.

Okay, I am going to revise the gtest in question to consider the velocity at that moment, as of now from what I can tell is on Mac the delta on pan end events is zero even if there are following momentum events. So maybe it might be a problem to calculate the velocity at that moment.

I did revise the gtest in D139561, now the pan-end event happens at 65 which is in the middle of [30, 100], then I believe with pan momentum events the final snap position should be 100, but unfortunately it's now 30. We probably need to tweak somewhere if the gtest is doing the right thing to do.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)

I did revise the gtest in D139561, now the pan-end event happens at 65 which is in the middle of [30, 100], then I believe with pan momentum events the final snap position should be 100, but unfortunately it's now 30. We probably need to tweak somewhere if the gtest is doing the right thing to do.

In the revised version, I did use a 10-delta pan-start event and a 55-delta pan event, with these the test failed to snap to 100. Now I split the 55-delta pan event into two, 35-delta pan and 20-delta pan events, with these new two events, the test passes. I believe it's more realistic rather than the single pan event, so now I think it's working properly.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)

Also note that with these changes we'd also need to tweak this velocity check in TestSnappingOnMomentum.cpp since the check fails because the velocity at that moment is now zero.

I am going to change the velocity check.

Backed out for causing GTest failures. CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/420e4c14e144f75e5cf1412bc11132008b465bee

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=977e030270dbfee1cdece430adae0d24ca1f6728&selectedTaskRun=Mk_WUyWNTYChx1WfXonItw.0

Link to failure log: https://treeherder.mozilla.org/logviewer?job_id=369405459&repo=autoland&lineNumber=4901

Failure message: TEST-UNEXPECTED-FAIL | APZCSnappingTesterMock.SnapOnPanEnd | Expected equality of these values

I modifed D139561 to add desktop's default pref values controlling fling behaviors. With these values APZCSnappingTesterMock.SnapOnPanEnd passes on Android as well.

That said, now with these values APZCSnappingOnMomentumTesterMock.SnapOnPanMomentumEnd fails. The test is that there are two snap points 30 and 100 and a pan-end event happens at 65 following momentum events, so I believe it should snap to 100 rather than 30. I am now mostly sure that this is a regression and I have no idea to solve this regression other than tweaking ScrollSnapUtils.

As Botond and I discussed in an APZ meeting, I am going to land the patches with a gtest which is able to be passed with the patches, and will file a follow up bug for the reason the other gtest which has following momentum events at the middle of two snap points fails.

There's a big mistake of the desktop's default value, I did use 1.0 for apz.fling_friction, but it should be 0.002. With the correct value, the test in question APZCSnappingOnMomentumTesterMock.SnapOnPanMomentumEnd f passes.

That said, with the correct value (for desktop), APZCSnappingTesterMock.OnPanEnd fails on Android. I did track down what's the difference is, and the difference is on Android we use AndroidVelocityTracker to tell velocities on pan gestures and the velocity at that moment the APZCSnappingTesterMock.OnPanEnd sends a pan-end event is not zero on Android. I did try some heuristic values to pass the gtests both on Android and desktops, but can't find any good value. I am going to skip the gtests on Android for now.

Anyway, what I missed before and what the tests we need are that we need to test two difference cases;

1. a pan-end event happens at zero velocity
2. a pan-end event happens at non-zero velocity

regardless whether there's any following momentum events or not.

I've revised D139561.

https://hg.mozilla.org/mozilla-central/rev/a67295f1c281
https://hg.mozilla.org/mozilla-central/rev/36d9699ad49c
https://hg.mozilla.org/mozilla-central/rev/55ec02f4e83b
https://hg.mozilla.org/mozilla-central/rev/1dbd870418c7
https://hg.mozilla.org/mozilla-central/rev/5015eff7f1bc

Marked dev-doc-complete - the bug reference to this has been fixed up in BCD: https://github.com/mdn/browser-compat-data/pull/18097 , as discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1749352#c17