868052 - Toolbar does not appear when hardware menu button is pressed (HTC G2, Android 2.3)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Bug 853300 added code to show the toolbar when the menu key is pressed, but this is not working in today's Nightly on my T-Mobile G2 running stock Android 2.3.  I'm not sure if it every worked on this device or not.  I can test old nightly builds to find out.

Comment 1

[Lucas Rocha (:lucasr)]

Looks like a regression. Chris, maybe one of your recent changes to dynamic toolbar?

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> Looks like a regression. Chris, maybe one of your recent changes to dynamic
> toolbar?

Possibly, but nothing comes to mind and the code looks like it should work - I don't have a device with a hardware menu key to test right now and I didn't write the original patch either, might be worth contacting them?

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #2)
> (In reply to Lucas Rocha (:lucasr) from comment #1)
> > Looks like a regression. Chris, maybe one of your recent changes to dynamic
> > toolbar?
> 
> Possibly, but nothing comes to mind and the code looks like it should work -
> I don't have a device with a hardware menu key to test right now and I
> didn't write the original patch either, might be worth contacting them?

Oh, well that was you :) So I'll leave you to investigate, but ping me on IRC if you have any questions.

Comment 4

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

Margaret - Can you get a hardware menu phone to track it down?

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Margaret - Can you get a hardware menu phone to track it down?

Sure, I was able to reproduce on my Nexus S.

Comment 6

[:Margaret Leibovic]

Without really diving into history, I'm not quite sure how the patch in bug 853300 worked. We don't go through the openOptionsMenu() code path for SDK versions < 11:
http://hg.mozilla.org/mozilla-central/annotate/8eebe35aae63/mobile/android/base/GeckoApp.java#l592

GeckoApp shouldn't know about the dynamic toolbar, so maybe I should move this logic into BrowserApp so that I can show the toolbar if necessary.

Comment 7

[:Margaret Leibovic]

Reading the description of this bug, I realize the patch in bug 853300 may have just never worked on 2.3 devices.

Comment 8

[:Margaret Leibovic]

Attachment: patch

I don't have a post-Gingerbread device with a hardware menu button to test, but this works on my Nexus S (2.3 w/ hardware menu button), and doesn't appear to regress the custom menu on my Nexus 4 (4.2 w/ soft menu button).

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 753403 [details] [diff] [review]
patch

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

The changes look a bit suspicious to me. Maybe a bit more background would help.

::: mobile/android/base/BrowserApp.java
@@ +1408,5 @@
>      @Override
>      public void openOptionsMenu() {
> +        if (isDynamicToolbarEnabled() && mLayerView != null) {
> +            mLayerView.getLayerMarginsAnimator().showMargins(false);
> +        }

Not sure I understand why you have to move this call up in the method. For instance, we probably want to just bail when the menu button is pressed while the tabs tray is open.

@@ +1414,5 @@
> +        // Show the default menu on pre-Honeycomb devices.
> +        if (Build.VERSION.SDK_INT < 11) {
> +            super.openOptionsMenu();
> +            return;
> +        }

Wouldn't that naturally happen in the super.openOptionsMenu() call below? mBrowserToolbar.openOptionsMenu() definitely return false because we never show the soft menu button in it on pre-ICS devices.

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 753403 [details] [diff] [review]
> patch
> 
> Review of attachment 753403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes look a bit suspicious to me. Maybe a bit more background would
> help.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1408,5 @@
> >      @Override
> >      public void openOptionsMenu() {
> > +        if (isDynamicToolbarEnabled() && mLayerView != null) {
> > +            mLayerView.getLayerMarginsAnimator().showMargins(false);
> > +        }
> 
> Not sure I understand why you have to move this call up in the method. For
> instance, we probably want to just bail when the menu button is pressed
> while the tabs tray is open.

Hm, that's a good catch. I moved this call up in the method so that we would always be sure to show the dynamic toolbar before returning in the pre-Honeycomb case.

> @@ +1414,5 @@
> > +        // Show the default menu on pre-Honeycomb devices.
> > +        if (Build.VERSION.SDK_INT < 11) {
> > +            super.openOptionsMenu();
> > +            return;
> > +        }
> 
> Wouldn't that naturally happen in the super.openOptionsMenu() call below?
> mBrowserToolbar.openOptionsMenu() definitely return false because we never
> show the soft menu button in it on pre-ICS devices.

Looking at this more closely now, I'm not really sure what I was thinking when I wrote this... you're right that I can just let the call to super.openOptionsMenu() below handle this.

I decided to look at some history to figure out why we have this special case for pre-Honeycomb devices in onKeyDown, and I found that this was introduced as a crash fix in bug 761929. I think calling openOptionsMenu on all devices, not just Honeycomb+ devices, will preserve the effectiveness of that patch, but I can also ask Sriram for review to be sure.

Comment 11

[:Margaret Leibovic]

Attachment: patch v2

Comment 12

[:Margaret Leibovic]

This isn't a regression.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 755027 [details] [diff] [review]
patch v2

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

LGTM

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 755027 [details] [diff] [review]
patch v2

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

Much simpler :-)

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d49e51c7163

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d49e51c7163

Comment 17

[:Margaret Leibovic]

Comment on attachment 755027 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dynamic toolbar
User impact if declined: pressing hardware menu button pre-Honeycomb won't make the toolbar show itself
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): low-risk, removes special-case logic for pre-Honeycomb devices
String or IDL/UUID changes made by this patch: n/a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a6f07b8abbf