Menu is incorrectly accessible via hardware menu button in editing mode

VERIFIED FIXED in Firefox 27

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 wontfix, firefox27 verified, firefox28 verified, firefox29 wontfix, firefox30 wontfix, firefox31 wontfix, firefox32 wontfix, firefox33 verified, firefox34 verified, firefox35 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

0) On a device with a hardware menu button (e.g. Galaxy Tab)...
1) Click the url bar
2) Hit back
3) Hit the hardware menu button

Expected: No menu pops up
Actual: Menu appears

We don't handle the code paths properly when this occurs so selecting any options puts the browser into an inconsistent and unintuitive state.

Possible fixes:
* Band-aid: Disable the hardware menu button in editing mode, perhaps with a toast explaining the button is disabled
* Fixing the editing mode code paths such that you can use the menu with a hardware button and editing mode will just be exited first before the action is taken

There are a few strange UX flows around the current UI (hit back twice to exit editing mode, especially strange on tablet) so it may be worth investigating a more comprehensive solution. Ian, what do you think?
Flags: needinfo?(ibarlow)
I would actually like to think about the opposite flow too. I do not like pressing BACK twice to get out of editing mode. It's confusing.
What about putting an 'up' button in the title bar when you're in edit mode? Tapping it takes you right out of edit mode, while we maintain the same 2-tap model with the android back button (1 to hide the keyboard, 2 to leave edit mode)

Sketch: http://cl.ly/image/2q1e033U0G3s
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #2)
> What about putting an 'up' button in the title bar when you're in edit mode?

Created bug 957232 for this.

I agree that this is a better flow for exiting edit mode, but what should we do about the menu accessibility in editing mode?
Flags: needinfo?(ibarlow)
Oh heh. I should read bug titles more closely. 

What exactly are you trying to do in the menu while in edit mode? I'm kind of inclined to leave the behaviour as is, unless there is a use case I am missing here.
Flags: needinfo?(ibarlow)
Well, the problem is the menu appears at all and we haven't developed the UX or code paths to correctly take actions when the menu items are pressed.

For example, on tablet, you can click "New Tab", but the toolbar will remain disabled since we're still in editing mode (among other issues). This is solved on devices without hardware menu buttons because the menu button is hidden (phones) or disabled (tablet).

From an engineering standpoint, the two easiest (and thus band-aid) fixes are to exit editing mode before taking any action (losing user progress on the awesomescreen) or to disable the hardware menu button altogether in editing mode.
Either of those sound like reasonable approaches, though I think I slightly favor the latter one, just disable the menu control while in edit mode.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Comment on attachment 8357313 [details] [diff] [review]
Make menu inaccessible during editing mode.

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

