969247 - Get rid of related code of NS_VK_ENTER and nsIDOMKeyEvent::DOM_VK_ENTER

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

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

VK_ENTER is never dispatched by widget. We should remove all handlers of them and test.

Note that I don't think that we should remove KeyboardEvent.DOM_VK_ENTER for preventing JS error.

Comment 1

[Masatoshi Kimura [:emk]]

What kind of JS errors?
KeyboardEvent.DOM_VK_ENTER will not throw even if KeyboardEvent.DOM_VK_ENTER is undefined.

Comment 2

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

Oh, really?

Comment 3

[Masatoshi Kimura [:emk]]

You can easily test the behavior by typing "KeyboardEvent.WHATEVER_NOT_PRESENT" into Web Console.

Comment 4

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

But you'll get undefined if you assign that to some js variable and use that then later, yet expected
it to be a number.

Comment 5

[Masatoshi Kimura [:emk]]

Again, the loose type system will ignore type errors. JavaScript will convert undefined to NaN. Nobody will write a code such as "KeyboardEvent.DOM_VK_ENTER * 3" to calculate the answer to the ultimate question of life, the universe, and everything. The most possible usage would be:
switch (event.keyCode) {
...
case KeyboardEvent.DOM_VK_ENTER:
...
}
which is already dead anyway because we have never generated a keyboard event with DOM_VK_ENTER.

Comment 6

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

Attachment: part.1 Remove or replace DOM_VK_ENTER and NS_VK_ENTER users

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=0b7cb1484ccb

This patch just removes VK_ENTER handlers/dispatchers or replace them with VK_RETURN.

I'm not familiar with marionette part, though...

Comment 7

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

Attachment: part.2 Remove DOM_VK_ENTER and NS_VK_ENTER definition

This is removing VK_ENTER both from nsIDOMKeyEvent and KeyEvent.webidl. If we keep not removing from webidl, I'll remove the change of webidl part before landing.

Comment 8

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

Comment on attachment 8374552 [details] [diff] [review]
part.1 Remove or replace DOM_VK_ENTER and NS_VK_ENTER users

Oops, I realized that I missed catching *.jsm.

Comment 9

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

Attachment: part.1 Remove or replace DOM_VK_ENTER and NS_VK_ENTER users

Comment 10

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

Comment on attachment 8374553 [details] [diff] [review]
part.2 Remove DOM_VK_ENTER and NS_VK_ENTER definition

Tiny bit scary, but I guess we can try.

Comment 11

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

Comment on attachment 8374709 [details] [diff] [review]
part.1 Remove or replace DOM_VK_ENTER and NS_VK_ENTER users

In browser/components/tabview/search.js

+    if (event.keyCode == event.DOM_VK_RETURN &&
+        (matches.length > 0 || others.length > 0)) {
+      thsis.hide(event);
thsis

Are we missing some tests if that passed on try :/

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8374709 [details] [diff] [review]
> part.1 Remove or replace DOM_VK_ENTER and NS_VK_ENTER users
> 
> In browser/components/tabview/search.js
> 
> +    if (event.keyCode == event.DOM_VK_RETURN &&
> +        (matches.length > 0 || others.length > 0)) {
> +      thsis.hide(event);
> thsis
> 
> Are we missing some tests if that passed on try :/

Thank you, nice catch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e947cee33a92
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e9eba7dd1f

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e947cee33a92
https://hg.mozilla.org/mozilla-central/rev/e2e9eba7dd1f

Comment 14

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode#Constants_for_keyCode_value
https://developer.mozilla.org/en-US/Firefox/Releases/30/Site_Compatibility#KeyboardEvent.DOM_VK_ENTER_has_been_removed
https://blog.mozilla.org/addons/2014/05/27/compatibility-for-firefox-30/