1641171 - Find a better way to handle browser keyboard shortcut for user activation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Edgar Chen [:edgar]]

+++ This bug was initially created as a follow-up of Bug #1640883 +++

Currently, we don't treat key events with some modifiers as user activation given that we think the user might intend to interact with the browser at that time. But we should find a better way to handle it instead of just ignoring all combinations.

Comment 1

[Edgar Chen [:edgar]]

https://phabricator.services.mozilla.com/D77046#2347175

But could we somehow utilize the lists in https://searchfox.org/mozilla-central/source/dom/events/ShortcutKeyDefinitionsForInputCommon.h and
https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/dom/events/unix/ShortcutKeyDefinitions.cpp
and other similar files. Those should catch, I think, most common cases. (There can be still other cases where some JS code adds event listener.)

Comment 2

[Edgar Chen [:edgar]]

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

But could we somehow utilize the lists in https://searchfox.org/mozilla-central/source/dom/events/ShortcutKeyDefinitionsForInputCommon.h and
https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/dom/events/unix/ShortcutKeyDefinitions.cpp
and other similar files. Those should catch, I think, most common cases. (There can be still other cases where some JS code adds event listener.)

Masayuki, besides https://searchfox.org/mozilla-central/source/dom/events/ShortcutKeyDefinitionsForInputCommon.h and https://searchfox.org/mozilla-central/rev/bc3600def806859c31b2c7ac06e3d69271052a89/dom/events/unix/ShortcutKeyDefinitions.cpp, do you know if there is any other additional thing we could use to filter out the browser key shortcut? Thanks!

Comment 3

[Mike Kaply [:mkaply]]

Can you provide some more detail? I'm unclear why the presence of a modifier changes user intent.

Comment 4

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

edgar, sorry, I'm not sure what are you asking me.

If you meant that "Are there some other shortcut key definitions?", yes there are. XUL all <key> elements are shortcut key definition and the files you pointed is created for each platform. Additonaly, as far as I know some other shortcut keys like F7 for caret browsing and Ctrl+Tab to switch tabs are handled by chrome's keyboard event listeners, so, the latter case, it's hard to look for.

If you meant that "Are there some other shortcut keys in the file which should be treated as user's intentional activation, I'll check it later.

Comment 5

[Edgar Chen [:edgar]]

(In reply to Mike Kaply [:mkaply] from comment #3)

Can you provide some more detail? I'm unclear why the presence of a modifier changes user intent.

We don't want to treat browser shortcut as user's intentional activation given that we think the user is trying to interact with browser instead of web page, so we just filter out key event when modifier is pressed, see https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/widget/TextEvents.h#308-329.

Comment 6

[Edgar Chen [:edgar]]

So what I would like to do in this bug is trying to find a way to filter out the browser shortcut, so we won't treat them as user's intentional activation given that we think that the user is trying to interact with the browser, not the web page.

I found those lists and thought that maybe we could utilize those lists, but wonder if there are any other things I should consider. And it seems we probably should also check the <key> element and the chrome keyboard event listener. Do you know if there is any way that we could know the keyboard event would be handle by the browser as a shortcut around https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/dom/events/EventStateManager.cpp#772 and then we would not try to notify user activation for it? Right now we just ignore keyboard events when a modifier is pressed with some exceptions. Thanks!

Comment 7

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

Ah, maybe, I got it what you mean "browser shortcuts" vs. "Ctrl-X" etc. So, you must want to hook all commands which interact with content only when user kick them with keyboard. Okay, let me think how to do it...

Comment 8

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

Well, if you'd like to check it in content process, it might be a big hint that the keyboard event is requested a reply for the main process.
https://searchfox.org/mozilla-central/rev/819be4899a92213abf121b449779ced662f2ce13/dom/ipc/BrowserChild.cpp#2033
Most shortcut keys should be cancelable. Therefore, chrome process marks keypress events which matched with a non-reserved shortcut key as "wants a reply" before sending the event to focused content process. Currently, this information is not included in localEvent which is dispatched in a remote process, but you can add new flag if it's required. Then, you can ignore such event with whitelist which matches "cmd_cut", etc.

Is this what you want to know?

Comment 9

[Edgar Chen [:edgar]]

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

Is this what you want to know?

Yes, thanks for this information! I will try in this direction.

Comment 10

[Edgar Chen [:edgar]]

Attachment: Bug 1641171 - Have a better way to handle keyboard shortcut for user activation;

We used to filter out all non-printable keys and printable keys combined with
other modifier keys to prevent web content from being activated when users used
certain shortcuts to interact with the browser. However, this was too strict,
as most key combinations are not used for browser shortcuts.

In this patch, we check whether a key handler is registered to a execute command
for the key combination. It no handler is found, the key event is considered user
input for interacting with web content, and user activiation is generated.

Note that we cannot reuse the mWantReplyFromContentProcess event flag directly
because user activation is generated only on the keydown event, whereas key handlers
are ususally registered for the keypress event.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a67bedab0a3a
Have a better way to handle keyboard shortcut for user activation; r=masayuki

Comment 12

[Cristina Horotan [:chorotan]]

Backed out for causing mda failures at test_autoplay_policy_key_blacklist.html

Backout link

Push with failures

Failure log

Comment 13

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&revision=650056d5e89d39f2bfd1d98b18421c3254b04c71

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d96ef3a49db
Have a better way to handle keyboard shortcut for user activation; r=masayuki,media-playback-reviewers,alwu

Comment 15

[Atila Butkovits]

Backed out for causing failures at test_autoplay_policy_key_blacklist.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/e6ffcd10fd26195afe364c7cd03ef5ae135c7ad9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=5d96ef3a49db4c45c882347a0b0d831788bcfd60&selectedTaskRun=f4RmvuHbTseeez-EMazv9g.1

Failure log: https://treeherder.mozilla.org/logviewer?job_id=497817291&repo=autoland&lineNumber=1822

Comment 16

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&revision=1befd0030a328a0856a31ff4b20d561dda4b4680

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb6e58be3101
Have a better way to handle keyboard shortcut for user activation; r=masayuki,media-playback-reviewers,alwu

Comment 18

[Serban Stanca [:SerbanS]]

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