Tom Tromey :tromey Assignee
	Description • 8 years ago

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.

	Tom Tromey :tromey Assignee
	Comment 1 • 8 years ago

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

	Tom Tromey :tromey Assignee
	Comment 2 • 8 years ago

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

	Brian Grinstead [:bgrins]
	Comment 4 • 8 years ago
mozreview-review

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

	Tom Tromey :tromey Assignee
	Comment 5 • 8 years ago

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

	Brian Grinstead [:bgrins]
	Comment 7 • 8 years ago
mozreview-review

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

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

	Tom Tromey :tromey Assignee
	Comment 8 • 8 years ago

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

	Pulsebot
	Comment 11 • 8 years ago

Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7daaee5c4c72
add replacement module for DOM_VK_ constants; r=bgrins

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 8 years ago
bugherder

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