Closed Bug 1030113 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

31 Branch
ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox30 --- wontfix
firefox31 + wontfix
firefox32 + verified
firefox33 + verified
firefox34 --- verified
fennec 33+ ---

People

(Reporter: aaronmt, Assigned: liuche)

References

Details

(Keywords: regression, reproducible)

Attachments

(4 files, 1 obsolete file)

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.
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.
Summary: Regression: Menu breaks on Firefox 31 on tablets with hardware menu → Regression: Menu breaks on Firefox 29+ on tablets with hardware menu
The above was filed against a Samsung GALAXY Tab 3 10" Android 4.2
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.
Regression and seems to affect quite a few users.
Assignee: nobody → kbrosnan
tracking-fennec: ? → 33+
Kevin, any news on this subject? Thanks
Flags: needinfo?(kbrosnan)
This needs an assignee for 31; devices are in MV/SF.
Assignee: kbrosnan → nobody
QA Contact: kbrosnan
The first bad revision is:
changeset:   161571:6d3504faf95d
user:        Chenxia Liu <liuche@mozilla.com>
date:        Fri Dec 20 10:35:10 2013 -0800
summary:     Bug 910189: Part 1 - Add "restore defaults" hooks into settings. r=margaret
Blocks: 910189
Flags: needinfo?(kbrosnan)
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.
Assignee: nobody → liuche
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?
Attached patch Patch: Remove reflection hack (obsolete) — Splinter Review
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.
Attachment #8450693 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #8450693 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Chenxia, are you going to push that commit? 31 beta 8 will go to build today.
Flags: needinfo?(liuche)
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.
Attachment #8450693 - Attachment is obsolete: true
Attachment #8451944 - Flags: review?(mark.finkle)
Flags: needinfo?(liuche)
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.
OK. Thanks for the feedback.
FYI, beta 9 is going to build next Thursday. We probably won't accept a patch later than this.
Attachment #8451944 - Flags: review?(mark.finkle) → review+
Target Milestone: --- → Firefox 33
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).
We don't build beta 9 for Firefox for Android.
https://hg.mozilla.org/mozilla-central/rev/3d7306c70b1e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Chenxia, can we have an uplift request for aurora (32)? Except if Kevin or Mark disagree, I won't take it in 31.
Flags: needinfo?(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
Attachment #8451944 - Flags: approval-mozilla-aurora?
Flags: needinfo?(liuche)
Comment on attachment 8451944 [details] [diff] [review]
Patch: Remove reflection hack

Aurora+

Can you please verify the fix before 32 moves to Beta?
Attachment #8451944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: qawantedverifyme
Duplicate of this bug: 966784
See Also: → 1052394
Duplicate of this bug: 1058027
Keywords: verifyme
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.