From 4c5b7ca511d86349b9bd97c910aa709dac31b901 Mon Sep 17 00:00:00 2001
From: scheinriese <futurized@icloud.com>
Date: Tue, 10 Mar 2026 11:17:57 +0100
Subject: [PATCH] Address remaining should-fix items from pr-review

- Extract animate-element!/highlight-row! helpers in shortcut-press! (was 4x duplication)
- Extract matches-keystroke? predicate (was duplicated in count + render)
- Replace <a> tags with <button> for all interactive elements (remove, clear, reset, undo, toggle, refresh)
- Add aria-label to icon-only buttons for screen reader support
- Add button reset CSS for elements changed from <a> to <button>
- Replace hardcoded rgba colors in CSS glow/shadow/row-pressed with theme variables
- Define press-animation-ms constant (was magic number 160)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 deps/shui/src/logseq/shui/shortcut.cljs    | 61 ++++++++-----------
 resources/css/shui.css                     | 22 ++++---
 src/main/frontend/components/shortcut.cljs | 71 ++++++++++++----------
 src/main/frontend/components/shortcut.css  | 17 ++++++
 4 files changed, 94 insertions(+), 77 deletions(-)

diff --git a/deps/shui/src/logseq/shui/shortcut.cljs b/deps/shui/src/logseq/shui/shortcut.cljs
index 61357856d6..02f1de58a3 100644
--- a/deps/shui/src/logseq/shui/shortcut.cljs
+++ b/deps/shui/src/logseq/shui/shortcut.cljs
@@ -149,11 +149,33 @@
                            (string/split % #"\+")
                            %)))))))
 
+(def ^:private press-animation-ms 160)
+
+(defn- highlight-row!
+  "Add or remove the row highlight class on the closest .shui-shortcut-row ancestor."
+  [^js container add?]
+  (when-let [^js row (or (.closest container ".shui-shortcut-row")
+                         (.-parentElement container))]
+    (if add?
+      (.add (.-classList row) "shui-shortcut-row--pressed")
+      (.remove (.-classList row) "shui-shortcut-row--pressed"))))
+
+(defn- animate-element!
+  "Add pressed class, optionally highlight row, then auto-reset after animation."
+  [^js el ^js container highlight-row?]
+  (.add (.-classList el) "shui-shortcut-key-pressed")
+  (when highlight-row? (highlight-row! container true))
+  (js/setTimeout
+   (fn []
+     (.remove (.-classList el) "shui-shortcut-key-pressed")
+     (when highlight-row? (highlight-row! container false)))
+   press-animation-ms))
+
 (defn shortcut-press!
   "Central helper to trigger key press animation.
    Finds all nodes with matching data-shortcut-binding and animates individual keys.
    Optionally highlights parent row.
-   
+
    Args:
    - binding: normalized shortcut binding string (e.g., \"cmd+k\")
    - highlight-row?: if true, also highlights parent row (default: false)"
@@ -163,44 +185,11 @@
          selector (str "[data-shortcut-binding=\"" normalized "\"]")
          containers (.querySelectorAll js/document selector)]
      (doseq [^js container (array-seq containers)]
-       ;; Find all individual keys within this container
        (let [keys (.querySelectorAll container "kbd.shui-shortcut-key")]
          (if (> (.-length keys) 0)
-           ;; Animate individual keys
            (doseq [^js key-el (array-seq keys)]
-             (.add (.-classList key-el) "shui-shortcut-key-pressed")
-             (when highlight-row?
-               (let [^js row (or (.closest container ".shui-shortcut-row")
-                                 (.-parentElement container))]
-                 (when row
-                   (.add (.-classList row) "shui-shortcut-row--pressed"))))
-             ;; Auto-reset after animation duration
-             (js/setTimeout
-              (fn []
-                (.remove (.-classList key-el) "shui-shortcut-key-pressed")
-                (when highlight-row?
-                  (let [^js row (or (.closest container ".shui-shortcut-row")
-                                    (.-parentElement container))]
-                    (when row
-                      (.remove (.-classList row) "shui-shortcut-row--pressed")))))
-              160))
-           ;; Fallback: animate container if no keys found
-           (do
-             (.add (.-classList container) "shui-shortcut-key-pressed")
-             (when highlight-row?
-               (let [^js row (or (.closest container ".shui-shortcut-row")
-                                 (.-parentElement container))]
-                 (when row
-                   (.add (.-classList row) "shui-shortcut-row--pressed"))))
-             (js/setTimeout
-              (fn []
-                (.remove (.-classList container) "shui-shortcut-key-pressed")
-                (when highlight-row?
-                  (let [^js row (or (.closest container ".shui-shortcut-row")
-                                    (.-parentElement container))]
-                    (when row
-                      (.remove (.-classList row) "shui-shortcut-row--pressed")))))
-              160))))))))
+             (animate-element! key-el container highlight-row?))
+           (animate-element! container container highlight-row?)))))))
 
 (rum/defc combo-keys
   "Renders combo keys (simultaneous key combinations) with separator."
diff --git a/resources/css/shui.css b/resources/css/shui.css
index ea453ae4fa..05f20ff1e6 100644
--- a/resources/css/shui.css
+++ b/resources/css/shui.css
@@ -283,13 +283,15 @@ div[data-radix-popper-content-wrapper] {
 /* Glow effect for combo and separate keys */
 /* Combo keys: glow on container (wraps all keys together) */
 .shui-shortcut-combo.shui-shortcut-glow {
-  box-shadow: rgba(255, 255, 255, 0.15) 0px 1px 0px 0px inset, rgba(0, 0, 0, 0.15) 0px -1px 0px 0px inset;
+  box-shadow: var(--lx-gray-01-alpha, rgba(255, 255, 255, 0.15)) 0px 1px 0px 0px inset,
+              var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.15)) 0px -1px 0px 0px inset;
   border: none;
 }
 
 /* Separate keys: glow on individual keys (not container) */
 .shui-shortcut-separate.shui-shortcut-glow kbd.shui-shortcut-key {
-  box-shadow: rgba(255, 255, 255, 0.15) 0px 1px 0px 0px inset, rgba(0, 0, 0, 0.15) 0px -1px 0px 0px inset;
+  box-shadow: var(--lx-gray-01-alpha, rgba(255, 255, 255, 0.15)) 0px 1px 0px 0px inset,
+              var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.15)) 0px -1px 0px 0px inset;
   border: none;
 }
 
@@ -350,7 +352,7 @@ kbd.shui-shortcut-key,
   line-height: 16px;
   min-width: fit-content;
   transition: transform 140ms ease-out, box-shadow 140ms ease-out;
-  box-shadow: 0 0 0 rgba(0, 0, 0, 0);
+  box-shadow: 0 0 0 transparent;
 }
 
 /* Keys in separate containers get their own styling */
@@ -383,23 +385,27 @@ kbd.shui-shortcut-key-pressed,
 /* Key press animation with glow - preserve glow effect */
 /* Combo keys: animate the container */
 .shui-shortcut-combo.shui-shortcut-glow.shui-shortcut-key-pressed {
-  box-shadow: rgba(255, 255, 255, 0.15) 0px 2px 0px 0px inset, rgba(0, 0, 0, 0.15) 0px 0px 0px 0px inset, 0 1px 2px rgba(0, 0, 0, 0.2);
+  box-shadow: var(--lx-gray-01-alpha, rgba(255, 255, 255, 0.15)) 0px 2px 0px 0px inset,
+              var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.15)) 0px 0px 0px 0px inset,
+              0 1px 2px var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.2));
 }
 
 /* Separate keys: animate individual keys */
 .shui-shortcut-separate.shui-shortcut-glow kbd.shui-shortcut-key-pressed {
-  box-shadow: rgba(255, 255, 255, 0.15) 0px 2px 0px 0px inset, rgba(0, 0, 0, 0.15) 0px 0px 0px 0px inset, 0 1px 2px rgba(0, 0, 0, 0.2);
+  box-shadow: var(--lx-gray-01-alpha, rgba(255, 255, 255, 0.15)) 0px 2px 0px 0px inset,
+              var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.15)) 0px 0px 0px 0px inset,
+              0 1px 2px var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.2));
 }
 
 /* Key press animation without glow */
 .shui-shortcut-combo:not(.shui-shortcut-glow) kbd.shui-shortcut-key-pressed,
 .shui-shortcut-separate:not(.shui-shortcut-glow) kbd.shui-shortcut-key-pressed {
-  box-shadow: 0 1px 2px rgba(0, 0, 0, 0.2);
+  box-shadow: 0 1px 2px var(--lx-gray-12-alpha, rgba(0, 0, 0, 0.2));
 }
 
 /* Row highlight animation */
 .shui-shortcut-row--pressed {
-  background-color: rgba(223, 239, 254, 0.1);
+  background-color: var(--lx-accent-04-alpha, rgba(223, 239, 254, 0.1));
   transition: background-color 160ms ease-out;
 }
 
@@ -417,7 +423,7 @@ kbd.shui-shortcut-key-pressed,
   .shui-shortcut-row--pressed {
     transition: none;
     transform: none;
-    box-shadow: 0 0 0 rgba(0, 0, 0, 0);
+    box-shadow: 0 0 0 transparent;
   }
   
   .shui-shortcut-row--pressed {
diff --git a/src/main/frontend/components/shortcut.cljs b/src/main/frontend/components/shortcut.cljs
index f9f30777b6..fac1916ef9 100644
--- a/src/main/frontend/components/shortcut.cljs
+++ b/src/main/frontend/components/shortcut.cljs
@@ -170,10 +170,11 @@
         [:div {:class (str "shortcut-input-binding"
                            (when accumulating? " shortcut-input-binding--pending"))}
          (shui/shortcut keystroke)
-         [:a.shortcut-binding-remove
+         [:button.shortcut-binding-remove
           {:on-click (fn [^js e]
                        (.stopPropagation e)
-                       (clear!))}
+                       (clear!))
+           :aria-label "Remove filter"}
           (ui/icon "x" {:size 12})]])
       ;; Placeholder
       (when-not has-keystroke?
@@ -184,7 +185,7 @@
      [:div.shortcut-toolbar
       [:div
        (when has-keystroke?
-         [:a.shortcut-toolbar-action
+         [:button.shortcut-toolbar-action
           {:on-click clear!}
           (ui/icon "rotate" {:size 12})
           [:span "Clear"]])]
@@ -221,10 +222,11 @@
                          (set-q! v))}]
 
        (when-not (string/blank? q)
-         [:a.x
+         [:button.x
           {:on-click (fn []
                        (set-q! "")
-                       (js/setTimeout #(some-> (rum/deref *search-ref) (.focus)) 50))}
+                       (js/setTimeout #(some-> (rum/deref *search-ref) (.focus)) 50))
+           :aria-label "Clear search"}
           (ui/icon "x" {:size 12})])]
 
       ;; keystroke filter button
@@ -253,10 +255,11 @@
            [:span.shortcut-keystroke-keys
             (ui/icon "keyboard" {:size 14})
             (shui/shortcut keystroke)]
-           [:a.shortcut-keystroke-clear
+           [:button.shortcut-keystroke-clear
             {:on-click (fn [^js e]
                          (.stopPropagation e)
-                         (set-keystroke! ""))}
+                         (set-keystroke! ""))
+             :aria-label "Clear keystroke filter"}
             (ui/icon "x" {:size 12})]]
           [:button.shortcut-keystroke-inactive
            {:on-click open-filter!}
@@ -280,14 +283,14 @@
 
       (when (string/blank? q)
         [:div.flex.items-center.gap-2
-         [:a.flex.items-center.icon-link
+         [:button.flex.items-center.icon-link
           {:on-click toggle-categories-fn
-           :title "Toggle categories pane"}
+           :aria-label "Toggle categories pane"}
           (ui/icon "fold")]
 
-         [:a.flex.items-center.icon-link
+         [:button.flex.items-center.icon-link
           {:on-click refresh-shortcuts-list!
-           :title "Refresh all"}
+           :aria-label "Refresh all"}
           (ui/icon "refresh")]])]]))
 
 (rum/defc shortcut-desc-label
@@ -357,9 +360,10 @@
                :when (not (nil? m))]
            [:li
             {:key (str id')}
-            [:a.select-none.hover:underline
+            [:button.select-none.hover:underline
              {:on-click (fn [^js e] (open-customize-shortcut-dialog! e id'))
-              :title (str handler-id)}
+              :title (str handler-id)
+              :aria-label (dh/get-shortcut-desc m)}
              [:code.inline-block.mr-1.text-xs
               (shortcut-utils/decorate-binding k)]
              [:span
@@ -647,8 +651,9 @@
         [:div.shortcut-input-binding {:key x}
          (shui/shortcut x)
          (when (#{:idle :accepted :esc-hint :removed :reset} rec-state)
-           [:a.shortcut-binding-remove
-            {:on-click (fn [^js e]
+           [:button.shortcut-binding-remove
+            {:aria-label "Remove binding"
+             :on-click (fn [^js e]
                          (.stopPropagation e)
                          (let [new-binding (vec (concat (subvec current-binding 0 idx)
                                                         (subvec current-binding (inc idx))))
@@ -664,8 +669,9 @@
         [:div.shortcut-input-binding.shortcut-input-binding--pending
          (shui/shortcut keystroke)
          (when (#{:conflict-cross :conflict-same} rec-state)
-           [:a.shortcut-binding-remove
-            {:on-click (fn [^js e]
+           [:button.shortcut-binding-remove
+            {:aria-label "Remove binding"
+             :on-click (fn [^js e]
                          (.stopPropagation e)
                          (cancel-fn!))}
             (ui/icon "x" {:size 12})])])
@@ -676,7 +682,7 @@
      ;; FEEDBACK BANNER (conditional)
      (let [undo-link
            (when undo-snapshot
-             [:a.shortcut-feedback-action
+             [:button.shortcut-feedback-action
               {:on-click (fn []
                            (execute-undo! undo-snapshot)
                            (when-let [own (some #(when (= (:action-id %) k) %) (:entries undo-snapshot))]
@@ -743,7 +749,7 @@
        ;; Reset (only when changed from default)
        (when (and (#{:idle :accepted :removed} rec-state)
                   (not= current-binding binding))
-         [:a.shortcut-toolbar-action
+         [:button.shortcut-toolbar-action
           {:on-click reset-fn!}
           (ui/icon "rotate" {:size 12})
           [:span "Reset"]])]
@@ -798,6 +804,17 @@
             {:All 0 :Custom 0 :Unset 0 :Disabled 0}
             all-bindings)))
 
+(defn- matches-keystroke?
+  "Check if any of the shortcut's bindings match the recorded keystroke filter."
+  [binding user-binding keystroke]
+  (let [binding' (or user-binding binding)
+        keystroke' (some-> (shortcut-utils/safe-parse-string-binding keystroke) (bean/->clj))]
+    (when (sequential? binding')
+      (some #(when-let [s (some-> % (dh/mod-key) (shortcut-utils/safe-parse-string-binding) (bean/->clj))]
+               (or (= s keystroke')
+                   (and (sequential? s) (sequential? keystroke')
+                        (apply = (map first [s keystroke']))))) binding'))))
+
 (defn- count-visible-shortcuts
   "Count shortcuts visible after applying category filter and keystroke filter."
   [result-list-map filter-key in-keystroke? keystroke]
@@ -815,13 +832,7 @@
                           (contains? cats filter-key))
                 :when (or (not in-keystroke?)
                           (and (not disabled?) (not unset?)
-                               (let [binding' (or user-binding binding)
-                                     keystroke' (some-> (shortcut-utils/safe-parse-string-binding keystroke) (bean/->clj))]
-                                 (when (sequential? binding')
-                                   (some #(when-let [s (some-> % (dh/mod-key) (shortcut-utils/safe-parse-string-binding) (bean/->clj))]
-                                            (or (= s keystroke')
-                                                (and (sequential? s) (sequential? keystroke')
-                                                     (apply = (map first [s keystroke']))))) binding')))))]
+                               (matches-keystroke? binding user-binding keystroke)))]
             id)))
        count))
 
@@ -935,13 +946,7 @@
                   (when (or (not in-keystroke?)
                             (and (not disabled?)
                                  (not unset?)
-                                 (let [binding' (or user-binding binding)
-                                       keystroke' (some-> (shortcut-utils/safe-parse-string-binding keystroke) (bean/->clj))]
-                                   (when (sequential? binding')
-                                     (some #(when-let [s (some-> % (dh/mod-key) (shortcut-utils/safe-parse-string-binding) (bean/->clj))]
-                                              (or (= s keystroke')
-                                                  (and (sequential? s) (sequential? keystroke')
-                                                       (apply = (map first [s keystroke']))))) binding')))))
+                                 (matches-keystroke? binding user-binding keystroke)))
 
                     (let [row-action (when (and id (not disabled?))
                                        (fn [^js e]
diff --git a/src/main/frontend/components/shortcut.css b/src/main/frontend/components/shortcut.css
index b129d65d64..7321577b29 100644
--- a/src/main/frontend/components/shortcut.css
+++ b/src/main/frontend/components/shortcut.css
@@ -13,6 +13,23 @@
   }
 }
 
+/* Reset button styles for interactive elements changed from <a> to <button> */
+.cp__shortcut-page-x button.shortcut-binding-remove,
+.cp__shortcut-page-x button.shortcut-toolbar-action,
+.cp__shortcut-page-x button.shortcut-keystroke-clear,
+.cp__shortcut-page-x button.icon-link,
+.cp__shortcut-page-x button.x,
+.shortcut-popover button.shortcut-binding-remove,
+.shortcut-popover button.shortcut-toolbar-action,
+.shortcut-filter-popover button.shortcut-binding-remove,
+.shortcut-filter-popover button.shortcut-toolbar-action,
+button.shortcut-feedback-action {
+  all: unset;
+  cursor: pointer;
+  display: inline-flex;
+  align-items: center;
+}
+
 .cp__shortcut-page-x {
   @apply relative;