🐛 Fix custom shortcut with shift (#11669)

CLAUDE.md

@@ -1,3 +1,10 @@
 Read and follow the instructions in `AGENTS.md`.
 
 Treat `AGENTS.md` as the canonical project instruction file.
+
+## Comments and docstrings
+
+Keep comments and docstrings brief. They must always describe what the code
+currently does (its behavior, invariants, non-obvious rationale) — never
+what changed, when it changed, or history like "originally reported" /
+"before this existed" / "used to be X".

frontend/src/app/main/ui/shortcuts.cljs

@@ -45,51 +45,76 @@
    "Tab"        "tab"
    " "          "space"})
 
+;; Mousetrap (the runtime shortcut matcher, vendored at
+;; frontend/packages/mousetrap) resolves a keydown positionally - via keyCode
+;; - ONLY for this punctuation set (its _KEYCODE_MAP); it falls back to
+;; `event.key.toLowerCase()` (glyph-based) for letters and digits, which
+;; aren't in that map. Recording punctuation from `code` therefore matches
+;; what Mousetrap triggers regardless of layout (e.g. the key next to Right
+;; Shift on a US layout is always "Period", whatever glyph it types on a
+;; given ISO layout). Letters/digits must stay glyph-based to match
+;; Mousetrap's own resolution for them.
+(def ^:private code-name-map
+  {"Minus"        "-"
+   "Equal"        "="
+   "BracketLeft"  "["
+   "BracketRight" "]"
+   "Backslash"    "\\"
+   "Semicolon"    ";"
+   "Quote"        "'"
+   "Backquote"    "`"
+   "Comma"        ","
+   "Period"       "."
+   "Slash"        "/"})
+
+(defn- event->native
+  "Unwraps a goog.events.BrowserEvent to the native event, since the
+   wrapper copies `key` but not `code`."
+  [^js event]
+  (if (fn? (.-getBrowserEvent event))
+    (or (.getBrowserEvent event) event)
+    event))
+
+(defn- normalize-key
+  [^js event]
+  (let [key  (.-key event)]
+    (when (and key (not (contains? modifier-keys key)))
+      (or (get key-name-map key)
+          (get code-name-map (.-code (event->native event)))
+          (some-> key .toLowerCase)))))
+
+(defn- event->modifier-parts
+  [^js event]
+  (cond-> []
+    (and (.-ctrlKey event)
+         (not (cf/check-platform? :macos)))
+    (conj "ctrl")
+
+    (and (.-metaKey event)
+         (cf/check-platform? :macos))
+    (conj "command")
+
+    (.-altKey event)
+    (conj "alt")
+
+    (.-shiftKey event)
+    (conj "shift")))
+
 (defn- keyboard-event->mousetrap
   [^js event]
-  (let [parts (cond-> []
+  (let [parts (event->modifier-parts event)
-                (and (.-ctrlKey event)
+        key   (normalize-key event)]
-                     (not (cf/check-platform? :macos)))
-                (conj "ctrl")
-
-                (and (.-metaKey event)
-                     (cf/check-platform? :macos))
-                (conj "command")
-
-                (.-altKey event)
-                (conj "alt")
-
-                (.-shiftKey event)
-                (conj "shift"))
-        key   (.-key event)
-        key   (if (contains? modifier-keys key)
-                nil
-                (or (get key-name-map key)
-                    (.toLowerCase key)))]
     (when key
       (str/join "+" (conj parts key)))))
 
 (defn- keyboard-event->display-parts
   [^js event]
-  (let [parts (cond-> []
+  (let [parts (event->modifier-parts event)
-                (and (.-ctrlKey event)
-                     (not (cf/check-platform? :macos)))
-                (conj "ctrl")
-
-                (and (.-metaKey event)
-                     (cf/check-platform? :macos))
-                (conj "command")
-
-                (.-altKey event)
-                (conj "alt")
-
-                (.-shiftKey event)
-                (conj "shift"))
         key   (.-key event)]
     (if (contains? modifier-keys key)
       {:modifiers parts :finalized? false}
       {:modifiers parts
-       :final-key (or (get key-name-map key) (.toLowerCase key))
+       :final-key (normalize-key event)
        :finalized? true})))
 
 (defn translation-keyname
@@ -122,9 +147,10 @@
     (into {} (filter (fn [[k _]] (contains? known-keys k))) all-shortcuts)))
 
 (defn- import-context-group
-  "Imports a single context group from the payload, disabling any default
+  "Builds the custom-shortcuts map for one imported context group, disabling
-   shortcut whose command collides with a newly imported one, and any
+   any default shortcut (from `context-shortcuts`) whose command collides
-   previously-imported entry in the same batch with a duplicate command."
+   with an imported one, and any earlier entry in the same batch with a
+   duplicate command."
   [group context-shortcuts]
   (reduce
    (fn [acc [command recorded-command]]
@@ -163,11 +189,10 @@
   (map (fn [[k v]] [k (assoc v :translation (translation-keyname type k))]) item))
 
 (defn shortcut->command-string
-  "Extract a lowercase searchable string from a shortcut entry's key combo(s).
+  "Returns a lowercase, searchable string for a shortcut's key combo(s).
-  Prefers `:show-command` (display override) over `:command` (Mousetrap format),
+  Prefers `:show-command` over `:command`; joins a vector command (key
-  matching what the keycap UI renders. Joins vector commands (key sequences)
+  sequence) with spaces so each token is searchable; returns \"\" when
-  with a space so every token is searchable. Returns \"\" when there is no
+  there is no command (e.g. a section/subsection node)."
-  command (e.g. a section/subsection node)."
   [shortcut]
   (let [cmd (or (:show-command shortcut) (:command shortcut))]
     (-> (cond

frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs

@@ -1,5 +1,6 @@
 (ns frontend-tests.ui.settings-shortcuts-test
   (:require
+   [app.config :as cf]
    [app.main.data.profile :as du]
    [app.main.ui.settings.import-shortcuts-diff-modal :as diff-modal]
    [app.main.ui.settings.restore-shortcuts-modal :as restore-modal]
@@ -224,12 +225,10 @@
           "Should return a default command for :next-frame in :viewer context")))
 
 ;; --- shortcut->command-string + command-based search --------------------
-;; The search in both the settings shortcuts page and the workspace sidebar
+;; Shortcut search matches by translated name or key-combo string.
-;; matches shortcut entries by their translated name AND by their key-combo
+;; `shortcut->command-string` (in `app.main.ui.shortcuts`) extracts the
-;; string. `shortcut->command-string` (in `app.main.ui.shortcuts`) extracts the
+;; searchable combo from `:command`/`:show-command`; `matches-search` does
-;; searchable form from `:command`/`:show-command`; `matches-search` does the
+;; the case-insensitive substring match.
-;; case-insensitive substring match. These tests pin that contract so searching
-;; e.g. "ctrl" surfaces every shortcut whose combo includes ctrl.
 
 (t/deftest shortcut->command-string-extracts-string-command
   (t/testing "a plain string command is returned lowercased"
@@ -291,3 +290,130 @@
       ;; by command
       (t/is (or (matches-search (:translation shortcut) "ctrl")
                 (matches-search (ui-shortcuts/shortcut->command-string shortcut) "ctrl"))))))
+
+;; --- keyboard-event->mousetrap across keyboard layouts -------------------
+;; `keyboard-event->mousetrap` (private, exercised via #') turns a recorded
+;; keydown into a Mousetrap-style combo string, reading `event.code`
+;; (layout-independent) rather than `event.key` (layout- and Shift-dependent)
+;; so the recorded combo matches what Mousetrap resolves positionally. Real
+;; keydown events are wrapped by goog.events in a goog.events.BrowserEvent,
+;; which forwards `key` but not `code`; the fixtures below simulate that
+;; wrapper via `getBrowserEvent`.
+
+(defn- mock-wrapped-keydown
+  "Simulates a goog.events.BrowserEvent-wrapped keydown: `key` and modifiers
+   are forwarded directly, and `code` is only reachable via `getBrowserEvent`.
+   `key` is passed through as-is (not defaulted) so callers can simulate a
+   missing `event.key` with `:key nil` - substituting \"\" would defeat that,
+   since an empty string is truthy in ClojureScript."
+  [{:keys [key code shift ctrl alt meta]}]
+  #js {:key key
+       :shiftKey (boolean shift)
+       :ctrlKey (boolean ctrl)
+       :altKey (boolean alt)
+       :metaKey (boolean meta)
+       :getBrowserEvent (fn [] #js {:code code})})
+
+(defn- mock-native-keydown
+  "Simulates a plain, unwrapped native KeyboardEvent (no getBrowserEvent),
+   exercising the recorder's defensive fallback to read `code` directly."
+  [{:keys [key code shift ctrl alt meta]}]
+  #js {:key key
+       :code code
+       :shiftKey (boolean shift)
+       :ctrlKey (boolean ctrl)
+       :altKey (boolean alt)
+       :metaKey (boolean meta)})
+
+(t/deftest mousetrap-shift-period-on-us-layout
+  (t/testing "US layout: Shift + Period key types \">\""
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "shift+."
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key ">" :code "Period" :shift true})))))))
+
+(t/deftest mousetrap-shift-period-on-spanish-iso-layout
+  (t/testing "Spanish (ISO) layout: Shift + Period key types \":\" instead of
+              \">\", but the physical key (code \"Period\") is the same as on
+              a US layout, so the recorded combo must match"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "shift+."
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key ":" :code "Period" :shift true})))))))
+
+(t/deftest mousetrap-shift-period-on-arbitrary-layout
+  (t/testing "an arbitrary layout where Shift + Period produces a character
+              unrelated to \".\" or \":\" still normalizes to \"shift+.\",
+              since matching is driven by the physical key, not the glyph"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "shift+."
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key "?" :code "Period" :shift true})))))))
+
+(t/deftest mousetrap-letter-key-on-azerty-layout
+  (t/testing "French AZERTY: the key printed \"A\" sits where \"Q\" is on a US
+              layout (code \"KeyQ\") and types \"a\". Mousetrap doesn't
+              resolve letters positionally (absent from its
+              _MAP/_KEYCODE_MAP), so recording must stay glyph-based
+              (\"a\", not \"q\") to match what that physical key triggers"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "a"
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key "a" :code "KeyQ"})))))))
+
+(t/deftest mousetrap-digit-key-on-azerty-layout
+  (t/testing "French AZERTY: the unshifted top-row key at the US \"1\"
+              position (code \"Digit1\") types \"&\". Digits, like letters,
+              are absent from Mousetrap's _MAP/_KEYCODE_MAP, so recording
+              must stay glyph-based (\"&\") rather than resolve positionally
+              to \"1\""
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "&"
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key "&" :code "Digit1"})))))))
+
+(t/deftest mousetrap-reads-code-from-unwrapped-native-event
+  (t/testing "falls back to reading `code` straight off the event when it is
+              not a goog.events.BrowserEvent wrapper (no getBrowserEvent)"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "shift+."
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-native-keydown {:key ":" :code "Period" :shift true})))))))
+
+(t/deftest mousetrap-tolerates-getBrowserEvent-returning-nil
+  (t/testing "does not throw when getBrowserEvent() itself returns nil/undefined
+              (defensive: not reachable via goog.events' real dispatch today,
+              but normalize-key must not crash the recorder if it happens)"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (let [event #js {:key ":" :shiftKey true :ctrlKey false :altKey false
+                       :metaKey false :getBrowserEvent (fn [] nil)}]
+        (t/is (= "shift+:"
+                 (#'ui-shortcuts/keyboard-event->mousetrap event)))))))
+
+(t/deftest mousetrap-tolerates-missing-key
+  (t/testing "does not throw when `event.key` itself is nil/undefined"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (nil? (#'ui-shortcuts/keyboard-event->mousetrap
+                   (mock-wrapped-keydown {:key nil :code "Period"})))))))
+
+(t/deftest mousetrap-ignores-bare-modifier-press
+  (t/testing "pressing only Shift (no other key yet) records nothing"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (nil? (#'ui-shortcuts/keyboard-event->mousetrap
+                   (mock-wrapped-keydown {:key "Shift" :shift true})))))))
+
+(t/deftest mousetrap-falls-back-to-lowercased-key-when-unmapped
+  (t/testing "a key with no entry in the named-key or code maps (e.g. a
+              function key) falls back to the lowercased `key` value"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "f13"
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key "F13" :code "F13"})))))))
+
+(t/deftest mousetrap-full-modifier-combo-on-non-mac
+  (t/testing "ctrl+alt+shift all combine with the normalized key, in order"
+    (with-redefs [cf/check-platform? (constantly false)]
+      (t/is (= "ctrl+alt+shift+q"
+               (#'ui-shortcuts/keyboard-event->mousetrap
+                (mock-wrapped-keydown {:key "q" :code "KeyQ"
+                                       :ctrl true :alt true :shift true})))))))