From 713c8757b883b1fcfe9d50f6ab7c3b87f15299ff Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 04:18:50 +0000 Subject: [PATCH] fix(web): enter activates highlighted search row (#406) caveman: enter on search input go to recents pile, not row. fix: enter check active_index. row there? activate row, same as click. row gone (no results, stale index)? fall back to recents push so empty-query still work. new prop SearchInput.on_activate, surface wires it to same on_select_result the row click uses. keyboard + mouse converge on one path. three browser tests pin contract: - enter on row 1 of 2 fires on_activate(m-1), no submit - enter with empty results falls back to on_submit(query) - enter with active_index past tail falls back, no panic Refs #406 --- crates/web/src/components/search/input.rs | 37 +++- crates/web/src/components/search/surface.rs | 5 +- crates/web/tests/browser.rs | 193 ++++++++++++++++++++ 3 files changed, 230 insertions(+), 5 deletions(-) diff --git a/crates/web/src/components/search/input.rs b/crates/web/src/components/search/input.rs index fa9e9278..9ec35614 100644 --- a/crates/web/src/components/search/input.rs +++ b/crates/web/src/components/search/input.rs @@ -14,9 +14,14 @@ //! target. The active row is also announced via //! `aria-activedescendant`, which points at the row's //! `id="search-row-{message_id}"` per WAI-ARIA listbox guidance. +//! - `Enter` activates the highlighted row (same path mouse click takes +//! on the row) when `active_index` points at a real result; otherwise +//! it falls back to the recents-push `on_submit` so the empty-query +//! affordance still works. Wired this way per issue #406 — keyboard +//! and pointer converge on a single activation path. use leptos::prelude::*; -use willow_client::SearchScope; +use willow_client::{SearchResult, SearchScope}; use crate::state::{AppState, AppWriteSignals}; @@ -33,10 +38,17 @@ fn placeholder_for(scope: &SearchScope) -> &'static str { /// Sticky search input at the top of the surface. #[component] pub fn SearchInput( - /// Fired with the current query text when the user presses Enter - /// (used for recents push). + /// Fired with the current query text when Enter is pressed and there + /// is no highlighted row to activate (no results, or `active_index` + /// out of range). Used for the empty-query "push to recents" path. #[prop(into)] on_submit: Callback, + /// Fired with the highlighted result row when Enter is pressed and + /// `active_index` points at a real row. Same path a mouse click on + /// the row takes — keyboard and pointer must converge here per + /// issue #406. + #[prop(into)] + on_activate: Callback, ) -> impl IntoView { let state = use_context::().expect("AppState"); let write = use_context::().expect("AppWriteSignals"); @@ -74,8 +86,25 @@ pub fn SearchInput( } } "Enter" => { + // Always swallow Enter — the surrounding `
` + // would otherwise navigate / submit and tear down the surface. ev.prevent_default(); - on_submit.run(state.search.query.get_untracked()); + // If a result row is highlighted (active_index in range of + // the flat, in-display-order results), activate it via the + // same callback a click on the row fires. Otherwise (no + // results, or stale out-of-bounds index) fall back to the + // recents-push submit path so the empty-query affordance + // still works. Keeping both paths behind one Enter handler + // is what closes #406. + let flat = super::results::flat_ordered( + &state.search.results.get_untracked(), + &state.search.scope.get_untracked(), + ); + let i = state.search.active_index.get_untracked(); + match flat.get(i) { + Some(row) => on_activate.run(row.clone()), + None => on_submit.run(state.search.query.get_untracked()), + } } "ArrowDown" => { let n = result_count(); diff --git a/crates/web/src/components/search/surface.rs b/crates/web/src/components/search/surface.rs index 42a0f260..540d05d5 100644 --- a/crates/web/src/components/search/surface.rs +++ b/crates/web/src/components/search/surface.rs @@ -147,7 +147,10 @@ pub fn SearchSurface( view! {
- + // Both the Enter-activate (keyboard) and row-click (mouse) + // paths funnel through `on_select_result` so navigation is + // identical regardless of input modality (issue #406). + {move || { let q = state.search.query.get(); diff --git a/crates/web/tests/browser.rs b/crates/web/tests/browser.rs index efc73b87..5c5d3653 100644 --- a/crates/web/tests/browser.rs +++ b/crates/web/tests/browser.rs @@ -9782,6 +9782,199 @@ mod phase_2e_search_active_row { } } +// ── Phase 2e — Enter activates highlighted row (issue #406) ───────────────── +// +// Follow-up to #344 / PR #403. Once `aria-activedescendant` advertised an +// active row, Enter still routed to the recents-push path instead of +// activating the highlighted row. These tests pin the keyboard contract: +// +// 1. Enter with an active row in range → fires `on_activate(row)` and +// does NOT fire the recents-push `on_submit` (keyboard converges on +// the same activation path as a mouse click). +// 2. Enter with no results / no active row → falls back to `on_submit` +// so the empty-query "push to recents" affordance is preserved. +// +// Pinned to `ThisChannel` scope to keep grouping trivial — the cross-group +// flat-index walk has its own coverage in `phase_2e_search_active_row`. +mod phase_2e_search_enter_activate { + use super::*; + use std::sync::{Arc, Mutex}; + use willow_client::{SearchResult, SearchScope}; + use willow_web::components::SearchInput; + use willow_web::state::{create_signals, InitialSignals}; + + fn fixture_result(id: &str, body: &str, ts: u64) -> SearchResult { + SearchResult { + message_id: id.into(), + channel_id: "general".into(), + channel_name: "general".into(), + grove_id: Some("grove-fixture".into()), + letter_id: None, + author_display_name: "Mira".into(), + author_handle: "mira".into(), + timestamp_ms: ts, + body: body.into(), + matched_ranges: Vec::new(), + } + } + + /// Captured callback invocations from a single test mount. Keeps the + /// activate path and the submit path distinct so the test can assert + /// exactly which branch the Enter handler chose. `Arc>` so + /// the shared cell satisfies `Callback::new`'s `Send + Sync` bound. + #[derive(Default)] + struct Captured { + activated: Vec, + submitted: Vec, + } + + /// Mount `` with seeded results / active_index and + /// callbacks that record which path Enter took. Returns the container + /// and the shared capture cell. + fn mount_input_with( + results: Vec, + active: usize, + query: &str, + ) -> (web_sys::HtmlElement, Arc>) { + let captured: Arc> = Arc::new(Mutex::new(Captured::default())); + let captured_for_mount = captured.clone(); + let query = query.to_string(); + + let container = mount_test(move || { + let InitialSignals { + app_state, + write, + trust_store: _, + } = create_signals(); + + // Pin scope so grouping is trivial; seed results + query so + // the input handler observes the same world the user would. + write + .search + .set_scope + .set(SearchScope::ThisChannel("general".into())); + write.search.set_query.set(query.clone()); + write.search.set_results.set(results.clone()); + write.search.set_active_index.set(active); + + provide_context(app_state); + provide_context(write); + + let cap_activate = captured_for_mount.clone(); + let cap_submit = captured_for_mount.clone(); + + view! { + + } + }); + + (container, captured) + } + + #[wasm_bindgen_test] + async fn enter_with_active_row_activates_that_row_not_recents_push() { + // Two rows; active_index = 1 (second row). Enter must activate + // the second row via on_activate, NOT push the query to recents. + let results = vec![ + fixture_result("m-0", "first hit", 30_000), + fixture_result("m-1", "second hit", 20_000), + ]; + let (container, captured) = mount_input_with(results, 1, "hit"); + tick().await; + + let pressed = test_support::press_key(&container, ".search-input", "Enter"); + assert!(pressed, "search-input must exist to receive Enter"); + tick().await; + + let cap = captured.lock().unwrap(); + assert_eq!( + cap.activated.len(), + 1, + "Enter with a valid active row must fire on_activate exactly once \ + (regression guard for #406); got {} activations", + cap.activated.len() + ); + assert_eq!( + cap.activated[0].message_id, "m-1", + "on_activate must receive the row at active_index (m-1, the \ + second row) — not the first row, not a stale row" + ); + assert!( + cap.submitted.is_empty(), + "Enter on a highlighted row must NOT also fire the recents-push \ + on_submit path; got {:?}", + cap.submitted + ); + } + + #[wasm_bindgen_test] + async fn enter_with_no_results_falls_back_to_recents_push() { + // No results. Enter has nothing to activate, so it must fall back + // to the legacy "push to recents" submit path so the empty-query + // affordance keeps working. + let (container, captured) = mount_input_with(Vec::new(), 0, "no-match"); + tick().await; + + let pressed = test_support::press_key(&container, ".search-input", "Enter"); + assert!(pressed, "search-input must exist to receive Enter"); + tick().await; + + let cap = captured.lock().unwrap(); + assert!( + cap.activated.is_empty(), + "with no results there is no row to activate — on_activate must \ + not fire; got {} activations", + cap.activated.len() + ); + assert_eq!( + cap.submitted.len(), + 1, + "with no results Enter must fall back to on_submit (recents \ + push); got {} submissions", + cap.submitted.len() + ); + assert_eq!( + cap.submitted[0], "no-match", + "on_submit must receive the current query text" + ); + } + + #[wasm_bindgen_test] + async fn enter_with_active_index_past_tail_falls_back_to_submit() { + // Defensive: if a stale active_index outlived its result set + // (e.g., the surface effect hasn't yet snapped it back to 0), + // Enter must not panic and must not invent a row — fall back to + // the submit path. This protects callers from a transient + // out-of-bounds window. + let results = vec![fixture_result("m-0", "only hit", 10_000)]; + // active_index past tail. + let (container, captured) = mount_input_with(results, 5, "stale"); + tick().await; + + let pressed = test_support::press_key(&container, ".search-input", "Enter"); + assert!(pressed, "search-input must exist to receive Enter"); + tick().await; + + let cap = captured.lock().unwrap(); + assert!( + cap.activated.is_empty(), + "out-of-bounds active_index must not fabricate an activation" + ); + assert_eq!( + cap.submitted.len(), + 1, + "out-of-bounds active_index must fall back to on_submit" + ); + } +} + // ── Foundation tokens (Phase 0) ───────────────────────────────────────────── // // Closes Task 14 of `docs/plans/2026-04-19-ui-phase-0-foundation.md`.