1028705 - Move new tab button back to the right

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[bug.zilla]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 (Beta/Release)
Build ID: 20140620030201

Steps to reproduce:

Pressed the tabs button on the top right


Actual results:

New tab button "+" has been moved to the left, requiring the user to move their finger to the left to press it.




Expected results:

Not the biggest issue, but seems an unnecessary complication; it was much easier and quicker to press the tabs and then in the same spot the "+".

Comment 1

[:Margaret Leibovic]

This is similar to bug 1023270, which is about possibly removing that "+" button altogether.

Comment 2

[Ian Barlow (:ibarlow)]

First of all, I definitely do not want us to remove the "+"

As for switching the order of the icons in the tab tray, this is kind of an interesting problem. Depending on the device you're using, this new tab tray menu has either regressed *or* improved the ergonomics of the New Tab button location. Check this out: http://cl.ly/image/1T3x0p1F1r18

If you are on a device that has a hardware menu button, then your tab tray button is on the far top right, so this design change has moved the "+" one space away from where your thumb was when you opened the tray. I agree this is a regression in ergonomics.

However if you are on a device *without* a hardware menu button, that means your browser UI has a menu button next to the tabs button. Which also means that the new location of the "+" icon in the tab tray is in the exact same spot you just tapped to open the tab tray. I would say this is an improvement in ergonomics. 

I'm not sure what to do here, to be honest. Would we consider changing the UI based on whether a hardware menu button exists?

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Yes I think so. If you are on a device with a hardware menu button the hardware button should trigger the menu and we should not display the 3 dot menu.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Gecko menu for tabs panel

This is on a 2.3 device, using our custom Gecko menu instead of the system-style menu. (Next patch makes Gingerbread-style menu.)

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Patch: Hide 3-dot menu for devices with hardware menu buttons

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Gingerbread-style menu

This is a screenshot of the Gingerbread-style menu on 2.3 devices.

The menu looks kind of weird. I don't think I like it. It also requires changing a lot of the browser_app_menu files (for all devices), which I don't think is worth if we are considering dropping Gingerbread soon.

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Patch: Hide 3-dot menu for devices with hardware menu buttons

Realized we have HardwareUtils.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Alt patch

I didn't know you were working on this!

I was playing with a different approach here, reusing the normal menu. Unfortunately, Android doesn't let you inflate new menus into it at runtime very well, so I had to add these items to our normal menus as well... I'm not sure its a good solution. Thought I'd throw it up though. I do think this menu should anchor itself to the bottom of the window when its shown.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Part 2 (alt patch): Use browser_app_menu

Heh, I also made another patch regarding using the browser menu - I used menu item groups, but this is just a proof of concept patch. To actually do it correctly, we need to add these menu items to all the v11, v11-large, v11* etc resources, and also handle setting the default visible/invisible states :/

I'm not really sure it's worth it, because there are going to be lots of edge cases...

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8449861 [details] [diff] [review]
Alt patch

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

Nice! I think I'm going to steal some of this for my first patch.

This actually makes me feel better about this approach, and maybe it's not so bad after all. I was also hoping that we'd get the menu rotation for bug 1028885 for free, but that doesn't seem to be the case.

::: mobile/android/base/BrowserApp.java
@@ +2356,5 @@
> +            // Hide everything else in the menu
> +            final int size = aMenu.size();
> +            for (int i = 0; i < size; i++) {
> +                MenuItem item = aMenu.getItem(i);
> +                item.setVisible(false);

I ran into problems when I tried to switch back to opening the menu for the main app - it wasn't as simple as getting inverting the selection because we have some items that are shown by default, or not (e.g. guest mode).

From just a brief look at our resources, we have different resources for different screen sizes (mainly what's in a submen). The submenus might not make a big difference though, as long as we don't hide/show different menu items on different screens, but it might be more complicated if we different sets of menu items for the resources of different screen sizes.

@@ +2360,5 @@
> +                item.setVisible(false);
> +            }
> +        }
> +
> +        TabsPanel.prepareMenu(aMenu);

