Bug 1421811 - Part 3: Update shortcut in sidebar on update r?mixedpuppy,gijs draft
authorMark Striemer <mstriemer@mozilla.com>
Tue, 06 Feb 2018 12:55:40 -0600
changeset 753177 4eb8f0b4220d5a0cb1a1b1b2e93ec8e280ba17e6
parent 752334 b189a895fa3365b5a5a389686ef9f2661ccb7eed
push id98496
push userbmo:mstriemer@mozilla.com
push dateFri, 09 Feb 2018 18:41:16 +0000
reviewersmixedpuppy, gijs
bugs1421811
milestone60.0a1
Bug 1421811 - Part 3: Update shortcut in sidebar on update r?mixedpuppy,gijs MozReview-Commit-ID: 4y02mCqwacg
browser/base/content/browser-sidebar.js
browser/components/extensions/ext-commands.js
browser/components/extensions/ext-sidebarAction.js
browser/components/extensions/test/browser/browser_ext_commands_update.js
browser/components/extensions/test/browser/browser_ext_sidebarAction.js
--- a/browser/base/content/browser-sidebar.js
+++ b/browser/base/content/browser-sidebar.js
@@ -114,29 +114,43 @@ var SidebarUI = {
                   gNavigatorBundle.getString("sidebar.moveToRight");
     this._reversePositionButton.setAttribute("label", label);
 
     this._switcherPanel.hidden = false;
     this._switcherPanel.openPopup(this._icon);
     this._switcherTarget.classList.add("active");
   },
 
+  updateShortcut({button, key}) {
+    // If the shortcuts haven't been rendered yet then it will be set correctly
+    // on the first render so there's nothing to do now.
+    if (!this._addedShortcuts) {
+      return;
+    }
+    if (key) {
+      let keyId = key.getAttribute("id");
+      button = this._switcherPanel.querySelector(`[key="${keyId}"]`);
+    } else if (button) {
+      let keyId = button.getAttribute("key");
+      key = document.getElementById(keyId);
+    }
+    if (!button || !key) {
+      return;
+    }
+    button.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(key));
+  },
+
   _addedShortcuts: false,
   _ensureShortcutsShown() {
     if (this._addedShortcuts) {
       return;
     }
     this._addedShortcuts = true;
     for (let button of this._switcherPanel.querySelectorAll("toolbarbutton[key]")) {
-      let keyId = button.getAttribute("key");
-      let key = document.getElementById(keyId);
-      if (!key) {
-        continue;
-      }
-      button.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(key));
+      this.updateShortcut({button});
     }
   },
 
   /**
    * Change the pref that will trigger a call to setPosition
    */
   reversePosition() {
     Services.prefs.setBoolPref(this.POSITION_START_PREF, !this._positionStart);
--- a/browser/components/extensions/ext-commands.js
+++ b/browser/components/extensions/ext-commands.js
@@ -8,16 +8,20 @@
 
 ChromeUtils.defineModuleGetter(this, "ExtensionParent",
                                "resource://gre/modules/ExtensionParent.jsm");
 ChromeUtils.defineModuleGetter(this, "ExtensionSettingsStore",
                                "resource://gre/modules/ExtensionSettingsStore.jsm");
 
 var XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
 
+const EXECUTE_PAGE_ACTION = "_execute_page_action";
+const EXECUTE_BROWSER_ACTION = "_execute_browser_action";
+const EXECUTE_SIDEBAR_ACTION = "_execute_sidebar_action";
+
 function normalizeShortcut(shortcut) {
   return shortcut ? shortcut.replace(/\s+/g, "") : null;
 }
 
 this.commands = class extends ExtensionAPI {
   static async onUninstall(extensionId) {
     // Cleanup the updated commands. In some cases the extension is installed
     // and uninstalled so quickly that `this.commands` hasn't loaded yet. To
@@ -149,23 +153,30 @@ this.commands = class extends ExtensionA
    */
   registerKeysToDocument(window, commands) {
     let doc = window.document;
     let keyset = doc.createElementNS(XUL_NS, "keyset");
     keyset.id = `ext-keyset-id-${this.id}`;
     if (this.keysetsMap.has(window)) {
       this.keysetsMap.get(window).remove();
     }
+    let sidebarKey;
     commands.forEach((command, name) => {
       if (command.shortcut) {
         let keyElement = this.buildKey(doc, name, command.shortcut);
         keyset.appendChild(keyElement);
+        if (name == EXECUTE_SIDEBAR_ACTION) {
+          sidebarKey = keyElement;
+        }
       }
     });
     doc.documentElement.appendChild(keyset);
+    if (sidebarKey) {
+      window.SidebarUI.updateShortcut({key: sidebarKey});
+    }
     this.keysetsMap.set(window, keyset);
   }
 
   /**
    * Builds a XUL Key element and attaches an onCommand listener which
    * emits a command event with the provided name when fired.
    *
    * @param {Document} doc The XUL document.
@@ -182,21 +193,21 @@ this.commands = class extends ExtensionA
     // and it is currently ignored when set to the empty string.
     keyElement.setAttribute("oncommand", "//");
 
     /* eslint-disable mozilla/balanced-listeners */
     // We remove all references to the key elements when the extension is shutdown,
     // therefore the listeners for these elements will be garbage collected.
     keyElement.addEventListener("command", (event) => {
       let action;
-      if (name == "_execute_page_action") {
+      if (name == EXECUTE_PAGE_ACTION) {
         action = pageActionFor(this.extension);
-      } else if (name == "_execute_browser_action") {
+      } else if (name == EXECUTE_BROWSER_ACTION) {
         action = browserActionFor(this.extension);
-      } else if (name == "_execute_sidebar_action") {
+      } else if (name == EXECUTE_SIDEBAR_ACTION) {
         action = sidebarActionFor(this.extension);
       } else {
         this.extension.tabManager
             .addActiveTabPermission();
         this.emit("command", name);
         return;
       }
       if (action) {
@@ -224,17 +235,17 @@ this.commands = class extends ExtensionA
 
     let parts = shortcut.split("+");
 
     // The key is always the last element.
     let chromeKey = parts.pop();
 
     // The modifiers are the remaining elements.
     keyElement.setAttribute("modifiers", this.getModifiersAttribute(parts));
-    if (name == "_execute_sidebar_action") {
+    if (name == EXECUTE_SIDEBAR_ACTION) {
       let id = `ext-key-id-${this.id}-sidebar-action`;
       keyElement.setAttribute("id", id);
     }
 
     if (/^[A-Z]$/.test(chromeKey)) {
       // We use the key attribute for all single digits and characters.
       keyElement.setAttribute("key", chromeKey);
     } else {
--- a/browser/components/extensions/ext-sidebarAction.js
+++ b/browser/components/extensions/ext-sidebarAction.js
@@ -146,17 +146,17 @@ this.sidebarAction = class extends Exten
     if (this.browserStyle) {
       url += "&browser-style=1";
     }
 
     return url;
   }
 
   createMenuItem(window, details) {
-    let {document} = window;
+    let {document, SidebarUI} = window;
 
     // Use of the broadcaster allows browser-sidebar.js to properly manage the
     // checkmarks in the menus.
     let broadcaster = document.createElementNS(XUL_NS, "broadcaster");
     broadcaster.setAttribute("id", this.id);
     broadcaster.setAttribute("autoCheck", "false");
     broadcaster.setAttribute("type", "checkbox");
     broadcaster.setAttribute("group", "sidebar");
@@ -185,16 +185,17 @@ this.sidebarAction = class extends Exten
     toolbarbutton.setAttribute("observes", this.id);
     toolbarbutton.setAttribute("class", "subviewbutton subviewbutton-iconic webextension-menuitem");
     this.setMenuIcon(toolbarbutton, details);
 
     document.getElementById("mainBroadcasterSet").appendChild(broadcaster);
     document.getElementById("viewSidebarMenu").appendChild(menuitem);
     let separator = document.getElementById("sidebar-extensions-separator");
     separator.parentNode.insertBefore(toolbarbutton, separator);
+    SidebarUI.updateShortcut({button: toolbarbutton});
 
     return menuitem;
   }
 
   setMenuIcon(menuitem, details) {
     let getIcon = size => IconDetails.escapeUrl(
       IconDetails.getPreferredIcon(details.icon, this.extension, size).icon);
 
--- a/browser/components/extensions/test/browser/browser_ext_commands_update.js
+++ b/browser/components/extensions/test/browser/browser_ext_commands_update.js
@@ -231,8 +231,77 @@ add_task(async function test_update_defi
     },
   });
   await updatedExtension.startup();
 
   await TestUtils.waitForCondition(() => extensionKeyset(extension.id));
   // Shortcut is unchanged since it was previously updated.
   checkKey(extension.id, "P", "alt shift");
 });
+
+add_task(async function updateSidebarCommand() {
+  let extension = ExtensionTestUtils.loadExtension({
+    useAddonManager: "temporary",
+    manifest: {
+      commands: {
+        _execute_sidebar_action: {
+          suggested_key: {
+            default: "Ctrl+Shift+E",
+          },
+        },
+      },
+      sidebar_action: {
+        default_panel: "sidebar.html",
+      },
+    },
+    background() {
+      browser.test.onMessage.addListener(async (msg, data) => {
+        if (msg == "updateShortcut") {
+          await browser.commands.update(data);
+          return browser.test.sendMessage("done");
+        }
+        throw new Error("Unknown message");
+      });
+    },
+    files: {
+      "sidebar.html": `
+        <!DOCTYPE html>
+        <html>
+        <head><meta charset="utf-8"/>
+        <script src="sidebar.js"></script>
+        </head>
+        <body>
+        A Test Sidebar
+        </body></html>
+      `,
+
+      "sidebar.js": function() {
+        window.onload = () => {
+          browser.test.sendMessage("sidebar");
+        };
+      },
+    },
+  });
+  await extension.startup();
+  await extension.awaitMessage("sidebar");
+
+  // Show and hide the switcher panel to generate the initial shortcuts.
+  let switcherShown = promisePopupShown(SidebarUI._switcherPanel);
+  SidebarUI.showSwitcherPanel();
+  await switcherShown;
+  let switcherHidden = promisePopupHidden(SidebarUI._switcherPanel);
+  SidebarUI.hideSwitcherPanel();
+  await switcherHidden;
+
+  let buttonId = `button_${makeWidgetId(extension.id)}-sidebar-action`;
+  let button = document.getElementById(buttonId);
+  let shortcut = button.getAttribute("shortcut");
+  ok(shortcut.endsWith("E"), "The button has the shortcut set");
+
+  extension.sendMessage(
+    "updateShortcut", {name: "_execute_sidebar_action", shortcut: "Ctrl+Shift+M"});
+  await extension.awaitMessage("done");
+
+  shortcut = button.getAttribute("shortcut");
+  ok(shortcut.endsWith("M"), "The button shortcut has been updated");
+
+  await extension.unload();
+});
--- a/browser/components/extensions/test/browser/browser_ext_sidebarAction.js
+++ b/browser/components/extensions/test/browser/browser_ext_sidebarAction.js
@@ -1,23 +1,16 @@
 /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set sts=2 sw=2 et tw=80: */
 "use strict";
 
 requestLongerTimeout(2);
 
 let extData = {
   manifest: {
-    commands: {
-      _execute_sidebar_action: {
-        suggested_key: {
-          default: "Ctrl+Shift+I",
-        },
-      },
-    },
     sidebar_action: {
       default_panel: "sidebar.html",
     },
   },
   useAddonManager: "temporary",
 
   files: {
     "sidebar.html": `
@@ -52,84 +45,89 @@ let extData = {
         let isOpen = await browser.sidebarAction.isOpen(arg);
         browser.test.assertEq(result, isOpen, "expected value from isOpen");
       }
       browser.test.sendMessage("done");
     });
   },
 };
 
+function getExtData(manifestUpdates = {}) {
+  return {
+    ...extData,
+    manifest: {
+      ...extData.manifest,
+      ...manifestUpdates,
+    },
+  };
+}
+
+
 async function sendMessage(ext, msg, data = undefined) {
   ext.sendMessage({msg, data});
   await ext.awaitMessage("done");
 }
 
 add_task(async function sidebar_initial_install() {
   ok(document.getElementById("sidebar-box").hidden, "sidebar box is not visible");
-  let extension = ExtensionTestUtils.loadExtension(extData);
+  let extension = ExtensionTestUtils.loadExtension(getExtData());
   await extension.startup();
   // Test sidebar is opened on install
   await extension.awaitMessage("sidebar");
   ok(!document.getElementById("sidebar-box").hidden, "sidebar box is visible");
 
   await extension.unload();
   // Test that the sidebar was closed on unload.
   ok(document.getElementById("sidebar-box").hidden, "sidebar box is not visible");
 });
 
 
 add_task(async function sidebar_two_sidebar_addons() {
-  let extension2 = ExtensionTestUtils.loadExtension(extData);
+  let extension2 = ExtensionTestUtils.loadExtension(getExtData());
   await extension2.startup();
   // Test sidebar is opened on install
   await extension2.awaitMessage("sidebar");
   ok(!document.getElementById("sidebar-box").hidden, "sidebar box is visible");
 
   // Test second sidebar install opens new sidebar
-  let extension3 = ExtensionTestUtils.loadExtension(extData);
+  let extension3 = ExtensionTestUtils.loadExtension(getExtData());
   await extension3.startup();
   // Test sidebar is opened on install
   await extension3.awaitMessage("sidebar");
   ok(!document.getElementById("sidebar-box").hidden, "sidebar box is visible");
   await extension3.unload();
 
   // We just close the sidebar on uninstall of the current sidebar.
   ok(document.getElementById("sidebar-box").hidden, "sidebar box is not visible");
 
   await extension2.unload();
 });
 
 add_task(async function sidebar_empty_panel() {
-  let extension = ExtensionTestUtils.loadExtension(extData);
+  let extension = ExtensionTestUtils.loadExtension(getExtData());
   await extension.startup();
   // Test sidebar is opened on install
   await extension.awaitMessage("sidebar");
   ok(!document.getElementById("sidebar-box").hidden, "sidebar box is visible in first window");
   await sendMessage(extension, "set-panel");
   await extension.unload();
 });
 
 add_task(async function sidebar_isOpen() {
   info("Load extension1");
-  let extension1 = ExtensionTestUtils.loadExtension(extData);
+  let extension1 = ExtensionTestUtils.loadExtension(getExtData());
   await extension1.startup();
 
   info("Test extension1's sidebar is opened on install");
   await extension1.awaitMessage("sidebar");
   await sendMessage(extension1, "isOpen", {result: true});
   let sidebar1ID = SidebarUI.currentID;
 
-  // Test that the key is set for the extension.
-  let button = document.getElementById(`button_${makeWidgetId(extension1.id)}-sidebar-action`);
-  ok(button.hasAttribute("key"), "The menu item has a key specified");
-  let key = document.getElementById(button.getAttribute("key"));
-  ok(key, "The key attribute finds the related key element");
-
   info("Load extension2");
-  let extension2 = ExtensionTestUtils.loadExtension(extData);
+  let extension2 = ExtensionTestUtils.loadExtension(getExtData());
   await extension2.startup();
 
   info("Test extension2's sidebar is opened on install");
   await extension2.awaitMessage("sidebar");
   await sendMessage(extension1, "isOpen", {result: false});
   await sendMessage(extension2, "isOpen", {result: true});
 
   info("Switch back to extension1's sidebar");
@@ -163,8 +161,66 @@ add_task(async function sidebar_isOpen()
   info("Close the sidebar in the original window");
   SidebarUI.hide();
   await sendMessage(extension1, "isOpen", {result: false});
   await sendMessage(extension2, "isOpen", {result: false});
 
   await extension1.unload();
   await extension2.unload();
 });
+
+add_task(async function testShortcuts() {
+  function verifyShortcut(id, commandKey) {
+    // We're just testing the command key since the modifiers have different
+    // icons on different platforms.
+    let button = document.getElementById(`button_${makeWidgetId(id)}-sidebar-action`);
+    ok(button.hasAttribute("key"), "The menu item has a key specified");
+    let key = document.getElementById(button.getAttribute("key"));
+    ok(key, "The key attribute finds the related key element");
+    ok(button.getAttribute("shortcut").endsWith(commandKey),
+       "The shortcut has the right key");
+  }
+
+  let extension1 = ExtensionTestUtils.loadExtension(
+    getExtData({
+      commands: {
+        _execute_sidebar_action: {
+          suggested_key: {
+            default: "Ctrl+Shift+I",
+          },
+        },
+      },
+    }));
+  let extension2 = ExtensionTestUtils.loadExtension(
+    getExtData({
+      commands: {
+        _execute_sidebar_action: {
+          suggested_key: {
+            default: "Ctrl+Shift+E",
+          },
+        },
+      },
+    }));
+
+  await extension1.startup();
+  await extension1.awaitMessage("sidebar");
+
+  // Open and close the switcher panel to trigger shortcut content rendering.
+  let switcherPanelShown = promisePopupShown(SidebarUI._switcherPanel);
+  SidebarUI.showSwitcherPanel();
+  await switcherPanelShown;
+  let switcherPanelHidden = promisePopupHidden(SidebarUI._switcherPanel);
+  SidebarUI.hideSwitcherPanel();
+  await switcherPanelHidden;
+
+  // Test that the key is set for the extension after the shortcuts are rendered.
+  verifyShortcut(extension1.id, "I");
+
+  await extension2.startup();
+  await extension2.awaitMessage("sidebar");
+
+  // Once the switcher panel has been opened new shortcuts should be added
+  // automatically.
+  verifyShortcut(extension2.id, "E");
+
+  await extension1.unload();
+  await extension2.unload();
+});