Match editor keyboard shortcuts by event.code as a fallback

So that Ctrl+A, Ctrl+Z, etc. still fire on non-US keyboard layouts where the physical "A" key produces a non-Latin character (Cyrillic, Greek, some AZERTY combinations, ...). KeyboardManager now tries event.key first and falls back to a US-layout translation of event.code (KeyA => a, Digit1 => 1, Numpad1 => 1) when no shortcut is bound on event.key. Also refactors KeyboardManager to store modifiers as a bitmask instead of a serialized string, and treats a shortcut array without any "mac+"-prefixed entry as applying on all platforms, letting us drop the redundant "mac+X" duplicates of bare "X" entries across the editor code.
2026-06-12 21:21:04 +02:00 · 2026-05-27 14:45:32 +02:00 · 2026-05-27 14:45:32 +02:00 · 836a08084e
commit 836a08084e
parent 19046a6949
6 changed files with 257 additions and 76 deletions
--- a/src/display/editor/color_picker.js
+++ b/src/display/editor/color_picker.js
@ -55,21 +55,12 @@ class ColorPicker {
      this,
      "_keyboardManager",
      new KeyboardManager([
-        [
-          ["Escape", "mac+Escape"],
-          ColorPicker.prototype._hideDropdownFromKeyboard,
-        ],
-        [[" ", "mac+ "], ColorPicker.prototype._colorSelectFromKeyboard],
-        [
-          ["ArrowDown", "ArrowRight", "mac+ArrowDown", "mac+ArrowRight"],
-          ColorPicker.prototype._moveToNext,
-        ],
-        [
-          ["ArrowUp", "ArrowLeft", "mac+ArrowUp", "mac+ArrowLeft"],
-          ColorPicker.prototype._moveToPrevious,
-        ],
-        [["Home", "mac+Home"], ColorPicker.prototype._moveToBeginning],
-        [["End", "mac+End"], ColorPicker.prototype._moveToEnd],
+        [["Escape"], ColorPicker.prototype._hideDropdownFromKeyboard],
+        [["Space"], ColorPicker.prototype._colorSelectFromKeyboard],
+        [["ArrowDown", "ArrowRight"], ColorPicker.prototype._moveToNext],
+        [["ArrowUp", "ArrowLeft"], ColorPicker.prototype._moveToPrevious],
+        [["Home"], ColorPicker.prototype._moveToBeginning],
+        [["End"], ColorPicker.prototype._moveToEnd],
      ])
    );
  }
--- a/src/display/editor/editor.js
+++ b/src/display/editor/editor.js
@ -137,26 +137,23 @@ class AnnotationEditor {
      this,
      "_resizerKeyboardManager",
      new KeyboardManager([
-        [["ArrowLeft", "mac+ArrowLeft"], resize, { args: [-small, 0] }],
+        [["ArrowLeft"], resize, { args: [-small, 0] }],
        [
          ["ctrl+ArrowLeft", "mac+shift+ArrowLeft"],
          resize,
          { args: [-big, 0] },
        ],
-        [["ArrowRight", "mac+ArrowRight"], resize, { args: [small, 0] }],
+        [["ArrowRight"], resize, { args: [small, 0] }],
        [
          ["ctrl+ArrowRight", "mac+shift+ArrowRight"],
          resize,
          { args: [big, 0] },
        ],
-        [["ArrowUp", "mac+ArrowUp"], resize, { args: [0, -small] }],
+        [["ArrowUp"], resize, { args: [0, -small] }],
        [["ctrl+ArrowUp", "mac+shift+ArrowUp"], resize, { args: [0, -big] }],
-        [["ArrowDown", "mac+ArrowDown"], resize, { args: [0, small] }],
+        [["ArrowDown"], resize, { args: [0, small] }],
        [["ctrl+ArrowDown", "mac+shift+ArrowDown"], resize, { args: [0, big] }],
-        [
-          ["Escape", "mac+Escape"],
-          AnnotationEditor.prototype._stopResizingWithKeyboard,
-        ],
+        [["Escape"], AnnotationEditor.prototype._stopResizingWithKeyboard],
      ])
    );
  }
