[Linux] [Wayland] Page gets stuck in overscroll if a touchpad pan ends without velocity for momentum scrolling
Categories
(Core :: Panning and Zooming, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox107 | --- | fixed |
People
(Reporter: botond, Assigned: rzvncj)
References
Details
Attachments
(1 file)
Steps to reproduce
- Run Firefox Nightly on a Linux laptop, with either
MOZ_ENABLE_WAYLAND=1
in a Wayland session,or withMOZ_USE_XINPUT2=1
in an X11 session- Edit: the bug only reproduces on Wayland, not X11
- Set
apz.overscroll.enabled
totrue
inabout:config
- Load any page that's vertically scrollable
- Overscroll the page at the top using the touchpad
- Lift fingers from the touchpad while page is overscrolled and not moving, such that there isn't any velocity to trigger momentum scrolling
Expected results
A snap-back animation is triggered to relieve the overscroll.
Actual results
The page is stuck in the overscrolled state until you manually relieve the overscroll by scrolling in the opposite direction.
Assignee | ||
Comment 1•7 months ago
|
||
Could this have been fixed by some other commit in the meantime? It's either that or I'm not understanding how to reproduce this correctly...
Reporter | ||
Comment 2•7 months ago
|
||
I double-checked the STR on latest nightly, and at least on Wayland with MOZ_ENABLE_WAYLAND=1
, I can still reproduce the issue.
(On X11 with MOZ_USE_XINPUT2=1
, for some reason I'm not able to trigger overscroll in the first place. Not sure why that might be.)
What behaviour are you seeing -- that you can trigger overscroll, but the snap-back is successfully triggered even if the page is not moving when you lift your fingers?
Reporter | ||
Comment 3•7 months ago
|
||
(In reply to Botond Ballo [:botond] from comment #2)
(On X11 with
MOZ_USE_XINPUT2=1
, for some reason I'm not able to trigger overscroll in the first place. Not sure why that might be.)
Update: the reason was that I was using the synaptics touchpad driver. Switching to the libinput driver, I can now trigger overscrolling on X11, but the bug no longer reproduces!
(The bug still reproduces with libinput on Wayland.)
==> The issue is somehow specific to Wayland. Very curious.
Assignee | ||
Comment 4•7 months ago
|
||
(In reply to Botond Ballo [:botond] from comment #3)
(In reply to Botond Ballo [:botond] from comment #2)
(On X11 with
MOZ_USE_XINPUT2=1
, for some reason I'm not able to trigger overscroll in the first place. Not sure why that might be.)Update: the reason was that I was using the synaptics touchpad driver. Switching to the libinput driver, I can now trigger overscrolling on X11, but the bug no longer reproduces!
(The bug still reproduces with libinput on Wayland.)
==> The issue is somehow specific to Wayland. Very curious.
I'm using X11, so that explains it.
Assignee | ||
Comment 5•7 months ago
|
||
(In reply to Botond Ballo [:botond] from comment #2)
I double-checked the STR on latest nightly, and at least on Wayland with
MOZ_ENABLE_WAYLAND=1
, I can still reproduce the issue.(On X11 with
MOZ_USE_XINPUT2=1
, for some reason I'm not able to trigger overscroll in the first place. Not sure why that might be.)What behaviour are you seeing -- that you can trigger overscroll, but the snap-back is successfully triggered even if the page is not moving when you lift your fingers?
Yes, I can trigger the overscroll but I get the proper snapback - the same thing you're now experiencing on X11, after switching to libinput.
Comment 6•7 months ago
|
||
FWIW, I can't reproduce on Ubuntu 22.04 with Wayland.
Assignee | ||
Comment 7•7 months ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
FWIW, I can't reproduce on Ubuntu 22.04 with Wayland.
Indeed, as I've noticed in the chat channel, I can't either, on Arch Linux / Gnome 42.4 / Wayland 1.21. Looks like it's been fixed in newer versions of Wayland, whatever it was.
Reporter | ||
Comment 8•7 months ago
|
||
The issue here seems to be that the OS sends two pan-end events (i.e. two events for which this condition is true) in a row. This confuses APZ and we end up with a prematurely terminated overscroll animation and thus stuck in overscroll.
It should be straightforward to make APZ handle this event sequence gracefully.
Assignee | ||
Comment 9•6 months ago
•
|
||
(In reply to Botond Ballo [:botond] from comment #8)
The issue here seems to be that the OS sends two pan-end events (i.e. two events for which this condition is true) in a row. This confuses APZ and we end up with a prematurely terminated overscroll animation and thus stuck in overscroll.
It should be straightforward to make APZ handle this event sequence gracefully.
I could take a stab at this (always keep the type of the last event around and bail (do nothing) if a pan-end event comes while the previous event was also a pan-event), but somebody else would have to confirm the fix since I can't reproduce this (my Wayland version behaves properly). Would this help, or do you prefer to fix this yourself?
Reporter | ||
Comment 10•6 months ago
|
||
Please feel free to take a stab at it! I'm happy to test patches on my affected machine.
I think a good first step is to write a gtest that reproduces the problem by sending two pan-end events. Then you can debug the gtest to understand how the problematic situation arises, and that should help inform what the appropriate fix is. I suspect we can fix it without adding a new piece of state, for example it may be sufficient to just check for already being in the OVERSCROLL_ANIMATION
state when receiving a pan-end event.
Assignee | ||
Comment 11•5 months ago
|
||
As discussed in the APZ channel, my gtest passes (when it shouldn't, if I've understood the suggestion correctly):
diff --git a/gfx/layers/apz/test/gtest/TestOverscroll.cpp b/gfx/layers/apz/test/gtest/TestOverscroll.cpp
--- a/gfx/layers/apz/test/gtest/TestOverscroll.cpp
+++ b/gfx/layers/apz/test/gtest/TestOverscroll.cpp
@@ -353,6 +353,36 @@ TEST_F(APZCOverscrollTester, OverscrollB
}
#endif
+#ifndef MOZ_WIDGET_ANDROID // Only happens with Wayland/Linux
+TEST_F(APZCOverscrollTester, StuckInOverscroll_Bug1767337) {
+ SCOPED_GFX_PREF_BOOL("apz.overscroll.enabled", true);
+
+ PanGesture(PanGestureInput::PANGESTURE_START, apzc, ScreenIntPoint(50, 80),
+ ScreenPoint(0, -2), mcc->Time());
+ mcc->AdvanceByMillis(5);
+ apzc->AdvanceAnimations(mcc->GetSampleTime());
+ PanGesture(PanGestureInput::PANGESTURE_PAN, apzc, ScreenIntPoint(50, 80),
+ ScreenPoint(0, -10), mcc->Time());
+ mcc->AdvanceByMillis(5);
+ apzc->AdvanceAnimations(mcc->GetSampleTime());
+ PanGesture(PanGestureInput::PANGESTURE_PAN, apzc, ScreenIntPoint(50, 80),
+ ScreenPoint(0, -2), mcc->Time());
+ mcc->AdvanceByMillis(5);
+ apzc->AdvanceAnimations(mcc->GetSampleTime());
+ PanGesture(PanGestureInput::PANGESTURE_END, apzc, ScreenIntPoint(50, 80),
+ ScreenPoint(0, 0), mcc->Time());
+ SampleAnimationOnce();
+ PanGesture(PanGestureInput::PANGESTURE_END, apzc, ScreenIntPoint(50, 80),
+ ScreenPoint(0, 0), mcc->Time());
+
+ EXPECT_TRUE(apzc->IsOverscrolled());
+
+ // Check that we recover from overscroll via an animation.
+ ParentLayerPoint expectedScrollOffset(0, 0);
+ SampleAnimationUntilRecoveredFromOverscroll(expectedScrollOffset);
+}
+#endif
+
#ifndef MOZ_WIDGET_ANDROID // Currently fails on Android
TEST_F(APZCOverscrollTester, OverscrollByVerticalAndHorizontalPanGestures) {
SCOPED_GFX_PREF_BOOL("apz.overscroll.enabled", true);
(This includes the suggested addition of a SampleAnimationOnce()
call between the two PANGESTURE_END
gestures.)
Reporter | ||
Comment 12•5 months ago
•
|
||
The missing ingredient seems to be that the Linux widget code generates pan gesture events with mSimulateMomentum=true but the test code does not.
The following additional change makes the test fail in the expected way for me:
diff --git a/gfx/layers/apz/test/gtest/InputUtils.h b/gfx/layers/apz/test/gtest/InputUtils.h
--- a/gfx/layers/apz/test/gtest/InputUtils.h
+++ b/gfx/layers/apz/test/gtest/InputUtils.h
@@ -126,9 +126,11 @@ APZEventResult PanGesture(PanGestureInpu
const RefPtr<InputReceiver>& aTarget,
const ScreenIntPoint& aPoint,
const ScreenPoint& aDelta, TimeStamp aTime,
- Modifiers aModifiers = MODIFIER_NONE) {
+ Modifiers aModifiers = MODIFIER_NONE,
+ bool aSimulateMomentum = false) {
PanGestureInput input(aType, MillisecondsSinceStartup(aTime), aTime, aPoint,
aDelta, aModifiers);
+ input.mSimulateMomentum = aSimulateMomentum;
return aTarget->ReceiveInputEvent(input);
}
diff --git a/gfx/layers/apz/test/gtest/TestOverscroll.cpp b/gfx/layers/apz/test/gtest/TestOverscroll.cpp
--- a/gfx/layers/apz/test/gtest/TestOverscroll.cpp
+++ b/gfx/layers/apz/test/gtest/TestOverscroll.cpp
@@ -370,10 +370,10 @@ TEST_F(APZCOverscrollTester, StuckInOver
mcc->AdvanceByMillis(5);
apzc->AdvanceAnimations(mcc->GetSampleTime());
PanGesture(PanGestureInput::PANGESTURE_END, apzc, ScreenIntPoint(50, 80),
- ScreenPoint(0, 0), mcc->Time());
+ ScreenPoint(0, 0), mcc->Time(), MODIFIER_NONE, true);
SampleAnimationOnce();
PanGesture(PanGestureInput::PANGESTURE_END, apzc, ScreenIntPoint(50, 80),
- ScreenPoint(0, 0), mcc->Time());
+ ScreenPoint(0, 0), mcc->Time(), MODIFIER_NONE, true);
EXPECT_TRUE(apzc->IsOverscrolled());
Reporter | ||
Comment 13•5 months ago
|
||
(By the way, how did you get Bugzilla to color your diff?)
Assignee | ||
Comment 14•5 months ago
|
||
(In reply to Botond Ballo [:botond] from comment #13)
(By the way, how did you get Bugzilla to color your diff?)
Ah, it's a trick I've learned from using Jira a lot in a previous job, and it appears to work with Bugzilla too. You just need to specify the type of text after
```
such as
```diff
Reporter | ||
Comment 15•5 months ago
|
||
Ah, neat -- thanks!
Assignee | ||
Comment 16•5 months ago
|
||
Assignee | ||
Comment 17•5 months ago
|
||
Updated•5 months ago
|
Updated•5 months ago
|
Comment 18•5 months ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5d5752b0bdbb Ignore duplicate pan-end events at the end of a pan gesture. r=botond
Comment 19•5 months ago
|
||
bugherder |
Reporter | ||
Comment 20•5 months ago
|
||
Thanks for fixing this, Razvan! Having this fixed helps me test other overscroll-related issues without this behaviour getting in the way.
Assignee | ||
Comment 21•5 months ago
|
||
(In reply to Botond Ballo [:botond] from comment #20)
Thanks for fixing this, Razvan! Having this fixed helps me test other overscroll-related issues without this behaviour getting in the way.
Happy to help! I'm glad my contribution was useful.
Updated•5 months ago
|
Description
•