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?

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

(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?

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.

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.

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.

(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

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.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f64f1f2c4c4 https://hg.mozilla.org/releases/mozilla-beta/rev/b40e5b4e6f4f

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.

Either that or find a way to quickly land bug 965548

(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.

(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.

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.

No idea if the intent is to backport this or not.

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.

Wes, does the behavior here seem okay? If so, I'd like to uplift.

We've got much better indicators that you're in editing mode now, so I haven't hit problems with it for awhile. Sure :)

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

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.

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)

https://hg.mozilla.org/releases/mozilla-aurora/rev/1fdd40c163c6 https://hg.mozilla.org/releases/mozilla-beta/rev/91f4e2aed979

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)