Closed Bug 837113 Opened 7 years ago Closed 7 years ago

Provide UI for closing tabs [closing tabs button has been removed]

Categories

(Firefox for Android :: Theme and Visual Design, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 --- verified
firefox21 --- verified

People

(Reporter: aryx, Assigned: sriram)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Bug 834525 removed the 'Close tab' button. Starting with Firefox for Android nightly 20130201, there is no UI to close tabs. Bug 834525#c3 assumed the tabs had a context menu for closing the tabs, but it hasn't. Furthermore, closing tabs with the context menu would be slow.

Also, some phones tell you to download the manual online, how many users do? So I doubt everybody is aware of swiping for moving the tabs. What do you think about showing UI how to close the button while the user taps on a tab (e.g. an arrow to one side and a 'x' symbol next to it?
This is actually problem with accessibility too. Unfortunately we cannot have context menu since we have a swipe behavior. Android lets us to have either of those, but not both. So, we might need to have a small close button.
Flags: needinfo?(ibarlow)
Req tracking, because this is silly
tracking-fennec: --- → ?
tracking-fennec: ? → ---
(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> Unfortunately we cannot
> have context menu since we have a swipe behavior. Android lets us to have
> either of those, but not both. 

Lucas, any thoughts here? I believe you and I were the ones who had a conversation about removing this originally, so are you aware of any way around these rules in Android?
Flags: needinfo?(ibarlow)
I believe Sriram has a Popup Menu approach working
Attached patch Patch (obsolete) — Splinter Review
This uses a context-menu to show the "close tab". Additionally it shows the title of the page as the context-menu header to give it some context. AdapterContextMenuInfo has a id property, which we dont use (Tab rows don't have an id in our case). I'm re-using it to send the tab's id corresponding to the ContextMenu shown. (If not, we need to cast the view to TabRow and take the value in BrowserApp).
There are few more cleanups.
Attachment #709827 - Flags: review?(mark.finkle)
Why the non-conventional use of unbeknownst context menus and a regression from familiarity for those using the product for over the year?
(In reply to Aaron Train [:aaronmt] from comment #6)
> Why the non-conventional use of unbeknownst context menus and a regression
> from familiarity for those using the product for over the year?

I believe the original intent was to be more "Android-like" and several examples of "closing items in a list" were done using swipe only.

Personally, I am not strongly in favor of either approach. Maybe I need to use it a bit more.

A counterpoint: I notice that Google Chrome does keep a little 'x' in the tab list.
Comment on attachment 709827 [details] [diff] [review]
Patch

Got a screenshot of this for Ian to look at?
http://cl.ly/image/2B3K171B272n <-- This is a screenshot that Ian approved.
http://developer.android.com/guide/practices/ui_guidelines/menu_design.html#dont_put_commands

An entirely orchestrated menu for one item? What else would be planned to justify having a menu?
(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> http://cl.ly/image/2B3K171B272n <-- This is a screenshot that Ian approved.

The menu item's label is not vertically centered. Known issue?
Re: close button. b(In reply to Ian Barlow (:ibarlow) from comment #3)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> > Unfortunately we cannot
> > have context menu since we have a swipe behavior. Android lets us to have
> > either of those, but not both. 
> 
> Lucas, any thoughts here? I believe you and I were the ones who had a
> conversation about removing this originally, so are you aware of any way
> around these rules in Android?

The Android app switcher uses swipe as the main way of removing apps from the list and context menu (via long tap) as the a11y-friendly alternative. We'd be using the same approach. On the other hand, the swipe gestures might not be that obvious for pre-ICS users.

I have a slight preference for removing the close button but I'm not strongly against it. I think the open question here is where we'd place the close button when the tab items are just thumbnails (as opposed to the horizontal layout with thumbnail and label on the side). There is very little space for a close button there, at least with the dimensions we're working with right now. Ian, maybe it's the case to experiment with that a bit?
(In reply to Lucas Rocha (:lucasr) from comment #11)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> > http://cl.ly/image/2B3K171B272n <-- This is a screenshot that Ian approved.
> 
> The menu item's label is not vertically centered. Known issue?

I am not touching android's styles here. When a header is added, it adds some padding below, hence not vertically centered.
(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> This is actually problem with accessibility too. Unfortunately we cannot
> have context menu since we have a swipe behavior. Android lets us to have
> either of those, but not both. So, we might need to have a small close
> button.

This is a bit deceptive. We have a (custom in this case) gesture detector here and can detect long press. We'll just have to create the menu in a slightly different way.

That said, my 2cents, I'm not a huge fan of UI interactions that can only be performed through gestures. We've tried heavily gesture based interfaces multiple times and they almost always not discoverable to users.
(In reply to Wesley Johnston (:wesj) from comment #14)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> > This is actually problem with accessibility too. Unfortunately we cannot
> > have context menu since we have a swipe behavior. Android lets us to have
> > either of those, but not both. So, we might need to have a small close
> > button.
> 
> This is a bit deceptive. We have a (custom in this case) gesture detector
> here and can detect long press. We'll just have to create the menu in a
> slightly different way.

Our gesture detector can detect long press. My patch does it now.
Comment on attachment 709827 [details] [diff] [review]
Patch

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

::: mobile/android/base/TabsTray.java
@@ +414,5 @@
>              switch (e.getActionMasked()) {
>                  case MotionEvent.ACTION_DOWN: {
>                      // Check if we should set pressed state on the
>                      // touched view after a standard delay.
> +                    triggerViewTaps();

This method name is a it misleading. triggerCheckForTaps or something?

@@ +435,5 @@
>                  case MotionEvent.ACTION_UP: {
>                      if (mSwipeView == null)
>                          break;
>  
> +                    cancelViewTaps();

Ditto.

@@ +440,4 @@
>                      mSwipeView.setPressed(false);
>  
>                      if (!mSwiping) {
> +                        TabsTray.this.performItemClick(mSwipeView, TabsTray.this.getPositionForView(mSwipeView), 0);

Nice catch.
We can add a subtle X if we really need to, like this maybe http://cl.ly/image/1x1P1o1t3t0Y
(In reply to Ian Barlow (:ibarlow) from comment #17)
> We can add a subtle X if we really need to, like this maybe
> http://cl.ly/image/1x1P1o1t3t0Y

This looks good to me. We just need to ensure that the hit area for the close button is large enough (definitely larger than the icon itself).
(In reply to Lucas Rocha (:lucasr) from comment #18)
> (In reply to Ian Barlow (:ibarlow) from comment #17)
> > We can add a subtle X if we really need to, like this maybe
> > http://cl.ly/image/1x1P1o1t3t0Y
> 
> This looks good to me. We just need to ensure that the hit area for the
> close button is large enough (definitely larger than the icon itself).

LGTM too. Does this mean we are moving ahead with this proposed design?
Sure let's do it. I'll post some graphics shortly.
(In reply to Lucas Rocha (:lucasr) from comment #16)
> Comment on attachment 709827 [details] [diff] [review]
> Patch
> 
> Review of attachment 709827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/TabsTray.java
> @@ +414,5 @@
> >              switch (e.getActionMasked()) {
> >                  case MotionEvent.ACTION_DOWN: {
> >                      // Check if we should set pressed state on the
> >                      // touched view after a standard delay.
> > +                    triggerViewTaps();
> 
> This method name is a it misleading. triggerCheckForTaps or something?

I tried to think of a name but couldn't. I left it to mfinkle to come up with a name ;)
Attached patch PatchSplinter Review
Copied back the code from beta.
Attachment #714064 - Flags: review?(mark.finkle)
Comment on attachment 714064 [details] [diff] [review]
Patch

This is a patch for Aurora (Fx20) only
Attachment #714064 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #24)
> Comment on attachment 714064 [details] [diff] [review]
> Patch
> 
> This is a patch for Aurora (Fx20) only

Or is that not correct? The "copied from beta" comment made me think "for aurora"
(In reply to Mark Finkle (:mfinkle) from comment #25)
> (In reply to Mark Finkle (:mfinkle) from comment #24)
> > Comment on attachment 714064 [details] [diff] [review]
> > Patch
> > 
> > This is a patch for Aurora (Fx20) only
> 
> Or is that not correct? The "copied from beta" comment made me think "for
> aurora"

This is for 20 and 21. They both don't have a close button.
Comment on attachment 714064 [details] [diff] [review]
Patch

OK. Still r+ for Fx20 and Fx21
Comment on attachment 714064 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New need from users. They want to see an ugly X mark with every tab row!
User impact if declined: Users can still close by swiping. But they won't know they can close it by swiping! No affordance.
Testing completed (on m-c, etc.): Landed in m-i now. Been in the release version for few releases. This code is added back.
Risk to taking this patch (and alternatives if risky): (OMG! OMG!! Merge time!). Very low.
String or UUID changes made by this patch: None.
Attachment #714064 - Flags: approval-mozilla-aurora?
Please make sure that whatever is done here is ouya-friendly. Swiping as the only way to close a tab means you can't close tabs on ouya.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> Please make sure that whatever is done here is ouya-friendly. Swiping as the
> only way to close a tab means you can't close tabs on ouya.

Sriram feels like ordering a ouya ;)
https://hg.mozilla.org/mozilla-central/rev/4e644d23390d
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment on attachment 714064 [details] [diff] [review]
Patch

Approving low risk, regression fix.
Attachment #714064 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:
-build: Firefox for Android 20.0a2 (2013-02-17), Firefox for Android 21.0a1 (2013-02-17)
-device: Samsung Galaxy Nexus
-OS: Android 4.2.1
Status: RESOLVED → VERIFIED
Attachment #709827 - Attachment is obsolete: true
Attachment #709827 - Flags: review?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.