1290227 - replace uses of nsIDOMKeyEvent

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

De-chrome-ification will require replacing all uses of nsIDOMKeyEvent.
This is used for various DOM_VK_* constants.

This is available to content via KeyEvent.DOM_VK_*, but that seems to
be unportable.

One option might be to expose a "keycode-js" pseudo-module, that just
resolves to KeyEvent; and then use this npm package:
https://www.npmjs.com/package/keycode-js
Though, strangely, that package exposes the constants with different names...
maybe there's some other option.

Comment 1

[Tom Tromey :tromey]

Note the names are used in key-shortcuts.js; which might be missed normally
since it doesn't reference nsIDOMKeyEvent.

Comment 2

[Tom Tromey :tromey]

Maybe we can switch everything to use KeyboardEvent.key rather than the code.

Comment 3

[Tom Tromey :tromey]

Attachment: Bug 1290227 - add replacement module for DOM_VK_ constants;

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8779335 [details]
Bug 1290227 - add replacement module for DOM_VK_ constants;

https://reviewboard.mozilla.org/r/70330/#review67820

I like the approach, just a couple of questions

::: devtools/client/inspector/shared/utils.js
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -/* eslint-disable mozilla/reject-some-requires */
> -const {Ci} = require("chrome");

Nice

::: devtools/client/shared/keycodes.js:15
(Diff revision 1)
> +
> +/**
> + * Keyboard handling is a mess. http://unixpapa.com/js/key.html
> + * It would be good to use DOM L3 Keyboard events,
> + * http://www.w3.org/TR/2010/WD-DOM-Level-3-Events-20100907/#events-keyboardevents
> + * however only Webkit supports them, and there isn't a shim on Monernizr:

typo: Modernizr

::: devtools/client/shared/keycodes.js:150
(Diff revision 1)
> +  DOM_VK_VOLUME_DOWN: 182,
> +  DOM_VK_VOLUME_UP: 183,
> +};
> +
> +if (this.KeyEvent) {
> +  exports.KeyCodes = this.KeyEvent;

I don't understand why we have this condition.  Can't we just unconditionally use the list defined here for simplicity?

::: devtools/client/shared/test/browser_keycodes.js:8
(Diff revision 1)
> +
> +"use strict";
> +
> +const {CompatKeyCodes, KeyCodes} = require("devtools/client/shared/keycodes");
> +
> +function test() {

Please use add_task instead of test()/finish()

::: devtools/shared/builtin-modules.js:250
(Diff revision 1)
>    XMLHttpRequest: function () {
>      return Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
>             .createInstance(Ci.nsIXMLHttpRequest);
>    },
>  
> +  KeyEvent: Ci.nsIDOMKeyEvent,

Ditto

Comment 5

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #4)

> typo: Modernizr

I'll fix it at the original spot as well.

> > +if (this.KeyEvent) {
> > +  exports.KeyCodes = this.KeyEvent;
> 
> I don't understand why we have this condition.  Can't we just
> unconditionally use the list defined here for simplicity?

Yeah.  I think my thinking was that it's possible that at some point in the future,
we'll need to change the constants for some other browser.  This area isn't very 
portable.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8779335 [details]
Bug 1290227 - add replacement module for DOM_VK_ constants;

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

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8779335 [details]
Bug 1290227 - add replacement module for DOM_VK_ constants;

https://reviewboard.mozilla.org/r/70330/#review68304

Comment 8

[Tom Tromey :tromey]

Testing showed one bad hunk in the patch (in StyleEditorUI.jsm)
and one test case needing a small update (browser_flame-graph-05.js
made a phony key event, but the change made some code fire that
previously was not).

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8779335 [details]
Bug 1290227 - add replacement module for DOM_VK_ constants;

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

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8779335 [details]
Bug 1290227 - add replacement module for DOM_VK_ constants;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70330/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7daaee5c4c72
add replacement module for DOM_VK_ constants; r=bgrins

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7daaee5c4c72