Closed Bug 1904906 Opened 1 year ago Closed 7 months ago

Too easy to accidentally close tabs when scrolling vertically

Categories

(Firefox for Android :: Tabs, defect, P1)

Firefox 127
All
Android
defect

Tracking

()

VERIFIED FIXED
136 Branch
Tracking Status
firefox136 --- verified

People

(Reporter: certt4fz, Assigned: 007)

References

(Blocks 1 open bug, Regressed 3 open bugs)

Details

(Whiteboard: [fxdroid][group4])

Attachments

(3 files)

Steps to reproduce:

I have noticed this change very recently. Could be since the last update to 127.

I have the tabs in list view and when I wanted to scroll vertically with my thumb I always accidentally close a tab if my swiping motion is even a little bit horizontal. This is really annoying and it definitely wasn't as sensitive before. I need to be very careful and slow when scrolling the tab list now.

Perhaps related to this bug which is marked as fixed: https://bugzilla.mozilla.org/show_bug.cgi?id=1854335

Actual results:

Tabs get closed by accident when scrolling through tabs in list view.

Expected results:

Should be able to scroll vertically through the tab list without being afraid of it being misinterpreted as a horizontal "close tabs" swipe. This worked fine in the past.

Can confirm -- happens to me all the time now. I'm using the Grid view so it's not limited to the List view.

Thank goodness for the 'Undo' banner that slides in at the bottom, but sometimes I don't notice in time to get my tab back 😠

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Too easy to accidentally close tabs in list view when scrolling vertically → Too easy to accidentally close tabs when scrolling vertically

Perhaps related to this bug which is marked as fixed: https://bugzilla.mozilla.org/show_bug.cgi?id=1854335

bug 1854335 was supposed to fix this problem, not make it worse. Noah: is this still known as a problem?

Flags: needinfo?(nbond)
See Also: → 1854335

Hey Sugar, thanks for filing this bug. I've confirmed that some accidental tab swipes are occurring when trying to vertically scroll through the Tabs Tray. I've prioritized this bug for my team to investigate this further.

Severity: -- → S3
Component: General → Tabs
Flags: needinfo?(nbond)
Priority: -- → P2
Whiteboard: [fxdroid][group4]

This issue is a recurring complaint in the Play Store reviews. We really need to fix it.

Is this just a matter of finding a better (higher) value for VELOCITY_THRESHOLD_DP?

Blocks: 1894804

