Closed Bug 1229967 Opened 8 years ago Closed 8 years ago

Hardware menu button no longer closes the menu

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox45 affected, firefox46 verified, firefox47 fixed, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox45 --- affected
firefox46 --- verified
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mcomella, Assigned: JanH, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(1 file)

After bug 1209967, an open menu cannot be closed by a hardware menu key. I realized this was an issue but after digging into it for a little to no avail, figured it wasn't worth fixing over shipping the new feature.

Work-around is to click outside the menu, as you would with a software menu key.
Capella, since you filed bug 1233153, I'm guessing you use a device with a hardware menu button as your primary device. How important is it that the hardware menu button close the 3-dot menu? I figured the change to always have the software menu button on the screen was more important than the ability to close the menu again by the hardware button (when you can click outside of it), but I don't use a device with a hardware menu button.
Flags: needinfo?(markcapella)
I haven't looked, but was assuming this'd be easy to fix. The GS3 for example was pretty popular so I can't be the only one who will fall into this case.

If I couldn't hack a personal fix (as an average user), my options would be adjust to open one way / close another, or just use the 3-dot all the time, and both options there kinda feel wrong.
Flags: needinfo?(markcapella)
We spoke about this today during the front-end team meeting: Finkle doesn't think this is high priority if 1) we can close the menu by touching outside of it and 2) we can close the menu via the back button. On my GS4, I have confirmed that both of these are true. Sebastian also mentioned that the hardware menu button is completely ignored in the Toolbar API so if we ever switched to that, we'd be doing worse. Also, users probably don't expect the hardware menu button to always work.

But we should remain open to feedback from hardware menu key users.
Mentor: michael.l.comella
Whiteboard: [lang=java]
Thanks. I find "users probably don't expect the hardware menu button to always work", to be troubling, and agree we/moz should be alert to further user feedback. (I understand my phone is a little long in the tooth age-wise).
(In reply to Michael Comella (:mcomella) from comment #4)
> Sebastian also mentioned that the
> hardware menu button is completely ignored in the Toolbar API so if we ever
> switched to that, we'd be doing worse. Also, users probably don't expect the
> hardware menu button to always work.

But it seems this is not always the case:

11:23 <@mfinkle> mcomella, sebastian: i have a samsung galaxy s 3, with hardware menu button
11:23 <@mfinkle> mcomella, sebastian: chrome supports both, menu button and 3-dot

However, I don't think this changes the priority.
Or, for consistency, maybe we should just ignore the hardware menu button completely.

This way, it's not unexpected if it doesn't close the menu, since it won't have opened it in the first place.

Sebastian mentioned that Google apps are moving in this direction, so let's move with the platform leaders.
+1 for blocking entirely ...

The annoying part is in breaking the flow /after/ opening the menu then getting briefly stranded mentally trying to close/undo the thing you just did.

I'd personally rather the workflow block occurs at the point where I need to learn the new forward-path, not after I've gone down a rabbit hole  :)
Thinking about this more... Given that other Google apps don't ignore the menu button, I'm starting to think that we should try to find a way to close the menu when the user presses the menu button.

I still don't think this is high priority, but it's something we should try to fix at some point.
I'll look at this on the side (or after) my Telemetry work. If you want to work on it sooner, please take it.

Note that the menu button changes landed in 45 so we should try to uplift if possible.
Assignee: nobody → michael.l.comella
I've just registered just to say you killed my UI experience with this menu button change. I have a SGS4 with HW buttons and I don't want to see the three dots button. I'm always tapping the menu instead of my current tabs

I don't want to see that button taking up the limited space of the address bar! I don't like to see SS buttons, that's why I always buy phones with HW buttons! And Samsung Galaxy series are one of the most popular phones out there.

I don't like that 45.0b change at all!
I don't like to see SW buttons*

Also, I've using the beta for years and this was my first big disappointment with new "features".
comment 11 & comment 12 are more relevant to bug 1236156 – let's continue the discussion there.
I haven't investigated very closely why exactly the hardware menu button stopped closing the menu, but in any case after a bit of digging around, the solution seems relatively simple: Override OnKeyDown() in GeckoMenu and close the menu from there if the menu button was pressed.
Assignee: michael.l.comella → jh+bugzilla
This restores the functionality to close the menu by pressing the hardware menu key.

Review commit: https://reviewboard.mozilla.org/r/45509/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45509/
Attachment #8740025 - Flags: review?(s.kaspari)
Comment on attachment 8740025 [details]
MozReview Request: Bug 1229967 - Handle onKeyDown() for the menu key in GeckoMenu. r=sebastian

The code looks good but I do not have an Android 4+ devices with menu key.

@ahunt: Do you have such a device at the office and can verify that this is working?
Attachment #8740025 - Flags: review?(s.kaspari) → review?(ahunt)
Just to note that the hardware menu key can now also close the menu on the tabs tray screen (New Private Tab/Close All Tabs), which apparently didn't work even before bug 1209967 landed (tested with FF44 release).
driveby: This works great on my GS3 hardware menu button for both main menu and tabs menu cases.
Comment on attachment 8740025 [details]
MozReview Request: Bug 1229967 - Handle onKeyDown() for the menu key in GeckoMenu. r=sebastian

https://reviewboard.mozilla.org/r/45509/#review42451

Looks good to me!

Works on this Nexus S (?) with 4.1 too
Attachment #8740025 - Flags: review?(ahunt) → review+
https://hg.mozilla.org/mozilla-central/rev/d7ccace9b11c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8740025 [details]
MozReview Request: Bug 1229967 - Handle onKeyDown() for the menu key in GeckoMenu. r=sebastian

Approval Request Comment
[Feature/regressing bug #]: bug 1209967
[User impact if declined]: The main menu can no longer be closed via the hardware menu key (where available).
[Describe test coverage new/current, TreeHerder]: manual
[Risks and why]: Low, we just listen for the menu key in GeckoMenu if the menu is visible and close it in response. 
String/UUID change made/needed]: none
Attachment #8740025 - Flags: approval-mozilla-beta?
Attachment #8740025 - Flags: approval-mozilla-aurora?
Blocks: 1265011
Comment on attachment 8740025 [details]
MozReview Request: Bug 1229967 - Handle onKeyDown() for the menu key in GeckoMenu. r=sebastian

46 migrated from beta to release channel today so we should also aim this on m-r.
Attachment #8740025 - Flags: approval-mozilla-release?
Comment on attachment 8740025 [details]
MozReview Request: Bug 1229967 - Handle onKeyDown() for the menu key in GeckoMenu. r=sebastian

Fix for menu key, please uplift to aurora, beta, and m-r. 
We will be doing the 46 RC build from m-r on Monday.
Attachment #8740025 - Flags: approval-mozilla-release?
Attachment #8740025 - Flags: approval-mozilla-release+
Attachment #8740025 - Flags: approval-mozilla-beta?
Attachment #8740025 - Flags: approval-mozilla-beta+
Attachment #8740025 - Flags: approval-mozilla-aurora?
Attachment #8740025 - Flags: approval-mozilla-aurora+
Verified as fixed on Lenovo A536 (Android 4.4.2) on Firefox 46 Beta 12
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.