1436926 - EventUtils.synthesizeKey() should set code value automatically unless the caller specifies code value explicitly

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement, P2)

Description

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

When I touch existing tests, I rewrite some tests as: |synthesizeKey("VK_RETURN", {})| to |synthesizeKey("KEY_Enter", { code: "Enter"})| to synthesize actual key event caused by physical keyboard operation.

However, the code value is a code value of ANSI keyboard layout in almost all cases. So, if the caller doesn't specify the code value explicitly, synthesizeKey() should set code value from key or keyCode value automatically.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: Bug 1436926 - part 1: EventUtils.synthesizeKey() should guess KeyboardEvent.code value if it's not specified explicitly

For testing key operation stricter, all automated tests should set
KeyboardEvent.code properly.  However, in most cases, automated tests
assumes that active keyboard layout is US (ANSI) keyboard layout.

Therefore, synthesizeKey() should guess KeyboardEvent.code value from
KeyboardEvent.key value as US keyboard layout automatically.

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

Comment 4

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

Attachment: Bug 1436926 - part 2: Remove unnecessary KeyboardEvent.code specification of callers of EventUtils.synthesizeKey()

Now, callers of EventUtils.synthesizeKey() don't need to specify
KeyboardEvent.code value anymore if they assume that active keyboard layout
is US keyboard layout.

Note that this patch changes the meaning of only test_bug551434.html.
Some callers in it don't match the key value and code value but that looks
like that they don't checking such odd keyboard events.  So, they must be
bug of the test.

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950528 [details]
Bug 1436926 - part 1: EventUtils.synthesizeKey() should guess KeyboardEvent.code value if it's not specified explicitly

https://reviewboard.mozilla.org/r/219056/#review225668

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8950529 [details]
Bug 1436926 - part 2: Remove unnecessary KeyboardEvent.code specification of callers of EventUtils.synthesizeKey()

https://reviewboard.mozilla.org/r/219058/#review225670

rs+

::: toolkit/components/satchel/test/test_form_autocomplete.html:550
(Diff revision 1)
>        break;
>  
>      case 61:
>        checkMenuEntries(["value2"], testNum);
>        // Check the new first entry (of 1)
> -      synthesizeKey("KEY_ArrowDown", {code: "ArrowDown"});
> +      synthesizeKey("KEY_ArrowDown", {});

all fine, bug in case {} is the last param, do we really need it?

Comment 7

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

Comment on attachment 8950529 [details]
Bug 1436926 - part 2: Remove unnecessary KeyboardEvent.code specification of callers of EventUtils.synthesizeKey()

https://reviewboard.mozilla.org/r/219058/#review225670

> all fine, bug in case {} is the last param, do we really need it?

Yeah, I think that it should be optional argument.  Then, we can get rid of sendKey().

Comment 8

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a1ad81ceeed29f6de94c3b64b3358b7606e26b51 -d 0aab6c91fb14: rebasing 447036:a1ad81ceeed2 "Bug 1436926 - part 1: EventUtils.synthesizeKey() should guess KeyboardEvent.code value if it's not specified explicitly r=smaug"
rebasing 447037:077240f46df8 "Bug 1436926 - part 2: Remove unnecessary KeyboardEvent.code specification of callers of EventUtils.synthesizeKey() r=smaug" (tip)
merging layout/forms/test/test_bug348236.html
warning: conflicts while merging layout/forms/test/test_bug348236.html! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

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

Comment on attachment 8950528 [details]
Bug 1436926 - part 1: EventUtils.synthesizeKey() should guess KeyboardEvent.code value if it's not specified explicitly

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

Comment 10

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

Comment on attachment 8950529 [details]
Bug 1436926 - part 2: Remove unnecessary KeyboardEvent.code specification of callers of EventUtils.synthesizeKey()

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6768ec01e5a
part 1: EventUtils.synthesizeKey() should guess KeyboardEvent.code value if it's not specified explicitly r=smaug
https://hg.mozilla.org/integration/autoland/rev/b1aba58ebcdd
part 2: Remove unnecessary KeyboardEvent.code specification of callers of EventUtils.synthesizeKey() r=smaug

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b6768ec01e5a
https://hg.mozilla.org/mozilla-central/rev/b1aba58ebcdd