664149 - Replace the menu key with an action bar button in the tablet interface

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110613 Firefox/7.0a1
Build Identifier: 

When using the tablet interface, the menu has an on-screen button and doesn't need the 'menu' shortcut that appears in the shortcuts bar in Honeycomb.

If possible, we should dynamically disable this button when using the tablet interface.

Reproducible: Always

Comment 1

[Chris Lord [:cwiiis]]

The menu key appears because we're targeting an API level lower than 11 - if we change this to 11 in AndroidManifest.xml.in (no need to change the minimum version), it disappears.

Of course, I suppose at some point non-tablet devices will have an API version >= 11, so we probably need to check the screen size and sub-class the application to implement Activity::onCreateOptionsMenu() to get the menu button back again.

I'll work on a patch that does this.

Comment 2

[Matt Brubeck (:mbrubeck)]

Is there any way to hide the menu key while remaining compatible with API level <11?  I suspect not, which means this might need to wait until we can drop support for Android 2.3 and earlier...

Comment 3

[Chris Lord [:cwiiis]]

Targeting API level 11 doesn't mean API level 11 will be required - our minimum version is 5 and that goes unchanged. Targeting API level 11 is the only way to get rid of the menu key (and there's currently no way to get it back, so my idea of having different activities for menu/non-menu is currently invalid).

I guess once ice-cream comes out, they'll be a new API level that will allow conditional menu-key display, perhaps. It's a tiny one liner, but I'll attach a patch anyway.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Target API level 11 to remove the menu key

Comment 5

[Chris Lord [:cwiiis]]

For future reference, we can get the display metrics like this:

DisplayMetrics metrics = new DisplayMetrics();
 getWindowManager().getDefaultDisplay().getMetrics(metrics);

And work out the display size with something like:

double horiz = Math.sqrt(Math.pow(metrics.widthPixels, 2), Math.pow(metrics.heightPixels, 2));
double size = horiz / metrics.densityDpi;

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

I assume that targeting API level 11 will require the sdk on the build bots be updated to include api level 11? If not, can someone who hasn't updated their sdk to include api-level 11 please confirm this doesn't break anything?

Chris, pushing this to try would also be nice.

Comment 7

[Chris Lord [:cwiiis]]

I've been linking against API level 8, so I think this works, but I do have a copy of level 11, so I'm not totally certain. I'd push it to try, but my comitting account is currently frozen (filed a bug, pending).

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 539456 [details] [diff] [review]
Target API level 11 to remove the menu key

try run was green

Comment 9

[Matt Brubeck (:mbrubeck)]

This should not be checked in until bug 656329 (and any related bugs) are checked in and add a menu button to the top toolbar.

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Add a menu button for tablets

Patch to add a tablet menu (previously attached to bug #656329, depends on that bug being applied).

Comment 11

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

Comment on attachment 542679 [details] [diff] [review]
Add a menu button for tablets


>diff --git a/mobile/chrome/content/AppMenu.js b/mobile/chrome/content/AppMenu.js

>     return this.panel = document.getElementById("appmenu");
>   },
> 
>+  get popup() {
>+    delete this.popup;
>+    return this.popup = document.getElementById("appmenu-popup");
>+  },

I wonder if we need both "panel" and "popup"? Could we get by with just a "menu" getter which returns the right value

>+      let chromeReg = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry);
>+      this.popup.setAttribute(chromeReg.isLocaleRTL("global") ? "left" : "right", 0);

Do we need this? Doesn't the anchorTo handle positioning the popup relative to the button?


Overall this is fine and we could land it, but let's remove the "chromeReg" code if we don't need it. Also, I'd like to try to reduce some of the duplication of showing hiding both "panel" and "popup". Maybe a follow up bug?

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: Turn on tablet mode by default

We need to turn on tablet mode by default before pushing attachment 539456 [details] [diff] [review], otherwise there will be no way to open the menu on tablets.

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dae898bb6bf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e157c073cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/da55a48b990f

Comment 14

[Matt Brubeck (:mbrubeck)]

Backed out the platform and pref patches because of test failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/dddbe27e21a8

With the pref set to -1, the Tegra boards trigger tablet mode because of their screen sizes.  We'll need to fix our browser-chrome tests (and possibly other tests) to work in tablet mode or to run with it disabled.

Let's resolve this bug when dae898bb6bf3 (attachment 542679 [details] [diff] [review]) is merged to mozilla-central, and open a new bug for enabling tablet mode by default and updating the target API level.

Comment 15

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

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> Backed out the platform and pref patches because of test failures:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/dddbe27e21a8
> 
> With the pref set to -1, the Tegra boards trigger tablet mode because of
> their screen sizes.  We'll need to fix our browser-chrome tests (and
> possibly other tests) to work in tablet mode or to run with it disabled.

Let's get the tests to run with the pref forced to 0 (forced off). Then we can make a few tablet specific tests that run with the pref forced to "1" (forced on). We can do some simple UI feature tests in that mode and then rever the pref when we are finished.

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/dae898bb6bf3

resolving based on comment 14

Comment 17

[Matt Brubeck (:mbrubeck)]

Moved the two remaining patches to bug 681980.

Comment 18

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110902 Firefox/9.0a1 Fennec/9.0a1