--- a/src/display/editor/freetext.js
+++ b/src/display/editor/freetext.js
@ -73,12 +73,10 @@ class FreeTextEditor extends AnnotationEditor {
          proto.commitOrRemove,
          { bubbles: true },
        ],
+        [["ctrl+Enter", "mac+meta+Enter"], proto.commitOrRemove],
+        [["Escape"], proto.commitOrRemove],
        [
-          ["ctrl+Enter", "mac+meta+Enter", "Escape", "mac+Escape"],
-          proto.commitOrRemove,
-        ],
-        [
-          ["ArrowLeft", "mac+ArrowLeft"],
+          ["ArrowLeft"],
          proto._translateEmpty,
          { args: [-small, 0], checker: arrowChecker },
        ],
@ -88,7 +86,7 @@ class FreeTextEditor extends AnnotationEditor {
          { args: [-big, 0], checker: arrowChecker },
        ],
        [
-          ["ArrowRight", "mac+ArrowRight"],
+          ["ArrowRight"],
          proto._translateEmpty,
          { args: [small, 0], checker: arrowChecker },
        ],
@ -98,7 +96,7 @@ class FreeTextEditor extends AnnotationEditor {
          { args: [big, 0], checker: arrowChecker },
        ],
        [
-          ["ArrowUp", "mac+ArrowUp"],
+          ["ArrowUp"],
          proto._translateEmpty,
          { args: [0, -small], checker: arrowChecker },
        ],
@ -108,7 +106,7 @@ class FreeTextEditor extends AnnotationEditor {
          { args: [0, -big], checker: arrowChecker },
        ],
        [
-          ["ArrowDown", "mac+ArrowDown"],
+          ["ArrowDown"],
          proto._translateEmpty,
          { args: [0, small], checker: arrowChecker },
        ],
--- a/src/display/editor/highlight.js
+++ b/src/display/editor/highlight.js
@ -94,10 +94,10 @@ class HighlightEditor extends AnnotationEditor {
      this,
      "_keyboardManager",
      new KeyboardManager([
-        [["ArrowLeft", "mac+ArrowLeft"], proto._moveCaret, { args: [0] }],
-        [["ArrowRight", "mac+ArrowRight"], proto._moveCaret, { args: [1] }],
-        [["ArrowUp", "mac+ArrowUp"], proto._moveCaret, { args: [2] }],
-        [["ArrowDown", "mac+ArrowDown"], proto._moveCaret, { args: [3] }],
+        [["ArrowLeft"], proto._moveCaret, { args: [0] }],
+        [["ArrowRight"], proto._moveCaret, { args: [1] }],
+        [["ArrowUp"], proto._moveCaret, { args: [2] }],
+        [["ArrowDown"], proto._moveCaret, { args: [3] }],
      ])
    );
  }
