Browse Source

remove keyboard acceleration feature and related tests

Mega Yu 3 days ago
parent
commit
ad0363c86e

+ 0 - 111
clj-e2e/test/logseq/e2e/cmdk_scroll_basic_test.clj

@@ -139,114 +139,3 @@
 
     ;; Verify the highlighted element is within the visible scroll area.
     (is (kbd-highlight-in-viewport?) "highlight is in viewport after navigating through lazy-visible items")))
-
-;; ---------------------------------------------------------------------------
-;; Lazy-visible hold-ArrowDown scroll verification
-;;
-;; Goal:
-;;   Verify that holding ArrowDown (triggering keyboard acceleration) through
-;;   lazy-visible :nodes items keeps the highlighted row in the visible
-;;   scroll area after keyup.  This exercises the reconcile path inside
-;;   start-scroll-animation!, which recomputes the scroll target on every
-;;   rAF frame while lazy placeholders are being replaced by mounted rows.
-;;
-;; Why long titles:
-;;   Pages carry a 230-character suffix.  The rendered row is taller than
-;;   the lazy placeholder (a fixed 32 px div), maximising height variance
-;;   and thus the scroll-target drift that reconcile must correct.
-;;
-;; Key-hold simulation (see inline comment for the protocol):
-;;   1 initial keydown  +  210 ms OS-initial-repeat delay
-;;   +  20 repeat keydowns at 30 ms intervals  +  1 keyup
-;;   ≈ 820 ms total JS wall time.
-;;   eval-js blocks the Clojure thread until the returned Promise resolves,
-;;   so the full hold completes before wait-timeout begins.
-;;
-;; Layout:
-;;   viewport_height = 65dvh × 720 px (Playwright default) = 468 px
-;;   row_height >> 32 px with long titles (text wraps).
-;;   60 nodes items × tall row ≫ viewport — deep lazy territory.
-;;
-;; Expected navigation depth (accel-delay=200 ms, interval=150 ms, max-step=5):
-;;
-;;   Note: keydown-accel-step tracks hold duration via ::accel-start-ts rather
-;;   than event.repeat, because goog.events.BrowserEvent does not forward the
-;;   native repeat field.  ::accel-start-ts is nil after keyup and is set to
-;;   Date.now() on the first keydown that finds it nil.
-;;
-;;   T=0      first keydown → set accel-start-ts=0  step 1  →  +1
-;;   T=210    repeat  1  held=210     step 1  →  +1
-;;   T=240–330  repeats  2–5          step 1  →  +5   (total  +7)
-;;   T=360–480  repeats  6–10         step 2  →  +10  (total +17)
-;;   T=510–630  repeats 11–15         step 3  →  +15  (total +32)
-;;   T=660–780  repeats 16–20         step 4  →  +20  (total +52 → item-index ≈ 53)
-;;   threshold 40 is a conservative lower bound well below the expected ~52.
-;; ---------------------------------------------------------------------------
-;; current disable keyboard acceleration, no need to test it for now
-(deftest ^:fix-me cmdk-lazy-visible-hold-arrow-down-scroll
-  (testing "holding ArrowDown in expanded lazy-visible results keeps highlight in viewport after keyup"
-    ;; Use long titles to increase row-height variance between lazy placeholder and mounted rows.
-    (setup-search "cmdkholdlong" 60 (apply str (repeat 230 "x")))
-
-    ;; Expand :nodes group so we navigate a long lazy-visible list.
-    (k/arrow-down)
-    (util/wait-timeout 100)
-    (k/press "ControlOrMeta+ArrowDown")
-    (util/wait-timeout 300)
-
-    ;; Simulate a real OS key hold:
-    ;; • keydown (repeat=false) starts the acceleration timer at T=0.
-    ;; • 210 ms pause — crosses accel-delay-ms (200 ms), matching the OS
-    ;;   initial-repeat delay so that acceleration is already active when the
-    ;;   first repeat fires (same as real hardware behavior).
-    ;; • 20 repeated keydown events (repeat=true) at 30 ms intervals — OS
-    ;;   key-repeat rate; each setTimeout yields the browser event loop so
-    ;;   rAF frames and React re-renders run between keydowns.
-    ;; • keyup resets the accel timer.
-    ;;
-    ;; Async (setTimeout) is required: without yielding between events,
-    ;; all keydowns fire in the same tick, Date.now() does not advance,
-    ;; held-ms ≈ 0, and accel-step returns 1 throughout — no acceleration.
-    ;;
-    ;; (See block comment above for the step-by-step derivation of ≈52 total moves.)
-    (w/eval-js
-     "((async () => {
-         const input = document.querySelector('.cp__cmdk-search-input');
-         if (!input) return;
-         input.focus();
-         const fire = (type, repeat = false) =>
-           input.dispatchEvent(new KeyboardEvent(type, {
-             key: 'ArrowDown',
-             bubbles: true,
-             cancelable: true,
-             repeat
-           }));
-         fire('keydown', false);
-         await new Promise(r => setTimeout(r, 210));
-         for (let i = 0; i < 20; i++) {
-           fire('keydown', true);
-           await new Promise(r => setTimeout(r, 30));
-         }
-         fire('keyup', false);
-       })())")
-
-    ;; eval-js awaits the async IIFE (~810 ms); this timeout covers rAF settle.
-    (util/wait-timeout 400)
-
-    (is (= 1 (util/count-elements kbd-highlight))
-        "keyboard highlight exists after hold + keyup")
-
-    ;; Verify the highlight has actually advanced deep into the list.
-    ;; item-index must exceed 20 (the initial 10-item :less window cap),
-    ;; confirming navigation reached lazy territory.
-    (let [item-index (w/eval-js
-                      (str "(() => {
-                              const el = document.querySelector('" kbd-highlight "');
-                              if (!el) return -1;
-                              const w = el.closest('[data-item-index]');
-                              return w ? parseInt(w.getAttribute('data-item-index'), 10) : -1;
-                            })()"))]
-      (is (>= item-index 40)
-          (str "highlight should have advanced deep into list (got item-index " item-index ")")))
-
-    (is (kbd-highlight-in-viewport?) "highlight should remain in viewport after hold + keyup")))

+ 4 - 35
src/main/frontend/components/cmdk/core.cljs

@@ -659,26 +659,6 @@
   "Pixel clearance reserved at the top and bottom of the scroll container."
   32)
 
-;; --- Keyboard acceleration ---
-;; When holding ArrowDown/Up, step starts at 1 and ramps up
-;; so the highlight moves through items progressively faster.
-
-(def ^:private need-accel?
-  "Whether to enable keyboard acceleration."
-  false)
-
-(def ^:private accel-delay-ms
-  "Milliseconds to hold before acceleration kicks in."
-  200)
-
-(def ^:private accel-step-interval-ms
-  "Milliseconds between each step increase after delay."
-  150)
-
-(def ^:private accel-max-step
-  "Maximum items to move per keypress during acceleration."
-  5)
-
 ;; --- Synchronous keyboard highlight DOM manipulation ---
 ;; React/Rum re-renders asynchronously (via rAF). When a keydown fires,
 ;; scrollTop is set synchronously but the highlight attribute is only updated in
@@ -959,17 +939,6 @@
       :else
       (notification/show! "No link for this search item." :warning))))
 
-(defn- keydown-accel-step
-  "Returns the number of items to move per keydown, ramping up while the key is
-  held.  Uses `::accel-start-ts` to track hold duration instead of event.repeat,
-  which is not reliably forwarded by the Goog events BrowserEvent wrapper."
-  [state _e]
-  (let [now (js/Date.now)
-        start (or @(::accel-start-ts state)
-                  (reset! (::accel-start-ts state) now))
-        held-ms (- now start)]
-    (scroll/accel-step held-ms accel-delay-ms accel-step-interval-ms accel-max-step)))
-
 (defn- keydown-handler
   [state e]
   (let [meta? (util/meta-key? e)
@@ -998,14 +967,14 @@
         (state/sidebar-add-block! repo input :search))
       as-keydown? (if meta?
                     (show-more)
-                    (let [step (if need-accel? (keydown-accel-step state e) 1)]
+                    (do
                       (reset! (::focus-source state) :keyboard)
-                      (move-highlight state step)))
+                      (move-highlight state 1)))
       as-keyup? (if meta?
                   (show-less)
-                  (let [step (if need-accel? (keydown-accel-step state e) 1)]
+                  (do
                     (reset! (::focus-source state) :keyboard)
-                    (move-highlight state (- step))))
+                    (move-highlight state -1)))
       (and enter? (not composing?)) (do
                                       (handle-action :default state e)
                                       (util/stop-propagation e))

