Bug 1520123: Notify users when there is a duplicate shortcut r=mstriemer,fluent-reviewers,flod,Gijs

Notify users when there is a duplicate shortcut Differential Revision: https://phabricator.services.mozilla.com/D29125 --HG-- extra : moz-landing-system : lando
2019-08-15 10:15:28 +00:00 · 2019-08-15 10:15:28 +00:00 · 9129ff2c95
--- a/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
+++ b/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
@ -328,6 +328,15 @@ shortcuts-modifier-other = Include Ctrl or Alt
 shortcuts-invalid = Invalid combination
 shortcuts-letter = Type a letter
 shortcuts-system = Can’t override a { -brand-short-name } shortcut
+
+# String displayed in warning label when there is a duplicate shortcut
+shortcuts-duplicate = Duplicate shortcut
+
+# String displayed when a keyboard shortcut is already assigned to more than one add-on
+# Variables:
+#   $shortcut (string) - Shortcut string for the add-on
+shortcuts-duplicate-warning-message = { $shortcut } is being used as a shortcut in more than one case. Duplicate shortcuts may cause unexpected behavior.
+
 # String displayed when a keyboard shortcut is already used by another add-on
 # Variables:
 #   $addon (string) - Name of the add-on
--- a/toolkit/mozapps/extensions/content/shortcuts.css
+++ b/toolkit/mozapps/extensions/content/shortcuts.css
@ -1,10 +1,9 @@
 .body {
  margin-inline-start: 28px;
+  width: 664px;
 }

 .shortcut.card {
-  /* Preferences content is 664px and the cards have 16px of left/right padding. */
-  width: 632px;
  margin-bottom: 16px;
 }

@ -53,6 +52,13 @@
  margin: 8px 0 0;
 }

