1633695 - Move tree code away from system group keypress event listeners and make them use keydown listeners instead

Status: NEW

(Core :: XUL, defect, P5)

Description

[:Gijs (he/him)]

Quoting Emilio:

The benefits are not having to use mozSystemGroup and thus not interacting with the system event listeners, behaving more like the rest of the web.

There's some keypress listeners at https://searchfox.org/mozilla-central/rev/158bac3df3a1890da55bdb6ffdaf9a7ffc0bfb0a/toolkit/content/widgets/tree.js#997-1026 .

Then there are system group listeners in browser-places.js at https://searchfox.org/mozilla-central/rev/158bac3df3a1890da55bdb6ffdaf9a7ffc0bfb0a/browser/base/content/browser-places.js#50 .

I think we can just switch to keydown, as long as we update all the consumers that also interact with these listeners / where the value of defaultPrevented ends up mattering, to match. But having read bug 1449018 (which switched us to system group) I'm not 100% sure anymore.

Masayuki-san, can you confirm if it makes sense to switch away from system group and to use keydown listeners instead?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ideally, we should move it unless keydown event is followed by one or more keypress events on web contents.

However, there are some issues:

1. Using keydown event to handle something means that it gets higher priority than other keypress event listeners. I.e., it may change priority between event handlers.
2. Currently, there is no API to distinguish whether a keydown event is followed by one or more keypress events. I suggested it on UI Events WG, but it was rejected. But if our chrome code needs it, we could implement the API with chrome-only accessible one.
3. There are blacklists to keep the legacy behavior on specific web apps, dom.keyboardevent.keypress.hack.dispatch_non_printable_keys and dom.keyboardevent.keypress.hack.dispatch_non_printable_keys.addl. Therefore, switching event listeners cause breaking something only on the listed web apps.

So, it might be difficult to do it right now, but I guess that the blocklisted apps won't be updated for current behavior though...

Comment 2

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick, wait for a couple of days for ni?) from comment #1)

Ideally, we should move it unless keydown event is followed by one or more keypress events on web contents.

However, there are some issues:

1. Using keydown event to handle something means that it gets higher priority than other keypress event listeners. I.e., it may change priority between event handlers.
2. Currently, there is no API to distinguish whether a keydown event is followed by one or more keypress events. I suggested it on UI Events WG, but it was rejected. But if our chrome code needs it, we could implement the API with chrome-only accessible one.
3. There are blacklists to keep the legacy behavior on specific web apps, dom.keyboardevent.keypress.hack.dispatch_non_printable_keys and dom.keyboardevent.keypress.hack.dispatch_non_printable_keys.addl. Therefore, switching event listeners cause breaking something only on the listed web apps.

OK, but this bug is about <tree>, which isn't web-exposed, so I guess (2) or (3) shouldn't matter here? I guess this means we should check whether there were repercussions for the changes in bug 1627520 though...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, right. So, the issue of here is, the priority changes between <xul:key> vs. <tree>.

Currently, there is no API to check this line:
https://searchfox.org/mozilla-central/rev/158bac3df3a1890da55bdb6ffdaf9a7ffc0bfb0a/toolkit/content/widgets/tree.js#1013
However, this bans any key presses with modifiers. So, I guess that this can be replaced with KeyboardEvent.key value check. In strictly speaking, JS cannot distinguish whether the key value indicates whether specific key name or inputting string, but the former is at least 2 characters (e.g., F1, Fn). So, the following check must be valid for checking same as KeyboardEvent.charCode > 0:

if (event.key.length > 1 || event.key.charCodeAt(0) > 0x7F)

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:enndeakin, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, sorry, I misunderstood the word, "tree". I'll file a meta bug for these changes.