This keyset element is used to inject key shortcuts to toolbox doc (specifically used in the debugger: https://dxr.mozilla.org/mozilla-central/search?q=useKeyWithSplitConsole&redirect=true. When this is done we should also remove the <keyset id="toolbox-keyset"> element in toolbox.xul. But note that there are elements with the same ID used for window host / browser toolbox (see _addKeysToWindow) -- we don't need to change those for now since the window host only makes sense when XUL is available.

Couple of observations before I submit the first patches for review. First of all, the only current user of this API is the old debugger, and I don't think it's worth migrating it to use key-shortcuts. The approach I'm taking is just to attempt to translate the current <key> elements to something that can be used with key-shortcut.js. Even though 8 shortcuts are forwarded to the toolbox (in english: F8, F10, F11, shift+F11, /, ', ;, shift+;) only the Function keys can actually be used when the splitconsole is focused. Not sure which mechanism is preventing the others from being processed, but typing ";" while the focus is in the splitconsole will type ";" in the console input but will have no impact on the debugger. To preserve this behavior, I think it's fair to restrict the useKeysWithSplitConsole API to function keys only, since all the other keys should be preserved to type & navigate in the console input.

(In reply to Julian Descottes [:jdescottes] from comment #1) > > Even though 8 shortcuts are forwarded to the toolbox (in english: F8, F10, > F11, shift+F11, /, ', ;, shift+;) only the Function keys can actually be > used when the splitconsole is focused. Not sure which mechanism is > preventing the others from being processed, but typing ";" while the focus > is in the splitconsole will type ";" in the console input but will have no > impact on the debugger. To preserve this behavior, I think it's fair to > restrict the useKeysWithSplitConsole API to function keys only, since all > the other keys should be preserved to type & navigate in the console input. I missed the fact that the non-function shortcuts were relying on modifiers. That being said, I can't get any of the following shortcuts to work on OSX & Windows: - CmdOrCtrl+/ (resume) - CmdOrCtrl+' (step over) - CmdOrCtrl+; (step in) - CmdOrCtrl+shift+; (step out) None of those are documented on https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts, might be just dead code? James can you confirm?

Comment on attachment 8791206 [details] Bug 1297113 - Fix eslint errors in toolbox.js; https://reviewboard.mozilla.org/r/78364/#review77308

Comment on attachment 8791207 [details] Bug 1297113 - Remove unused debugger shortcut keys; https://reviewboard.mozilla.org/r/78366/#review77310 I feel like James or someone maintaining the debugger should review that. Note that sometimes, alternative key shortcuts are used in non-engligh locales to accomodate different keyboard layout.

Comment on attachment 8791208 [details] Bug 1297113 - Convert useKeysWithSplitConsole to use key-shorcut.js; https://reviewboard.mozilla.org/r/78368/#review77290 Looks good to me. It may have been better to refactor to use key-shotrcuts but it looks like many test are depending on xul keys so that xul:key to electron-string converter is a better deal. ::: devtools/client/debugger/panel.js:65 (Diff revision 1) > - this._toolbox.useKeyWithSplitConsole(elm, "jsdebugger"); > + let keycode = elm.getAttribute("keycode"); > + let modifiers = elm.getAttribute("modifiers"); > + let command = elm.getAttribute("command"); > + let handler = this._view.Toolbar.getCommandHandler(command); > + > + keycode = this.translateToKeyShortcut(keycode, modifiers); I would have named the result `key` or `shortcut` as it isn't a keycode but an electron key string. ::: devtools/client/framework/test/browser_toolbox_split_console.js:57 (Diff revision 1) > + }, "jsdebugger"); > > info("synthesizeKey with the console focused"); > let consoleInput = gToolbox.getPanel("webconsole").hud.jsterm.inputNode; > consoleInput.focus(); > - synthesizeKeyElement(keyElm); > + EventUtils.synthesizeKey("F3", {keyCode: 114}, panelWin); You may be interested into: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#182-203 ::: devtools/client/framework/toolbox.js:573 (Diff revision 1) > - * The tool the key belongs to. The corresponding command > - * will only trigger if this tool is active. > - */ > - useKeyWithSplitConsole: function (keyElement, whichTool) { > - let cloned = keyElement.cloneNode(); > - cloned.setAttribute("oncommand", "void(0)"); > + * The callback that should be called when the provided key shortcut is pressed. > + * @param {String} whichTool > + * The tool the key belongs to. The corresponding command will only trigger if > + * this tool is active. > + */ > + useKeyWithSplitConsole: function (key, command, whichTool) { Let's call the second argument `handler` as in documentation, `command` sounds like a XUL relic!

(In reply to Julian Descottes [:jdescottes] from comment #2) > (In reply to Julian Descottes [:jdescottes] from comment #1) > > > > Even though 8 shortcuts are forwarded to the toolbox (in english: F8, F10, > > F11, shift+F11, /, ', ;, shift+;) only the Function keys can actually be > > used when the splitconsole is focused. Not sure which mechanism is > > preventing the others from being processed, but typing ";" while the focus > > is in the splitconsole will type ";" in the console input but will have no > > impact on the debugger. To preserve this behavior, I think it's fair to > > restrict the useKeysWithSplitConsole API to function keys only, since all > > the other keys should be preserved to type & navigate in the console input. > > I missed the fact that the non-function shortcuts were relying on modifiers. > That being said, I can't get any of the following shortcuts to work on OSX & > Windows: > - CmdOrCtrl+/ (resume) > - CmdOrCtrl+' (step over) > - CmdOrCtrl+; (step in) > - CmdOrCtrl+shift+; (step out) > > None of those are documented on > https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts, might be > just dead code? James can you confirm? Yes, I've never heard of those before. I would assume those are dead code. If someone complains, we can talk to them and see how much they are really used. It seems like we should only have one keybinding per action.

You might want to talk with jbhoosreddy and jlast as they've been working on keyboard shortcuts in the new debugger. Jason has looked at the split console handling too, so it's worth seeing what he was thinking.

(In reply to James Long (:jlongster) from comment #11) > You might want to talk with jbhoosreddy and jlast as they've been working on > keyboard shortcuts in the new debugger. Jason has looked at the split > console handling too, so it's worth seeing what he was thinking. Thanks for the heads up, commented on github -> https://github.com/devtools-html/debugger.html/issues/717#issuecomment-247303031

Jason: See my comment at https://github.com/devtools-html/debugger.html/issues/717#issuecomment-247303031 . Are you fine with reusing toolbox::useKeysWithSplitConsole with the new debugger, or do you have something else in mind?

Thanks for the reviews Alex, just uploaded new versions of the patches following your review comments. (In reply to Alexandre Poirot [:ochameau] from comment #8) > Comment on attachment 8791207 [details] > Bug 1297113 - Remove unused debugger shortcut keys; > > https://reviewboard.mozilla.org/r/78366/#review77310 > > I feel like James or someone maintaining the debugger should review that. > Note that sometimes, alternative key shortcuts are used in non-engligh > locales to accomodate different keyboard layout. Switched the flag to James. Mozreview assigned you the review even though I didn't have r=ochameau in the summary. I looked at the current localizations for these shortcuts. The primary keys (F8, F10, F11 in english) are almost never changed. When they are, it's in favor of other Function keys. I don't think function keys are impacted a lot keyboard layout differences. And again the alternate shortcuts simply don't work for me, at least in enUS.

Comment on attachment 8791208 [details] Bug 1297113 - Convert useKeysWithSplitConsole to use key-shorcut.js; https://reviewboard.mozilla.org/r/78368/#review77784 ::: devtools/client/debugger/views/toolbar-view.js:107 (Diff revision 2) > _addCommands: function () { > XULUtils.addCommands(document.getElementById("debuggerCommands"), { > - resumeCommand: () => this._onResumePressed(), > - stepOverCommand: () => this._onStepOverPressed(), > - stepInCommand: () => this._onStepInPressed(), > - stepOutCommand: () => this._onStepOutPressed() > + resumeCommand: this.getCommandHandler("resumeCommand"), > + stepOverCommand: this.getCommandHandler("stepOverCommand"), > + stepInCommand: this.getCommandHandler("stepInCommand"), > + stepOutCommand: this.getCommandHandler("stepOutCommand") what's the benefit of this refactor?

Comment on attachment 8791208 [details] Bug 1297113 - Convert useKeysWithSplitConsole to use key-shorcut.js; https://reviewboard.mozilla.org/r/78368/#review77784 > what's the benefit of this refactor? I am using this method to get the handler for a given command in devtools/client/debugger/panel.js

Comment on attachment 8791209 [details] Bug 1297113 - Remove keyset from toolbox.xul; https://reviewboard.mozilla.org/r/78702/#review77798 This keyset also exists in window hosts but I don't think we should remove it there since they contain window key shortcuts: https://dxr.mozilla.org/mozilla-central/search?q=toolbox-keyset. In other words, LGTM!

Comment on attachment 8791207 [details] Bug 1297113 - Remove unused debugger shortcut keys; https://reviewboard.mozilla.org/r/78366/#review78500

Thanks for the reviews! Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a504f14197d49aec8fd886d9b476c23e00b5353 Landing. (removing the ni? for jlast, since I got my answer on Github : https://github.com/devtools-html/debugger.html/issues/717#issuecomment-247303031, thanks!)

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/8705a8173770 Fix eslint errors in toolbox.js;r=ochameau https://hg.mozilla.org/integration/fx-team/rev/0bf1a6786a76 Remove unused debugger shortcut keys;r=jlongster https://hg.mozilla.org/integration/fx-team/rev/74c3bbe47393 Convert useKeysWithSplitConsole to use key-shorcut.js;r=ochameau https://hg.mozilla.org/integration/fx-team/rev/bd6a9044d4bf Remove keyset from toolbox.xul;r=bgrins

https://hg.mozilla.org/mozilla-central/rev/8705a8173770 https://hg.mozilla.org/mozilla-central/rev/0bf1a6786a76 https://hg.mozilla.org/mozilla-central/rev/74c3bbe47393 https://hg.mozilla.org/mozilla-central/rev/bd6a9044d4bf