From 927ba18d79fb892f43f2f4c8bddb7a3cc6455ce5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 13:16:40 +0000 Subject: [PATCH 1/3] settings: hit-test scrolled search results in absolute index space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only the visible search-result rows are registered in the settings layout, so `hit_test` returned a viewport-slot index (0 = first visible row) while its consumers — the hover compare in the renderer and the click handler — treat the value as an absolute index into the state's `search_results`. Once the list was scrolled, hover highlighted and clicks activated the result `search_scroll_offset` rows above the pointer (issue #2860). Store the absolute result index on each registered row and return that from `hit_test`, so hover and click stay in the same index space as the state no matter how far the list is scrolled. The web frontend already sends absolute indices for its `searchResult` hits, so its behavior is unchanged. Fixes half of #2860; the wheel-scroll clamp half is a separate commit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- .../fresh-editor/src/view/settings/layout.rs | 52 ++++++++++++++++--- .../fresh-editor/src/view/settings/render.rs | 9 +++- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/crates/fresh-editor/src/view/settings/layout.rs b/crates/fresh-editor/src/view/settings/layout.rs index efc31d4348..f6da7b57b5 100644 --- a/crates/fresh-editor/src/view/settings/layout.rs +++ b/crates/fresh-editor/src/view/settings/layout.rs @@ -50,6 +50,11 @@ pub struct SettingsLayout { /// Layout info for a search result #[derive(Debug, Clone)] pub struct SearchResultLayout { + /// Absolute index into the state's `search_results` list. Only the + /// visible rows are registered in the layout, so this is NOT the same + /// as the position within `SettingsLayout::search_results` once the + /// list is scrolled (#2860). + pub result_index: usize, /// Page index (category) pub page_index: usize, /// Item index within the page @@ -131,9 +136,18 @@ impl SettingsLayout { }); } - /// Add a search result to the layout - pub fn add_search_result(&mut self, page_index: usize, item_index: usize, area: Rect) { + /// Add a search result to the layout. `result_index` is the absolute + /// index into the state's `search_results` list (not the on-screen + /// slot), so hit-testing keeps working when the list is scrolled. + pub fn add_search_result( + &mut self, + result_index: usize, + page_index: usize, + item_index: usize, + area: Rect, + ) { self.search_results.push(SearchResultLayout { + result_index, page_index, item_index, area, @@ -215,10 +229,14 @@ impl SettingsLayout { } } - // Check search results (before regular items, since they replace the item list during search) - for (idx, result) in self.search_results.iter().enumerate() { + // Check search results (before regular items, since they replace the + // item list during search). The hit carries the ABSOLUTE result + // index: only visible rows are registered here, so the position in + // this vec is a viewport slot and would be off by the scroll offset + // once the list is scrolled (#2860). + for result in &self.search_results { if point_in_rect(result.area, x, y) { - return Some(SettingsHit::SearchResult(idx)); + return Some(SettingsHit::SearchResult(result.result_index)); } } @@ -392,7 +410,8 @@ pub enum SettingsHit { CategoriesScrollbar, /// Click on a setting item (index) Item(usize), - /// Click on a search result (index in search_results) + /// Click on a search result (absolute index into the state's + /// `search_results`, not the on-screen slot) SearchResult(usize), /// Click on toggle control ControlToggle(usize), @@ -520,6 +539,27 @@ mod tests { assert_eq!(layout.hit_test(40, 11), Some(SettingsHit::Item(0))); } + /// Reproducer for issue #2860: only VISIBLE search results are registered + /// in the layout, so when the list is scrolled the first registered row + /// is not result 0. `hit_test` must report the absolute result index the + /// row was registered with, not the row's position in the layout vec — + /// otherwise hover and click resolve to a result `scroll_offset` rows + /// above the pointer. + #[test] + fn test_hit_test_search_result_scrolled_uses_absolute_index() { + let modal = Rect::new(0, 0, 100, 40); + let mut layout = SettingsLayout::new(modal); + + // Scrolled viewport: visible rows are results 3, 4, 5 (3 rows each). + layout.add_search_result(3, 0, 3, Rect::new(25, 3, 70, 3)); + layout.add_search_result(4, 0, 4, Rect::new(25, 6, 70, 3)); + layout.add_search_result(5, 0, 5, Rect::new(25, 9, 70, 3)); + + assert_eq!(layout.hit_test(30, 4), Some(SettingsHit::SearchResult(3))); + assert_eq!(layout.hit_test(30, 7), Some(SettingsHit::SearchResult(4))); + assert_eq!(layout.hit_test(30, 10), Some(SettingsHit::SearchResult(5))); + } + /// Reproducer for issue #1825: clicking on the value area between the /// brackets of a Number control used to return `Item`, which only /// changes selection. It must now return a dedicated hit that the mouse diff --git a/crates/fresh-editor/src/view/settings/render.rs b/crates/fresh-editor/src/view/settings/render.rs index 06a87884ed..592fc29fa6 100644 --- a/crates/fresh-editor/src/view/settings/render.rs +++ b/crates/fresh-editor/src/view/settings/render.rs @@ -2579,6 +2579,7 @@ fn render_search_results( frame, item_area, result, + idx, is_selected, is_hovered, theme, @@ -2615,11 +2616,15 @@ fn render_search_results( } } -/// Render a single search result with breadcrumb +/// Render a single search result with breadcrumb. `result_index` is the +/// absolute index into the state's `search_results` (needed for hit-testing +/// because only the visible rows get registered in the layout). +#[allow(clippy::too_many_arguments)] fn render_search_result_item( frame: &mut Frame, area: Rect, result: &SearchResult, + result_index: usize, is_selected: bool, is_hovered: bool, theme: &Theme, @@ -2717,7 +2722,7 @@ fn render_search_result_item( } // Track this item in layout - layout.add_search_result(result.page_index, result.item_index, area); + layout.add_search_result(result_index, result.page_index, result.item_index, area); } /// Build a line with highlighted match positions From 5ecaf340961cca36ccb4a1d181a7e8b5aea8c099 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 13:16:47 +0000 Subject: [PATCH 2/3] settings: let the wheel reach the first/last search result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wheel-scrolling the settings search results clamps the scroll offset at `len - max_visible` and pins the selection to the viewport edge, so at the end of the list further wheel-downs were no-ops and the selection could never land on the last result (issue #2860) — while keyboard navigation reaches it fine. Once the viewport is already at the bottom, keep advancing the selection on each wheel-down until it reaches the last result, and symmetrically walk the selection up to the first result when wheeling up at the top, matching the keyboard behavior. Fixes the second half of #2860. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- .../fresh-editor/src/view/settings/state.rs | 106 +++++++++++++++++- 1 file changed, 104 insertions(+), 2 deletions(-) diff --git a/crates/fresh-editor/src/view/settings/state.rs b/crates/fresh-editor/src/view/settings/state.rs index 9109a65204..798568d7e8 100644 --- a/crates/fresh-editor/src/view/settings/state.rs +++ b/crates/fresh-editor/src/view/settings/state.rs @@ -1562,9 +1562,19 @@ impl SettingsState { /// Scroll search results up by delta items pub fn search_scroll_up(&mut self, delta: usize) -> bool { - if self.search_results.is_empty() || self.search_scroll_offset == 0 { + if self.search_results.is_empty() { return false; } + if self.search_scroll_offset == 0 { + // Viewport already at the top: keep walking the selection up so + // the wheel can reach (and select) the first result, matching + // keyboard navigation (#2860). + if self.selected_search_result == 0 { + return false; + } + self.selected_search_result = self.selected_search_result.saturating_sub(delta); + return true; + } self.search_scroll_offset = self.search_scroll_offset.saturating_sub(delta); // Keep selection visible if self.selected_search_result >= self.search_scroll_offset + self.search_max_visible { @@ -1583,7 +1593,15 @@ impl SettingsState { .len() .saturating_sub(self.search_max_visible); if self.search_scroll_offset >= max_offset { - return false; + // Viewport already at the bottom: keep walking the selection down + // so the wheel can reach (and select) the last result, matching + // keyboard navigation (#2860). + let last = self.search_results.len() - 1; + if self.selected_search_result >= last { + return false; + } + self.selected_search_result = (self.selected_search_result + delta).min(last); + return true; } self.search_scroll_offset = (self.search_scroll_offset + delta).min(max_offset); // Keep selection visible @@ -4236,4 +4254,88 @@ mod tests { state.search_delete(); assert_eq!(state.search_query(), "ที่"); } + + /// Schema with enough same-prefix settings that a search for "opt" has + /// more results than a small viewport can show — used to exercise the + /// mouse-wheel scroll clamping of the search-result list. + const TEST_SCHEMA_MANY_OPTS: &str = r#" +{ + "type": "object", + "properties": { + "opt_a": { "type": "boolean", "default": true }, + "opt_b": { "type": "boolean", "default": true }, + "opt_c": { "type": "boolean", "default": true }, + "opt_d": { "type": "boolean", "default": true }, + "opt_e": { "type": "boolean", "default": true }, + "opt_f": { "type": "boolean", "default": true } + }, + "$defs": {} +} +"#; + + fn search_scroll_state() -> SettingsState { + let config = test_config(); + let mut state = SettingsState::new(TEST_SCHEMA_MANY_OPTS, &config).unwrap(); + state.show(); + state.search_active = true; + for c in "opt".chars() { + state.search_push_char(c); + } + assert!( + state.search_results.len() >= 6, + "expected all opt_* settings to match, got {}", + state.search_results.len() + ); + // Small viewport: 2 visible rows, so the list scrolls. + state.search_max_visible = 2; + state + } + + /// Reproducer for issue #2860 (wheel can't reach the last result): + /// wheel-down used to clamp the scroll offset at `len - max_visible` and + /// pin the selection to the viewport top, so the selection could never + /// land on the last result. Once the viewport is at the bottom, further + /// wheel-downs must keep advancing the selection — matching keyboard + /// navigation, which reaches the last item fine. + #[test] + fn test_search_wheel_down_reaches_last_result() { + let mut state = search_scroll_state(); + let last = state.search_results.len() - 1; + + // Wheel down more than enough notches to pass the end of the list. + for _ in 0..(2 * state.search_results.len()) { + state.search_scroll_down(1); + } + + assert_eq!( + state.selected_search_result, last, + "wheel-down must be able to select the last search result" + ); + // One more wheel-down at the very end is a no-op. + assert!(!state.search_scroll_down(1)); + assert_eq!(state.selected_search_result, last); + } + + /// Symmetric to `test_search_wheel_down_reaches_last_result`: once the + /// viewport is back at the top, further wheel-ups keep moving the + /// selection until it reaches the first result. + #[test] + fn test_search_wheel_up_reaches_first_result() { + let mut state = search_scroll_state(); + + // Go all the way down first, then all the way back up. + for _ in 0..(2 * state.search_results.len()) { + state.search_scroll_down(1); + } + for _ in 0..(2 * state.search_results.len()) { + state.search_scroll_up(1); + } + + assert_eq!(state.search_scroll_offset, 0); + assert_eq!( + state.selected_search_result, 0, + "wheel-up must be able to select the first search result" + ); + assert!(!state.search_scroll_up(1)); + } } From 20d5dc876aa2d8ce1289f0ae7e2b80dd3317571a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 13:16:54 +0000 Subject: [PATCH 3/3] settings e2e: cover scrolled-search mouse flows and scalar control clicks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rendered-output reproducers for the #2860 mouse desync (click a result row after a wheel notch and land on exactly that setting; wheel past the end of the list and watch the selection marker walk onto the last visible result) — both fail without the two preceding fixes. Also add regression coverage for the remaining mouse half of #1112: clicking a Toggle's checkbox flips it and clicking a Dropdown's button opens the option list. Investigating that issue showed the behavior already works (the widget-derived hit rects are registered correctly); only the Number value-cell click had e2e coverage, so pin down the other two scalar controls too. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- crates/fresh-editor/tests/e2e/settings.rs | 257 ++++++++++++++++++++++ 1 file changed, 257 insertions(+) diff --git a/crates/fresh-editor/tests/e2e/settings.rs b/crates/fresh-editor/tests/e2e/settings.rs index 02185ac9bb..d1eb65c5ce 100644 --- a/crates/fresh-editor/tests/e2e/settings.rs +++ b/crates/fresh-editor/tests/e2e/settings.rs @@ -635,6 +635,131 @@ fn test_settings_number_value_click_enters_edit_mode() { .unwrap(); } +/// Clicking the checkbox of a Toggle setting flips the value (issue #1112 +/// regression coverage: the widget-derived hit rects must keep routing a +/// checkbox click to the toggle action rather than plain row selection). +#[test] +fn test_settings_toggle_checkbox_click_flips_value() { + let mut harness = EditorTestHarness::new(120, 40).unwrap(); + harness.open_settings().unwrap(); + + // Jump to a Toggle setting that defaults to unchecked. + harness + .send_key(KeyCode::Char('/'), KeyModifiers::NONE) + .unwrap(); + for c in "ensure final newline".chars() { + harness + .send_key(KeyCode::Char(c), KeyModifiers::NONE) + .unwrap(); + } + harness.render().unwrap(); + harness + .send_key(KeyCode::Enter, KeyModifiers::NONE) + .unwrap(); + harness.render().unwrap(); + + let (label_col, row) = harness + .find_text_on_screen("Ensure Final Newline On Save") + .expect("setting visible after search jump"); + let row_text = harness.screen_row_text(row); + assert!( + row_text.contains("[ ]"), + "toggle starts unchecked:\n{row_text}" + ); + // `screen_row_text` yields char indices which can drift from screen + // columns (multi-char cells earlier in the row); anchor on the label, + // whose true column `find_text_on_screen` reports, and shift the + // char-index of the checkbox by the same offset. + let label_idx = row_text.find("Ensure Final Newline On Save").unwrap() as u16; + let drift = label_idx - label_col; + let checkbox_col = row_text.find(": [").expect("checkbox on the row") as u16 - drift + 2; + + // Click the checkbox glyph itself. + harness.mouse_click(checkbox_col + 1, row).unwrap(); + harness.render().unwrap(); + + let after = harness.screen_row_text(row); + assert!( + after.contains("[v]"), + "clicking the checkbox must flip the toggle on:\n{after}" + ); + + // Discard changes and close. + harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); + harness.render().unwrap(); + harness + .send_key(KeyCode::Right, KeyModifiers::NONE) + .unwrap(); + harness + .send_key(KeyCode::Enter, KeyModifiers::NONE) + .unwrap(); +} + +/// Clicking the `[value ▼]` button of a Dropdown setting opens the inline +/// option list (issue #1112 regression coverage, same hit-rect routing as +/// the toggle test above). +#[test] +fn test_settings_dropdown_button_click_opens_options() { + let mut harness = EditorTestHarness::new(120, 40).unwrap(); + harness.open_settings().unwrap(); + + harness + .send_key(KeyCode::Char('/'), KeyModifiers::NONE) + .unwrap(); + for c in "default line ending".chars() { + harness + .send_key(KeyCode::Char(c), KeyModifiers::NONE) + .unwrap(); + } + harness.render().unwrap(); + harness + .send_key(KeyCode::Enter, KeyModifiers::NONE) + .unwrap(); + harness.render().unwrap(); + + let (label_col, row) = harness + .find_text_on_screen("Default Line Ending") + .expect("setting visible after search jump"); + let row_text = harness.screen_row_text(row); + assert!( + row_text.contains("▼"), + "dropdown starts closed:\n{row_text}" + ); + // Same char-index-vs-column drift correction as the toggle test above. + let label_idx = row_text.find("Default Line Ending").unwrap() as u16; + let drift = label_idx - label_col; + let button_col = row_text.find(": [").expect("dropdown button on the row") as u16 - drift + 2; + + // Click the `[lf ▼]` button. + harness.mouse_click(button_col + 1, row).unwrap(); + harness.render().unwrap(); + + // The dropdown is now open: the button shows the ▲ indicator and the + // inline option list renders each option on a row of its own (opening + // grows the item, which may re-scroll the panel — so look for the + // option rows anywhere on screen rather than at fixed coordinates). + // A bare "crlf" row only exists as an option row; the description + // mentions it only inside a longer sentence. + let screen = harness.screen_to_string(); + assert!( + screen.contains('▲'), + "dropdown button must show the open indicator:\n{screen}" + ); + let has_crlf_option_row = screen + .lines() + .any(|l| l.trim_matches(|c: char| c == ' ' || c == '~' || c == '│') == "crlf"); + assert!( + has_crlf_option_row, + "clicking the dropdown button must open the option list:\n{screen}" + ); + + // Close the dropdown, discard, and close settings. + harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); + harness.render().unwrap(); + harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); + harness.render().unwrap(); +} + /// Left arrow on a focused Number control no longer decrements — it now /// behaves like every other control (navigates back to Categories), since /// numbers are edited by direct typing. @@ -950,6 +1075,138 @@ fn test_settings_search_result_click_navigates() { harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); } +/// Reproducer for issue #2860 (mouse desync in scrolled search results): +/// only the visible result rows are registered in the layout, and +/// hit-testing used to return the row's viewport slot (0 = first visible) +/// while the click handler treated it as an absolute result index. Once the +/// list was scrolled, a click resolved to the result `scroll_offset` rows +/// above the pointer. After one wheel notch, clicking the top visible row +/// must jump to exactly the setting named on that row. +#[test] +fn test_settings_search_result_click_after_wheel_scroll() { + let mut harness = EditorTestHarness::new(120, 40).unwrap(); + harness.open_settings().unwrap(); + + // "line" matches far more settings than fit on one screen. + harness + .send_key(KeyCode::Char('/'), KeyModifiers::NONE) + .unwrap(); + for c in "line".chars() { + harness + .send_key(KeyCode::Char(c), KeyModifiers::NONE) + .unwrap(); + } + harness.render().unwrap(); + + // The selected top result carries the "▸ " marker; its position anchors + // the results area geometry (each result is 3 rows tall, names start + // right after the 2-char indicator). + let (marker_col, top_row) = harness + .find_text_on_screen("▸ ") + .expect("selected search result marker visible"); + let name_col = marker_col + 2; + // Extract the top slot's result name from its row text. Anchor on the + // "▸" marker char itself (char indices in `screen_row_text` can drift + // from screen columns), and cut at the first double-space run. + let name_of_top_slot = |harness: &EditorTestHarness| -> String { + let row_text = harness.screen_row_text(top_row); + let idx = match row_text.find('▸') { + Some(i) => i + '▸'.len_utf8(), + None => return String::new(), + }; + row_text[idx..] + .trim_start() + .split(" ") + .next() + .unwrap_or("") + .trim() + .to_string() + }; + let first_result = name_of_top_slot(&harness); + assert!(!first_result.is_empty()); + + // One wheel notch over the results area scrolls the list; the top + // visible row now shows a different (later) result. + harness + .mouse_scroll_down(name_col + 5, top_row + 4) + .unwrap(); + harness.render().unwrap(); + let clicked_name = name_of_top_slot(&harness); + assert!(!clicked_name.is_empty()); + assert_ne!( + clicked_name, first_result, + "wheel notch must scroll the search results" + ); + + // Click the top visible row: the jump must land on the setting that row + // names, not on the pre-scroll result 0. + harness.mouse_click(name_col + 2, top_row).unwrap(); + harness.render().unwrap(); + + let screen = harness.screen_to_string(); + assert!( + screen.contains(&format!("> {}", clicked_name)), + "clicking the row showing '{clicked_name}' must select that setting; screen:\n{screen}" + ); + + harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); +} + +/// Reproducer for issue #2860 (wheel cannot select the last result): at the +/// end of the list further wheel-downs were no-ops while the selection was +/// pinned to the first visible row, so the last result was unreachable by +/// mouse. Wheel-downs past the end must keep advancing the selection until +/// the bottom-most visible row is the selected one (matching keyboard). +#[test] +fn test_settings_search_wheel_selects_last_result() { + let mut harness = EditorTestHarness::new(120, 40).unwrap(); + harness.open_settings().unwrap(); + + harness + .send_key(KeyCode::Char('/'), KeyModifiers::NONE) + .unwrap(); + for c in "line".chars() { + harness + .send_key(KeyCode::Char(c), KeyModifiers::NONE) + .unwrap(); + } + harness.render().unwrap(); + + let (marker_col, top_row) = harness + .find_text_on_screen("▸ ") + .expect("selected search result marker visible"); + + // Wheel down until the screen stops changing (bottom of the list plus + // the selection walk to the last item). Bounded by twice the notch count + // any realistic result list needs, and exits early once stable. + let mut prev_screen = harness.screen_to_string(); + for _ in 0..300 { + harness + .mouse_scroll_down(marker_col + 5, top_row + 4) + .unwrap(); + harness.render().unwrap(); + let screen = harness.screen_to_string(); + if screen == prev_screen { + break; + } + prev_screen = screen; + } + + // The selection marker must have walked below the first visible row — + // onto the last result — instead of staying pinned to the viewport top. + let (_, marker_row_after) = harness + .find_text_on_screen("▸ ") + .expect("selected search result marker still visible"); + assert!( + marker_row_after >= top_row + 3, + "selection must reach past the first visible row at the end of the \ + list (marker at row {marker_row_after}, results start at {top_row})" + ); + + harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); + harness.send_key(KeyCode::Esc, KeyModifiers::NONE).unwrap(); +} + /// Test theme dropdown can be cycled with Enter or Right arrow /// BUG: Theme dropdown doesn't cycle - it stays on the same value #[test]