1439500 - browser_treeWidget_keyboard_interaction.js will be timed out if we stop dispatching keypress events for non-printable key combinations on web content

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

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

browser_treeWidget_keyboard_interaction.js tests widget for devtools as web content.  Therefore, if we'll stop dispatching keypress events for non-printable keys and key combinations, widget fails to handle them since they use keypress events for handling arrow keys etc.

It's easy to fix this bug with changing only tree widget or making the test to chrome. However, in strictly speaking, we should stop using keypress events in chrome code too if it's easy to fix. So, I'll investigate if all widget in devtools can be rewritten with keydown event listeners.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

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

Comment 5

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

Attachment: Bug 1439500 - Make devtools/client/shared/widgets handle non-printable keys and key combinations with keydown event

We'll stop dispatching keypress event for non-printable keys and key
combinations at least in web content.  This means that widget of
devtools/client cannot be tested with loading them into tabs with mochitest
browser chrome.  Fortunately, they are cleanly independent from other module's
keypress event listeners.  So, we can make they use keydown event to handle
non-printable keys and key combinations.

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

Comment 6

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

FYI: I tried to change all keypress event listeners under devtools/client, but it caused a lot of oranges and I don't understand which keypress event listeners missed to catch expected events due to preventing default of preceding keydown event. (See comment 2's tryserver result.)

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Sorry for the delay, I hope to review this by Monday at the latest.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8953004 [details]
Bug 1439500 - Make devtools/client/shared/widgets handle non-printable keys and key combinations with keydown event

https://reviewboard.mozilla.org/r/222258/#review228912

Thanks for working on this! :) These changes looks reasonable to me.

::: commit-message-994a6:1
(Diff revision 1)
> +Bug 1439500 - Make devtools/client/sared/widgets handle non-printable keys and key combinations with keydown event r?jryans

Nit: sared -> shared

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is it correct that all `keypress` listeners for non-printables in content documents must be converted to `keydown`?

In addition, is it correct that for chrome documents, no change is required, but it is suggested to convert them as well as a best practice?  Do we plan to leave chrome documents like this forever, or will there be an eventual migration there as well?

Comment 10

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

Thank you for your review!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> Is it correct that all `keypress` listeners for non-printables in content
> documents must be converted to `keydown`?

Ideally, yes. However, if keydown event's preventDefault() is called, following keypress event won't be fired. So, if a keypress event listener handles a key before another one but the another one is just replaced with keydown event handler, the handling order is swapped. So, this point makes me difficult to rewrite a lot of chrome keypress event listeners with keydown event listeners.

> In addition, is it correct that for chrome documents, no change is required,
> but it is suggested to convert them as well as a best practice?

So, yes.

> Do we plan
> to leave chrome documents like this forever, or will there be an eventual
> migration there as well?

Currently, I have no plan because this change even in chrome makes Thunderbird and Seamonkey broken completely. But when you and your colleagues start to handle keyboard events newly, I'd be happy if you use keydown event listener as far as possible.

Comment 11

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

Comment on attachment 8953004 [details]
Bug 1439500 - Make devtools/client/shared/widgets handle non-printable keys and key combinations with keydown event

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efa7b8392cac
Make devtools/client/shared/widgets handle non-printable keys and key combinations with keydown event r=jryans

Comment 13

[Tiberius Oros[:tiberius_oros]]

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

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(Just for future investigation...  I was confused why only this these few widgets were impacted.  `browser_treeWidget_keyboard_interaction.js` creates a data: URI document inside the toolbox to use as the UI during testing, so it follows the new content document rules for these key events.  In our production usage of the toolbox, panels load as chrome documents, so they aren't currently impacted, but it would be better to update them anyway.)