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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

Steps to reproduce

1. 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
2. Set apz.overscroll.enabled to true in about:config
3. Load any page that's vertically scrollable
4. Overscroll the page at the top using the touchpad
5. 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.

Comment 1

[Razvan Cojocaru]

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

Comment 2

[Botond Ballo [:botond]]

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?

Comment 3

[Botond Ballo [:botond]]

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

Comment 4

[Razvan Cojocaru]

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

Comment 5

[Razvan Cojocaru]

(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

[Hiroyuki Ikezoe (:hiro)]

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

Comment 7

[Razvan Cojocaru]

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

Comment 8

[Botond Ballo [:botond]]

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.

Comment 9

[Razvan Cojocaru]

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

Comment 10

[Botond Ballo [:botond]]

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.

Comment 11

[Razvan Cojocaru]

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

Comment 12

[Botond Ballo [:botond]]

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

Comment 13

[Botond Ballo [:botond]]

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

Comment 14

[Razvan Cojocaru]

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

Comment 15

[Botond Ballo [:botond]]

Ah, neat -- thanks!

Comment 16

[Razvan Cojocaru]

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

Ah, neat -- thanks!

Anytime! :)

Comment 17

[Razvan Cojocaru]

Attachment: Bug 1767337 - Ignore duplicate pan-end events at the end of a pan gesture. r?botond

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d5752b0bdbb
Ignore duplicate pan-end events at the end of a pan gesture. r=botond

Comment 19

[Cosmin Sabou [:CosminS]]

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

Comment 20

[Botond Ballo [:botond]]

Thanks for fixing this, Razvan! Having this fixed helps me test other overscroll-related issues without this behaviour getting in the way.

Comment 21

[Razvan Cojocaru]

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

Comment 23

[Botond Ballo [:botond]]

An interesting side note: it seems that in this bug we assumed that the reason this bug reproduced in KDE but not in GNOME is that GNOME was using a newer Wayland version in which the underlying issue had been fixed.

However, it looks like in fact, KDE and GNOME use different Wayland compositors altogether, and the KDE one remains affected (Emilio ran into bug 1833950, which is another bug caused by the same underlying duplicate-pan-end issue, on a very recent KDE version).

Comment 24

[Razvan Cojocaru]

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

An interesting side note: it seems that in this bug we assumed that the reason this bug reproduced in KDE but not in GNOME is that GNOME was using a newer Wayland version in which the underlying issue had been fixed.

However, it looks like in fact, KDE and GNOME use different Wayland compositors altogether, and the KDE one remains affected (Emilio ran into bug 1833950, which is another bug caused by the same underlying duplicate-pan-end issue, on a very recent KDE version).

Very interesting. Different how? I have both Gnome and KDE installed on my Arch Linux box but a single Wayland version (wayland 1.22.0-1). Do you mean that the qtX-wayland packages use a different (perhaps static) version of Wayland, and that's how it ends up in KDE?

Comment 25

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #24)

I have both Gnome and KDE installed on my Arch Linux box but a single Wayland version (wayland 1.22.0-1). Do you mean that the qtX-wayland packages use a different (perhaps static) version of Wayland, and that's how it ends up in KDE?

I'm not very familiar with the details of Wayland, but based on https://en.wikipedia.org/wiki/Wayland_(protocol)#Wayland_compositors, there is a "compositor" component which is different between KDE (which uses KWin) and GNOME (which uses... Weston? I'm not sure, but in any case, something different from KWin).