956858 - Menu is incorrectly accessible via hardware menu button in editing mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 2

[Ian Barlow (:ibarlow)]

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

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 4

[Ian Barlow (: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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 6

[Ian Barlow (:ibarlow)]

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 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Make menu inaccessible during editing mode.

Comment 8

[Wesley Johnston (:wesj)]

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.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/1199a2ef815b

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1199a2ef815b

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Flaviu Cos]

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)

Comment 16

[Wesley Johnston (:wesj)]

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.

Comment 17

[Ian Barlow (:ibarlow)]

Either that or find a way to quickly land bug 965548

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Backout is being taken care of in bug 974179.

Comment 20

[Aaron Train [:aaronmt]]

So shouldn't this be re-opened? I just saw bug 979430 which looks like this.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 23

[Catalin Suciu [:csuciu]]

I'm still able to reproduce this issue on the latest builds

Comment 24

[Aaron Train [:aaronmt]]

This has shipped broken for a while, I think at this point this should be +'ed.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Make menu inaccessible during editing mode

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/47a4f171d3d3

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/47a4f171d3d3

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 31

[Wesley Johnston (:wesj)]

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 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 33

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 34

[Cristina Madaras, QA [:CristinaM]]

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)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Cristina Madaras, QA [:CristinaM]]

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)