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

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({dev-doc-complete})

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
What kind of JS errors?
KeyboardEvent.DOM_VK_ENTER will not throw even if KeyboardEvent.DOM_VK_ENTER is undefined.
You can easily test the behavior by typing "KeyboardEvent.WHATEVER_NOT_PRESENT" into Web Console.
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.
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.
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...
Attachment #8374552 - Flags: review?(bugs)
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.
Attachment #8374553 - Flags: superreview?(bugs)
Attachment #8374553 - Flags: review?(bugs)
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.
Attachment #8374552 - Flags: review?(bugs)
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.
Attachment #8374553 - Flags: superreview?(bugs)
Attachment #8374553 - Flags: superreview+
Attachment #8374553 - Flags: review?(bugs)
Attachment #8374553 - Flags: review+
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 :/
Attachment #8374709 - Flags: review?(bugs) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/e947cee33a92
https://hg.mozilla.org/mozilla-central/rev/e2e9eba7dd1f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 973299
Blocks: 977544
Blocks: 975381
Blocks: 106327
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.