+.expand-button[warning]:not(:focus) {
+  outline: 2px solid var(--yellow-60);
+  outline-offset: -1px;
+  -moz-outline-radius: 3px;
+  box-shadow: 0 0 0 4px var(--yellow-60-a30);
+}
+
 .shortcut-input {
  font-size: 12px;
  padding: 4px 8px;
@ -64,6 +70,8 @@

 .error-message {
  --error-background: var(--red-60);
+  --warning-background: var(--yellow-50);
+  --warning-text-color: var(--yellow-90);
  color: white;
  display: flex;
  flex-direction: column;
@ -80,6 +88,11 @@
  -moz-context-properties: fill, stroke;
 }

+.error-message[type="warning"] > .error-message-icon {
+  fill: var(--warning-background);
+  stroke: var(--warning-background);
+}
+
 .error-message-label {
  background-color: var(--error-background);
  border-radius: 2px;
@ -90,6 +103,11 @@
  word-wrap: break-word;
 }

+.error-message[type="warning"] > .error-message-label {
+  background-color: var(--warning-background);
+  color: var(--warning-text-color);
+}
+
 .error-message-arrow {
  background-color: var(--error-background);
  content: "";
@ -98,3 +116,8 @@
  transform: translate(4px, -6px) rotate(45deg);
  position: absolute;
 }
+
+/* The margin between message bars. */
+message-bar-stack > * {
+  margin-bottom: 8px;
+}
--- a/toolkit/mozapps/extensions/content/shortcuts.html
+++ b/toolkit/mozapps/extensions/content/shortcuts.html
@ -11,10 +11,14 @@
    <link rel="localization" href="branding/brand.ftl"/>
    <link rel="localization" href="toolkit/about/aboutAddons.ftl"/>

+    <script src="chrome://mozapps/content/extensions/message-bar.js"></script>
    <script src="chrome://mozapps/content/extensions/shortcuts.js"></script>
  </head>
  <body id="body">
    <div class="body">
+      <message-bar-stack id="duplicate-warning-messages" reverse max-message-bar-count="5">
+      </message-bar-stack>
+
      <div class="error-message">
        <img class="error-message-icon" src="chrome://global/skin/arrow/panelarrow-vertical.svg"/>
        <div class="error-message-label"></div>
--- a/toolkit/mozapps/extensions/content/shortcuts.js
+++ b/toolkit/mozapps/extensions/content/shortcuts.js
@ -20,6 +20,7 @@ const COLLAPSE_OPTIONS = {
  limit: 5, // We only want to show 5 when collapsed.
  allowOver: 1, // Avoid collapsing to hide 1 row.
 };
+const SHORTCUT_KEY_SEPARATOR = "|";

 let templatesLoaded = false;
 let shortcutKeyMap = new Map();
@ -219,7 +220,15 @@ function getShortcutValue(shortcut) {

 let error;

-function setError(input, messageId, args) {
+function setError(...args) {
+  setInputMessage("error", ...args);
+}
+
+function setWarning(...args) {
+  setInputMessage("warning", ...args);
+}
+
+function setInputMessage(type, input, messageId, args) {
  if (!error) {
    error = document.querySelector(".error-message");
  }
@ -227,6 +236,7 @@ function setError(input, messageId, args) {
  let { x, y, height } = input.getBoundingClientRect();
  error.style.top = `${y + window.scrollY + height - 5}px`;
  error.style.left = `${x}px`;
+  error.setAttribute("type", type);
  document.l10n.setAttributes(
    error.querySelector(".error-message-label"),
    messageId,
@ -244,8 +254,13 @@ function inputBlurred(e) {
  e.target.value = getShortcutValue(e.target.getAttribute("shortcut"));
 }

-function clearValue(e) {
+function onFocus(e) {
  e.target.value = "";
+
+  let warning = e.target.getAttribute("warning");
+  if (warning) {
+    setWarning(e.target, warning);
+  }
 }

 function getShortcutForEvent(e) {
@ -273,19 +288,38 @@ function getShortcutForEvent(e) {
    .join("+");
 }

-function recordShortcut(shortcut, addonName) {
+function buildDuplicateShortcutsMap(addons) {
+  return Promise.all(
+    addons.map(async addon => {
+      let extension = extensionForAddonId(addon.id);
+      if (extension && extension.shortcuts) {
+        let commands = await extension.shortcuts.allCommands();
+        for (let command of commands) {
+          recordShortcut(command.shortcut, addon.name, command.name);
+        }
+      }
+    })
+  );
+}
+
+function recordShortcut(shortcut, addonName, commandName) {
+  if (!shortcut) {
+    return;
+  }
  let addons = shortcutKeyMap.get(shortcut);
+  let addonString = `${addonName}${SHORTCUT_KEY_SEPARATOR}${commandName}`;
  if (addons) {
-    addons.add(addonName);
+    addons.add(addonString);
  } else {
-    shortcutKeyMap.set(shortcut, new Set([addonName]));
+    shortcutKeyMap.set(shortcut, new Set([addonString]));
  }
 }

-function removeShortcut(shortcut, addonName) {
+function removeShortcut(shortcut, addonName, commandName) {
  let addons = shortcutKeyMap.get(shortcut);
+  let addonString = `${addonName}${SHORTCUT_KEY_SEPARATOR}${commandName}`;
  if (addons) {
-    addons.delete(addonName);
+    addons.delete(addonString);
    if (addons.size === 0) {
      shortcutKeyMap.delete(shortcut);
    }
@ -295,7 +329,63 @@ function removeShortcut(shortcut, addonName) {
 function getAddonName(shortcut) {
  let addons = shortcutKeyMap.get(shortcut);
  // Get the first addon name with given shortcut.
-  return addons.values().next().value;
+  let name = addons.values().next().value;
+  return name.split(SHORTCUT_KEY_SEPARATOR)[0];
+}
+
+function setDuplicateWarnings() {
+  let warningHolder = document.getElementById("duplicate-warning-messages");
+  clearWarnings(warningHolder);
+  for (let [shortcut, addons] of shortcutKeyMap) {
+    if (addons.size > 1) {
+      warningHolder.appendChild(createDuplicateWarningBar(shortcut));
+      markDuplicates(shortcut);
+    }
+  }
+}
+
+function clearWarnings(warningHolder) {
+  warningHolder.textContent = "";
+  let inputs = document.querySelectorAll(".shortcut-input[warning]");
+  for (let input of inputs) {
+    input.removeAttribute("warning");
+    let row = input.closest(".shortcut-row");
+    if (row.hasAttribute("hide-before-expand")) {
+      row
+        .closest(".card")
+        .querySelector(".expand-button")
+        .removeAttribute("warning");
+    }
+  }
+}
+
+function createDuplicateWarningBar(shortcut) {
+  let messagebar = document.createElement("message-bar");
+  messagebar.setAttribute("type", "warning");
+
+  let message = document.createElement("span");
+  document.l10n.setAttributes(message, "shortcuts-duplicate-warning-message", {
+    shortcut,
+  });
+
+  messagebar.append(message);
+  return messagebar;
+}
+
+function markDuplicates(shortcut) {
+  let inputs = document.querySelectorAll(
+    `.shortcut-input[shortcut="${shortcut}"]`
+  );
+  for (let input of inputs) {
+    input.setAttribute("warning", "shortcuts-duplicate");
+    let row = input.closest(".shortcut-row");
+    if (row.hasAttribute("hide-before-expand")) {
+      row
+        .closest(".card")
+        .querySelector(".expand-button")
+        .setAttribute("warning", "shortcuts-duplicate");
+    }
+  }
 }

 function onShortcutChange(e) {
@ -350,9 +440,10 @@ function onShortcutChange(e) {
        // Update the shortcut if it isn't reserved or assigned.
        let oldShortcut = input.getAttribute("shortcut");
        let addonName = input.closest(".card").getAttribute("addon-name");
+        let commandName = input.getAttribute("name");

-        removeShortcut(oldShortcut, addonName);
-        recordShortcut(shortcutString, addonName);
+        removeShortcut(oldShortcut, addonName, commandName);
+        recordShortcut(shortcutString, addonName, commandName);
      }

      // This is async, but we're not awaiting it to keep the handler sync.
@ -362,6 +453,7 @@ function onShortcutChange(e) {
      });
      input.setAttribute("shortcut", shortcutString);
      input.blur();
+      setDuplicateWarnings();
      break;
    case ShortcutUtils.MODIFIER_REQUIRED:
      if (AppConstants.platform == "macosx") {
@ -395,6 +487,17 @@ function renderNoShortcutAddons(addons) {
 async function renderAddons(addons) {
  let frag = document.createDocumentFragment();
  let noShortcutAddons = [];
+
+  await buildDuplicateShortcutsMap(addons);
+
+  let isDuplicate = command => {
+    if (command.shortcut) {
+      let dupes = shortcutKeyMap.get(command.shortcut);
+      return dupes.size > 1;
+    }
+    return false;
+  };
+
  for (let addon of addons) {
    let extension = extensionForAddonId(addon.id);

@ -416,6 +519,15 @@ async function renderAddons(addons) {

      // Sort the commands so the ones with shortcuts are at the top.
      commands.sort((a, b) => {
+        if (isDuplicate(a) && isDuplicate(b)) {
+          return 0;
+        }
+        if (isDuplicate(a)) {
+          return -1;
+        }
+        if (isDuplicate(b)) {
+          return 1;
+        }
        // Boolean compare the shortcuts to see if they're both set or unset.
        if (!a.shortcut == !b.shortcut) {
          return 0;
@ -454,14 +566,13 @@ async function renderAddons(addons) {
        input.addEventListener("keydown", onShortcutChange);
        input.addEventListener("keyup", onShortcutChange);
        input.addEventListener("blur", inputBlurred);
-        input.addEventListener("focus", clearValue);
+        input.addEventListener("focus", onFocus);

        if (willHideCommands && i == limit) {
          firstHiddenInput = input;
        }

        card.appendChild(row);
-        recordShortcut(command.shortcut, addon.name);
      }

      // Add an expand button, if needed.
@ -523,4 +634,5 @@ async function render() {
  let container = document.getElementById("addon-shortcuts");
  container.textContent = "";
  container.appendChild(frag);
+  setDuplicateWarnings();
 }
--- a/toolkit/mozapps/extensions/test/browser/browser.ini
+++ b/toolkit/mozapps/extensions/test/browser/browser.ini
@ -91,6 +91,7 @@ skip-if = verify
 [browser_pluginprefs.js]
 [browser_recentupdates.js]
 [browser_reinstall.js]
+[browser_shortcuts_duplicate_check.js]
 [browser_task_next_test.js]
 [browser_updateid.js]
 [browser_updatessl.js]
--- a/toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
+++ b/toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
@ -0,0 +1,156 @@
+"use strict";
+
+const { PromiseTestUtils } = ChromeUtils.import(
+  "resource://testing-common/PromiseTestUtils.jsm"
+);
+PromiseTestUtils.whitelistRejectionsGlobally(/Message manager disconnected/);
+
+let gManagerWindow;
+
+async function loadShortcutsView() {
+  gManagerWindow = await open_manager(null);
+  let categoryUtilities = new CategoryUtilities(gManagerWindow);
+  await categoryUtilities.openType("extension");
+
+  // There should be a manage shortcuts link.
+  let doc = gManagerWindow.document;
+  let shortcutsLink = doc.getElementById("manage-shortcuts");
+
+  // Open the shortcuts view.
+  shortcutsLink.click();
+  await wait_for_view_load(gManagerWindow);
+
+  return doc.getElementById("shortcuts-view").contentDocument;
+}
+
+function closeView() {
+  return close_manager(gManagerWindow);
+}
+
+add_task(async function testDuplicateShortcutsWarnings() {
+  let duplicateCommands = {
+    commandOne: {
+      suggested_key: { default: "Shift+Alt+1" },
+    },
+    commandTwo: {
+      description: "Command Two!",
+      suggested_key: { default: "Shift+Alt+2" },
+    },
+  };
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      commands: duplicateCommands,
+      name: "Extension 1",
+    },
+    background() {
+      browser.test.sendMessage("ready");
+    },
+    useAddonManager: "temporary",
+  });
+
+  await extension.startup();
+  await extension.awaitMessage("ready");
+
+  let extension2 = ExtensionTestUtils.loadExtension({
+    manifest: {
+      commands: {
+        ...duplicateCommands,
+        commandThree: {
+          description: "Command Three!",
+          suggested_key: { default: "Shift+Alt+3" },
+        },
+      },
+      name: "Extension 2",
+    },
+    background() {
+      browser.test.sendMessage("ready");
+    },
+    useAddonManager: "temporary",
+  });
+
+  await extension2.startup();
+  await extension2.awaitMessage("ready");
+
+  let doc = await loadShortcutsView();
+
+  let warningBars = doc.querySelectorAll("message-bar");
+  // Ensure warning messages are shown for each duplicate shorctut.
+  is(
+    warningBars.length,
+    Object.keys(duplicateCommands).length,
+    "There is a warning message bar for each duplicate shortcut"
+  );
+
+  // Ensure warning messages are correct with correct shortcuts.
+  let count = 1;
+  for (let warning of warningBars) {
+    let warningMsg = warning.querySelector("span");
+    let l10nAttrs = doc.l10n.getAttributes(warningMsg);
+    is(
+      l10nAttrs.id,
+      "shortcuts-duplicate-warning-message",
+      "Warning message is shown"
+    );
+    Assert.deepEqual(
+      l10nAttrs.args,
+      { shortcut: `Shift+Alt+${count}` },
+      "Warning message shortcut is correct"
+    );
+    count++;
+  }
+
+  ["Shift+Alt+1", "Shift+Alt+2"].forEach((shortcut, index) => {
+    // Ensure warning messages are correct with correct shortcuts.
+    let warning = warningBars[index];
+    let warningMsg = warning.querySelector("span");
+    let l10nAttrs = doc.l10n.getAttributes(warningMsg);
+    is(
+      l10nAttrs.id,
+      "shortcuts-duplicate-warning-message",
+      "Warning message is shown"
+    );
+    Assert.deepEqual(
+      l10nAttrs.args,
+      { shortcut },
+      "Warning message shortcut is correct"
+    );
+
+    // Check if all inputs have warning style.
+    let inputs = doc.querySelectorAll(`input[shortcut="${shortcut}"]`);
+    for (let input of inputs) {
+      // Check if warning error message is shown on focus.
+      input.focus();
+      let error = doc.querySelector(".error-message");
+      let label = error.querySelector(".error-message-label");
+      is(error.style.visibility, "visible", "The error element is shown");
+      is(
+        error.getAttribute("type"),
+        "warning",
+        "Duplicate shortcut has warning class"
+      );
+      is(
+        label.dataset.l10nId,
+        "shortcuts-duplicate",
+        "Correct error message is shown"
+      );
+
+      // On keypress events wrning class should be removed.
+      EventUtils.synthesizeKey("A");
+      ok(
+        !error.classList.contains("warning"),
+        "Error element should not have warning class"
+      );
+
+      input.blur();
+      is(
+        error.style.visibility,
+        "hidden",
+        "The error element is hidden on blur"
+      );
+    }
+  });
+
+  await closeView();
+  await extension.unload();
+  await extension2.unload();
+});
--- a/toolkit/themes/shared/in-content/common.inc.css
+++ b/toolkit/themes/shared/in-content/common.inc.css
@ -89,6 +89,7 @@
  --yellow-10: #ffff98;
  --yellow-50: #ffe900;
  --yellow-60: #d7b600;
+  --yellow-60-a30: rgba(215, 182, 0, 0.3);
  --yellow-70: #a47f00;
  --yellow-80: #715100;
  --yellow-90: #3e2800;
@ -960,6 +961,11 @@ xul|treechildren::-moz-tree-image(selected) {
  list-style-image: url("chrome://browser/skin/warning.svg");
 }

+input[type="text"][warning]:enabled:not(:focus) {
+  border-color: var(--yellow-60);
+  box-shadow: 0 0 0 1px var(--yellow-60), 0 0 0 4px var(--yellow-60-a30);
+}
+
 .card {
  background: var(--in-content-box-background);
  /* Needed for high-contrast where the border will appear. */