1030113 - Regression: Menu breaks on Firefox 29+ on tablets with hardware menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently, tablets with a hardware menu button can encounter a situation where the menu button does not open the in-browser menu anymore.

This has been reported frequently on SUMO.

Steps to Reproduce (that I can reproduce the issue on)

  * Enter settings via the menu and back out
  * Attempt to hit the menu button again

Hitting the menu button afterwards does nothing.

The only console output on button press:

I/InputDispatcher( 2299): Delivering key to: action: 0x0
D/InputReader( 2299): Input event: value=0 when=1034627826000.

Regression range incoming.

Comment 1

[Aaron Train [:aaronmt]]

Shipped broken in Firefox 29 and 30.

Last good revision: c9ea463d36c3 (2013-12-20)
First bad revision: 7bc1fb6a21ae (2013-12-21)

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9ea463d36c3&tochange=7bc1fb6a21ae

No inbound-builds available.

Comment 2

[Aaron Train [:aaronmt]]

The above was filed against a Samsung GALAXY Tab 3 10" Android 4.2

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Bug 940997 seems rather likely. We have this device in MV and SF so any devs in either office should be able to fix this. Note that this is an x86 device.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Regression and seems to affect quite a few users.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Kevin, any news on this subject? Thanks

Comment 6

[Aaron Train [:aaronmt]]

This needs an assignee for 31; devices are in MV/SF.

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

The first bad revision is:
changeset:   161571:6d3504faf95d
user:        Chenxia Liu <[e-mail]>
date:        Fri Dec 20 10:35:10 2013 -0800
summary:     Bug 910189: Part 1 - Add "restore defaults" hooks into settings. r=margaret

Comment 8

[Chenxia Liu [:liuche]]

Yikes, this looks like something is going wrong with the reflection hack. I'll take a look, and if all else fails, we can try faking a menu item button and maybe use our own GeckoMenu item or something.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Device with hardware keyboard

Ian, I removed this atrocious hack that we had that was causing users to not be able to access our menu on some devices with a hardware button. This turns the 3-dot icon into text though (which is consistent for other apps on these devices, using text instead of icons).

Thoughts?

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Patch: Remove reflection hack

Lucas, I'm not sure if there is actually a way to force the 3-dot menu on some of these devices, other than that atrocious reflection hack - any ideas? I also tried nesting a menu inside an actionbar item, using our 3-dot menu drawable as the icon, but these devices seem to want to use the text instead of the icon, so that didn't work - there was just a clickable blank box where the item should be.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8450693 [details] [diff] [review]
Patch: Remove reflection hack

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

(In reply to Chenxia Liu [:liuche] from comment #10)
> Created attachment 8450693 [details] [diff] [review]
> Patch: Remove reflection hack
> 
> Lucas, I'm not sure if there is actually a way to force the 3-dot menu on
> some of these devices, other than that atrocious reflection hack - any
> ideas?

Yeah, I realize the hw menu has all sorts of discoverability issues but bending the platform to force a different behaviour leads to issues like this. Let's not do this.

> I also tried nesting a menu inside an actionbar item, using our 3-dot
> menu drawable as the icon, but these devices seem to want to use the text
> instead of the icon, so that didn't work - there was just a clickable blank
> box where the item should be.

Let's just let the platform do its thing.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Chenxia, are you going to push that commit? 31 beta 8 will go to build today.

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Patch: Remove reflection hack

I'm not sure if we should uplift this immediately without very much baking, even though it's a very small change, and actually decreases the code complexity significantly.

Screenshots to follow.

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Screenshot: [HW menu] Remove reflection but keep 3-dot menu for non-hw menu devices

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Screenshot: [non-HW menu] Remove reflection but keep 3-dot menu for non-hw menu devices

The first screenshot that was uploaded is for displaying the menu item on all devices (including those with hardware menu items), but this always displays as a text item, which may be more prominent than we want the "Restore defaults" item.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

OK. Thanks for the feedback.
FYI, beta 9 is going to build next Thursday. We probably won't accept a patch later than this.

Comment 17

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/3d7306c70b1e

Comment 18

[Chenxia Liu [:liuche]]

STR:

On previously affected devices, you should now be able to do the following:
1. Hit HW menu button to bring up Fennec menu.
2. Open Settings, and then hit back.
3. Tap on the hw menu button again to open the Fennec menu.

Note: On devices with a HW menu key, this affects how Customize > Search > "Restore defaults" menu item is displayed - the HW menu button must be pressed to display "Restore defaults." On all other devices, the behavior should remain the same (with the 3-dot menu).

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

We don't build beta 9 for Firefox for Android.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d7306c70b1e

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Chenxia, can we have an uplift request for aurora (32)? Except if Kevin or Mark disagree, I won't take it in 31.

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8451944 [details] [diff] [review]
Patch: Remove reflection hack

Approval Request Comment
[Feature/regressing bug #]: Bug 910189
[User impact if declined]: Users of some devices (samsung it seems) with hardware buttons will not be able to access the Settings more than once without restart
[Describe test coverage new/current, TBPL]: nightly
[Risks and why]: low, actually removes riskier code. Almost just code removal
[String/UUID change made/needed]: None

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8451944 [details] [diff] [review]
Patch: Remove reflection hack

Aurora+

Can you please verify the fix before 32 moves to Beta?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c66e5fd06de3