1441144 - browser/extensions/onboarding/content/onboarding.js should handle non-printable keys with keydown event listener rather than keypress event listener

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

We'll stop dispatching keypress event in the default event group of web content (bug 968056).  So, onboarding.js needs to handle non-printable keys with keydown event listener.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Attachment: Bug 1441144 - Make browser/extensions/onboarding/content/onboarding.js handle non-printable keys with "keydown" event listener rather than "keypress" event listener

We'll stop dispatching "keypress" event for non-printable keys and key
combinations in the default event group of web content.  Therefore,
onbording.js needs to stop handling some keys with "keypress" event listener.

This patch makes add a "keydown" event listener, handleKeydown(), and make it
handle only non-printable keys.  Note that we'll keep dispatching "keypress"
event for " " and "Enter".  So, we can keep handling them in "keypress"
event listener.

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Although not scope of this bug, checking KeyboardEvent.key with " " must be wrong. IIRC, spacebar of some keyboard layouts produces other Unicode whitespace character instead of ASCII whitespace. Perhaps, KeyboardEvent.code should be checked instead.

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8954027 [details]
Bug 1441144 - Make browser/extensions/onboarding/content/onboarding.js handle non-printable keys with "keydown" event listener rather than "keypress" event listener

https://reviewboard.mozilla.org/r/223176/#review229114


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/extensions/onboarding/content/onboarding.js:785
(Diff revision 1)
>      }
>      event.stopPropagation();
>    }
>  
> +  handleKeypress(event) {
> +    let { target, key, shiftKey } = event;

Error: 'shiftkey' is assigned a value but never used. allowed unused vars must match /^cc|ci|cu|cr|exported_symbols/. [eslint: no-unused-vars]

::: browser/extensions/onboarding/content/onboarding.js:802
(Diff revision 1)
> +    // Currently focused item could be tab container if previous navigation was done
> +    // via mouse.
> +    if (target.classList.contains("onboarding-tour-item-container")) {
> +      target = target.firstChild;
> +    }
> +    let targetIndex;

Error: 'targetindex' is defined but never used. allowed unused vars must match /^cc|ci|cu|cr|exported_symbols/. [eslint: no-unused-vars]

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Comment on attachment 8954027 [details]
Bug 1441144 - Make browser/extensions/onboarding/content/onboarding.js handle non-printable keys with "keydown" event listener rather than "keypress" event listener

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

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8954027 [details]
Bug 1441144 - Make browser/extensions/onboarding/content/onboarding.js handle non-printable keys with "keydown" event listener rather than "keypress" event listener

https://reviewboard.mozilla.org/r/223176/#review229640

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fa874194a00
Make browser/extensions/onboarding/content/onboarding.js handle non-printable keys with "keydown" event listener rather than "keypress" event listener r=mossop

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1fa874194a00