tor-browser

commit 41aeb796a64523a8dad142bffcf07f339810ec2d
parent e866f02b75251ed4cc3d237cc70dc532ced8b0ec
Author: Stephen Thompson <sthompson@mozilla.com>
Date:   Fri, 24 Oct 2025 23:39:31 +0000

Bug 1996113 - ShortcutUtils should not throw if key attribute is missing r=Gijs

This patch ensures that ShortcutUtils.prettifyShortcut will return a string instead of throwing if the supplied <key> element does not have a "key" attribute.

If a <key> doesn't have the "key" attribute, there is probably a more fundamental problem. However, it would be better if ShortcutUtils didn't make things worse.

I included a console warning to leave some bread crumbs to help anyone who has to debug any scenarios with missing shortcut data.

Additionally, if a pretty shortcut couldn't be computed, the DynamicShortcutTooltip won't cache that fact. Hopefully, the next time it runs, the necessary data might be present.

Note that `gNavigatorBundle.getFormattedString` can still throw if a string is not present in the strings bundle, so there are still ways for `DynamicShortcutTooltip.getText` to throw.

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

Diffstat:
M
browser/base/content/browser.js
 | 
13
+++++++++++--
M
toolkit/modules/ShortcutUtils.sys.mjs
 | 
53
+++++++++++++++++++++++++++++++++++++++++++++++------

2 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
@@ -3296,14 +3296,23 @@ const DynamicShortcutTooltip = {
     if (!this.cache.has(nodeId) && nodeId in this.nodeToTooltipMap) {
       let strId = this.nodeToTooltipMap[nodeId];
       let args = [];
+      let shouldCache = true;
       if (nodeId in this.nodeToShortcutMap) {
         let shortcutId = this.nodeToShortcutMap[nodeId];
         let shortcut = document.getElementById(shortcutId);
         if (shortcut) {
-          args.push(ShortcutUtils.prettifyShortcut(shortcut));
+          let prettyShortcut = ShortcutUtils.prettifyShortcut(shortcut);
+          args.push(prettyShortcut);
+          if (!prettyShortcut) {
+            shouldCache = false;
+          }
         }
       }
-      this.cache.set(nodeId, gNavigatorBundle.getFormattedString(strId, args));
+      let string = gNavigatorBundle.getFormattedString(strId, args);
+      if (shouldCache) {
+        this.cache.set(nodeId, string);
+      }
+      return string;
     }
     return this.cache.get(nodeId);
   },
diff --git a/toolkit/modules/ShortcutUtils.sys.mjs b/toolkit/modules/ShortcutUtils.sys.mjs
@@ -38,10 +38,22 @@ export var ShortcutUtils = {
   /**
    * Prettifies the modifier keys for an element.
    *
-   * @param Node aElemKey
-   *        The key element to get the modifiers from.
-   * @return string
-   *         A prettified and properly separated modifier keys string.
+   * @param {Element} aElemKey
+   *   The key element to get the modifiers from.
+   * @return {string}
+   *   A prettified and properly separated modifier keys string. If the data
+   *   on `aElemKey` is missing or incomplete, the return value is `""`.
+   * @example
+   *   // example1 (macOS): <key modifiers="ctrl" key="T"/>
+   *   prettifyShortcut(example1) => "⌃T"
+   *   // example1 (other): <key modifiers="ctrl" key="T"/>
+   *   prettifyShortcut(example1) => "Ctrl+T"
+   *   // example2 (macOS): <key modifiers="ctrl,shift" key="i"/>
+   *   prettifyShortcut(example2) => "⇧⌃I"
+   *   // example2 (other): <key modifiers="ctrl,shift" key="i"/>
+   *   prettifyShortcut(example2) => "Shift+Ctrl+I"
+   *   // example3: <key/>
+   *   prettifyShortcut(example3) => ""
    */
   prettifyShortcut(aElemKey) {
     let elemString = this.getModifierString(aElemKey.getAttribute("modifiers"));
@@ -49,6 +61,14 @@ export var ShortcutUtils = {
       aElemKey.getAttribute("keycode"),
       aElemKey.getAttribute("key")
     );
+    if (!key) {
+      console.warn(
+        "Key element",
+        aElemKey,
+        'is missing "key" and "keycode" attributes necessary to define a shortcut'
+      );
+      return "";
+    }
     return elemString + key;
   },
 
@@ -56,6 +76,17 @@ export var ShortcutUtils = {
     return AppConstants.platform == "macosx";
   },
 
+  /**
+   * @param {string|null} elemMod
+   *   Value of the `"modifiers"` attribute for a XUL <key> element.
+   *   Comma-separated list of key modifiers for a keyboard shortcut.
+   * @returns {string}
+   *   Pretty string representation of the set of modifiers that must be used
+   *   with another key in order to invoke a keyboard shortcut.
+   * @example getModifierString("shift,meta") => "⇧⌘"
+   * @example getModifierString("ctrl,alt") => "⌥⌃"
+   * @see KeyEventHandler
+   */
   getModifierString(elemMod) {
     if (!elemMod) {
       return "";
@@ -118,8 +149,18 @@ export var ShortcutUtils = {
     return elemString;
   },
 
+  /**
+   * @param {string|null} keyCode
+   *   Value of the `"keycode"` attribute of a XUL <key> element
+   * @param {string|null} keyAttribute
+   *   Value of the `"key"` attribute of a XUL <key> element
+   * @returns {string}
+   *   Pretty string representing a primary key that must be pressed to
+   *   engage a keyboard shortcut. Returns `""` if neither `keyCode` nor
+   *   `keyAttribute` are usable.
+   */
   getKeyString(keyCode, keyAttribute) {
-    let key;
+    let key = "";
     if (keyCode) {
       keyCode = keyCode.toUpperCase();
       if (AppConstants.platform == "macosx") {
@@ -139,7 +180,7 @@ export var ShortcutUtils = {
         console.error("Error finding ", keyCode, ": ", ex);
         key = keyCode.replace(/^VK_/, "");
       }
-    } else {
+    } else if (keyAttribute) {
       key = keyAttribute.toUpperCase();
     }