I like this, passing a Menu to TabsPanel to adjust as it needs. I might steal this for my patch.

::: mobile/android/base/Tabs.java
@@ +495,5 @@
>                      // Default to white if no color is given
>                      tab.setBackgroundColor(Color.WHITE);
>                  }
>                  tab.setErrorType(message.optString("errorType"));
> +                tab.setMetadata(message.optJSONObject("metadata"));

Hm, what's this for?

::: mobile/android/base/resources/menu-v11/browser_app_menu.xml
@@ +119,5 @@
> +    <item android:id="@+id/close_all_tabs"
> +          android:title="@string/close_all_tabs"/>
> +
> +    <item android:id="@+id/close_private_tabs"
> +          android:title="@string/close_private_tabs"/>

We actually have 4 different browser_app_menu files :/

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +193,4 @@
>  
> +    public static void prepareMenu(Menu  menu) {
> +        if (isShown()) {
> +            menu.findItem(R.id.close_all_tabs).setVisible(mCurrentPanel == Panel.NORMAL_TABS);

In a more final version, you would probably want to check for nulls, because this is static and anyone can call it. But fine for a first pass :)

@@ +439,5 @@
>                  mFooter.setVisibility(View.GONE);
>  
>              mAddTab.setVisibility(View.INVISIBLE);
>  
> +            mMenuButton.setVisibility(HardwareUtils.hasMenuButton() ? View.GONE : View.INVISIBLE);

Oh, I didn't even think about this - invisible is almost certainly better because you don't need to reflow the views, I bet.

@@ +447,5 @@
>  
>              mAddTab.setVisibility(View.VISIBLE);
>              mAddTab.setImageLevel(index);
>  
> +            mMenuButton.setVisibility(HardwareUtils.hasMenuButton() ? View.GONE : View.VISIBLE);

This could just be an if, esp if the menu item starts off as GONE.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8449817 [details] [diff] [review]
Patch: Hide 3-dot menu for devices with hardware menu buttons

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

Looking fine. I noticed that the last menu item looks cut off on devices with the hw menu button. Follow-up? I assume ibarlow has approved this hw menu behaviour?

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Menu item issue

Comment 13

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 8449817 [details] [diff] [review]

> Looking fine. I noticed that the last menu item looks cut off on devices
> with the hw menu button. Follow-up?

Can we fix please it now, and not in a follow up? 

> I assume ibarlow has approved this hw
> menu behaviour?

Yes. I'm a little nervous about discoverability, but we can run with it for now.

Comment 14

[Chenxia Liu [:liuche]]

Hm, I've been poking around the cut-off menu a little, but I'm at a bit of a loss as to what is causing it to behave differently from the original menu. We're using the same code, essentially, but firing off the showMenu from a different entry point. Lucas, any thoughts?

If I can't find the place to fix this, I would be inclined to land this and file a follow-up.

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #14)
> Hm, I've been poking around the cut-off menu a little, but I'm at a bit of a
> loss as to what is causing it to behave differently from the original menu.
> We're using the same code, essentially, but firing off the showMenu from a
> different entry point. Lucas, any thoughts?

My guess is that MenuPanel is mis-measuring the contents of the menu (a ListView). Have a look at what values you're getting after measuring it in:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/MenuPanel.java#32   
 
More specifically, see the values you get from getMeasuredHeight() after the super.onMeasure() calls.

> If I can't find the place to fix this, I would be inclined to land this and
> file a follow-up.

Let's at least try to figure out what's going on here before going ahead.

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Move new tab button back to the right.

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Menu anchored to +

Something is going wrong when a menu is anchored to a "GONE" view, and truncating the last item. I changed this to anchor the menu to the "+" when the menu anchor is gone.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8454280 [details] [diff] [review]
Move new tab button back to the right.

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

Nice.

Comment 19

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/63e2a9b4f650

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63e2a9b4f650

Comment 21

[Chenxia Liu [:liuche]]

Grr realized I missed a couple of lines in a conflict merge:
https://hg.mozilla.org/integration/fx-team/rev/4c74c2073738

Comment 22

[Carsten Book [:Tomcat]]

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