1369909 - ctrl+q doesn't quit nightly

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The keyboard shortcut ctrl+q isn't quitting nightly for me on linux. It seems to work in release, so I imagine this is a recent-ish regression.

STR:
Press ctrl and q.
Nightly should close.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:UX keuboards shortcut regression.

Regressed by: Bug 1368734

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1369909 - re-add quit command to quit shortcut (d'oh),

Review commit: https://reviewboard.mozilla.org/r/145610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145610/

Comment 4

[:Gijs (he/him)]

Note that technically, it seems the command wasn't present on the OSX shortcut before. I don't know why not, and I don't know if it matters. Quitting with the shortcut seemed to work fine after testing it on OS X, so I added the command back unconditionally.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8874184 [details]
Bug 1369909 - re-add quit command to quit shortcut (d'oh),

https://reviewboard.mozilla.org/r/145610/#review149518

Ahum. Whoops!

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8874184 [details]
Bug 1369909 - re-add quit command to quit shortcut (d'oh),

https://reviewboard.mozilla.org/r/145610/#review149520

::: browser/base/content/browser-sets.inc:362
(Diff revision 1)
>  #ifdef XP_WIN
>           modifiers="accel,shift"
>  #else
>           modifiers="accel"
>  #endif
> +         command="cmd_quitApplication"

Please exclude OSX from the patch (i.e. make the key conditional on non-OSX again). 
Up until now (including Nightly with the regression), Ctrl+Q works on macOS.

This seems to be caused by the key binding on the quit menu entry (menuitem#menu_FileQuitItem in hiddenWindow.xul)
I discovered this as follows:
1. Visit about:debugging and tick the "Enable add-on debugging" checkbox.
2. Put a breakpoint in goQuitApplication (type goQuitApplication, click on it to jump to the right file).
3. Focus the main Firefox window, press Cmd+Q - now the breakpoint will be triggered and the execution is paused.
4. Run appStartup={quit(){}} before the appStartup.quit() call to abort the quit.
5. Run document.getElementById('menu_FileQuitItem').remove() to remove the binding for the menu item.
6. Continue execution/
7. Press Cmd+Q again and observe that nothing happens (opposed to the browser attempting to quit).

macOS always has this hidden window, because it's the platform convention to keep the application active in the dock even when the windows are closed. I guess that hiddenWindow.xul does not exist on other platforms (at least Linux), hence the need for registering the Ctrl+Q shortcut in browser.xul.


So, why does this matter? Well, now incidentally an add-on can override the Cmd+Q shortcut on macOS because of the way it is implemented. Applying your current patch as-is would break my add-on on macOS (it is already broken on Linux at the moment, but it works on macOS) - https://addons.mozilla.org/en-US/firefox/addon/disable-ctrl-q-and-cmd-q/

Comment 7

[Mike de Boer [:mikedeboer]]

Hi Rob, I was also thinking about this - and adding a comment like 'I can see us ending up re-adding an OSX conditional' - but you just gave us the whole story! Thanks!
So yeah, let's enclose the command with `#ifndef XP_MACOSX` and have tomorrow's Nightly back to normal!
If we can have a short comment that explains the _why_ a bit in-situ, I'd be very happy.

Comment 8

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #6)
> This seems to be caused by the key binding on the quit menu entry
> (menuitem#menu_FileQuitItem in hiddenWindow.xul)

To the best of my knowledge, key="" on a menuitem is purely for decorative purposes (this happens in BuildAcceleratorText). I'm not aware of it actually doing anything, and I can't find any code that does this.

> I discovered this as follows:
> 1. Visit about:debugging and tick the "Enable add-on debugging" checkbox.
> 2. Put a breakpoint in goQuitApplication (type goQuitApplication, click on
> it to jump to the right file).
> 3. Focus the main Firefox window, press Cmd+Q - now the breakpoint will be
> triggered and the execution is paused.
> 4. Run appStartup={quit(){}} before the appStartup.quit() call to abort the
> quit.
> 5. Run document.getElementById('menu_FileQuitItem').remove() to remove the
> binding for the menu item.
> 6. Continue execution/
> 7. Press Cmd+Q again and observe that nothing happens (opposed to the
> browser attempting to quit).
> 
> macOS always has this hidden window, because it's the platform convention to
> keep the application active in the dock even when the windows are closed. I
> guess that hiddenWindow.xul does not exist on other platforms (at least
> Linux), hence the need for registering the Ctrl+Q shortcut in browser.xul.

The hidden window would be used if no browser windows are open. As a browser window *is* open in this case, that's not relevant. Both Linux and OS X have browser-menubar.inc loaded into browser.xul, which defines exactly the same key attribute on the quit item on all of XP_UNIX, so also on Linux, right? So this explanation doesn't make sense to me.

> So, why does this matter? Well, now incidentally an add-on can override the
> Cmd+Q shortcut on macOS because of the way it is implemented. Applying your
> current patch as-is would break my add-on on macOS (it is already broken on
> Linux at the moment, but it works on macOS) -
> https://addons.mozilla.org/en-US/firefox/addon/disable-ctrl-q-and-cmd-q/

I don't understand this either - to disable these shortcuts you would just need to remove the command attribute from the key element in both browser.xul and hiddenWindow.xul, no? What does it do instead? (There's no source link and I don't have time to download, unzip, and trawl through the code myself.)

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs from comment #8)
> The hidden window would be used if no browser windows are open. As a browser
> window *is* open in this case, that's not relevant.

So apparently this is wrong, and (maybe only for shortcuts that browser.xul doesn't handle?) somehow OS X's key handling manages to activate the menu *in the hidden window* (which wasn't obvious to me from comment #6) despite there being an open, focused browser window . This is confusing generally, as the menubar changes depending on what window has focus (open e.g. the browser console to see), but we load the entire normal browser window menus into the hidden window - so do other menus also work? And if so, why, and if not, why not? What's special about 'quit'?

I mean, it's pretty special - there's a lot of code for it in widget/cocoa/. The most bizarre thing is that the cocoa window code seems to *also* specify the quit command key (ie the 'q' bit of accel-q) in both a fallback .properties file as well as hardcoded in nsMenuBarX, but removing the XUL <key> element entirely, or removing the `key` attribute from the menuitem in the XUL also break the shortcut, so apparently those bits aren't being used here. :-\

So goodness knows exactly how this works, and even if I'm not convinced that the ifdef is *necessary*, adding it is probably the most backwards-compatible thing and we're about to branch 55, so meh.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8874184 [details]
Bug 1369909 - re-add quit command to quit shortcut (d'oh),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145610/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/350993973191
re-add quit command to quit shortcut (d'oh), r=mikedeboer

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Rob Wu [:robwu]]

(In reply to :Gijs from comment #8)
> (In reply to Rob Wu [:robwu] from comment #6)
> > So, why does this matter? Well, now incidentally an add-on can override the
> > Cmd+Q shortcut on macOS because of the way it is implemented. Applying your
> > current patch as-is would break my add-on on macOS (it is already broken on
> > Linux at the moment, but it works on macOS) -
> > https://addons.mozilla.org/en-US/firefox/addon/disable-ctrl-q-and-cmd-q/
> 
> I don't understand this either - to disable these shortcuts you would just
> need to remove the command attribute from the key element in both
> browser.xul and hiddenWindow.xul, no? What does it do instead? (There's no
> source link and I don't have time to download, unzip, and trawl through the
> code myself.)

The add-on registers a no-op "command" for the Cmd-Q/Ctrl-Q shortcut, which effectively causes a XUL <key> element to be inserted in the browser document. On macOS, this handler overrides the Cmd-Q/Ctrl-Q handler from the hidden window.
On other platforms, there is already a Ctrl-Q key in the browser document, so registering the Ctrl-Q key has no effect.

Comment 14

[Md. Majedul islam]

I have reproduced this bug with Nightly 55.0a1 (2017-06-02) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20170606100219
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 15

[Andrei Petcu]

Ctrl + Shift + Q does not open and close de devtools network monitor anymore. Is this related?

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to andrei from comment #15)
> Ctrl + Shift + Q does not open and close de devtools network monitor
> anymore. Is this related?

This is an intentional change, made in bug 1368734. Ctrl+Shift+E will now open the Network Monitor for you.

Comment 17

[Andrei Petcu]

Ok. Thank you!