1520263 - preventDefault() does not suppress Caret Browsing dialog when F7 key pressed

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[viktor.meszaros2]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce:

I have a page with following event listener (simplified to demonstrate the issue):

document.addEventListener('keydown',function(event){
event.preventDefault()
})

User press the F7 function key.

Actual results:

Caret Browsing dialog is displayed

Expected results:

Caret Browsing dialog is not displayed

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Masayuki,
Do you know where we handle this case in our code? Thanks!

Comment 2

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

I'm not sure whether here runs in the main process or not:
https://searchfox.org/mozilla-central/rev/78e8de968efd77ef3b30ea081bce07614cd6776b/toolkit/content/widgets/browser-custom-element.js#47-97

But if this runs in the main process, this must handle F7 key press before content process. If so, it is possible to be the reported symptom.

bgrins:

Brian Grinstead, looks like that you're the most active developer of this file.

Is it running on the main process? If so, why does this use event listener to handle F7 rather than XUL <key> element?

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #2)

I'm not sure whether here runs in the main process or not:
https://searchfox.org/mozilla-central/rev/78e8de968efd77ef3b30ea081bce07614cd6776b/toolkit/content/widgets/browser-custom-element.js#47-97

Yes, this runs in the main process (it's bound to the <xul:browser> element which is running in the chrome).

But if this runs in the main process, this must handle F7 key press before content process. If so, it is possible to be the reported symptom.

bgrins:

Brian Grinstead, looks like that you're the most active developer of this file.

Is it running on the main process? If so, why does this use event listener to handle F7 rather than XUL <key> element?

This file was recently renamed from browser.xml in https://bugzilla.mozilla.org/show_bug.cgi?id=1441935, so I'll show up in the blame almost entirely (even though I'm not the most active developer on the file overall). Did this bug start happening only after Bug 1441935? I could definitely see event handling somehow changing between the old (XBL) and new (Custom Element) <browser> implementations.

Previously we had https://searchfox.org/mozilla-central/rev/2a013cc2604004827fd302b247ee2639a5353cf5/toolkit/content/widgets/browser.xml#1974. The old code uses <handler event="keypress" keycode="VK_F7" group="system">, and the new code uses this.addEventListener("keypress", ..., { mozSystemGroup: true }). I thought these would be equivalent, but perhaps they aren't in some cases?

Comment 4

[Brian Grinstead [:bgrins]]

On OSX if I load data:text/html,<script>document.addEventListener('keydown',function(event){event.preventDefault()})</script> and press F7, then I see the Caret Browsing dialog both in Nightly (with the Custom Element <browser>) and in Release (with the XBL <browser>). So I'm assuming this is an existing bug and not a recent regression.

Comment 5

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

(In reply to Brian Grinstead [:bgrins] from comment #3)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #2)

I'm not sure whether here runs in the main process or not:
https://searchfox.org/mozilla-central/rev/78e8de968efd77ef3b30ea081bce07614cd6776b/toolkit/content/widgets/browser-custom-element.js#47-97

Yes, this runs in the main process (it's bound to the <xul:browser> element which is running in the chrome).

Thanks! It's really useful information.

But if this runs in the main process, this must handle F7 key press before content process. If so, it is possible to be the reported symptom.

bgrins:

Brian Grinstead, looks like that you're the most active developer of this file.

Is it running on the main process? If so, why does this use event listener to handle F7 rather than XUL <key> element?

This file was recently renamed from browser.xml in https://bugzilla.mozilla.org/show_bug.cgi?id=1441935, so I'll show up in the blame almost entirely (even though I'm not the most active developer on the file overall).

Oh, sorry for involving you...

Did this bug start happening only after Bug 1441935?

Probably, no. Perhaps, regression of e10s.

Previously we had https://searchfox.org/mozilla-central/rev/2a013cc2604004827fd302b247ee2639a5353cf5/toolkit/content/widgets/browser.xml#1974. The old code uses <handler event="keypress" keycode="VK_F7" group="system">, and the new code uses this.addEventListener("keypress", ..., { mozSystemGroup: true }). I thought these would be equivalent, but perhaps they aren't in some cases?

No, you're right they are completely equivalent as far as I know.

The problem here is, F7 is not registered as shortcut key with XUL <key> element. Therefore, its keydown and keypress events are handled in the chrome process first, and then, sent to focused content process.

Comment 6

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

Resetting assignee which I don't work on in this several months.

Comment 7

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

Attachment: Bug 1520263 - Make `tabbrowser` wait reply event for F7 key before toggling caret browsing mode r=smaug!,agi!

Unfortunately, it seems that F7 key handling in tabbrowser.js cannot
replaced with using XUL <key> element because it works with
ShortcutUtils.getSystemActionForEvent for mapping F7 is a toggle key of
caret browsing mode.

Therefore, this patch exposes some raw information of WidgetEvent related
to cross process event propagation and makes tabbrowser.js check it and
request reply event if it's required.

So, when a remote content has focus, tabbrowser.js will do nothing for both
keydown and keypress of F7 key with original events, then, will request
a reply event if its default is not prevented. Finally, reply F7 keypress
event will toggle caret browsing mode if the event is fired and not consumed.

Depends on D106590

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a1d820cfd7c
Make `tabbrowser` wait reply event for F7 key before toggling caret browsing mode r=smaug

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0a1d820cfd7c