Daniel Holbert [:dholbert] Reporter
	Description • 10 years ago

STR:
 1. View | Toolbars | Menu Bar (to make menu bar always visible)
 2. Focus something on the page (e.g. a textfield at https://pastebin.mozilla.org/ )
 3. Press & release "Alt"
 4. Type another character (e.g. "f")

ACTUAL RESULTS: Menu bar becomes focused in step 3 (though not visibly so -- it's not highlighted or anything), so subsequent characters will trigger menu items. (e.g. "f" opens file menu)

EXPECTED RESULTS: Menu bar shouldn't steal focus -- focus should remain where it was, and my "f" should end up in the textfield.


NOTE: Release version of Firefox (version 26) has EXPECTED BEHAVIOR. I'm guessing the new behavior was introduced with the Australis merge, as part of the new "menubar-hidden-by-default (including on Linux)" behavior.  The focus behavior makes sense when the menubar is hidden, but not when the user has chosen to make it visible.

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 10 years ago

The fact that the menu bar isn't *visibly* focused makes this particularly bad, IMHO - someone might just accidentally hit (and release) "alt" while they're typing (again, with no immediate visible effect) and then accidentally invoke some arbitrary menu command with the next few characters they type. In pre-Australis versions of Firefox, this wouldn't happen.

(Some particularly-footgun-ish menu commands that could be executed, depending on what the next few characters typed: quit firefox / close tab / close window (fq/fc/fw), work offline (fk), change page style to "no style" (vyn), select all & accidentally type over their entire textfield contents [ea])

	neil@parkwaycc.co.uk
	Comment 2 • 10 years ago

IIRC you can flip ui.key.menuAccessKeyFocuses as a workaround. (It was bug 896887 that flipped the preference in the first place.)

	Daniel Holbert [:dholbert] Reporter
	Comment 3 • 10 years ago

Yup, looks like that's a workaround. (Have to restart Firefox after toggling the pref, though.)

	:Gijs (he/him) Assignee
	Comment 4 • 10 years ago

(In reply to neil@parkwaycc.co.uk from comment #2)
> IIRC you can flip ui.key.menuAccessKeyFocuses as a workaround. (It was bug
> 896887 that flipped the preference in the first place.)

Isn't this a core/toolkit bug? That is, I don't think this is fixable on the Firefox frontend side of things. I guess as a workaround we could flip this pref dynamically whenever the menu bar is toggled to be on permanently, but we never needed to do this on Windows, so presumably we shouldn't here, either - or did I miss something?

	neil@parkwaycc.co.uk
	Comment 5 • 10 years ago

This is correct behaviour on Windows, so you've never needed to flip any prefs.

	Dão Gottwald [:dao]
	Comment 6 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #4)
> I guess as a workaround we could flip this
> pref dynamically whenever the menu bar is toggled to be on permanently

Doesn't seem like a viable option as long as this needs a restart.

Is this bug just about unintentionally hitting Alt? I don't understand why one would press and release Alt without expecting it to do anything, so I'm tempted to just wontfix this.

	Daniel Holbert [:dholbert] Reporter
	Comment 7 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #6)
> Is this bug just about unintentionally hitting Alt?

Partly, yeah. Not entirely though (see below).

> I don't understand why
> one would press and release Alt without expecting it to do anything

Maybe you're thinking of maybe executing a menu command, so you hold Alt for a second (preparing to complete the chord), and then you change your mind, release Alt, and try to continue typing? [only to execute some random menu command, based on whatever characters you type next]

Also, it's worth considering the value of consistency with the behavior we've had since forever on this, and consistency with other programs on Linux. (I just tested Gedit & LibreOffice & gnome-terminal, and they match our old behavior).

	:Gijs (he/him) Assignee
	Comment 8 • 10 years ago

Sure, but we can fix the pref needing a restart... How is this?

	Dão Gottwald [:dao]
	Comment 9 • 10 years ago

Comment on attachment 8375084 [details] [diff] [review]
automatically toggle accesskey-focuses on Linux to prevent menubar focus stealing,

But then we should probably also revert http://hg.mozilla.org/mozilla-central/diff/945af538c076/modules/libpref/src/init/all.js and set ui.key.menuAccessKeyFocuses to true in firefox.js.

> function setToolbarVisibility(toolbar, isVisible) {
>   var hidingAttribute = toolbar.getAttribute("type") == "menubar" ?
>                         "autohide" : "collapsed";
> 
>   toolbar.setAttribute(hidingAttribute, !isVisible);
>   document.persist(toolbar.id, hidingAttribute);
>+#ifdef MOZ_WIDGET_GTK
>+  if (hidingAttribute == "autohide") {
>+    Services.prefs.setBoolPref("ui.key.menuAccessKeyFocuses", !isVisible);
>+  }
>+#endif

how about:

  let hidingAttribute;
  if (toolbar.getAttribute("type") == "menubar") {
    hidingAttribute = "autohide";
#ifdef MOZ_WIDGET_GTK
    Services.prefs.setBoolPref("ui.key.menuAccessKeyFocuses", !isVisible);
#endif
  } else {
    hidingAttribute = "collapsed";
  }

	neil@parkwaycc.co.uk
	Comment 10 • 10 years ago

Comment on attachment 8375084 [details] [diff] [review]
automatically toggle accesskey-focuses on Linux to prevent menubar focus stealing,

Setting a pref is unfortunately using a global state to solve a local problem. Ideally there would be a way for nsMenuBarListener to figure out whether the menu is autohide (preferably without grovelling for attributes). Alternatively it might be possible for the menubar to cancel the activation somehow.

>+  if (!mHaveSetupFocusPrefCheck) {
>+    mHaveSetupFocusPrefCheck = true;
>+    Preferences::AddBoolVarCache(&mAccessKeyFocuses,
>+                                 "ui.key.menuAccessKeyFocuses");
>+  }
In layout we set these up from Init methods called from nsLayoutStatics.

	:Gijs (he/him) Assignee
	Comment 11 • 10 years ago

(In reply to neil@parkwaycc.co.uk from comment #10)
> Comment on attachment 8375084 [details] [diff] [review]
> automatically toggle accesskey-focuses on Linux to prevent menubar focus
> stealing,
> 
> Setting a pref is unfortunately using a global state to solve a local
> problem. Ideally there would be a way for nsMenuBarListener to figure out
> whether the menu is autohide (preferably without grovelling for attributes).

What other ways of determining state on the window do we have? Adding properties to the DOM window for this seems way too heavyweight and not so different from an attribute. Firing an event makes somewhat more sense, but is a lot more work to implement (I'm assuming we can't do this using the existing focus event, because it'll lead to other bad things like not being able to focus the bar even if you do actually click on it or hit alt-f or some other actual accesskey-based-shortcut to opening a particular menu.)

	neil@parkwaycc.co.uk
	Comment 12 • 10 years ago

Perhaps some hardcore layout hacker could find a way of determining whether a particular menubar was visible or not without relying on specific DOM behaviour.

The best idea within the boundaries of my knowledge is to add
toolbar[type="menubar"][autohide="true"][inactive="true"]:not([customizing="true"]) {
  visibility: hidden;
}
(there's an existing rule in xul.css for this) and then the menu bar listener could query the (inherited) computed visibility of the menubar and use that to determine whether to ignore the preference. (I use visibility because the other styles already set do not inherit as far as I know.)

	neil@parkwaycc.co.uk
	Comment 14 • 10 years ago

Comment on attachment 8377647 [details] [diff] [review]
Australis - automatically toggle accesskey-focuses on Linux to prevent menubar focus stealing,

Nit: InitializeStatics() confused me for a second there because I was reading nsLayoutStatics::Initialize() ;-)

	:Gijs (he/him) Assignee
	Comment 15 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c9de8e0ce745

	:Gijs (he/him) Assignee
	Comment 16 • 10 years ago

Oops.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c9de8e0ce745

	:Gijs (he/him) Assignee
	Comment 18 • 10 years ago

Comment on attachment 8377647 [details] [diff] [review]
Australis - automatically toggle accesskey-focuses on Linux to prevent menubar focus stealing,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 896887
User impact if declined: linux users find [alt] stealing focus, even when the menu bar is always visible and this is therefore not necessary; toggling the relevant pref requires a restart
Testing completed (on m-c, etc.): on m-c; there are existing automated tests for these features, although their coverage is probably not complete
Risk to taking this patch (and alternatives if risky): low. This patch:
- makes a pref take effect immediately when changed
- adds a minor frontend change to toggle this pref when users toggle the Firefox menubar
- tweaks default prefs so the current Firefox default isn't the default for non-Firefox toolkit apps

String or IDL/UUID changes made by this patch: none

	:Gijs (he/him) Assignee
	Comment 19 • 10 years ago

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/cbe12078ed78

	Ioana (away)
	Comment 20 • 10 years ago

I tried to reproduce this issue on Ubuntu 12.10 x86_x64 and Ubuntu 13.04 x86 with older Firefox 29 builds (including the one from 02/01). Whenever I press Alt, regardless what is focused in Firefox, the Ubuntu "Type your command" dialog is displayed. This, of course, also happens on builds with the fix.

Given the above, I suppose you reproduced this using another Linux version. Could you please verify that this is fixed for you on Firefox 29 and 30?

	Daniel Holbert [:dholbert] Reporter
	Comment 21 • 10 years ago

(In reply to Ioana Budnar, QA [:ioana] from comment #20)
> Whenever I
> press Alt, regardless what is focused in Firefox, the Ubuntu "Type your
> command" dialog is displayed.

Yes, this is one of the primary reasons I don't use Ubuntu's Unity interface. I'm using gnome-shell (installed on Ubuntu), which doesn't co-opt the "alt" key. Sorry for not mentioning that before.

> Could you please verify that this is fixed for you on Firefox 29 and 30?

Sure. I've just now tested 29 beta and 30 aurora (and 31 nightly for good measure). Unable to repro in any of those builds.
--> Verified fixed.