Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 10 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 1 • 10 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 10 years ago

Oh, really?

	Masatoshi Kimura [:emk]
	Comment 3 • 10 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 10 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 5 • 10 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 10 years ago

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...

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 10 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 8 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 10 years ago

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 :/

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 10 years ago

(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

	Phil Ringnalda (:philor)
	Comment 13 • 10 years ago

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

	Kohei Yoshino [:kohei]
	Comment 14 • 10 years ago

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/