(In reply to Markus Stange [:mstange] from comment #4)

This issue is a recurring complaint in the Play Store reviews. We really need to fix it.

Is this just a matter of finding a better (higher) value for VELOCITY_THRESHOLD_DP?

I am able to reproduce this with some very slow swipes, so I don't believe so. The velocity threshold is a parameter for defining what's the minimum swipe speed that indicates a "swipe to dismiss" has been triggered.

For some reason, the underlying Android component is triggering the drag gesture (when it's specified for horizontal drags) even when the gesture has a large degree of Y-axis movement. I'm looking into this further to see if this can be manually configured.

Thanks for finding that Android source code! I'll play around with some velocity thresholds to see if it at least minimizes part of the issue

Oh, I think I see what you are saying now.

A diagonal drag over a tab in the tabs tray can have the following outcomes:

  1. The tab is swiped away horizontally, no vertical scrolling is performed.
  2. The tab is swiped a bit horizontally but bounces back (remains open), no vertical scrolling is performed.
  3. The tab is not moved, only vertical scrolling is performed.

What I'm suggesting would turn more cases of 1 into 2. But you're saying you'd like 3 to happen in more cases. And I think I agree.

So for this we'd want the AnchoredDraggable view to detect that the scroll direction (within the touch slop) is more vertical than horizontal, and we'd want it to pass off scrolling to the outer scroll view. I found this issue which sounds similar (though it is about ViewPager2 rather than about AnchoredDraggable):

First, the horizontal slide of ViewPager2 is detected in the onInterceptTouchEvent method of the RecyclerView class, right?
And the criterion is whether the touch distance in the X direction is greater than mTouchSlop.
I think we should add to this criterion whether the touch distance in the X direction is greater than the touch distance in the Y direction.

I also found https://developer.android.com/develop/ui/compose/touch-input/pointer-input/scroll#nested-scrolling but haven't spent enough time with it to fully understand it.

(In reply to Markus Stange [:mstange] from comment #8)

A diagonal drag over a tab in the tabs tray can have the following outcomes:

  1. The tab is swiped away horizontally, no vertical scrolling is performed.
  2. The tab is swiped a bit horizontally but bounces back (remains open), no vertical scrolling is performed.
  3. The tab is not moved, only vertical scrolling is performed.

What I'm suggesting would turn more cases of 1 into 2. But you're saying you'd like 3 to happen in more cases. And I think I agree.

Ah gotcha. Then yeah, increasing the velocity threshold might reduce the scenario where flings to scroll the tabs tray are triggering swipe to dismiss flings. Interestingly though, Google's default in their samples and a component (noted below) is only 125

So for this we'd want the AnchoredDraggable view to detect that the scroll direction (within the touch slop) is more vertical than horizontal, and we'd want it to pass off scrolling to the outer scroll view. I found this issue which sounds similar (though it is about ViewPager2 rather than about AnchoredDraggable):

I also found https://developer.android.com/develop/ui/compose/touch-input/pointer-input/scroll#nested-scrolling but haven't spent enough time with it to fully understand it.

Good find on that Google Issue. Yes, that's exactly what I've been looking into: is it possible to manually ignore swipes within a certain y-axis threshold.

Much to my dismay, the Modifier.anchoredDraggable we leverage simply looks for a non-zero x-axis drag delta and declares this as a drag occurring, regardless of whatever the y delta is. It's also not clear how it is differentiating between scroll and drag gestures in this scenario.

I think I've ran into a wall though. After investigating a few hours yesterday and today, I've tried the following:

  • Pass-in a InteractionSource to Modifier.anchoredDraggable
    • The InteractionSource provides no drag measurement data. It simply returns a Flow of Interaction events that we can listen to.
  • Listen to drag gestures via detectDragGestures within Modifier.pointerInput
    • pointerInput consumes any drag gestures, so even trying to log drags prevents Modifier.anchoredDraggable from being triggered. Similarly, if I place this Modifier before anchoredDraggable, it doesn't get called because the drag handling has been consumed downstream.
  • Replace our in-house SwipeToDismissBox (which utilizes Modifier.anchoredDraggable) with the Material 3 SwipeToDismissBox
    • Even the official Google component has the exact same bug.

My educated assumption is that a nested scroll isn't at play here, but it might be worth investigating a little.

From there, the only approach I have not tried is completely rewriting our SwipeToDismissBox from scratch to use a custom Modifier.anchoredDraggable that would be able to be less sensitive to consuming drag gestures with a "large" y-axis delta.

SwipeToDismissBox was already a rewrite from earlier this year, as Google deprecated the SwipeToDismiss Composable in favor of Modifier.anchoredDraggable, so trying to handle the drag gesture within a scrolling parent in Compose has been fraught with development churn.

Is it possible to add setting to disable swipe-to-delete behaviour? It causes issue for more than a year making browsing tabs very annoying, but there is literally no reason in it while there is also X-mark button that does just the same thing.

(In reply to Dmitriy Nikiforov from comment #10)

Is it possible to add setting to disable swipe-to-delete behaviour? It causes issue for more than a year making browsing tabs very annoying, but there is literally no reason in it while there is also X-mark button that does just the same thing.

I made one: https://imgur.com/a/fenix-disable-swipable-tabs-kEVbgEW

See Also: → 1920005

I found another google issue tracker for this problem. I'm also going to propose another round of investigation of this for my team's next sprint

Could we just write our own swipe detection logic, to decouple ourselves from Google fixing this?

Flags: needinfo?(nbond)

Yes, and my team has it in our current sprint to start working towards that!! Stay tuned here for more updates!

The google issue from #12 has a potential starting point for this investigation.

Flags: needinfo?(nbond)
Priority: P2 → P1
Duplicate of this bug: 1933649
Assignee: nobody → nbond
Status: NEW → ASSIGNED
Severity: S3 → S2
Duplicate of this bug: 1939633
Attachment #9444821 - Attachment description: WIP: Bug 1904906 - Write custom drag Modifier → Bug 1904906 - Write custom drag Modifier
Duplicate of this bug: 1940048

I want to mention that in Thunderbird seems like there is no problem with accidental swiping, but it also has special configuration with options to disable swipe actions completely: https://i.imgur.com/zlYtDRr.jpeg

Pushed by nbond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/01f9fed7f685 Write custom drag Modifier r=android-reviewers,matt-tighe

I've landed a patch for Nightly only with a rewritten drag (swipe to close) gesture. It should be available in tonight's or tomorrow's Nightly build to try out.

We'll let this bake to gather some feedback, address any bugs, and fine tune anything we need before letting this ride the trains to Release

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 136 Branch
Duplicate of this bug: 1920005

QE, can you take a look at the swipe gesture when swiping to close a tab in the Tabs Tray?

The patch that landed yesterday is to make it so vertical scrolls do not accidentally trigger the swipe gesture (and therefore accidental tab closures). This change will only be available in Nightly, and we'll want to vet out the new swipe gesture and squash any more bugs before considering letting the fix ride the trains to Release.

Flags: qe-verify+
Attached video mobizen_20250118_123505.mp4 β€”

When you drag the tab, it doesn't go over the adjacent tab, it stays stable on the tab, when you keep your finger on the tab and we move it left, right.

Flags: needinfo?(nbond)

(In reply to eclaudiu64 from comment #25)

Created attachment 9460380 [details]
mobizen_20250118_123505.mp4

When you drag the tab, it doesn't go over the adjacent tab, it stays stable on the tab, when you keep your finger on the tab and we move it left, right.

And as an addition, when we hold the tab in the other direction and put the other tabs on top and then swipe, the tab closes, kind of like the error was.

Regressions: 1942735
Regressions: 1942855
Flags: needinfo?(nbond)
Regressions: 1943008

Verified the swipe to close gesture in Nightly 136.0a1 from 01/22 and indeed the issues mentioned above as regressions are reproducible on our side as well.
Apart from that:

  • I was able to reproduce the issue Claudiu mentioned in Comment 26, on all latest releases and filed bug 1943007 for it.
  • I saw another issue, in which the selected tab was displayed under the first or last tab while manually reordering tabs in tabs tray, and filed bug 1943008 for it. This might be a regression, since I could not reproduce it yet on other releases.

I'll leave the qe-needed flag to further test this once the regressions are fixed.

No longer regressions: 1943008
Regressions: 1943008

Thanks for testing Delia! After Bug 1942735 lands, can you please see if the issue is still reproducible?

Flags: needinfo?(dpop)
Attached video Nothing Phone.mp4 β€”

Verified as fixed in the latest Nightly 136.0a1 from 01/27. Vertical scrolls no longer trigger the swipe gesture and deletion of tabs.
Tested with Nothing Phone (2a) 5G (Android 14), OPPO A15s (Android 10) and Google Pixel 8 Pro (Android 14).

Flags: needinfo?(dpop)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1946858
Regressions: 1951931
Regressions: 1951932
No longer regressions: 1946858
Regressions: 1959099
Regressions: 1960148
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: