1767337 - [Linux] [Wayland] Page gets stuck in overscroll if a touchpad pan ends without velocity for momentum scrolling

Botond Ballo [:botond]

Reporter

Description

•

11 months ago

•

Edited

Steps to reproduce

Run Firefox Nightly on a Linux laptop, with either MOZ_ENABLE_WAYLAND=1 in a Wayland session, ~~or with MOZ_USE_XINPUT2=1 in an X11 session~~
- Edit: the bug only reproduces on Wayland, not X11
Set apz.overscroll.enabled to true in about: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.

Razvan Cojocaru

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...

Botond Ballo [:botond]

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?

Botond Ballo [:botond]

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.

Summary: [Linux] Page gets stuck in overscroll if a touchpad pan ends without velocity for momentum scrolling → [Linux] [Wayland] Page gets stuck in overscroll if a touchpad pan ends without velocity for momentum scrolling

Razvan Cojocaru

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.

Razvan Cojocaru

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.

Hiroyuki Ikezoe (:hiro)

Comment 6

•

7 months ago

FWIW, I can't reproduce on Ubuntu 22.04 with Wayland.

Razvan Cojocaru

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.

Botond Ballo [:botond]

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.

Razvan Cojocaru

Assignee

Comment 9

•

6 months ago

•

Edited

(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?

Botond Ballo [:botond]

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.

Razvan Cojocaru

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.)

Botond Ballo [:botond]

Reporter

Comment 12

•

5 months ago

•

Edited

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());

Botond Ballo [:botond]

Reporter

Comment 13

•

5 months ago

(By the way, how did you get Bugzilla to color your diff?)

Razvan Cojocaru

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

Here's the doc.

Botond Ballo [:botond]

Reporter

Comment 15

•

5 months ago

Ah, neat -- thanks!

Razvan Cojocaru

Assignee

Comment 16

•

5 months ago

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

Ah, neat -- thanks!

Anytime! :)

Razvan Cojocaru

Assignee

Comment 17

•

5 months ago

Attached file Bug 1767337 - Ignore duplicate pan-end events at the end of a pan gesture. r?botond — Details

Phabricator Automation

Updated

•

5 months ago

Assignee: nobody → rzvncj

Status: NEW → ASSIGNED

Phabricator Automation

Updated

•

5 months ago

Attachment #9298150 - Attachment description: Bug 1767337 - [Linux] [Wayland] Page gets stuck in overscroll if a touchpad pan ends without velocity for momentum scrolling. r?botond → Bug 1767337 - Ignore duplicate pan-end events at the end of a pan gesture. r?botond

Pulsebot

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

Cosmin Sabou [:CosminS]

Comment 19

•

5 months ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/5d5752b0bdbb

Status: ASSIGNED → RESOLVED

Closed: 5 months ago

status-firefox107: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 107 Branch

Botond Ballo [:botond]

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.

Razvan Cojocaru

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.

Catalin Sasca, QA [:csasca]

Updated

•

5 months ago

QA Whiteboard: [qa-107b-p2]