1434837 - autocomplete should listen to keypress events at the system event group

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, enhancement, P1)

Description

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

We'll stop dispatching keypress events for non-printable keys and non-printable key combinations. Therefore, autocomplete should stop using keypress events to handle such keys and key combinations.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6167dd241c6aea32cc406eeadc828df43921fb

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce39f790528dd7f9b6fea5815095fb7a0072ee2

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53ae7e9ef1a59b6ef77c79b4df787c94c2e7726f

Comment 4

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

Hmm, if we change some part of keypress event listeners to keydown event listeners, the order of key event listeners are changed between other modules. That causes a lot of oranges which are difficult to understand. Without event listener order debugging system, we cannot use such ideal approach...

So, perhaps, we should just use keypress event listeners at the system event group for now, unfortunately.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed5963b10fbe59f469f5df727bf04c445a9a8be

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e2329d62d016ae3e58f770a8b942fb5a440a539

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e9426401997c34cffcf342956628e59fd743854

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc46656ca2a606c58694e38773f31d1d0b3b4d9d

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38cbc1c7083b8bdcb1903b8e29d0ae4e3dd3bb6e

Comment 10

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

Attachment: Bug 1434837 - Make autocomplete and satchel listen to keypress event at the system event group

The autocomplete module listens to keypress event for both printable keys and
non-printable keys a lot.  However, we'll stop dispatching keypress event for
non-printable keys in the default event group of web content.  So, autocomplete
should listen to keypress events at the system event group.

Note that it's difficult to change keypress event listeners to keydown event
listeners because if we stop keypress events at preceding keydown event in
autocomplete or satchel module, some other modules fail to handle keydown or
keypress event before autocomplete and it's not easy to investigate which
module's which keypress event listener should be changed to keydown event
listener.  Therefore, this patch doesn't do it at least for now.

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

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8950533 [details]
Bug 1434837 - Make autocomplete and satchel listen to keypress event at the system event group

https://reviewboard.mozilla.org/r/219806/#review227218

LGTM, thanks!

::: toolkit/content/tests/chrome/test_autocomplete_mac_caret.xul:63
(Diff revision 1)
> +  }, {once: true});
> +  synthesizeKey(key, {});
> +  if (result) {
> +    ok(keypressFired, testid + " keypress event should be fired");
> +  } else {
> +    ok(!keypressFired, testid + " keypress event shouldn't be fired");

nit: you could probably unify these:
is(keypressFired, result,
   `${testid} keypress event should${result ? "" : " not"} be fired`);

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2180450ca3cefb25f81a99f5043ebd1537e22249

Comment 13

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

Comment on attachment 8950533 [details]
Bug 1434837 - Make autocomplete and satchel listen to keypress event at the system event group

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0b4ca69376c
Make autocomplete and satchel listen to keypress event at the system event group r=mak

Comment 15

[Stefan Hindli [:stefan_hindli]]

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