Swipe to close a tab from tab panel

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ywang, Assigned: mhaigh, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, relnote-firefox 41+, fennec+)

Details

(Whiteboard: [lang=java])

Attachments

(2 attachments)

Created attachment 8430441 [details]
Swipe up to close a tab.001.jpg

Swiping to close a tab is an existing gesture on Fennec. The existing left-right swiping direction won't work well with a multi-grid format on the new tab panel[bug1014996].

In this case, it makes better sense to do a "swiping up" gesture to close a tab. 

Android launcher also places the delete area on top of the screen, which aligns well with this motion.

[This is one the three ways to close a tab from tab panel. The other two interactions are tapping "close" icon[Bug1017326], and long-press to enter reordering + deleting state.]

Next step:
* Visual design
* Animation refinement
* Implementation
Moving to v2.
Blocks: 1024426
No longer blocks: 1014156
Mentor: michael.l.comella, mhaigh
Whiteboard: [lang=java]

Updated

4 years ago
Duplicate of this bug: 1138066
This is the biggest feedback I've seen about the new tablet UI.
tracking-fennec: --- → ?
We have a TabSwipeGestureListener [1] for the TabsListLayout (i.e. the tabs panel layout on phones) though the code has some TwoWayView specific scroll stuff in there, so I'm not sure how adaptable it'll be.

Other concerns:
  1) Orientation - the old view swipes left/right while this would swipe up so the listener might not be directly applicable
  2) UX - once the tab starts overlapping with the row above it, it'll start looking weird, so we'll have to do some animation magic

Martyn, any thoughts on how challenging this would be?

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabsListLayout.java?rev=7be62fa11c20#407
Flags: needinfo?(mhaigh)
(Assignee)

Comment 5

4 years ago
Technically I don't think it'd be too bad, but I do wonder if this is a good idea from a UX POV because of the overlap issue.
Flags: needinfo?(mhaigh)
(In reply to Martyn Haigh (:mhaigh) from comment #5)
> Technically I don't think it'd be too bad, but I do wonder if this is a good
> idea from a UX POV because of the overlap issue.

I feel like it could work if we use an alpha gradient to hide the thumbnail before it ever reaches the row above.

Martyn, do you have spare cycles to take a stab at this?
Flags: needinfo?(mhaigh)
A potential alternative would be to long-click select multiple tabs with one click in the action bar to remove them (ala gmail), but it may not fix most people's issues because it's not highly discoverable, nor convenient for one button.
tracking-fennec: ? → +
(Assignee)

Updated

4 years ago
Assignee: nobody → mhaigh
Flags: needinfo?(mhaigh)
(Assignee)

Comment 8

4 years ago
Created attachment 8624735 [details] [diff] [review]
Swipe up to close a tab from tab panel

There's a bit of tidy up work in here too, but not enough to warrant two patches imo.

Overview:
When we implemented the GridView in the tablet rewrite we didn't add swipe to close.  Now that we're thinking of moving the mobile tabs tray to a more grid like layout, I thought it'd be wise to implement this now.

This is basically the same code from the TabListLayout but simplified to remove the Y axis code.

Things to note:
I've removed the call to updateSelectedPosition from closeTab because this was causing an unnecessary redraw of the tab items because of the call to requestLayout in setSelection which is called from updateSelectedPosition.  The reason we can survive without this call is that when we remove (via swipe or x button) the selected tab, we get a message from GeckoView to tell up to update the selected item via the onTabChanged method.  When we remove any other tab, we don't need to update the selected item.  With this call in there, we were getting graphical artifacts before the sweet, sweet tab layout animation.
Attachment #8624735 - Flags: review?(s.kaspari)
Summary: Swipe up to close a tab from tab panel → Swipe to close a tab from tab panel
Blocks: 1161638
Comment on attachment 8624735 [details] [diff] [review]
Swipe up to close a tab from tab panel

Review of attachment 8624735 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, just some small nits and questions. I guess most of the things mentioned below have already been in TabsListLayout.TabSwipeGestureListener. I also assume that TabsListLayout is eventually going away? - Because now there's quite some duplicated code.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +22,2 @@
>  import android.util.AttributeSet;
> +import android.util.Log;

I feel overly nitpicking: This class doesn't log anything. :)

@@ +44,4 @@
>  
>  /**
>   * A tabs layout implementation for the tablet redesign (bug 1014156).
> + * Expected to replace TabsGridLayout once complete.

It is expected to replace itself?

@@ +465,5 @@
> +            mEnabled = true;
> +
> +            ViewConfiguration vc = ViewConfiguration.get(TabsGridLayout.this.getContext());
> +            mSwipeThreshold = vc.getScaledTouchSlop();
> +            mMinFlingVelocity = (int) (getContext().getResources().getDisplayMetrics().density * MIN_VELOCITY);

Here you use getContext() but two lines above you use TabsGridLayout.this.getContext(). As this class is not static all the methods should be accessible without the prefix, right? Did you add the prefix for clarity? I guess both approaches are acceptable but it should be consistent. It seems like later the prefix is always used in this class.

@@ +497,5 @@
> +            switch (e.getActionMasked()) {
> +                case MotionEvent.ACTION_DOWN: {
> +                    // Check if we should set pressed state on the
> +                    // touched view after a standard delay.
> +                    triggerCheckForTap();

I wonder if this is ever triggered? It should trigger a "click" if the view is not moved, right? I haven't been able to press the view without also triggering ACTION_MOVE events. I also wonder whether we even want to trigger a click without an ACTION_UP event?

@@ +626,5 @@
> +                View child = TabsGridLayout.this.getChildAt(i);
> +                child.getHitRect(rect);
> +
> +                if (rect.contains(x, y))
> +                    return child;

This class uses mixed styles for one line ifs (with and without braces).
Attachment #8624735 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 10

4 years ago
> LGTM, just some small nits and questions. I guess most of the things
> mentioned below have already been in TabsListLayout.TabSwipeGestureListener.
> I also assume that TabsListLayout is eventually going away? - Because now
> there's quite some duplicated code.

That's the plan - we will hopefully remove the TabsListLayout as part of bug 1158277.


> > +            switch (e.getActionMasked()) {
> > +                case MotionEvent.ACTION_DOWN: {
> > +                    // Check if we should set pressed state on the
> > +                    // touched view after a standard delay.
> > +                    triggerCheckForTap();
> 
> I wonder if this is ever triggered? It should trigger a "click" if the view
> is not moved, right? I haven't been able to press the view without also
> triggering ACTION_MOVE events. I also wonder whether we even want to trigger
> a click without an ACTION_UP event?

The triggerCheckForTap function seems to provide some feedback in the way of highlighting the pressed tab if the user holds the tab for a while.

> This class uses mixed styles for one line ifs (with and without braces).

Copy pasta. -> fixed.
https://hg.mozilla.org/mozilla-central/rev/192f73676e5d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1178794

Updated

4 years ago
Depends on: 1179195
Release Note Request (optional, but appreciated)
[Why is this notable]: New functionality
[Suggested wording]: Swipe-to-close tabs on tablets
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → 41+

Updated

3 years ago
Depends on: 1201054
No longer depends on: 1201054
You need to log in before you can comment on or make changes to this bug.