Closed Bug 1551801 Opened 5 years ago Closed 5 years ago

[css-scroll-snap] When scrolling toward/against a snapport boundary, snapping doesn't resolve at that boundary, instead animates towards the opposite boundary/edge of snapport

Categories

(Core :: Layout: Scrolling and Overflow, defect, P2)

68 Branch
Desktop
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- verified
firefox69 --- verified

People

(Reporter: hi, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

Attached file case.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Breakout from bug 1545316

Open test case and try gestural two finger scroll on trackpad upwards and on release notice how snapping flies to the bottom of the scroll container, which is the opposite direction and boundary from which the scroll attempted. Similarly, scrolling down while already at the bottom ends up snapping to the top.

Actual results:

Backwards snapping from the intended scroll direction.

Expected results:

Snapping should just sit/resolve on the edge that is furthest in the direction of the scroll.

...also, in this test case, when using ample force, I can barely scroll between snap alignments when going in the correct direction. It almost won't let me scroll to the bottom with a downward scroll when sitting at the top, or scroll to the top with an upward scroll when sitting at the bottom.

I can't still reproduce the issue, but I guess the issue you are seeing is something like the behavior when you drag the scrollbar a bit downward and release the scrollbar?

Component: Untriaged → Layout: Scrolling and Overflow
Product: Firefox → Core
Summary: [css-scroll-snap] Flinging backwards snapping from intended scroll direction towards snapport boundary. → [css-scroll-snap] When scrolling toward/against a snapport boundary, snapping doesn't resolve at that boundary, instead animates towards the opposite boundary/edge of snapport

Hey Kartikaya Gupta, since hiro doesn't have access to osx and I was told to break this off from bug 1545316...

but at least on the attached test page in the latest nightly, if I fling the bottom box hard in one direction, it will scroll all the way to the end, then "bounce" and scroll all the way to the other end and snap there. Which seems very wrong.

...want to follow up here?

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

I can't still reproduce the issue, but I guess the issue you are seeing is something like the behavior when you drag the scrollbar a bit downward and release the scrollbar?

Not really. Upon releasing the scrollbar the behaviour seems good - it snaps to the end if it's near, or doesn't snap anywhere. With trackpad it feels much more random than that.

(In reply to jonjohnjohnson from comment #2)

...want to follow up here?

I don't have anything in particular to say here. Hiro, if you can borrow somebody's mac for a few minutes and try it out you'll notice the behaviour, it's very obviously wrong. If you want to make a build with logging I'm happy to run it for you.

Thanks kats for your kindness. I decided to get a macbook to see what's going on there, probably I will get it within a week.

Assignee: nobody → hikezoe
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

I don't yet get the mac, but setting P2 for now.

Priority: -- → P2

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

I don't yet get the mac, but setting P2 for now.

I have a feeling this is closer to P1 than P2 because of it being quite clear and broken behavior, not just an enhancement.

“P2 = We want this, but it's not totally clear or extremely important.”

Though I understand as you’re assigned to fix something for which you don’t yet have the hardware is difficult. Thanks for the diligence Hiro.

I’ve recorded a screencast that demonstrates the issue. The video shows that it is not possible to scroll the page by flicking two fingers on the touchpad on Mac laptops. The only way to scroll is to slowly slide the fingers until the scroll offset is closer to the next snap target.

https://youtu.be/nbxAJcw561c

Maybe it would make sense to delay CSS Scroll Snap (the target is Firefox 68, I think) until this issue has been fixed.

I just started debugging this. The unintended snap position comes from ScrollSnapToDestination call in AsyncPanzoomController::OnPanMomentumStart. At first glance, it seems odd to me that we need to snap at the start point of the momentum, but there must be a reason for that, I will read bug 1235994. And another odd thing to me is, even if we snap at the start point, we should also snap at the end of the momentum, so there must be a problem at the end of the momentum.

Anyways, a good news is I got a macbook and can reproduce this issue locally. A bad news is I have no idea what's going on there yet. :)

OS: Unspecified → macOS
Hardware: Unspecified → Desktop

I just noticed that when the ScrollSnapToDestination is called from OnPanMomentumStart, the velocity.y is negative when scrolling down. This is opposed what I see on Android. On Android, the velocity.y is positive on scrolling down, it's negative on scrolling up.

Presumably this inverting operation on Android is a key factor.

It seems to me that we have to do the same thing for the two finger flicker-like scrolling on Mac (I don't know what it is called). CCing Botond.

I did confirm that this issue is not a regression by the new scroll snap implementation. It also happened on Firefox 65.

Now I think using un-negated velocity for pan-momentum in AsyncPanZoomController::ScrollSnapToDestination() is a reasonable way to fix this issue, and probably it will be able to be uplifted. For long term, we might want a dedicated VelocityTracker for flick-ish operations, but I am not totally sure at this moment.

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

At first glance, it seems odd to me that we need to snap at the start point of the momentum, but there must be a reason for that, I will read bug 1235994.

FWIW this behaviour is so that once you lift your fingers and start a momentum fling, we scroll smoothly to the snap point that's nearest where the momentum fling would have ended. By precomputing the snap point at the start of the momentum fling we can adjust the fling physics parameters so that it's smooth all the way through instead of changing velocity partway through the fling.

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

I just noticed that when the ScrollSnapToDestination is called from OnPanMomentumStart, the velocity.y is negative when scrolling down. This is opposed what I see on Android. On Android, the velocity.y is positive on scrolling down, it's negative on scrolling up.

Actually, the velocity computation is just wrong for pan gesture events :(

We discovered this in bug 1213601 and the patch in that bug fixes this, but the landing of the patch is blocked on some other issues. We can probably take the relevant part of that fix, which is:

diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -2572,9 +2601,9 @@
   // aEvent.mLocalStartPoint) would mess up velocity calculation. (This is
   // the only caller of UpdateWithTouchAtDevicePoint() for pan events, so
   // there is no risk of other calls resetting the position.)
-  mX.UpdateWithTouchAtDevicePoint(mX.GetPos() + logicalPanDisplacement.x,
+  mX.UpdateWithTouchAtDevicePoint(mX.GetPos() - logicalPanDisplacement.x,
                                   aEvent.mTime);
-  mY.UpdateWithTouchAtDevicePoint(mY.GetPos() + logicalPanDisplacement.y,
+  mY.UpdateWithTouchAtDevicePoint(mY.GetPos() - logicalPanDisplacement.y,
                                   aEvent.mTime);
 
   HandlePanningUpdate(physicalPanDisplacement);

and land it in this bug.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)

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

At first glance, it seems odd to me that we need to snap at the start point of the momentum, but there must be a reason for that, I will read bug 1235994.

FWIW this behaviour is so that once you lift your fingers and start a momentum fling, we scroll smoothly to the snap point that's nearest where the momentum fling would have ended. By precomputing the snap point at the start of the momentum fling we can adjust the fling physics parameters so that it's smooth all the way through instead of changing velocity partway through the fling.

Thanks for the explanation. Actually I read the bug comments but I couldn't understand how actually the problem looked like. I did try to move the ScrollSnapToDestination call into OnPanMomentumEnd and saw the result looks jiggling. Your explanation totally makes sense to me now.

(In reply to Botond Ballo [:botond] from comment #15)

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

I just noticed that when the ScrollSnapToDestination is called from OnPanMomentumStart, the velocity.y is negative when scrolling down. This is opposed what I see on Android. On Android, the velocity.y is positive on scrolling down, it's negative on scrolling up.

Actually, the velocity computation is just wrong for pan gesture events :(

We discovered this in bug 1213601 and the patch in that bug fixes this, but the landing of the patch is blocked on some other issues. We can probably take the relevant part of that fix, which is:

Oh! So if bug 1213601 stuck in m-c, this issue had been already solved. Unlucky me. :)

Anyways, I will push the fix with a gtest that I wrote yesterday. The gtest looks awkward, but presumably it will not be a big problem since we probably will not need more gtests for pan gestures.

Thank you both!

The gtest in this commit fails without the fix.

Great work folks!

Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6689a98f4a9f
Fix the velocity direction on pan gesture events. r=botond

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&fromchange=6689a98f4a9f59d7af7704006c476ddb7d05ebf7&tochange=6c975a65dc510fbfdcd7d31a6ecced792fb9d102&searchStr=gtest&selectedJob=251024826

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251024826&repo=autoland&lineNumber=3175

Back-out link: https://hg.mozilla.org/integration/autoland/rev/6c975a65dc510fbfdcd7d31a6ecced792fb9d102

[task 2019-06-10T22:13:51.012Z] 22:13:51 INFO - TEST-START | APZCSnappingTester.Snap_On_Momentum
[task 2019-06-10T22:13:51.012Z] 22:13:51 WARNING - TEST-UNEXPECTED-FAIL | APZCSnappingTester.Snap_On_Momentum | Expected equality of these values:
[task 2019-06-10T22:13:51.012Z] 22:13:51 INFO - 100.0f
[task 2019-06-10T22:13:51.012Z] 22:13:51 INFO - Which is: 100
[task 2019-06-10T22:13:51.012Z] 22:13:51 INFO - apzc->GetCurrentAsyncScrollOffset( AsyncPanZoomController::AsyncTransformConsumer::eForHitTesting) .y
[task 2019-06-10T22:13:51.012Z] 22:13:51 INFO - Which is: 0 @ /builds/worker/workspace/build/src/gfx/layers/apz/test/gtest/TestSnapping.cpp:202
[task 2019-06-10T22:13:51.012Z] 22:13:51 WARNING - TEST-UNEXPECTED-FAIL | APZCSnappingTester.Snap_On_Momentum | test completed (time: 0ms)
[task 2019-06-10T22:13:51.012Z] 22:13:51 INFO - TEST-START | MVMTester.ZoomBoundsRespectedAfterRotation_Bug1536755

Flags: needinfo?(hikezoe)

Oops. I haven't run it on Android? I am sorry about that.

Flags: needinfo?(hikezoe)

As a brief chat with Botond on IRC, I am going to split the gtest I added into a new test file and disable it for now, since I couldn't run the gtest locally due to bug 1545234.
Here is a try with the change.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44225b60c9598ef5899e2e898f477b42b379281c&selectedJob=251075119

Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16588525ccd9
Fix the velocity direction on pan gesture events. r=botond
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Is this a fix we need in 68 or can it ride the trains?

Flags: needinfo?(hikezoe)
Flags: in-testsuite+

If you ship CSS Scroll Snap in 68 without this fix, people who test this feature on MacBooks (so, a lot of developers) might assume that Firefox’s implementation is broken or buggy.

Agreed, I think we should uplift.

Comment on attachment 9070719 [details]
Bug 1551801 - Fix the velocity direction on pan gesture events. r?botond

Beta/Release Uplift Approval Request

  • User impact if declined: Scrolling doesn't work as expected on MacOSX with two finger scrolling, it ends up snapping the opposite direction.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is pretty simple. And the change affects only MacOSX.
  • String changes made/needed: None
Flags: needinfo?(hikezoe)
Attachment #9070719 - Flags: approval-mozilla-beta?

Thank you Šime and :kats. I was waiting for the nightly build containing this fix. :)

Comment on attachment 9070719 [details]
Bug 1551801 - Fix the velocity direction on pan gesture events. r?botond

scrolling fix, approved for 68.0b10

Attachment #9070719 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hi, I managed to reproduce this issue in Firefox 68.0b9 but this issue no longer occurs in Beta 68.0b10 or our latest Nightly build. I will mark this issue accordingly.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: