Closed Bug 1290227 Opened 4 years ago Closed 4 years ago

replace uses of nsIDOMKeyEvent

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file)

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.
Note the names are used in key-shortcuts.js; which might be missed normally
since it doesn't reference nsIDOMKeyEvent.
Maybe we can switch everything to use KeyboardEvent.key rather than the code.
Whiteboard: [devtools-html] → [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [reserve-html]
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
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
Attachment #8779335 - Flags: review?(bgrinstead)
(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 on attachment 8779335 [details]
Bug 1290227 - add replacement module for DOM_VK_ constants;

https://reviewboard.mozilla.org/r/70330/#review68304
Attachment #8779335 - Flags: review?(bgrinstead) → review+
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).
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7daaee5c4c72
add replacement module for DOM_VK_ constants; r=bgrins
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
https://hg.mozilla.org/mozilla-central/rev/7daaee5c4c72
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.