Bug 1491749 - Stop using xul:keyset and xul:key in toolbox-window.xul; r=bgrins,flod

Differential Revision: https://phabricator.services.mozilla.com/D6015

--HG--
extra : moz-landing-system : lando

devtools/client/framework/toolbox-hosts.js

@@ -11,7 +11,6 @@ const promise = require("promise");
 const Services = require("Services");
 const {DOMHelpers} = require("resource://devtools/client/shared/DOMHelpers.jsm");
 
-loader.lazyRequireGetter(this, "AppConstants", "resource://gre/modules/AppConstants.jsm", true);
 loader.lazyRequireGetter(this, "gDevToolsBrowser", "devtools/client/framework/devtools-browser", true);
 
 /* A host should always allow this much space for the page to be displayed.
@@ -257,14 +256,6 @@ WindowHost.prototype = {
         win.removeEventListener("load", frameLoad, true);
         win.focus();
 
-        let key;
-        if (AppConstants.platform === "macosx") {
-          key = win.document.getElementById("toolbox-key-toggle-osx");
-        } else {
-          key = win.document.getElementById("toolbox-key-toggle");
-        }
-        key.removeAttribute("disabled");
-
         this.frame = win.document.getElementById("toolbox-iframe");
         this.emit("ready", this.frame);
         resolve(this.frame);

devtools/client/framework/toolbox-window.xul

@@ -2,10 +2,6 @@
 <!-- This Source Code Form is subject to the terms of the Mozilla Public
    - License, v. 2.0. If a copy of the MPL was not distributed with this
    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
-<!DOCTYPE window [
-<!ENTITY % toolboxDTD SYSTEM "chrome://devtools/locale/toolbox.dtd" >
- %toolboxDTD;
-]>
 
 <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 
@@ -17,32 +13,6 @@
         windowtype="devtools:toolbox"
         width="900" height="320"
         persist="screenX screenY width height sizemode">
-
-  <commandset id="toolbox-commandset">
-    <command id="toolbox-cmd-close" oncommand="window.close();"/>
-  </commandset>
-
-  <keyset id="toolbox-keyset">
-    <key id="toolbox-key-close"
-         key="&closeCmd.key;"
-         command="toolbox-cmd-close"
-         modifiers="accel"/>
-    <key id="toolbox-key-toggle"
-         key="&toggleToolbox.key;"
-         command="toolbox-cmd-close"
-         modifiers="accel,shift"
-         disabled="true"/>
-    <key id="toolbox-key-toggle-osx"
-         key="&toggleToolbox.key;"
-         command="toolbox-cmd-close"
-         modifiers="accel,alt"
-         disabled="true"/>
-    <key id="toolbox-key-toggle-F12"
-         keycode="&toggleToolboxF12.keycode;"
-         keytext="&toggleToolboxF12.keytext;"
-         command="toolbox-cmd-close"/>
-  </keyset>
-
   <tooltip id="aHTMLTooltip" page="true"/>
   <iframe id="toolbox-iframe" flex="1" forceOwnRefreshDriver="" tooltip="aHTMLTooltip"></iframe>
 </window>

devtools/client/framework/toolbox.js

@@ -37,6 +37,8 @@ const { BrowserLoader } =
 const {LocalizationHelper} = require("devtools/shared/l10n");
 const L10N = new LocalizationHelper("devtools/client/locales/toolbox.properties");
 
+loader.lazyRequireGetter(this, "AppConstants",
+  "resource://gre/modules/AppConstants.jsm", true);
 loader.lazyRequireGetter(this, "getHighlighterUtils",
   "devtools/client/framework/toolbox-highlighter-utils", true);
 loader.lazyRequireGetter(this, "Selection",
@@ -901,6 +903,7 @@ Toolbox.prototype = {
   },
 
   _addHostListeners: function() {
+    // Add navigation keys
     this.shortcuts.on(L10N.getStr("toolbox.nextTool.key"),
                  event => {
                    this.selectNextTool();
@@ -917,6 +920,24 @@ Toolbox.prototype = {
                    event.preventDefault();
                  });
 
+    // Close toolbox key-shortcut handler
+    const onClose = event => this.destroy();
+    this.shortcuts.on(L10N.getStr("toolbox.toggleToolboxF12.key"), onClose);
+
+    // CmdOrCtrl+W is registered only when the toolbox is running in
+    // detached window. In the other case the entire browser tab
+    // is closed when the user uses this shortcut.
+    if (this.hostType == "window") {
+      this.shortcuts.on(L10N.getStr("toolbox.closeToolbox.key"), onClose);
+    }
+
+    if (AppConstants.platform == "macosx") {
+      this.shortcuts.on(L10N.getStr("toolbox.toggleToolboxOSX.key"), onClose);
+    } else {
+      this.shortcuts.on(L10N.getStr("toolbox.toggleToolbox.key"), onClose);
+    }
+
+    // Add event listeners
     this.doc.addEventListener("keypress", this._splitConsoleOnKeypress);
     this.doc.addEventListener("focus", this._onFocus, true);
     this.win.addEventListener("unload", this.destroy);
@@ -991,48 +1012,22 @@ Toolbox.prototype = {
       return;
     }
 
-    const doc = this.win.parent.document;
-
     for (const item of Startup.KeyShortcuts) {
-      // KeyShortcuts contain tool-specific and global key shortcuts,
-      // here we only need to copy shortcut specific to each tool.
-      if (!item.toolId) {
-        continue;
+      const { id, toolId, shortcut, modifiers } = item;
+      const electronKey = KeyShortcuts.parseXulKey(modifiers, shortcut);
+
+      if (id == "browserConsole") {
+        // Add key for toggling the browser console from the detached window
+        this.shortcuts.on(electronKey, () => {
+          HUDService.toggleBrowserConsole();
+        });
+      } else if (toolId) {
+        // KeyShortcuts contain tool-specific and global key shortcuts,
+        // here we only need to copy shortcut specific to each tool.
+        this.shortcuts.on(electronKey, () => {
+          this.selectTool(toolId, "key_shortcut").then(() => this.fireCustomKey(toolId));
+        });
       }
-      const { toolId, shortcut, modifiers } = item;
-
-      const key = doc.createXULElement("key");
-
-      key.id = "key_" + toolId;
-
-      if (shortcut.startsWith("VK_")) {
-        key.setAttribute("keycode", shortcut);
-      } else {
-        key.setAttribute("key", shortcut);
-      }
-
-      key.setAttribute("modifiers", modifiers);
-      // needed. See bug 371900
-      key.setAttribute("oncommand", "void(0);");
-      key.addEventListener("command", () => {
-        this.selectTool(toolId, "key_shortcut").then(() => this.fireCustomKey(toolId));
-      }, true);
-      doc.getElementById("toolbox-keyset").appendChild(key);
-    }
-
-    // Add key for toggling the browser console from the detached window
-    if (!doc.getElementById("key_browserconsole")) {
-      const key = doc.createXULElement("key");
-      key.id = "key_browserconsole";
-
-      key.setAttribute("key", L10N.getStr("browserConsoleCmd.commandkey"));
-      key.setAttribute("modifiers", "accel,shift");
-      // needed. See bug 371900
-      key.setAttribute("oncommand", "void(0)");
-      key.addEventListener("command", () => {
-        HUDService.toggleBrowserConsole();
-      }, true);
-      doc.getElementById("toolbox-keyset").appendChild(key);
     }
   },
 

devtools/client/locales/en-US/toolbox.properties

@@ -32,11 +32,6 @@ options.toolNotSupportedMarker=%1$S *
 # Needs to match scratchpad.keycode from browser.dtd
 scratchpad.keycode=VK_F4
 
-# LOCALIZATION NOTE (browserConsoleCmd.commandkey)
-# Used for toggling the browser console from the detached toolbox window
-# Needs to match browserConsoleCmd.commandkey from browser.dtd
-browserConsoleCmd.commandkey=j
-
 # LOCALIZATION NOTE (pickButton.tooltip)
 # This is the tooltip of the pick button in the toolbox toolbar
 pickButton.tooltip=Pick an element from the page
@@ -103,6 +98,18 @@ toolbox.forceReload2.key=CmdOrCtrl+F5
 # Key shortcut used to move the toolbox in bottom or side of the browser window
 toolbox.toggleHost.key=CmdOrCtrl+Shift+D
 
+# LOCALIZATION NOTE (toolbox.closeToolbox.key) Key shortcut used to close the toolbox
+toolbox.closeToolbox.key=CmdOrCtrl+W
+
+# LOCALIZATION NOTE (toolbox.toggleToolbox.key) Key shortcut used to toggle the toolbox
+toolbox.toggleToolbox.key=CmdOrCtrl+Shift+I
+
+# LOCALIZATION NOTE (toolbox.toggleToolboxOSX.key) Key shortcut used to toggle the toolbox
+toolbox.toggleToolboxOSX.key=CmdOrCtrl+Alt+I
+
+# LOCALIZATION NOTE (toolbox.toggleToolboxF12.key) Key shortcut used to toggle the toolbox
+toolbox.toggleToolboxF12.key=F12
+
 # LOCALIZATION NOTE (toolbox.frames.tooltip): This is the label for
 # the iframes menu list that appears only when the document has some.
 # It allows you to switch the context of the whole toolbox.

devtools/client/shared/key-shortcuts.js

@@ -59,7 +59,7 @@ const ElectronKeysMapping = {
 };
 
 /**
- * Helper to listen for keyboard events decribed in .properties file.
+ * Helper to listen for keyboard events described in .properties file.
  *
  * let shortcuts = new KeyShortcuts({
  *   window
@@ -184,6 +184,28 @@ KeyShortcuts.stringify = function(shortcut) {
   return list.join("+");
 };
 
+/*
+ * Parse an xul-like key string and return an electron-like string.
+ */
+KeyShortcuts.parseXulKey = function(modifiers, shortcut) {
+  modifiers = modifiers.split(",").map(mod => {
+    if (mod == "alt") {
+      return "Alt";
+    } else if (mod == "shift") {
+      return "Shift";
+    } else if (mod == "accel") {
+      return "CmdOrCtrl";
+    }
+    return mod;
+  }).join("+");
+
+  if (shortcut.startsWith("VK_")) {
+    shortcut = shortcut.substr(3);
+  }
+
+  return modifiers + "+" + shortcut;
+};
+
 KeyShortcuts.prototype = {
   destroy() {
     this.target.removeEventListener("keydown", this);
@@ -201,7 +223,7 @@ KeyShortcuts.prototype = {
       return false;
     }
     if (shortcut.shift != event.shiftKey) {
-      // Shift is a special modifier, it may implicitely be required if the expected key
+      // Shift is a special modifier, it may implicitly be required if the expected key
       // is a special character accessible via shift.
       const isAlphabetical = event.key && event.key.match(/[a-zA-Z]/);
       // OSX: distinguish cmd+[key] from cmd+shift+[key] shortcuts (Bug 1300458)