--- a/src/display/editor/tools.js
+++ b/src/display/editor/tools.js
@ -512,56 +512,102 @@ class CommandManager {
 * non-mac OSes.
 */
 class KeyboardManager {
+  static ALT = 0x1;
+
+  static CTRL = 0x2;
+
+  static META = 0x4;
+
+  static SHIFT = 0x8;
+
  /**
   * Create a new keyboard manager class.
   * @param {Array<Array>} callbacks - an array containing an array of shortcuts
-   * and a callback to call.
-   * A shortcut is a string like `ctrl+c` or `mac+ctrl+c` for mac OS.
+   * and a callback to call. If the array contains no `mac+`-prefixed entry,
+   * every shortcut applies on all platforms. As soon as it contains at least
+   * one `mac+` entry, the `mac+` ones become the macOS-only set and the bare
+   * entries apply only on non-Mac.
   */
  constructor(callbacks) {
-    this.buffer = [];
    this.callbacks = new Map();
-    this.allKeys = new Set();

    const { isMac } = FeatureTest.platform;
    for (const [keys, callback, options = {}] of callbacks) {
+      const hasMacOverride = keys.some(k => k.startsWith("mac+"));
      for (const key of keys) {
-        const isMacKey = key.startsWith("mac+");
-        if (isMac && isMacKey) {
-          this.callbacks.set(key.slice(4), { callback, options });
-          this.allKeys.add(key.split("+").at(-1));
-        } else if (!isMac && !isMacKey) {
-          this.callbacks.set(key, { callback, options });
-          this.allKeys.add(key.split("+").at(-1));
+        let shortcut = key;
+        if (hasMacOverride) {
+          const isMacKey = key.startsWith("mac+");
+          if (isMac !== isMacKey) {
+            continue;
+          }
+          if (isMacKey) {
+            shortcut = key.slice(4);
+          }
        }
+        const [keyName, modifiers] = KeyboardManager.#parseShortcut(shortcut);
+        if (keyName === null) {
+          continue;
+        }
+        this.callbacks
+          .getOrInsertComputed(keyName, () => [])
+          .push({ callback, options, modifiers });
      }
    }
  }

  /**
-   * Serialize an event into a string in order to match a
-   * potential key for a callback.
-   * @param {KeyboardEvent} event
-   * @returns {string}
+   * Parse a shortcut string like "ctrl+shift+a" into a `[key, modifiers]`
+   * pair. Modifier names are case-insensitive and may appear in any order;
+   * the key part is matched against `event.key` so `Space` is normalized to
+   * `" "` but other names like `ArrowLeft`, `Enter`, `Backspace`, and
+   * single-letter keys (`a`, `Z`) are preserved.
+   * @param {string} value
+   * @returns {[string|null, number]}
   */
-  #serialize(event) {
-    if (event.altKey) {
-      this.buffer.push("alt");
+  static #parseShortcut(value) {
+    let keyPart = null;
+    let modifiers = 0;
+    for (let part of value.split("+")) {
+      part = part.trim();
+      if (!part) {
+        continue;
+      }
+      const upper = part.toUpperCase();
+      const modifier = KeyboardManager[upper];
+      if (modifier) {
+        modifiers |= modifier;
+        continue;
+      }
+      if (keyPart !== null) {
+        warn(`KeyboardManager: multiple keys in shortcut "${value}"`);
+        break;
+      }
+      keyPart = upper === "SPACE" ? " " : part;
    }
-    if (event.ctrlKey) {
-      this.buffer.push("ctrl");
+    if (keyPart === null) {
+      warn(`KeyboardManager: no key found in shortcut "${value}"`);
    }
-    if (event.metaKey) {
-      this.buffer.push("meta");
-    }
-    if (event.shiftKey) {
-      this.buffer.push("shift");
-    }
-    this.buffer.push(event.key);
-    const str = this.buffer.join("+");
-    this.buffer.length = 0;
+    return [keyPart, modifiers];
+  }

-    return str;
+  /**
+   * Translate `event.code` (a layout-independent physical key identifier) to
+   * the equivalent `event.key` value on a US layout, so a Ctrl+A shortcut
+   * still fires when the user is on a layout where the "A" key produces a
+   * non-Latin character.
+   * @param {string} code
+   * @returns {string|null}
+   */
+  static #codeToKey(code) {
+    // KeyA..KeyZ -> a..z, Digit0..Digit9 / Numpad0..Numpad9 -> 0..9.
+    // Codes like NumpadEnter are intentionally skipped — their event.key
+    // already matches the corresponding non-numpad key.
+    const match = /^(?:Key([A-Z])|(?:Digit|Numpad)(\d))$/.exec(code);
+    if (!match) {
+      return null;
+    }
+    return match[1]?.toLowerCase() ?? match[2];
  }

  /**
@ -572,13 +618,38 @@ class KeyboardManager {
   * @returns
   */
  exec(self, event) {
-    if (!this.allKeys.has(event.key)) {
-      return;
+    let shortcuts = this.callbacks.get(event.key);
+    if (!shortcuts) {
+      // Layout-independent fallback: on a Cyrillic layout the physical "A"
+      // key reports event.key="ф" but event.code="KeyA". This must be skipped
+      // when event.key is already a Latin letter, otherwise on AZERTY (where
+      // the "A" key has event.key="a" but event.code="KeyQ") ctrl+A would
+      // wrongly trigger ctrl+Q.
+      if (/^[a-z]$/i.test(event.key)) {
+        return;
+      }
+      const fallback = KeyboardManager.#codeToKey(event.code);
+      if (fallback === null || fallback === event.key) {
+        return;
+      }
+      shortcuts = this.callbacks.get(fallback);
+      if (!shortcuts) {
+        return;
+      }
    }
-    const info = this.callbacks.get(this.#serialize(event));
+
+    const eventModifiers =
+      (event.altKey ? KeyboardManager.ALT : 0) |
+      (event.ctrlKey ? KeyboardManager.CTRL : 0) |
+      (event.metaKey ? KeyboardManager.META : 0) |
+      (event.shiftKey ? KeyboardManager.SHIFT : 0);
+    const info = shortcuts.find(
+      shortcut => shortcut.modifiers === eventModifiers
+    );
    if (!info) {
      return;
    }
+
    const {
      callback,
      options: { bubbles = false, args = [], checker = null },
@ -844,7 +915,7 @@ class AnnotationEditorUIManager {
          { checker: textInputChecker },
        ],
        [
-          ["Enter", "mac+Enter"],
+          ["Enter"],
          proto.addNewEditorFromKeyboard,
          {
            // Those shortcuts can be used in the toolbar for some other actions
@ -857,7 +928,7 @@ class AnnotationEditorUIManager {
          },
        ],
        [
-          [" ", "mac+ "],
+          ["Space"],
          proto.addNewEditorFromKeyboard,
          {
            // Those shortcuts can be used in the toolbar for some other actions
@ -868,9 +939,9 @@ class AnnotationEditorUIManager {
              self.#container.contains(document.activeElement),
          },
        ],
-        [["Escape", "mac+Escape"], proto.unselectAll],
+        [["Escape"], proto.unselectAll],
        [
-          ["ArrowLeft", "mac+ArrowLeft"],
+          ["ArrowLeft"],
          proto.translateSelectedEditors,
          { args: [-small, 0], checker: arrowChecker },
        ],
@ -880,7 +951,7 @@ class AnnotationEditorUIManager {
          { args: [-big, 0], checker: arrowChecker },
        ],
        [
-          ["ArrowRight", "mac+ArrowRight"],
+          ["ArrowRight"],
          proto.translateSelectedEditors,
          { args: [small, 0], checker: arrowChecker },
        ],
@ -890,7 +961,7 @@ class AnnotationEditorUIManager {
          { args: [big, 0], checker: arrowChecker },
        ],
        [
-          ["ArrowUp", "mac+ArrowUp"],
+          ["ArrowUp"],
          proto.translateSelectedEditors,
          { args: [0, -small], checker: arrowChecker },
        ],
@ -900,7 +971,7 @@ class AnnotationEditorUIManager {
          { args: [0, -big], checker: arrowChecker },
        ],
        [
-          ["ArrowDown", "mac+ArrowDown"],
+          ["ArrowDown"],
          proto.translateSelectedEditors,
          { args: [0, small], checker: arrowChecker },
        ],
--- a/test/unit/editor_spec.js
+++ b/test/unit/editor_spec.js
@ -13,7 +13,11 @@
 * limitations under the License.
 */

-import { CommandManager } from "../../src/display/editor/tools.js";
+import {
+  CommandManager,
+  KeyboardManager,
+} from "../../src/display/editor/tools.js";
+import { FeatureTest } from "../../src/shared/util.js";
 import { SignatureExtractor } from "../../src/display/editor/drawers/signaturedraw.js";

 describe("editor", function () {
@ -138,4 +142,124 @@ describe("editor", function () {
    decompressed = await SignatureExtractor.decompressSignature(compressed);
    expect(decompressed).toEqual(signature);
  });
+
+  describe("Keyboard Manager", function () {
+    function makeEvent(props) {
+      return {
+        altKey: false,
+        ctrlKey: false,
+        metaKey: false,
+        shiftKey: false,
+        preventDefault() {},
+        stopPropagation() {},
+        ...props,
+      };
+    }
+
+    function withPlatform(isMac, callback) {
+      const descriptor = Object.getOwnPropertyDescriptor(
+        FeatureTest,
+        "platform"
+      );
+      Object.defineProperty(FeatureTest, "platform", {
+        value: { isMac },
+        configurable: true,
+      });
+      try {
+        callback();
+      } finally {
+        Object.defineProperty(FeatureTest, "platform", descriptor);
+      }
+    }
+
+    it("should match a shortcut by event.key", function () {
+      let called = 0;
+      const manager = new KeyboardManager([[["ctrl+a"], () => called++]]);
+
+      manager.exec(null, makeEvent({ key: "a", code: "KeyA", ctrlKey: true }));
+      expect(called).toEqual(1);
+    });
+
+    it("should not fire when the modifiers don't match", function () {
+      let called = 0;
+      const manager = new KeyboardManager([[["ctrl+a"], () => called++]]);
+
+      manager.exec(null, makeEvent({ key: "a", code: "KeyA", metaKey: true }));
+      expect(called).toEqual(0);
+    });
+
+    it("should fall back to event.code on a non-Latin layout", function () {
+      let called = 0;
+      const manager = new KeyboardManager([[["ctrl+a"], () => called++]]);
+
+      manager.exec(null, makeEvent({ key: "ф", code: "KeyA", ctrlKey: true }));
+      expect(called).toEqual(1);
+    });
+
+    it("should not remap a Latin letter via event.code", function () {
+      let called = 0;
+      const manager = new KeyboardManager([[["ctrl+q"], () => called++]]);
+
+      manager.exec(null, makeEvent({ key: "a", code: "KeyQ", ctrlKey: true }));
+      expect(called).toEqual(0);
+
+      manager.exec(null, makeEvent({ key: "q", code: "KeyA", ctrlKey: true }));
+      expect(called).toEqual(1);
+    });
+
+    it("should match an uppercase event.key (e.g. shift on mac)", function () {
+      let called = 0;
+      const manager = new KeyboardManager([
+        [["ctrl+shift+z", "ctrl+shift+Z"], () => called++],
+      ]);
+
+      manager.exec(
+        null,
+        makeEvent({ key: "Z", code: "KeyZ", ctrlKey: true, shiftKey: true })
+      );
+      expect(called).toEqual(1);
+    });
+
+    it("should use the mac+ shortcut on macOS", function () {
+      withPlatform(true, () => {
+        let called = 0;
+        const manager = new KeyboardManager([
+          [["ctrl+a", "mac+meta+a"], () => called++],
+        ]);
+
+        manager.exec(
+          null,
+          makeEvent({ key: "a", code: "KeyA", metaKey: true })
+        );
+        expect(called).toEqual(1);
+
+        manager.exec(
+          null,
+          makeEvent({ key: "a", code: "KeyA", ctrlKey: true })
+        );
+        expect(called).toEqual(1);
+      });
+    });
+
+    it("should use the bare shortcut on non-macOS", function () {
+      withPlatform(false, () => {
+        let called = 0;
+        const manager = new KeyboardManager([
+          [["ctrl+a", "mac+meta+a"], () => called++],
+        ]);
+
+        manager.exec(
+          null,
+          makeEvent({ key: "a", code: "KeyA", ctrlKey: true })
+        );
+        expect(called).toEqual(1);
+
+        manager.exec(
+          null,
+          makeEvent({ key: "a", code: "KeyA", metaKey: true })
+        );
+        expect(called).toEqual(1);
+      });
+    });
+  });
 });