Last Comment Bug 1017338 - Swipe to close a tab from tab panel
: Swipe to close a tab from tab panel
Status: RESOLVED FIXED
[lang=java]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
-- normal (vote)
: Firefox 41
Assigned To: Martyn Haigh (:mhaigh)
:
: Stefan Arentz [:st3fan]
Mentors: Martyn Haigh (:mhaigh)
Michael Comella (:mcomella) [needinfo or I won't see it]
: 1138066 (view as bug list)
Depends on: 1178794 1179195 1180951
Blocks: new-tablet-v2 1161638
  Show dependency treegraph
 
Reported: 2014-05-28 17:48 PDT by Yuan Wang(:Yuan) – Mobile Firefox Design Lead
Modified: 2015-10-27 03:52 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
41+
+


Attachments
Swipe up to close a tab.001.jpg (1.00 MB, image/jpeg)
2014-05-28 17:48 PDT, Yuan Wang(:Yuan) – Mobile Firefox Design Lead
no flags Details
Swipe up to close a tab from tab panel (18.02 KB, patch)
2015-06-19 08:04 PDT, Martyn Haigh (:mhaigh)
sebastian: review+
Details | Diff | Splinter Review

Description User image Yuan Wang(:Yuan) – Mobile Firefox Design Lead 2014-05-28 17:48:33 PDT
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
Comment 1 User image Lucas Rocha (:lucasr) 2014-10-17 07:23:48 PDT
Moving to v2.
Comment 2 User image u421692 2015-03-02 06:19:03 PST
*** Bug 1138066 has been marked as a duplicate of this bug. ***
Comment 3 User image Richard Newman [:rnewman] 2015-03-17 09:54:12 PDT
This is the biggest feedback I've seen about the new tablet UI.
Comment 4 User image Michael Comella (:mcomella) [needinfo or I won't see it] 2015-03-17 10:25:49 PDT
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 User image Martyn Haigh (:mhaigh) 2015-03-17 13:04:39 PDT
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 User image Michael Comella (:mcomella) [needinfo or I won't see it] 2015-03-17 14:37:19 PDT
(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 User image Michael Comella (:mcomella) [needinfo or I won't see it] 2015-03-17 14:38:48 PDT
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 User image Martyn Haigh (:mhaigh) 2015-06-19 08:04:37 PDT
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.
Comment 9 User image Sebastian Kaspari (:sebastian; :pocmo) 2015-06-24 11:18:26 PDT
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 User image Martyn Haigh (:mhaigh) 2015-06-24 12:19:36 PDT
> 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 13 User image Carsten Book [:Tomcat] 2015-06-25 02:32:54 PDT
https://hg.mozilla.org/mozilla-central/rev/192f73676e5d
Comment 14 User image Lawrence Mandel [:lmandel] (use needinfo) 2015-07-03 09:39:57 PDT
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)]:

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