From ed0d6f0142e72f36bb3e65d87611fbc85db7fe57 Mon Sep 17 00:00:00 2001 From: scheinriese Date: Wed, 11 Mar 2026 12:16:53 +0100 Subject: [PATCH] Address pr-review findings: accessibility, dead code, and consistency fixes - Add ARIA live regions to feedback banners (role=alert/status) - Add role=dialog and aria-label to customize popover - Add aria-pressed to filter pill buttons - Remove disabled rows from tab order - Return focus to trigger on popover close - Add shortcut-badge-in to prefers-reduced-motion - Remove dead shortcut-conflicts-display component - Replace js/console.warn with lambdaisland.glogi - Merge duplicate cond branches in key handler - Add missing :shortcut-id to left sidebar Co-Authored-By: Claude Opus 4.6 --- .../frontend/components/left_sidebar.cljs | 3 +- src/main/frontend/components/shortcut.cljs | 180 ++++++++---------- src/main/frontend/components/shortcut.css | 5 + src/main/frontend/modules/shortcut/utils.cljs | 5 +- 4 files changed, 85 insertions(+), 108 deletions(-) diff --git a/src/main/frontend/components/left_sidebar.cljs b/src/main/frontend/components/left_sidebar.cljs index c077aa3b0e..bd47dc123a 100644 --- a/src/main/frontend/components/left_sidebar.cljs +++ b/src/main/frontend/components/left_sidebar.cljs @@ -123,7 +123,8 @@ (when shortcut [:span.ml-1 (ui/render-keyboard-shortcut - (ui/keyboard-shortcut-from-config shortcut {:pick-first? true}))]) + (ui/keyboard-shortcut-from-config shortcut {:pick-first? true}) + :shortcut-id shortcut)]) more]]) (rum/defc sidebar-graphs diff --git a/src/main/frontend/components/shortcut.cljs b/src/main/frontend/components/shortcut.cljs index bfc0b57510..03106467a8 100644 --- a/src/main/frontend/components/shortcut.cljs +++ b/src/main/frontend/components/shortcut.cljs @@ -312,6 +312,7 @@ [:button.shortcut-filter-pill {:key (name k) :class (when active? "shortcut-filter-pill--active") + :aria-pressed (str active?) :on-click #(set-filter-key! (when-not (or (= k :All) (= filter-key k)) k))} [:span.shortcut-filter-pill-title title] [:span.shortcut-filter-pill-count (str " \u00B7 " cnt)]])] @@ -363,7 +364,8 @@ {:id popup-id :force-popover? true :align "start" - :on-after-hide #(reset! *active-shortcut-id nil) + :on-after-hide #(do (reset! *active-shortcut-id nil) + (when anchor-el (.focus anchor-el))) :content-props {:class "p-0 w-auto" :collision-padding 12 @@ -376,36 +378,6 @@ (.preventDefault e) false)))}})))) -(rum/defc shortcut-conflicts-display - [_k conflicts-map] - - [:div.cp__shortcut-conflicts-list-wrap - (for [[g ks] conflicts-map] - ^{:key (str g)} - [:section.relative - [:h2 (ui/icon "alert-triangle" {:size 15}) - [:span (t :keymap/conflicts-for-label)] - [:code (shortcut-utils/decorate-binding g)]] - [:ul - (for [v (vals ks) - :let [k (first v) - vs (second v)] - [id' handler-id] vs - :let [m (dh/shortcut-item id')] - :when (not (nil? m))] - [:li - {:key (str id')} - [:button.select-none.hover:underline - {:on-click (fn [^js e] (open-customize-shortcut-dialog! e 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 - (dh/get-shortcut-desc m) - (ui/icon "external-link" {:size 18})] - [:code [:small (str id')]]]])]])]) - (defn- execute-undo! "Restore previous bindings for all affected actions." [snapshot] @@ -649,14 +621,8 @@ (= state :conflict-cross) nil - ;; Conflict-same / esc-hint + key => start new recording - (#{:conflict-same :esc-hint} state) - (when-let [kn (shortcut/keyname e)] - (set-rec-state! :recording) - (set-keystroke! (util/trim-safe kn))) - - ;; Idle / accepted / removed / reset / dismissing + key => start recording - (#{:idle :accepted :removed :reset :dismissing} state) + ;; Any non-recording, non-conflict-cross state + key => start new recording + (#{:conflict-same :esc-hint :idle :accepted :removed :reset :dismissing} state) (when-let [kn (shortcut/keyname e)] (set-rec-state! :recording) (set-keystroke! (util/trim-safe kn))) @@ -675,7 +641,9 @@ ;; === V3 LAYOUT === [:div.shortcut-popover {:tab-index -1 - :ref *ref-el} + :ref *ref-el + :role "dialog" + :aria-label action-name} ;; TITLE [:div.shortcut-popover-title action-name] @@ -717,79 +685,81 @@ (when (#{:idle :recording :accepted :removed :reset} render-state) [:span.shortcut-input-placeholder (t :keymap/press-a-shortcut)])] - ;; FEEDBACK BANNER (conditional) - (let [undo-link - (when undo-snapshot - [:button.shortcut-feedback-action - {:on-click (fn [] - (execute-undo! undo-snapshot) - (when-let [own (some #(when (= (:action-id %) k) %) (:entries undo-snapshot))] - (set-current-binding! (:previous-binding own))) - (set-undo-snapshot! nil) - (set-rec-state! :idle))} - (t :keymap/undo)])] - (case rec-state - :conflict-cross - [:div.shortcut-feedback.shortcut-feedback--error - [:span (t :keymap/used-by) - [:span.shortcut-feedback-name (conflict-action-names key-conflicts)]] - (ui/tooltip - (shui/button {:variant :destructive - :size :xs - :on-click override-fn!} - (t :keymap/reassign)) - (t :keymap/reassign-tooltip))] + ;; FEEDBACK BANNER (conditional) — wrapped in live region for screen readers + [:div {:role (if (#{:conflict-cross :conflict-same} rec-state) "alert" "status") + :aria-live (if (#{:conflict-cross :conflict-same} rec-state) "assertive" "polite")} + (let [undo-link + (when undo-snapshot + [:button.shortcut-feedback-action + {:on-click (fn [] + (execute-undo! undo-snapshot) + (when-let [own (some #(when (= (:action-id %) k) %) (:entries undo-snapshot))] + (set-current-binding! (:previous-binding own))) + (set-undo-snapshot! nil) + (set-rec-state! :idle))} + (t :keymap/undo)])] + (case rec-state + :conflict-cross + [:div.shortcut-feedback.shortcut-feedback--error + [:span (t :keymap/used-by) + [:span.shortcut-feedback-name (conflict-action-names key-conflicts)]] + (ui/tooltip + (shui/button {:variant :destructive + :size :xs + :on-click override-fn!} + (t :keymap/reassign)) + (t :keymap/reassign-tooltip))] - :conflict-same - [:div.shortcut-feedback.shortcut-feedback--error - [:span (t :keymap/already-bound)]] + :conflict-same + [:div.shortcut-feedback.shortcut-feedback--error + [:span (t :keymap/already-bound)]] - :accepted - (cond - (:cross-context? accepted-info) - [:div.shortcut-feedback.shortcut-feedback--warning - [:span (t :keymap/also-used-for) - [:span.shortcut-feedback-name - (:cross-action-name accepted-info)] - (when-let [ctx (:cross-context-label accepted-info)] - (str (t :keymap/in-context) ctx))]] + :accepted + (cond + (:cross-context? accepted-info) + [:div.shortcut-feedback.shortcut-feedback--warning + [:span (t :keymap/also-used-for) + [:span.shortcut-feedback-name + (:cross-action-name accepted-info)] + (when-let [ctx (:cross-context-label accepted-info)] + (str (t :keymap/in-context) ctx))]] - (:from accepted-info) - [:div.shortcut-feedback.shortcut-feedback--success - [:span (t :keymap/reassigned-from) - [:span.shortcut-feedback-name (:from accepted-info)]] - undo-link] + (:from accepted-info) + [:div.shortcut-feedback.shortcut-feedback--success + [:span (t :keymap/reassigned-from) + [:span.shortcut-feedback-name (:from accepted-info)]] + undo-link] - :else - [:div.shortcut-feedback.shortcut-feedback--success - [:span (t :keymap/shortcut-added)]]) + :else + [:div.shortcut-feedback.shortcut-feedback--success + [:span (t :keymap/shortcut-added)]]) - :removed - [:div.shortcut-feedback.shortcut-feedback--muted - [:span (t :keymap/shortcut-removed)] - undo-link] + :removed + [:div.shortcut-feedback.shortcut-feedback--muted + [:span (t :keymap/shortcut-removed)] + undo-link] - :reset - [:div.shortcut-feedback.shortcut-feedback--muted - [:span (t :keymap/reset-to-default)] - undo-link] + :reset + [:div.shortcut-feedback.shortcut-feedback--muted + [:span (t :keymap/reset-to-default)] + undo-link] - :esc-hint - [:div.shortcut-feedback.shortcut-feedback--muted - [:span (t :keymap/esc-is-reserved)]] + :esc-hint + [:div.shortcut-feedback.shortcut-feedback--muted + [:span (t :keymap/esc-is-reserved)]] - :dismissing - (let [prev (rum/deref *prev-rec-state) - variant (case prev - (:conflict-cross :conflict-same) "shortcut-feedback--error" - :accepted (if (:cross-context? accepted-info) - "shortcut-feedback--warning" - "shortcut-feedback--success") - "shortcut-feedback--muted")] - [:div {:class (str "shortcut-feedback " variant " is-dismissing") - :on-animation-end #(set-rec-state! :idle)}]) + :dismissing + (let [prev (rum/deref *prev-rec-state) + variant (case prev + (:conflict-cross :conflict-same) "shortcut-feedback--error" + :accepted (if (:cross-context? accepted-info) + "shortcut-feedback--warning" + "shortcut-feedback--success") + "shortcut-feedback--muted")] + [:div {:class (str "shortcut-feedback " variant " is-dismissing") + :on-animation-end #(set-rec-state! :idle)}]) - nil)) + nil))] ;; SEPARATOR + TOOLBAR (shui/separator) @@ -1049,7 +1019,7 @@ [:li.shortcut-row.flex.items-start.justify-between.text-sm {:key (str id) :class (when (= active-id id) "active") - :tab-index (when id 0) + :tab-index (when (and id (not disabled?)) 0) :role (when id "button") :aria-disabled (when disabled? "true") :on-click row-action diff --git a/src/main/frontend/components/shortcut.css b/src/main/frontend/components/shortcut.css index 04c6833787..33c33821fb 100644 --- a/src/main/frontend/components/shortcut.css +++ b/src/main/frontend/components/shortcut.css @@ -493,6 +493,11 @@ button.shortcut-feedback-action { } @media (prefers-reduced-motion: reduce) { + @keyframes shortcut-badge-in { + from { opacity: 0; } + to { opacity: 1; } + } + @keyframes shortcut-fade-in { from { opacity: 0; } to { opacity: 1; } diff --git a/src/main/frontend/modules/shortcut/utils.cljs b/src/main/frontend/modules/shortcut/utils.cljs index ad8b17e180..ea36cc86f0 100644 --- a/src/main/frontend/modules/shortcut/utils.cljs +++ b/src/main/frontend/modules/shortcut/utils.cljs @@ -1,6 +1,7 @@ (ns frontend.modules.shortcut.utils (:require [clojure.string :as string] - [frontend.util :as util]) + [frontend.util :as util] + [lambdaisland.glogi :as log]) (:import [goog.ui KeyboardShortcutHandler])) (def ^:private modifier-order @@ -39,7 +40,7 @@ (try (KeyboardShortcutHandler/parseStringShortcut binding) (catch js/Error e - (js/console.warn "[shortcuts] parse key error: " e) binding))) + (log/warn :shortcut/parse-key-error {:error e :binding binding}) binding))) (defn mod-key [binding] (string/replace binding #"(?i)mod"