1017338 - Swipe to close a tab from tab panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Attachment: 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

Comment 1

[Lucas Rocha (:lucasr)]

Moving to v2.

Comment 3

[Richard Newman [:rnewman]]

This is the biggest feedback I've seen about the new tablet UI.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 5

[Martyn Haigh (:mhaigh)]

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.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 8

[Martyn Haigh (:mhaigh)]

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 10

[Martyn Haigh (:mhaigh)]

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

Comment 11

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2972359250c

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/192f73676e5d

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/192f73676e5d

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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)]: