From 9fa0d9c996f0ee866eaf336777cb0f20b97d83e5 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Mon, 21 Sep 2026 12:36:10 +0200 Subject: [PATCH] :bug: Fix custom shortcut with shift (#11669) --- CLAUDE.md | 7 + frontend/src/app/main/ui/shortcuts.cljs | 109 ++++++++------ .../ui/settings_shortcuts_test.cljs | 138 +++++++++++++++++- 3 files changed, 206 insertions(+), 48 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 70bf134a48..9d165adb8a 100644 --- a/CLAUDE.md +++ b/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". diff --git a/frontend/src/app/main/ui/shortcuts.cljs b/frontend/src/app/main/ui/shortcuts.cljs index 47454fa2c8..efbde7fdd6 100644 --- a/frontend/src/app/main/ui/shortcuts.cljs +++ b/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-> [] - (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) - key (if (contains? modifier-keys key) - nil - (or (get key-name-map key) - (.toLowerCase key)))] + (let [parts (event->modifier-parts event) + key (normalize-key event)] (when key (str/join "+" (conj parts key))))) (defn- keyboard-event->display-parts [^js event] - (let [parts (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")) + (let [parts (event->modifier-parts event) 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 - shortcut whose command collides with a newly imported one, and any - previously-imported entry in the same batch with a duplicate command." + "Builds the custom-shortcuts map for one imported context group, disabling + any default shortcut (from `context-shortcuts`) whose command collides + 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). - Prefers `:show-command` (display override) over `:command` (Mousetrap format), - matching what the keycap UI renders. Joins vector commands (key sequences) - with a space so every token is searchable. Returns \"\" when there is no - command (e.g. a section/subsection node)." + "Returns a lowercase, searchable string for a shortcut's key combo(s). + Prefers `:show-command` over `:command`; joins a vector command (key + sequence) with spaces so each token is searchable; returns \"\" when + there is no command (e.g. a section/subsection node)." [shortcut] (let [cmd (or (:show-command shortcut) (:command shortcut))] (-> (cond diff --git a/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs b/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs index ba668650d7..e9913c9b26 100644 --- a/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs +++ b/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 -;; matches shortcut entries by their translated name AND by their key-combo -;; string. `shortcut->command-string` (in `app.main.ui.shortcuts`) extracts the -;; searchable form from `:command`/`:show-command`; `matches-search` does the -;; case-insensitive substring match. These tests pin that contract so searching -;; e.g. "ctrl" surfaces every shortcut whose combo includes ctrl. +;; Shortcut search matches by translated name or key-combo string. +;; `shortcut->command-string` (in `app.main.ui.shortcuts`) extracts the +;; searchable combo from `:command`/`:show-command`; `matches-search` does +;; the case-insensitive substring match. (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})))))))