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

Status: VERIFIED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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?

Comment 1

[Sriram Ramasubramanian [:sriram]]

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.

Comment 2

[Aaron Train [:aaronmt]]

Req tracking, because this is silly

Comment 3

[Ian Barlow (:ibarlow)]

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

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I believe Sriram has a Popup Menu approach working

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

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.

Comment 6

[Aaron Train [:aaronmt]]

Why the non-conventional use of unbeknownst context menus and a regression from familiarity for those using the product for over the year?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 709827 [details] [diff] [review]
Patch

Got a screenshot of this for Ian to look at?

Comment 9

[Sriram Ramasubramanian [:sriram]]

http://cl.ly/image/2B3K171B272n <-- This is a screenshot that Ian approved.

Comment 10

[Aaron Train [:aaronmt]]

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?

Comment 11

[Lucas Rocha (:lucasr)]

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

Comment 12

[Lucas Rocha (:lucasr)]

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?

Comment 13

[Sriram Ramasubramanian [:sriram]]

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

Comment 14

[Wesley Johnston (:wesj)]

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

Comment 15

[Sriram Ramasubramanian [:sriram]]

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

[Lucas Rocha (:lucasr)]

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.

Comment 17

[Ian Barlow (:ibarlow)]

We can add a subtle X if we really need to, like this maybe http://cl.ly/image/1x1P1o1t3t0Y

Comment 18

[Lucas Rocha (:lucasr)]

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

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 20

[Ian Barlow (:ibarlow)]

Sure let's do it. I'll post some graphics shortly.

Comment 21

[Sriram Ramasubramanian [:sriram]]

(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 ;)

Comment 22

[Ian Barlow (:ibarlow)]

Attachment: "close tab" icons, in mdpi/hdpi/xhdpi

Comment 23

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Copied back the code from beta.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 714064 [details] [diff] [review]
Patch

This is a patch for Aurora (Fx20) only

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 26

[Sriram Ramasubramanian [:sriram]]

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

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 714064 [details] [diff] [review]
Patch

OK. Still r+ for Fx20 and Fx21

Comment 28

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e644d23390d

Comment 29

[Sriram Ramasubramanian [:sriram]]

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.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 31

[Sriram Ramasubramanian [:sriram]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 ;)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e644d23390d

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 714064 [details] [diff] [review]
Patch

Approving low risk, regression fix.

Comment 34

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ec33ec55ff5

Comment 35

[Andreea Pod]

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