[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)
Tracking
()
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)
681 bytes,
text/html
|
Details | |
794 bytes,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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?
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 2•5 years ago
|
||
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?
Comment 3•5 years ago
|
||
(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.
Assignee | ||
Comment 4•5 years ago
|
||
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.
Reporter | ||
Comment 6•5 years ago
|
||
(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.
Comment 8•5 years ago
|
||
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.
Maybe it would make sense to delay CSS Scroll Snap (the target is Firefox 68, I think) until this issue has been fixed.
Assignee | ||
Comment 9•5 years ago
|
||
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. :)
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
•
|
||
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.
Assignee | ||
Comment 13•5 years ago
|
||
Updated•5 years ago
|
Comment 14•5 years ago
|
||
(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.
Comment 15•5 years ago
|
||
(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, thevelocity.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.
Assignee | ||
Comment 16•5 years ago
|
||
(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, thevelocity.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!
Assignee | ||
Comment 17•5 years ago
|
||
The gtest in this commit fails without the fix.
Reporter | ||
Comment 18•5 years ago
|
||
Great work folks!
Comment 19•5 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6689a98f4a9f Fix the velocity direction on pan gesture events. r=botond
Comment 20•5 years ago
|
||
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
Assignee | ||
Comment 21•5 years ago
|
||
Oops. I haven't run it on Android? I am sorry about that.
Assignee | ||
Comment 22•5 years ago
|
||
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
Comment 23•5 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16588525ccd9 Fix the velocity direction on pan gesture events. r=botond
Comment 24•5 years ago
|
||
bugherder |
Comment 25•5 years ago
|
||
Is this a fix we need in 68 or can it ride the trains?
Comment 26•5 years ago
|
||
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.
Comment 27•5 years ago
|
||
Agreed, I think we should uplift.
Assignee | ||
Comment 28•5 years ago
|
||
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
Assignee | ||
Comment 29•5 years ago
|
||
Thank you Šime and :kats. I was waiting for the nightly build containing this fix. :)
Comment 30•5 years ago
|
||
Comment on attachment 9070719 [details]
Bug 1551801 - Fix the velocity direction on pan gesture events. r?botond
scrolling fix, approved for 68.0b10
Comment 31•5 years ago
|
||
bugherder uplift |
Comment 32•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Comment 33•5 years ago
|
||
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.
Description
•