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 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)); + } } 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]