Closed Bug 1441144 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, enhancement)

60 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file)

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.
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 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 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
Attachment #8954027 - Flags: review?(dtownsend) → review+
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/1fa874194a00
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.