1593883 - Allow content to handle Accel+N and Accel+T in Full Screen mode

Status: UNCONFIRMED

(Firefox :: Keyboard Navigation, enhancement, P3)

Description

[penn.lv]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.2 Safari/605.1.15

Steps to reproduce:

• Write a simple html page with scripts that listen to keydown and prevent default when users press Ctrl+N (CMD+N on macOS)
• Open the page
• Go to Full Screen
• Press Ctrl+N

Actual results:

Firefox opens a new browser tab

Expected results:

On both Safari and Chrome, the js code will prevent Ctrl+N being sent to the browser. It's also true when the website is running in standalone mode (PWA). I would expect the same experience on Firefox.

Comment 1

[metastork]

Running Firefox 70.0 on Ubuntu 19.10, and it seems that since this or a very recent version, Firefox has been intercepting more hotkeys than previously from JavaScript keyboard bindings (have tested .

To reproduce:

• Go to https://rawgit.com/jeresig/jquery.hotkeys/master/test-static-01.html (for JQuery hotkeys plugin) or https://wangchujiang.com/hotkeys/ (for a standalone JS hotkeys demo), and try some of the following hotkeys, which don't register (but definitely used to in previous versions of Firefox).
• Press alt+a, alt+q or any other alt+letter combinations, including alt+d and alt+f and other Menu-reserved shortcuts,
• Press control+n (as above) or control+t.

Actual results:

• alt+[letter] hotkeys just register as plain [letter] presses
• control+n and control+t open new window and tab respectively.

Expected behaviour:
*alt+[letter] hotkeys should not bubble to the browser level and should be captured by JS instead - it was definitely possible to reserve these with JS and prevent from bubbling to the browser in prior FF versions (Chromium, for reference, allows JS scripts to capture pretty much all of the alt+[letter] keys).

• control+n and +t not being bindable may be expected behaviour, but for me at least Chromium doesn't seem to allow JS to block this.

Solution:
Am not sure if there's a JS hack that would make some of these functions work again, but as it stands, the lack of alt+[letter] access this has broken some web-apps in Firefox that have bound hotkeys to access their own menus or other functionality, for example.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Ctrl+N is a Firefox UI feature.
-> Firefox

Comment 3

[penn.lv]

Can we get this issue prioritized? Implementing keyboard lock can be one solution to this issue (tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=700123) or FF can be like Safari, which just relaxes the keybindings overrides in FullScreen mode.

Comment 4

[Pranav bhattarai]

This will certainly help one part of the problem which VS Code devs are trying to solve to make https://online.visualstudio.com/ work Firefox.
Currently, it works in Chrome, Edge (Chromium) and Safari (Maybe). As u can see the issue here: https://github.com/microsoft/vscode/issues/83343

Comment 5

[:Harald Kirschner :digitarald]

Referenced both in https://github.com/microsoft/vscode/issues/83343 and https://github.com/microsoft/vscode/issues/85252 as blockers. Might be a low hanging fruit to adapt our shortcut whitelist for fullscreen.

Comment 6

[James Graham [:jgraham]]

As far as I can tell this isn't actually breaking VSCode, but the underlying bug still exists. I'm going to file a specific site report for the VS Code issue.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds like reasonable to cancel reserving some shortcut keys in the fullscreen mode. I think that reserved attribute of <xul:key> should have a new value like "true-if-not-in-fullscreen" and "true-if-not-in-js-fullscreen" and make the shortcut key is reserved even in the both fullscreen modes for id=key_exitFullScreen* elements with "true", and allow Accel + T, Accel + Tab in the user initiated fullscreen mode with "true-if-not-in-js-fullscreen".

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, but looks like that there is no API to check whether the remote process is in the JS fullscreen mode or in the user initiated fullscreem mode. Therefore, GlobalKeyListener cannot consider it with current code. So, we need to make remote process notify parent of fullscreem mode type.

Comment 9

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

Oh, but looks like that there is no API to check whether the remote process is in the JS fullscreen mode or in the user initiated fullscreem mode. Therefore, GlobalKeyListener cannot consider it with current code. So, we need to make remote process notify parent of fullscreem mode type.

We can check if a given browser window is in DOM fullscreen / non-DOM fullscreen (there are attributes on the root of browser.xhtml). Is that sufficient here? Presumably we want to change behaviour in non-DOM fullscreen, and so then it doesn't matter which subframe is handling the key - the rest of the browser should already take care of not sending keypresses to background tabs?

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

Sounds like reasonable to cancel reserving some shortcut keys in the fullscreen mode. I think that reserved attribute of <xul:key> should have a new value like "true-if-not-in-fullscreen" and "true-if-not-in-js-fullscreen"

I'd probably suggest separate attributes rather than overloading the reserved attribute but that feels like bikeshedding. :-)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Gijs (he/him) from comment #9)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

Oh, but looks like that there is no API to check whether the remote process is in the JS fullscreen mode or in the user initiated fullscreem mode. Therefore, GlobalKeyListener cannot consider it with current code. So, we need to make remote process notify parent of fullscreem mode type.

We can check if a given browser window is in DOM fullscreen / non-DOM fullscreen (there are attributes on the root of browser.xhtml). Is that sufficient here?

I guess that it's inDOMFullscreen attribute. I think that it's fine if this is wrapped with an API which returns the state as bool in the C++ world. Perhaps, nsIBrowser?

Presumably we want to change behaviour in non-DOM fullscreen, and so then it doesn't matter which subframe is handling the key - the rest of the browser should already take care of not sending keypresses to background tabs?

Currently, if a shortcut key is reserved, we stop dispatching the keydown and keypress events to focused remote process. My idea here is, we should stop to marking the event as reserved if it's fine to use the reserved key combination only in the UI full screen mode or in the DOM fullscreen mode. Therefore, it does not matter which subframe has focus.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

Sounds like reasonable to cancel reserving some shortcut keys in the fullscreen mode. I think that reserved attribute of <xul:key> should have a new value like "true-if-not-in-fullscreen" and "true-if-not-in-js-fullscreen"

I'd probably suggest separate attributes rather than overloading the reserved attribute but that feels like bikeshedding. :-)

Well, adding new attribute is also fine if it's enough to check the attribute only when reserved="true" (from GlobalKeyListener point of view).

Comment 11

[Edgar Chen [:edgar]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #10)

(In reply to :Gijs (he/him) from comment #9)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

Oh, but looks like that there is no API to check whether the remote process is in the JS fullscreen mode or in the user initiated fullscreem mode. Therefore, GlobalKeyListener cannot consider it with current code. So, we need to make remote process notify parent of fullscreem mode type.

We can check if a given browser window is in DOM fullscreen / non-DOM fullscreen (there are attributes on the root of browser.xhtml). Is that sufficient here?

I guess that it's inDOMFullscreen attribute. I think that it's fine if this is wrapped with an API which returns the state as bool in the C++ world. Perhaps, nsIBrowser?

The chrome document will also enter into fullscreen mode when the fullscreen is initiated from remote process, so we could check if GetFullscreenElement() on chrome document returns a non-nullptr and it is a browser element (if we would like to check if the fullscreen request is from a remote process).

Comment 12

[:Gijs (he/him)]

(In reply to Edgar Chen [:edgar] from comment #11)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #10)

(In reply to :Gijs (he/him) from comment #9)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

Oh, but looks like that there is no API to check whether the remote process is in the JS fullscreen mode or in the user initiated fullscreem mode. Therefore, GlobalKeyListener cannot consider it with current code. So, we need to make remote process notify parent of fullscreem mode type.

We can check if a given browser window is in DOM fullscreen / non-DOM fullscreen (there are attributes on the root of browser.xhtml). Is that sufficient here?

I guess that it's inDOMFullscreen attribute. I think that it's fine if this is wrapped with an API which returns the state as bool in the C++ world. Perhaps, nsIBrowser?

The chrome document will also enter into fullscreen mode when the fullscreen is initiated from remote process, so we could check if GetFullscreenElement() on chrome document returns a non-nullptr and it is a browser element (if we would like to check if the fullscreen request is from a remote process).

I think inDOMFullscreen is only set when we're in DOM fullscreen, not when we're in F11 fullscreen, and I don't think there's a way to be in DOM fullscreen other than for a web content / remote process to have initiated this (and conversely, web content can't "make us" go into non-DOM fullscreen), so I don't understand why we'd need the additional check?

Comment 13

[Edgar Chen [:edgar]]

Yeah, we don't need the additional check. I just wanted to share that GetFullscreenElement() on chrome document can also be used to check if a browser window is in DOM fullscreen in C++. :)

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Describing my digging...

1. GlobalKeyListener calls MarkAsReservedByChrome for reserved shortcuts
2. MarkedAsReservedByChrome then calls StopCrossProcessForwarding which sets a flag
3. Later EventStateManager::HandleCrossProcessEvent checks whether the event is CanBeSentToRemoteProcess which checks that flag. The same handler function later sends IPC message but only when the forwarding is not stopped

Would be nice if HandleCrossProcessEvent can check the fullscreen status of the remote targets and still send it.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #14)

Would be nice if HandleCrossProcessEvent can check the fullscreen status of the remote targets and still send it.

Exactly. See also comment 8 - comment 13.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Yeah. And the document can probably be accessed with something like what's done for escape key.

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1593883 - Pass reserved key combinations to fullscreen content processes

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

F11 is explicitly not default-preventable on Chrome even on fullscreen, and we have a relevant security bug.

Comment 19

[Kagami Rosylight [:saschanaz] (they/them)]

I found what's done in https://phabricator.services.mozilla.com/D178262 and https://searchfox.org/firefox-main/rev/7b1a35d6c70c08d2966c6c8dfc9aa9c04319f721/layout/base/PresShell.cpp#9389-9435 are a bit similar. Can we merge them, so that Escape and F11 (and extra shortcuts) is treated in the same way? If yes, which way would you prefer - to PresShell or GlobalKeyListener? (If the latter, there should be two levels of reserved, while if the former it already implicitly works as a higher level of reserved)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #19)

I found what's done in https://phabricator.services.mozilla.com/D178262 and https://searchfox.org/firefox-main/rev/7b1a35d6c70c08d2966c6c8dfc9aa9c04319f721/layout/base/PresShell.cpp#9389-9435 are a bit similar. Can we merge them, so that Escape and F11 (and extra shortcuts) is treated in the same way?

Well, we could. After D178262, the shortcut keys are separated for entering to and exiting from the fullscreen mode so that we can add Escape only for the exiting from the fullscreen mode and we can specify key="keyup" to the <key> element.

If yes, which way would you prefer - to PresShell or GlobalKeyListener? (If the latter, there should be two levels of reserved, while if the former it already implicitly works as a higher level of reserved)

GlobalKeyListener for easier to maintain. However, it's not enough for the requirements of Escape key handling, I think we should keep current code in the both.