Closed
Bug 1290227
Opened 8 years ago
Closed 8 years ago
replace uses of nsIDOMKeyEvent
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox51 fixed)
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.
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.
Assignee | ||
Comment 2•8 years ago
|
||
Maybe we can switch everything to use KeyboardEvent.key rather than the code.
Updated•8 years ago
|
Whiteboard: [devtools-html] → [devtools-html] [triage]
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [reserve-html]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Updated•8 years ago
|
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Comment hidden (mozreview-request) |
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
Attachment #8779335 -
Flags: review?(bgrinstead)
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.
Comment hidden (mozreview-request) |
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
Attachment #8779335 -
Flags: review?(bgrinstead) → review+
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).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Updated•8 years ago
|
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7daaee5c4c72
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•