::: mobile/android/base/BrowserApp.java
@@ +2017,5 @@
>      @Override
>      public void openOptionsMenu() {
> +        // Disable menu access in edge cases only accessible to hardware menu buttons.
> +        if ((!hasTabsSideBar() && areTabsShown()) ||
> +                mBrowserToolbar.isEditing()) {

You don't need to wrap lines here (yet).

@@ -2475,5 @@
>      // HomePager.OnNewTabsListener
>      @Override
>      public void onNewTabs(String[] urls) {
>          final EnumSet<OnUrlOpenListener.Flags> flags = EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB);
> - 

There's a few whitespace changes in here that are unrelated and should probably be done separately.
Attachment #8357313 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #8)
> You don't need to wrap lines here (yet).

I did it for readability - it's a separation of logic.
Comment on attachment 8357313 [details] [diff] [review]
Make menu inaccessible during editing mode.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Unknown

User impact if declined:
  Users with a hardware menu buttons can open the menu in editing mode (i.e. url bar is selected). Selecting any menu items may cause the browser to go into a somewhat confusing and inconsistent state (in particular, for items from the tools menu), but none that should break the browser's usability. Honestly, I'm not sure if this patch is necessary, but I think it looks sloppy to not have it.
 
Testing completed (on m-c, etc.):
  Tested locally
 
Risk to taking this patch (and alternatives if risky):
  Low. We're using an established isEditing method to determine whether or not we should return early from displaying the options menu. Worst case, we disable the options menu entirely, but it's highly unlikely.

String or IDL/UUID changes made by this patch:
  None
Attachment #8357313 - Flags: approval-mozilla-beta?
Attachment #8357313 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1199a2ef815b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8357313 [details] [diff] [review]
Make menu inaccessible during editing mode.

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
>+        if ((!hasTabsSideBar() && areTabsShown()) ||
>+                mBrowserToolbar.isEditing()) {
>             return;
>+        }

This might be the strangest indent I have come across. Also, I have the exact opposite sense of wrapping for readability.
Attachment #8357313 - Flags: approval-mozilla-beta?
Attachment #8357313 - Flags: approval-mozilla-beta+
Attachment #8357313 - Flags: approval-mozilla-aurora?
Attachment #8357313 - Flags: approval-mozilla-aurora+
Verified as fixed in builds:
- 27 beta 6;
- Aurora 28.0a2 (2014-01-15);
- Nightly 29.0a1 (2014-01-15);
Device: Samsung Galaxy S II (Android 4.1.2)
I r+ this, but I ran into it today and find it really annoying. i.e. the only visual difference between being in editing and out of editing mode on my phone is the lack of a tabs indicator, but the menu button works in one case and requires me to hit back before working in the other. It feels like something is broken and the only reason I realized it wasn't is because I know a lot about our innards. We should back this out.
Flags: needinfo?(ibarlow)
Either that or find a way to quickly land bug 965548
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #17)
> Either that or find a way to quickly land bug 965548

This landed in beta and aurora, where bug 965548 won't get uplifted, even if finished. I'll backout.
Backout is being taken care of in bug 974179.
So shouldn't this be re-opened? I just saw bug 979430 which looks like this.
Duplicate of this bug: 979430
(In reply to Aaron Train [:aaronmt] from comment #20)
> So shouldn't this be re-opened? I just saw bug 979430 which looks like this.

This behavior should be hidden again once bug 965548 is implemented.
Depends on: 965548
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I'm still able to reproduce this issue on the latest builds
This has shipped broken for a while, I think at this point this should be +'ed.
Attachment #8357313 - Attachment is obsolete: true
Attachment #8483779 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/47a4f171d3d3
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
No idea if the intent is to backport this or not.
Flags: needinfo?(michael.l.comella)
Target Milestone: Firefox 29 → Firefox 35
Comment on attachment 8357313 [details] [diff] [review]
Make menu inaccessible during editing mode.

Removing old approval flags so this doesn't show up in the "needs uplift" queries.
Attachment #8357313 - Flags: approval-mozilla-beta+
Attachment #8357313 - Flags: approval-mozilla-aurora+
Wes, does the behavior here seem okay? If so, I'd like to uplift.
Flags: needinfo?(michael.l.comella) → needinfo?(wjohnston)
We've got much better indicators that you're in editing mode now, so I haven't hit problems with it for awhile. Sure :)
Flags: needinfo?(wjohnston)
Comment on attachment 8483779 [details] [diff] [review]
Make menu inaccessible during editing mode

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]:
  Users on devices with hardware menu buttons will be able to open the options menu while in editing mode, which makes assumptions that the menu cannot be opened, so the browser can be put into an inconsistent state. 

[Describe test coverage new/current, TBPL]: None

[Risks and why]: 
  It's conceivable that we could break the options menu open/close state, but it's a simple change so it's unlikely.

[String/UUID change made/needed]: None
Attachment #8483779 - Flags: approval-mozilla-beta?
Attachment #8483779 - Flags: approval-mozilla-aurora?
Comment on attachment 8483779 [details] [diff] [review]
Make menu inaccessible during editing mode

Taking it because it is a small changes and it could help some users.
We will still have time to backout this change in case of problem.
Attachment #8483779 - Flags: approval-mozilla-beta?
Attachment #8483779 - Flags: approval-mozilla-beta+
Attachment #8483779 - Flags: approval-mozilla-aurora?
Attachment #8483779 - Flags: approval-mozilla-aurora+
I am not able to access the menu via hardware button in edit mode using Motorola Razr (Android 4.1.2) and Samsunsg S3 (Android 4.3).
Build: Firefox for Android 35.0a1 (2014-09-21)
Verified as fixed in
Builds:
Firefox for Android 34.0a2 (2014-10-02)
Firefox for Android 33 Beta 8
Device:
Motorola Razr (Android 4.1.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.