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

VERIFIED FIXED in Firefox 9

Status

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

({ux-consistency, ux-discovery})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

8 years ago
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
Assignee

Updated

8 years ago
Blocks: 656329
Assignee

Comment 1

8 years ago
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.
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...
Assignee: nobody → chrislord.net
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → Android
Hardware: Other → All
Assignee

Comment 3

8 years ago
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.
Status: ASSIGNED → NEW
OS: Android → Linux
Hardware: All → Other
Assignee

Comment 4

8 years ago
Attachment #539456 - Flags: review?(blassey.bugs)
Assignee

Comment 5

8 years ago
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;
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.
Assignee

Comment 7

8 years ago
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 on attachment 539456 [details] [diff] [review]
Target API level 11 to remove the menu key

try run was green
Attachment #539456 - Flags: review?(blassey.bugs) → review+
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.
No longer blocks: 656329
Depends on: 656329
Whiteboard: [do not check in yet - waiting on dependencies]
Status: NEW → ASSIGNED
Assignee

Updated

8 years ago
Summary: Hide the menu key when using the tablet interface → Replace the menu key with a button in the tablet interface
Assignee

Comment 10

8 years ago
Patch to add a tablet menu (previously attached to bug #656329, depends on that bug being applied).
Attachment #542679 - Flags: review?(mark.finkle)
Depends on: 676275
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?
Attachment #542679 - Flags: review?(mark.finkle) → review+
Summary: Replace the menu key with a button in the tablet interface → Replace the menu key with an action bar button in the tablet interface
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.
Attachment #555584 - Flags: review?(mark.finkle)
Attachment #555584 - Flags: review?(mark.finkle) → review+
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.
(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.
http://hg.mozilla.org/mozilla-central/rev/dae898bb6bf3

resolving based on comment 14
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Moved the two remaining patches to bug 681980.
Whiteboard: [inbound]
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110902 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
No longer blocks: 653136
Blocks: 655762
You need to log in before you can comment on or make changes to this bug.