+ 0 - 20
src/main/frontend/components/cmdk/scroll.cljs

@@ -85,23 +85,3 @@
        (some? pending-item-index)
        (= pending-item-index mounted-item-index)
        (= highlighted-item-index mounted-item-index)))
-
-(defn accel-step
-  "Compute keyboard acceleration step based on held duration.
-
-  Returns 1 during the initial `delay-ms` and then increases by 1
-  every `interval-ms`, capped at `max-step`.
-
-  Input:
-  - `held-ms`:     how long the key has been held (non-negative).
-  - `delay-ms`:    grace period before acceleration starts.
-  - `interval-ms`: milliseconds between each step increase.
-  - `max-step`:    upper bound on returned step.
-
-  Output:
-  - A positive integer in [1, max-step]."
-  [held-ms delay-ms interval-ms max-step]
-  (if (< held-ms delay-ms)
-    1
-    (min max-step
-         (inc (quot (- held-ms delay-ms) (max 1 interval-ms))))))

+ 0 - 28
src/test/frontend/components/cmdk/scroll_test.cljs

@@ -94,31 +94,3 @@
     (is (false? (scroll/should-scroll-on-item-mounted? :keyboard 8 7 8)))
     (is (false? (scroll/should-scroll-on-item-mounted? :keyboard 8 8 9)))
     (is (false? (scroll/should-scroll-on-item-mounted? :keyboard 8 8 nil)))))
-
-(deftest accel-step-basic
-  (testing "returns 1 during grace period"
-    (is (= 1 (scroll/accel-step 0 200 150 5)))
-    (is (= 1 (scroll/accel-step 100 200 150 5)))
-    (is (= 1 (scroll/accel-step 199 200 150 5))))
-  (testing "returns 2 at exactly delay-ms"
-    ;; held=200, delay=200 → excess=0, quot(0,150)=0, inc→1 ... wait
-    ;; Actually at exactly delay-ms: excess=0, quot(0,150)=0, 1+0=1
-    ;; So step is still 1 at exactly delay boundary
-    ;; Step becomes 2 at delay + 1 interval
-    (is (= 1 (scroll/accel-step 200 200 150 5)))
-    (is (= 2 (scroll/accel-step 350 200 150 5))))
-  (testing "ramps up with held duration"
-    ;; delay=200, interval=150
-    ;; 500ms: excess=300, quot(300,150)=2, inc→3
-    (is (= 3 (scroll/accel-step 500 200 150 5)))
-    ;; 800ms: excess=600, quot(600,150)=4, inc→5
-    (is (= 5 (scroll/accel-step 800 200 150 5))))
-  (testing "caps at max-step"
-    (is (= 5 (scroll/accel-step 2000 200 150 5)))
-    (is (= 3 (scroll/accel-step 2000 200 150 3))))
-  (testing "returns 1 for zero held time"
-    (is (= 1 (scroll/accel-step 0 0 100 5))))
-  (testing "always returns positive integer"
-    (doseq [ms [0 50 100 200 300 500 1000 5000]]
-      (let [s (scroll/accel-step ms 200 150 5)]
-        (is (pos-int? s) (str "ms=" ms " step=" s))))))