Closed Bug 1767337 Opened 2 years ago Closed 1 year ago

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

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
107 Branch
Tracking Flags:
Tracking Status
firefox107 --- fixed

People

(Reporter: botond, Assigned: rzvncj)

References

Details

Attachments

(1 file)

Bug 1767337 - Ignore duplicate pan-end events at the end of a pan gesture. r?botond
48 bytes, text/x-phabricator-request
Details | Review

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.

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

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?

(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

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

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

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

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

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.

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

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.

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

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

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

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

Ah, neat -- thanks!

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

Ah, neat -- thanks!

Anytime! :)

Assignee: nobody → rzvncj
Status: NEW → ASSIGNED
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
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

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox107: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch

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

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

QA Whiteboard: [qa-107b-p2]
See Also: → 1833950

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

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

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

You need to log in before you can comment on or make changes to this bug.