From af8bfaeb05d9d477c3c177f00cd25b5f1055064a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 14:22:30 +0000 Subject: [PATCH 1/4] fix: scroll prompt-line input horizontally so the cursor stays visible With input longer than the prompt line (search prompt, open-file prompt, palette input), the renderer clipped the paragraph at the right edge and skipped set_cursor_position entirely once the cursor's logical column passed the width: the cursor vanished, the tail of the input was unviewable, and Left presses gave no visual feedback even though the internal cursor moved (issue #2876). The label now stays anchored at the left edge while the input renders in the remaining columns with a horizontal scroll that keeps the cursor inside the viewport (pinned to the last column while past the right edge), and the terminal cursor is always placed. Both prompt renderers (generic and file-open) share the new tail, which also replaces the file-open renderer's approximate cursor-column arithmetic with the actual display width of the label spans. Fixes #2876 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- crates/fresh-editor/src/view/ui/status_bar.rs | 239 +++++++++++++++--- .../e2e/issue_2876_prompt_input_hscroll.rs | 58 +++++ crates/fresh-editor/tests/e2e/mod.rs | 1 + 3 files changed, 256 insertions(+), 42 deletions(-) create mode 100644 crates/fresh-editor/tests/e2e/issue_2876_prompt_input_hscroll.rs diff --git a/crates/fresh-editor/src/view/ui/status_bar.rs b/crates/fresh-editor/src/view/ui/status_bar.rs index 62d32a910a..494ead63af 100644 --- a/crates/fresh-editor/src/view/ui/status_bar.rs +++ b/crates/fresh-editor/src/view/ui/status_bar.rs @@ -778,6 +778,22 @@ fn format_cursor_position_compact(line: usize, col: usize, line_count: usize) -> } } +/// Horizontal scroll (in display cells) for a single-line input rendered +/// in a viewport `width` cells wide, so the cursor at display column +/// `cursor_cells` is always visible (issue #2876). Returns 0 while the +/// cursor fits; otherwise scrolls just enough that the cursor lands on +/// the viewport's last column. +/// +/// Invariant (for `width > 0`): `scroll <= cursor_cells` and +/// `cursor_cells - scroll < width`. +pub(crate) fn input_hscroll(cursor_cells: usize, width: usize) -> usize { + if width == 0 { + 0 + } else { + cursor_cells.saturating_sub(width - 1) + } +} + /// Renders the status bar and prompt/minibuffer pub struct StatusBarRenderer; @@ -808,8 +824,9 @@ impl StatusBarRenderer { ) { let base_style = Style::default().fg(theme.prompt_fg).bg(theme.prompt_bg); - // Create spans for the prompt - let mut spans = vec![Span::styled(prompt.message.clone(), base_style)]; + // Create spans for the input (the message/label is rendered + // separately so it stays anchored while the input scrolls). + let mut spans = Vec::new(); // If there's a selection, split the input into parts if let Some((sel_start, sel_end)) = prompt.selection_range() { @@ -841,20 +858,66 @@ impl StatusBarRenderer { spans.push(Span::styled(prompt.input.clone(), base_style)); } - let line = Line::from(spans); - let prompt_line = Paragraph::new(line).style(base_style); + Self::render_prompt_label_and_input( + frame, + area, + vec![Span::styled(prompt.message.clone(), base_style)], + spans, + base_style, + str_width(&prompt.input[..prompt.cursor_pos.min(prompt.input.len())]), + ); + } - frame.render_widget(prompt_line, area); + /// Shared tail of the prompt-line renderers: paint the label spans + /// anchored at the left edge, then the input spans in the remaining + /// columns with a horizontal scroll that keeps the cursor visible + /// (issue #2876), and always place the terminal cursor on the input. + /// + /// `cursor_cells` is the display width of the input up to the cursor. + fn render_prompt_label_and_input( + frame: &mut Frame, + area: Rect, + label_spans: Vec>, + input_spans: Vec>, + base_style: Style, + cursor_cells: usize, + ) { + // Label, clipped to the area. Use display width (not byte length) + // for proper handling of double-width CJK and zero-width + // combining characters. + let label_cells: usize = label_spans.iter().map(|s| str_width(&s.content)).sum(); + let label_cols = (label_cells.min(area.width as usize)) as u16; + let label_area = Rect { + x: area.x, + y: area.y, + width: label_cols, + height: area.height, + }; + frame.render_widget( + Paragraph::new(Line::from(label_spans)).style(base_style), + label_area, + ); + + // Input, horizontally scrolled so the cursor never leaves the + // viewport: with text longer than the box the line scrolls left + // and the cursor rides the last column. + let input_area = Rect { + x: area.x + label_cols, + y: area.y, + width: area.width - label_cols, + height: area.height, + }; + let scroll = input_hscroll(cursor_cells, input_area.width as usize); + frame.render_widget( + Paragraph::new(Line::from(input_spans)) + .style(base_style) + .scroll((0, scroll as u16)), + input_area, + ); - // Set cursor position in the prompt - // Use display width (not byte length) for proper handling of: - // - Double-width CJK characters - // - Zero-width combining characters (Thai diacritics, etc.) - let message_width = str_width(&prompt.message); - let input_width_before_cursor = str_width(&prompt.input[..prompt.cursor_pos]); - let cursor_x = (message_width + input_width_before_cursor) as u16; - if cursor_x < area.width { - frame.set_cursor_position((area.x + cursor_x, area.y)); + if input_area.width > 0 { + // `input_hscroll` guarantees cursor_cells - scroll < width. + frame.set_cursor_position((input_area.x + (cursor_cells - scroll) as u16, area.y)); } } @@ -937,34 +1000,18 @@ impl StatusBarRenderer { spans.push(Span::styled(path_display, dir_style)); } - // User input (the filename part) - normal color - spans.push(Span::styled(prompt.input.clone(), base_style)); - - let line = Line::from(spans); - let prompt_line = Paragraph::new(line).style(base_style); - - frame.render_widget(prompt_line, area); - - // Set cursor position in the prompt - // Use display width for proper handling of Unicode characters - // We need to calculate the visual width of: "Open: " + dir_display + input[..cursor_pos] - let prefix_width = str_width(&open_prompt); - let dir_display_width = if truncated.truncated { - let suffix_with_slash = if truncated.suffix.ends_with('/') { - &truncated.suffix - } else { - // We already added "/" in the suffix_with_slash above, so approximate - &truncated.suffix - }; - str_width(&truncated.prefix) + str_width("/[...]") + str_width(suffix_with_slash) + 1 - } else { - str_width(&truncated.suffix) + 1 // +1 for trailing slash - }; - let input_width_before_cursor = str_width(&prompt.input[..prompt.cursor_pos]); - let cursor_x = (prefix_width + dir_display_width + input_width_before_cursor) as u16; - if cursor_x < area.width { - frame.set_cursor_position((area.x + cursor_x, area.y)); - } + // The label here is the whole prefix (message + colorized dir path); + // the user input (the filename part) scrolls after it so the cursor + // stays visible even when the typed name overflows the line. + let input_spans = vec![Span::styled(prompt.input.clone(), base_style)]; + Self::render_prompt_label_and_input( + frame, + area, + spans, + input_spans, + base_style, + str_width(&prompt.input[..prompt.cursor_pos.min(prompt.input.len())]), + ); } /// Render a single element to its text representation. @@ -2962,4 +3009,112 @@ mod tests { let line_start = buf.line_start_offset(1).unwrap(); assert_eq!(cursor_column(&mut buf, line_start), 0); } + + /// Invariant of the prompt-input horizontal scroll window (issue #2876): + /// for any viewport width > 0 the cursor always lands inside the + /// viewport (`cursor - scroll < width`), scrolling never overshoots the + /// cursor, and no scrolling happens while the cursor already fits. + #[test] + fn test_input_hscroll_keeps_cursor_visible() { + for width in 1usize..=120 { + for cursor in 0usize..=200 { + let scroll = input_hscroll(cursor, width); + assert!(scroll <= cursor, "scroll {scroll} > cursor {cursor}"); + assert!( + cursor - scroll < width, + "cursor not visible: cursor={cursor} scroll={scroll} width={width}" + ); + if cursor < width { + assert_eq!( + scroll, 0, + "no scroll needed at cursor={cursor} width={width}" + ); + } + } + } + // Degenerate zero-width viewport must not underflow. + assert_eq!(input_hscroll(50, 0), 0); + } + + /// Reproducer for issue #2876 at the renderer level: with input wider + /// than the prompt line, the tail must scroll into view and the + /// terminal cursor must always be placed (the old renderer clipped the + /// paragraph at the right edge and skipped `set_cursor_position` + /// whenever the cursor's logical column was past it). + #[test] + fn test_render_prompt_scrolls_long_input_and_places_cursor() { + use ratatui::backend::{Backend, TestBackend}; + use ratatui::Terminal; + + let theme = + crate::view::theme::Theme::load_builtin(crate::view::theme::THEME_DARK).unwrap(); + let mut prompt = Prompt::new( + "Search: ".to_string(), + crate::view::prompt::PromptType::Search, + ); + // 100 chars in an 80-column line: 8 label cols + 72 input cols. + let input: String = ('a'..='z').cycle().take(100).collect(); + prompt.input = input.clone(); + prompt.cursor_pos = prompt.input.len(); + + let width: u16 = 80; + let backend = TestBackend::new(width, 1); + let mut terminal = Terminal::new(backend).unwrap(); + terminal + .draw(|frame| { + let area = Rect::new(0, 0, width, 1); + StatusBarRenderer::render_prompt(frame, area, &prompt, &theme); + }) + .unwrap(); + + // The label stays anchored and the *tail* of the input is visible. + let buffer = terminal.backend().buffer().clone(); + let row: String = (0..width).map(|x| buffer[(x, 0)].symbol()).collect(); + assert!( + row.starts_with("Search: "), + "label must stay visible: {row:?}" + ); + // Scroll = 100 - 71 = 29 cells, so chars 29.. are visible and the + // last column is left free for the cursor. + let tail: String = input.chars().skip(100 - 71).collect(); + assert!( + row.trim_end().ends_with(&tail), + "tail of the input must scroll into view: {row:?}" + ); + // Cursor rides the last column instead of being skipped. + let cursor = terminal.backend_mut().get_cursor_position().unwrap(); + assert_eq!((cursor.x, cursor.y), (width - 1, 0)); + + // Move the cursor 15 chars left: still visible (pinned to the last + // column), and the window follows it leftward. + prompt.cursor_pos -= 15; + terminal + .draw(|frame| { + let area = Rect::new(0, 0, width, 1); + StatusBarRenderer::render_prompt(frame, area, &prompt, &theme); + }) + .unwrap(); + let buffer = terminal.backend().buffer().clone(); + let row: String = (0..width).map(|x| buffer[(x, 0)].symbol()).collect(); + let shifted_window: String = input.chars().skip(85 - 71).take(72).collect(); + assert!( + row.ends_with(&shifted_window[shifted_window.len() - 20..]), + "window must shift with the cursor: {row:?}" + ); + let cursor = terminal.backend_mut().get_cursor_position().unwrap(); + assert_eq!((cursor.x, cursor.y), (width - 1, 0)); + + // With a short input nothing scrolls and the cursor sits right + // after the typed text. + prompt.input = "abc".to_string(); + prompt.cursor_pos = 3; + terminal + .draw(|frame| { + let area = Rect::new(0, 0, width, 1); + StatusBarRenderer::render_prompt(frame, area, &prompt, &theme); + }) + .unwrap(); + let cursor = terminal.backend_mut().get_cursor_position().unwrap(); + assert_eq!((cursor.x, cursor.y), (8 + 3, 0)); + } } diff --git a/crates/fresh-editor/tests/e2e/issue_2876_prompt_input_hscroll.rs b/crates/fresh-editor/tests/e2e/issue_2876_prompt_input_hscroll.rs new file mode 100644 index 0000000000..7712ec4975 --- /dev/null +++ b/crates/fresh-editor/tests/e2e/issue_2876_prompt_input_hscroll.rs @@ -0,0 +1,58 @@ +//! Reproducer for issue #2876: prompt-line inputs (search prompt, open-file +//! prompt, palette input) had no horizontal scrolling. With text longer than +//! the line, the paragraph was clipped at the right edge and +//! `set_cursor_position` was skipped entirely once the cursor's logical +//! column passed the width — the cursor vanished and the tail of the input +//! was unviewable, even though the internal cursor kept moving. +//! +//! With the fix, the input scrolls horizontally so the cursor is always +//! visible (pinned to the last column while past the right edge), and the +//! terminal cursor is always placed. + +use crate::common::harness::EditorTestHarness; +use crossterm::event::{KeyCode, KeyModifiers}; + +#[test] +fn test_search_prompt_long_input_scrolls_and_keeps_cursor_visible() { + let mut harness = EditorTestHarness::new(80, 24).unwrap(); + + // Open the search prompt and type 100 characters — far wider than the + // 80-column line. The last 10 form a unique marker so the visible + // window is observable on screen. + harness + .send_key(KeyCode::Char('f'), KeyModifiers::CONTROL) + .unwrap(); + harness.render().unwrap(); + let input = format!("{}TAILMARKER", "x".repeat(90)); + harness.type_text(&input).unwrap(); + + // The tail of the input must be scrolled into view (the old renderer + // showed only the first ~72 chars and never the tail) … + let cursor = harness.render_observing_cursor().unwrap(); + harness.assert_screen_contains("TAILMARKER"); + // … and the hardware cursor must be visible, riding the last column of + // the prompt line (the old renderer skipped `set_cursor_position`, so + // the frame ended with the cursor hidden). + assert_eq!( + cursor, + Some((79, 23)), + "cursor must be visible at the right edge of the prompt line" + ); + + // Press Left 15 times: the cursor is still inside the overflowing + // region, so it must stay visible while the window scrolls back with + // it (the old renderer showed no cursor until ~28 presses). + for _ in 0..15 { + harness.send_key(KeyCode::Left, KeyModifiers::NONE).unwrap(); + } + let cursor = harness.render_observing_cursor().unwrap(); + assert_eq!( + cursor, + Some((79, 23)), + "cursor must stay visible while moving left through clipped text" + ); + // The window now ends just after the cursor (char 85 of 100), so the + // tail marker (chars 90..100) must have scrolled off the right edge — + // proof the window follows the cursor rather than staying pinned. + harness.assert_screen_not_contains("TAILMARKER"); +} diff --git a/crates/fresh-editor/tests/e2e/mod.rs b/crates/fresh-editor/tests/e2e/mod.rs index dfd4c2e107..9e74f68eaa 100644 --- a/crates/fresh-editor/tests/e2e/mod.rs +++ b/crates/fresh-editor/tests/e2e/mod.rs @@ -100,6 +100,7 @@ pub mod issue_2796_key_release_duplicates; #[cfg(feature = "plugins")] pub mod issue_2810_session_escape_flush; pub mod issue_2843_single_line_viewport; +pub mod issue_2876_prompt_input_hscroll; pub mod issue_2878_split_cursor_independence; pub mod issue_2893_replace_all_many_matches; pub mod issue_779_after_eof_shade; From 3f89d8c260def2bbf9b3c1a2231a787739b1e6ce Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 15:16:09 +0000 Subject: [PATCH 2/4] fix: show a scrollbar when a prompt dropdown hides entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prompt suggestion dropdowns (Select Locale, command palette, and every other bottom-anchored suggestions popup) do scroll, but nothing said so: the popup's right border stayed a plain frame line, so a 14-locale list looked like a 10-locale list — issue #623, "at first glance, I thought only these". The command palette showed no scrollbar at all, the remaining item of issue #1593. Both lists render through the same SuggestionsRenderer, so one fix covers both. SuggestionsRenderer now draws the shared scrollbar widget over the popup's right border whenever the list overflows, and the scrollbar rect is registered in the chrome layout so the existing prompt-scrollbar mouse handlers (click-to-jump, thumb drag) work here exactly like in the Live Grep overlay. Scrollbar interaction latches the prompt's manual-scroll flag so the renderer's keep-selection-visible pass doesn't immediately undo it (mirroring the wheel-scroll behaviour of issue #2119), and moving the selection clears the latch again. The stale-rect case is also closed: the chrome's suggestions_scrollbar_rect now resets every frame instead of surviving after the overlay prompt closes. The dropdown deliberately keeps its 10-row height. Sizing it to the terminal instead was tried and dropped: neither issue asks for more rows, and a height-sized palette is worse to use — on an 80x24 terminal it covers all but one line of the buffer, and in a split layout it hides the other pane and the split separator entirely. MAX_VISIBLE_SUGGESTIONS now documents that reasoning; the unused Prompt::ensure_selected_visible wrapper is gone, since every caller passes the height it actually rendered. The Comprehensive UI B visual snapshot is regenerated for the one cell that changed: the palette's right border column now carries scrollbar cells instead of the plain frame glyph. Fixes #623 Fixes #1593 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- crates/fresh-editor/src/app/mouse_input.rs | 7 + .../fresh-editor/src/app/prompt_lifecycle.rs | 4 + crates/fresh-editor/src/app/render.rs | 28 +++- crates/fresh-editor/src/view/prompt.rs | 45 +++--- .../fresh-editor/src/view/ui/suggestions.rs | 82 +++++++++- ..._testing__Comprehensive UI B__state_b.snap | 20 +-- .../issue_623_prompt_dropdown_scrollbar.rs | 141 ++++++++++++++++++ crates/fresh-editor/tests/e2e/mod.rs | 1 + 8 files changed, 287 insertions(+), 41 deletions(-) create mode 100644 crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs diff --git a/crates/fresh-editor/src/app/mouse_input.rs b/crates/fresh-editor/src/app/mouse_input.rs index 22c31a57b8..1b3a4bea98 100644 --- a/crates/fresh-editor/src/app/mouse_input.rs +++ b/crates/fresh-editor/src/app/mouse_input.rs @@ -1987,6 +1987,10 @@ impl Editor { let click_row = row.saturating_sub(sb_rect.y) as usize; let state = ScrollbarState::new(total, visible, prompt.scroll_offset); prompt.scroll_offset = state.click_to_offset(track_height, click_row); + // Latch manual scroll so the renderer's keep-selection-visible + // pass doesn't immediately yank the offset back to the selection + // (same latch the wheel uses; reset when the selection moves). + prompt.manual_scroll = true; // Hand off to the drag follow-up so subsequent mouse moves // keep tracking the thumb. self.active_window_mut() @@ -3000,6 +3004,9 @@ impl Editor { let click_row = clamped_row.saturating_sub(sb_rect.y) as usize; let state = ScrollbarState::new(total, visible, prompt.scroll_offset); prompt.scroll_offset = state.click_to_offset(track_height, click_row); + // Keep the manual-scroll latch through the drag so the + // renderer doesn't pull the offset back to the selection. + prompt.manual_scroll = true; } return Ok(()); } diff --git a/crates/fresh-editor/src/app/prompt_lifecycle.rs b/crates/fresh-editor/src/app/prompt_lifecycle.rs index efdeeda8af..1eb984e35a 100644 --- a/crates/fresh-editor/src/app/prompt_lifecycle.rs +++ b/crates/fresh-editor/src/app/prompt_lifecycle.rs @@ -989,6 +989,10 @@ impl Editor { }; prompt.selected_suggestion = Some(new_selected); + // The wheel moved the selection, so re-engage the renderer's + // keep-selection-visible behaviour (clears any latch a + // scrollbar click/drag set). + prompt.manual_scroll = false; // Update input to match selected suggestion for non-plugin prompts if !matches!(prompt.prompt_type, PromptType::Plugin { .. }) { diff --git a/crates/fresh-editor/src/app/render.rs b/crates/fresh-editor/src/app/render.rs index 90c8e71fb1..848fd56c2c 100644 --- a/crates/fresh-editor/src/app/render.rs +++ b/crates/fresh-editor/src/app/render.rs @@ -748,6 +748,7 @@ impl Editor { // Initialize popup/suggestion layout state (rendered after status bar below) self.active_chrome_mut().suggestions_area = None; self.active_chrome_mut().suggestions_outer_area = None; + self.active_chrome_mut().suggestions_scrollbar_rect = None; self.active_chrome_mut().prompt_results_area = None; self.active_chrome_mut().prompt_preview_area = None; self.active_window_mut().file_browser_layout = None; @@ -2453,9 +2454,12 @@ impl Editor { return; } - let suggestion_count = prompt.suggestions.len().min(10); let is_quick_open = prompt.prompt_type == crate::view::prompt::PromptType::QuickOpen; let hints_height: u16 = if is_quick_open { 1 } else { 0 }; + let suggestion_count = prompt + .suggestions + .len() + .min(crate::view::prompt::MAX_VISIBLE_SUGGESTIONS); let height = suggestion_count as u16 + 2 + hints_height; let suggestions_area = ratatui::layout::Rect { @@ -2478,9 +2482,14 @@ impl Editor { } // Adjust the prompt's scroll position to keep the selected item - // visible, scrolling the minimum amount required. + // visible, scrolling the minimum amount required — unless the user + // has scrolled the list with the scrollbar, in which case pulling + // the offset back would undo their scroll (same reasoning as the + // overlay prompt, issue #2119). if let Some(prompt) = self.active_window_mut().prompt.as_mut() { - prompt.ensure_selected_visible(); + if !prompt.manual_scroll { + prompt.ensure_selected_visible_within(suggestion_count); + } } let Some(prompt) = &self.active_window().prompt else { return; @@ -2500,6 +2509,19 @@ impl Editor { if chrome.suggestions_area.is_some() { chrome.suggestions_outer_area = Some(suggestions_area); } + // When the list overflows, the renderer drew a scrollbar over the + // popup's right border; record its rect so the shared prompt- + // scrollbar mouse handlers (click-to-jump, thumb drag) work here + // exactly like in the overlay prompt (issue #623 / #1593). + chrome.suggestions_scrollbar_rect = + new_suggestions_area.and_then(|(inner, _, visible, total)| { + (total > visible).then_some(ratatui::layout::Rect { + x: inner.x + inner.width, + y: inner.y, + width: 1, + height: inner.height, + }) + }); // The quick-open hints row is chrome drawn into cells; the web renders // no hints, so in `suppress_chrome_cells` mode we skip it entirely diff --git a/crates/fresh-editor/src/view/prompt.rs b/crates/fresh-editor/src/view/prompt.rs index 920fe901e9..d7e458e0be 100644 --- a/crates/fresh-editor/src/view/prompt.rs +++ b/crates/fresh-editor/src/view/prompt.rs @@ -296,9 +296,17 @@ pub struct Prompt { pub status: String, } -/// Maximum number of suggestion rows shown at once. Mirrors the cap used by -/// `SuggestionsRenderer` so `Prompt::ensure_selected_visible` can compute the -/// viewport size without inspecting render state. +/// Maximum number of suggestion rows a bottom-anchored dropdown shows at once. +/// The palette is a transient overlay drawn over the document, so it stays +/// deliberately short rather than growing with the terminal — on an 80x24 +/// terminal a height-sized dropdown would cover all but one line of the +/// buffer, and on a split layout it would hide the other pane entirely. +/// Overflow is communicated by the scrollbar the renderer draws over the +/// popup's right border (issues #623 / #1593) instead of by more rows. +/// +/// Renderers with their own geometry (the floating Live Grep overlay, which +/// can be 30+ rows tall) pass their actual height to +/// [`Prompt::ensure_selected_visible_within`] rather than using this cap. pub const MAX_VISIBLE_SUGGESTIONS: usize = 10; impl Prompt { @@ -674,28 +682,15 @@ impl Prompt { self.manual_scroll = false; } - /// Adjust `scroll_offset` so that `selected_suggestion` is inside the - /// viewport, scrolling the minimum amount required. A selection that's - /// already on-screen leaves the viewport untouched — this is what stops - /// a click on a near-bottom item from snapping the list upward and - /// recentering under the cursor (issue #1660). - /// - /// Uses the bottom-popup default cap (`MAX_VISIBLE_SUGGESTIONS`). - /// Callers rendering into a different-sized area (e.g. the - /// floating Live Grep overlay, where the suggestion list can be - /// 30+ rows tall) should call - /// [`ensure_selected_visible_within`] with the actual height - /// instead — otherwise the scroll moves prematurely once the - /// selection passes the 10th row even though the rest of the - /// list is still visible on-screen. - pub fn ensure_selected_visible(&mut self) { - self.ensure_selected_visible_within(MAX_VISIBLE_SUGGESTIONS); - } - - /// Like [`ensure_selected_visible`] but with an explicit - /// `visible_count` argument, so renderers in differently-sized - /// frames don't all share the bottom-popup `MAX_VISIBLE_SUGGESTIONS` - /// assumption. + /// Adjust `scroll_offset` so that `selected_suggestion` is inside a + /// viewport of `visible_count` rows, scrolling the minimum amount + /// required. A selection that's already on-screen leaves the viewport + /// untouched — this is what stops a click on a near-bottom item from + /// snapping the list upward and recentering under the cursor (issue + /// #1660). Callers pass the actual rendered height of their list + /// (bottom-anchored popup and floating Live Grep overlay alike), so + /// the scroll only moves when the selection genuinely leaves the + /// visible window. pub fn ensure_selected_visible_within(&mut self, visible_count: usize) { let total = self.suggestions.len(); let visible = total.min(visible_count.max(1)); diff --git a/crates/fresh-editor/src/view/ui/suggestions.rs b/crates/fresh-editor/src/view/ui/suggestions.rs index 46cae38c51..4a4a8912df 100644 --- a/crates/fresh-editor/src/view/ui/suggestions.rs +++ b/crates/fresh-editor/src/view/ui/suggestions.rs @@ -3,6 +3,7 @@ use crate::input::commands::{CommandSource, Suggestion}; use crate::primitives::display_width::{char_width, str_width}; use crate::view::prompt::Prompt; +use crate::view::ui::scrollbar::{render_scrollbar, ScrollbarColors, ScrollbarState}; use ratatui::layout::Rect; use ratatui::style::{Color, Modifier, Style}; use ratatui::text::{Line, Span}; @@ -72,9 +73,10 @@ impl SuggestionsRenderer { // The scroll position is owned by the Prompt itself and only adjusted // when the selection moves out of the viewport (see - // `Prompt::ensure_selected_visible`, called once before render). This - // keeps a stable list under the cursor so a click near the bottom - // doesn't trigger a recenter that shifts items mid-double-click. + // `Prompt::ensure_selected_visible_within`, called once before + // render). This keeps a stable list under the cursor so a click near + // the bottom doesn't trigger a recenter that shifts items + // mid-double-click. let (start_idx, end_idx) = visible_range(prompt, visible_count); let visible_suggestions = &prompt.suggestions[start_idx..end_idx]; let layout = ColumnLayout::compute(visible_suggestions, available_width); @@ -108,6 +110,27 @@ impl SuggestionsRenderer { if draw { let paragraph = Paragraph::new(lines).block(block); frame.render_widget(paragraph, area); + + // When the list overflows the viewport, draw a scrollbar over + // the right border so the user can tell more entries exist and + // how far through them the viewport is (issues #623 / #1593). + // The borderless variant (floating overlay) carves its own + // scrollbar lane next to the list instead. + if with_border && prompt.suggestions.len() > visible_count && inner_area.height > 0 { + let scrollbar_area = Rect { + x: inner_area.x + inner_area.width, + y: inner_area.y, + width: 1, + height: inner_area.height, + }; + let state = ScrollbarState::new(prompt.suggestions.len(), visible_count, start_idx); + render_scrollbar( + frame, + scrollbar_area, + &state, + &ScrollbarColors::from_theme(theme), + ); + } } // Return area info for mouse hit testing @@ -832,6 +855,59 @@ mod tests { ); } + /// When the suggestion list overflows the popup, a scrollbar must be + /// drawn over the right border so the user can tell more entries exist + /// (issues #623 / #1593). The scrollbar paints background-colored + /// cells, so the border column's `│` glyphs disappear on those rows. + #[test] + fn overflowing_list_draws_scrollbar_on_right_border() { + let suggestions: Vec = (0..30) + .map(|i| Suggestion::new(format!("Command {i:02}"))) + .collect(); + let mut prompt = palette_prompt(suggestions); + prompt.selected_suggestion = Some(0); + + // 12 rows: 10 inner rows for 30 suggestions -> overflow. + let width: u16 = 60; + let rows = render_rows(&prompt, width, 12); + let border_col = (width - 1) as usize; + let scrollbar_cells = (1..11) + .filter(|&y| { + let cell: String = rows[y].chars().nth(border_col).unwrap().to_string(); + cell != "│" + }) + .count(); + assert_eq!( + scrollbar_cells, + 10, + "all inner border rows must be covered by the scrollbar track/thumb:\n{}", + rows.join("\n") + ); + } + + /// A list that fits entirely in the popup keeps its plain right border — + /// no scrollbar noise. + #[test] + fn fitting_list_keeps_plain_right_border() { + let suggestions: Vec = (0..5) + .map(|i| Suggestion::new(format!("Command {i}"))) + .collect(); + let prompt = palette_prompt(suggestions); + + let width: u16 = 60; + let rows = render_rows(&prompt, width, 12); + let border_col = (width - 1) as usize; + for y in 1..11 { + let cell: String = rows[y].chars().nth(border_col).unwrap().to_string(); + assert_eq!( + cell, + "│", + "row {y} must keep the plain border when the list fits:\n{}", + rows.join("\n") + ); + } + } + /// Test that truncation produces valid UTF-8 output #[test] fn test_truncation_preserves_valid_utf8() { diff --git a/crates/fresh-editor/tests/common/snapshots/e2e_tests__common__visual_testing__Comprehensive UI B__state_b.snap b/crates/fresh-editor/tests/common/snapshots/e2e_tests__common__visual_testing__Comprehensive UI B__state_b.snap index e917684b36..40f73f788f 100644 --- a/crates/fresh-editor/tests/common/snapshots/e2e_tests__common__visual_testing__Comprehensive UI B__state_b.snap +++ b/crates/fresh-editor/tests/common/snapshots/e2e_tests__common__visual_testing__Comprehensive UI B__state_b.snap @@ -19,16 +19,16 @@ expression: "&screen_text" ~ ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Plugin Demo: Open Help Open the editor help page (uses built-in action) welcome│ -│ Show Signature Help Show function parameter hints builtin│ -│ Shell Command (Replace) Alt+Shift+| Run shell command on buffer/selection, replace con... builtin│ -│ Search and Replace in Project Alt+A Search and replace text across all git-tracked files search_replace│ -│ Show Manual F1 Open the help manual builtin│ -│ Git Log: Close Close the git log panel git_log│ -│ Toggle Inlay Hints Show or hide LSP inlay hints (type hints, paramete... builtin│ -│ Start/Restart LSP Server Start or restart the LSP server for the current la... builtin│ -│ Copy File Path Copy the absolute path of the current buffer's fil... builtin│ -│ Git Blame: Close Close the git blame panel git_blame│ +│ Plugin Demo: Open Help Open the editor help page (uses built-in action) welcome +│ Show Signature Help Show function parameter hints builtin +│ Shell Command (Replace) Alt+Shift+| Run shell command on buffer/selection, replace con... builtin +│ Search and Replace in Project Alt+A Search and replace text across all git-tracked files search_replace +│ Show Manual F1 Open the help manual builtin +│ Git Log: Close Close the git log panel git_log +│ Toggle Inlay Hints Show or hide LSP inlay hints (type hints, paramete... builtin +│ Start/Restart LSP Server Start or restart the LSP server for the current la... builtin +│ Copy File Path Copy the absolute path of the current buffer's fil... builtin +│ Git Blame: Close Close the git blame panel git_blame └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ file | >command | :line | #buffer >help diff --git a/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs b/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs new file mode 100644 index 0000000000..b88f74652f --- /dev/null +++ b/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs @@ -0,0 +1,141 @@ +//! Reproducers for issue #623 (a suggestion dropdown that scrolls, but gives +//! no sign that entries are hidden — "at first glance, I thought only these") +//! and the remaining item of issue #1593 (the command palette rendered no +//! scrollbar at all). Both lists go through the same `SuggestionsRenderer`, +//! so one fix covers both: when the list overflows the dropdown, the shared +//! scrollbar widget is drawn over the popup's right border, and it responds +//! to clicks like every other scrollbar in the editor. + +use crate::common::harness::EditorTestHarness; +use crossterm::event::{KeyCode, KeyModifiers}; + +/// The popup's suggestion rows: full-width popup rows start with the `│` +/// left border glyph (the top/bottom borders start with `┌`/`└`). +fn suggestion_rows(harness: &EditorTestHarness, height: u16) -> Vec { + (0..height) + .filter(|&y| harness.get_row_text(y).starts_with('│')) + .collect() +} + +/// Issue #1593: the command palette lists far more commands than fit, so +/// every suggestion row must carry a scrollbar cell on the popup's right +/// border. Before the fix that column was a plain `│` frame line, so nothing +/// signalled that the list continued. +#[test] +fn test_command_palette_overflow_draws_scrollbar() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + + harness + .send_key(KeyCode::Char('p'), KeyModifiers::CONTROL) + .unwrap(); + harness.render().unwrap(); + harness.assert_screen_contains("Add Cursor Above"); + + let rows = suggestion_rows(&harness, 24); + assert!(!rows.is_empty(), "command palette must render suggestions"); + + let scrollbar_col = 99; + let scrollbar_cells = rows + .iter() + .filter(|&&y| { + harness.is_scrollbar_thumb_at(scrollbar_col, y) + || harness.is_scrollbar_track_at(scrollbar_col, y) + }) + .count(); + assert_eq!( + scrollbar_cells, + rows.len(), + "every suggestion row must carry a scrollbar track/thumb cell on the right border" + ); +} + +/// Issue #623 verbatim: the Select Locale dropdown shows 10 of the 14 +/// locales. It scrolled even before the fix — what was missing was any +/// indication that more entries existed, which is now the right-border +/// scrollbar. +#[test] +fn test_select_locale_dropdown_shows_scrollbar_for_hidden_entries() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + + harness + .send_key(KeyCode::Char('p'), KeyModifiers::CONTROL) + .unwrap(); + harness.type_text("Select Locale").unwrap(); + harness + .send_key(KeyCode::Enter, KeyModifiers::NONE) + .unwrap(); + harness.render().unwrap(); + + // The locale list is open: 14 locales, 10 rows of room, so 4 are hidden. + let rows = suggestion_rows(&harness, 24); + assert_eq!( + rows.len(), + 10, + "locale dropdown must fill its 10 rows with the 14 locales" + ); + + let scrollbar_col = 99; + let scrollbar_cells = rows + .iter() + .filter(|&&y| { + harness.is_scrollbar_thumb_at(scrollbar_col, y) + || harness.is_scrollbar_track_at(scrollbar_col, y) + }) + .count(); + assert_eq!( + scrollbar_cells, + rows.len(), + "the hidden locales must be signalled by a scrollbar on the right border" + ); +} + +/// A list that fits the dropdown keeps its plain `│` right border — the +/// indicator only appears when entries are actually hidden. +#[test] +fn test_short_suggestion_list_shows_no_scrollbar() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + + harness + .send_key(KeyCode::Char('p'), KeyModifiers::CONTROL) + .unwrap(); + harness.type_text("Toggle Line Wrap").unwrap(); + harness.render().unwrap(); + harness.assert_screen_contains("Toggle Line Wrap"); + + let rows = suggestion_rows(&harness, 24); + assert!( + !rows.is_empty() && rows.len() < 10, + "filtered list should fit the dropdown, got {} rows", + rows.len() + ); + for &y in &rows { + assert!( + harness.get_row_text(y).trim_end().ends_with('│'), + "row {y} must keep its plain right border when the list fits" + ); + } +} + +/// Issue #1593 asked for a palette scrollbar that behaves like the editor's: +/// clicking low on the track must jump the list there and keep it there +/// (the renderer's keep-selection-visible pass must not yank it back). +#[test] +fn test_clicking_palette_scrollbar_scrolls_the_list() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + + harness + .send_key(KeyCode::Char('p'), KeyModifiers::CONTROL) + .unwrap(); + harness.render().unwrap(); + harness.assert_screen_contains("Add Cursor Above"); + + let rows = suggestion_rows(&harness, 24); + let bottom_row = *rows.last().expect("popup must have suggestion rows"); + + // Click the bottom of the scrollbar track: the list must jump towards + // the end, so the alphabetically-first command scrolls out of view. + harness.mouse_click(99, bottom_row).unwrap(); + harness.render().unwrap(); + + harness.assert_screen_not_contains("Add Cursor Above"); +} diff --git a/crates/fresh-editor/tests/e2e/mod.rs b/crates/fresh-editor/tests/e2e/mod.rs index 9e74f68eaa..d3131a6bdb 100644 --- a/crates/fresh-editor/tests/e2e/mod.rs +++ b/crates/fresh-editor/tests/e2e/mod.rs @@ -103,6 +103,7 @@ pub mod issue_2843_single_line_viewport; pub mod issue_2876_prompt_input_hscroll; pub mod issue_2878_split_cursor_independence; pub mod issue_2893_replace_all_many_matches; +pub mod issue_623_prompt_dropdown_scrollbar; pub mod issue_779_after_eof_shade; pub mod issue_close_file_in_split_hides_buffer_group; pub mod language_dialog_esc_cancels_edit; From 76afd794dca10f8d2a390e979886e2695daf3224 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 19:35:16 +0000 Subject: [PATCH 3/4] fix: land the dropdown scrollbar thumb on the row that was clicked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking the command palette's new scrollbar put the thumb one row above the row under the cursor, and the bottom row of the track could never reach the end of the list — reported by the maintainer while testing #2954. The prompt scrollbar handlers mapped a click with `ScrollbarState::click_to_offset`, which divides the click row by the whole track height. The thumb's top can only travel `track_height - thumb_size` rows, so dividing by the track over-estimates the denominator and the result drifts up by a factor of `thumb_size / track_height` — one row for a palette-sized dropdown, and never quite `max_scroll` at the bottom. `offset_for_thumb_top` is the function that inverts the geometry the renderer actually draws, so both the press and the drag follow-up use it now, through one shared helper so the two can't drift apart again. `offset_for_thumb_top` itself was only accurate to within a row: `thumb_geometry` floors offset -> thumb row, so inverting it with a *rounding* division still landed a row high whenever the quotient wasn't integral. It now takes the smallest offset that reaches the requested row, and — because a list barely longer than its viewport has fewer scroll positions than track rows, so some rows are unreachable — falls back to whichever neighbouring offset renders closest. Its round-trip test asserts equality instead of a one-row tolerance, and fails on the old implementation. Refs #623 Refs #1593 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- crates/fresh-editor/src/app/mouse_input.rs | 76 ++++++--- crates/fresh-editor/src/view/ui/scrollbar.rs | 73 +++++++- .../issue_623_prompt_dropdown_scrollbar.rs | 161 +++++++++++++++++- 3 files changed, 277 insertions(+), 33 deletions(-) diff --git a/crates/fresh-editor/src/app/mouse_input.rs b/crates/fresh-editor/src/app/mouse_input.rs index 1b3a4bea98..951768e74f 100644 --- a/crates/fresh-editor/src/app/mouse_input.rs +++ b/crates/fresh-editor/src/app/mouse_input.rs @@ -12,7 +12,7 @@ use crate::input::keybindings::Action; use crate::model::event::{ContainerId, CursorId, LeafId, SplitDirection}; use crate::services::plugins::hooks::HookArgs; use crate::view::popup_mouse::{popup_areas_to_layout_info, PopupHitTester}; -use crate::view::prompt::PromptType; +use crate::view::prompt::{PromptType, MAX_VISIBLE_SUGGESTIONS}; use crate::view::ui::tabs::TabHit; use anyhow::Result as AnyhowResult; use ratatui::layout::Rect; @@ -23,6 +23,33 @@ fn in_rect(col: u16, row: u16, rect: Rect) -> bool { col >= rect.x && col < rect.x + rect.width && row >= rect.y && row < rect.y + rect.height } +/// Map a screen row on a suggestion list's scrollbar track to the prompt +/// scroll offset that puts the thumb's top on exactly that row. +/// +/// Shared by the press and the drag-follow-up so the thumb tracks the cursor +/// identically in both. `ScrollbarState::click_to_offset` is deliberately +/// *not* used here: it divides by the whole track height instead of by the +/// thumb's actual travel, so it lands the thumb a row above the row the user +/// pointed at and can never reach the bottom of the track. +/// [`ScrollbarState::offset_for_thumb_top`] is the real inverse of the thumb +/// geometry the renderer draws. +/// +/// Rows above/below the track clamp to its ends rather than being rejected, +/// so a fast drag doesn't drop the thumb. +fn prompt_scrollbar_offset_for_row( + total: usize, + visible: usize, + scroll_offset: usize, + sb_rect: Rect, + row: u16, +) -> usize { + use crate::view::ui::scrollbar::ScrollbarState; + let clamped_row = row.clamp(sb_rect.y, sb_rect.y + sb_rect.height.saturating_sub(1)); + let track_row = clamped_row.saturating_sub(sb_rect.y) as usize; + ScrollbarState::new(total, visible, scroll_offset) + .offset_for_thumb_top(sb_rect.height as usize, track_row) +} + /// Where a screen cell lands inside a floating widget panel. See /// [`Editor::probe_floating_widget`]. struct FloatingWidgetProbe { @@ -1953,13 +1980,10 @@ impl Editor { Some(self.handle_action(Action::PromptConfirm)) } - /// Click/drag on the floating-overlay prompt's scrollbar - /// (issue #1796). Reuses - /// `view::ui::scrollbar::ScrollbarState::click_to_offset` for - /// the same math the popup-scrollbar handler uses, so thumb - /// behaviour is consistent across the editor. + /// Click/drag on a suggestion-list scrollbar: the floating-overlay + /// prompt's (issue #1796) and the bottom-anchored dropdown's + /// (issues #623 / #1593), which share `suggestions_scrollbar_rect`. fn handle_click_prompt_scrollbar(&mut self, col: u16, row: u16) -> Option> { - use crate::view::ui::scrollbar::ScrollbarState; let sb_rect = self.active_chrome().suggestions_scrollbar_rect?; if col < sb_rect.x || col >= sb_rect.x + sb_rect.width @@ -1981,15 +2005,18 @@ impl Editor { .windows .get_mut(&active_window_id) .and_then(|w| w.prompt.as_mut())?; - let visible = suggestions_area_visible.unwrap_or(prompt.suggestions.len().min(10)); - let total = prompt.suggestions.len(); - let track_height = sb_rect.height as usize; - let click_row = row.saturating_sub(sb_rect.y) as usize; - let state = ScrollbarState::new(total, visible, prompt.scroll_offset); - prompt.scroll_offset = state.click_to_offset(track_height, click_row); + let visible = suggestions_area_visible + .unwrap_or_else(|| prompt.suggestions.len().min(MAX_VISIBLE_SUGGESTIONS)); + prompt.scroll_offset = prompt_scrollbar_offset_for_row( + prompt.suggestions.len(), + visible, + prompt.scroll_offset, + sb_rect, + row, + ); // Latch manual scroll so the renderer's keep-selection-visible // pass doesn't immediately yank the offset back to the selection - // (same latch the wheel uses; reset when the selection moves). + // (same latch the wheel uses; released when the selection moves). prompt.manual_scroll = true; // Hand off to the drag follow-up so subsequent mouse moves // keep tracking the thumb. @@ -2980,7 +3007,6 @@ impl Editor { .mouse_state .dragging_prompt_scrollbar { - use crate::view::ui::scrollbar::ScrollbarState; // Snapshot chrome rects up front so the prompt borrow on // active_window_mut() doesn't conflict. let sb_rect = self.active_chrome().suggestions_scrollbar_rect; @@ -2993,17 +3019,15 @@ impl Editor { .get_mut(&active_window_id) .and_then(|w| w.prompt.as_mut()), ) { - let visible = suggestions_area_visible.unwrap_or(prompt.suggestions.len().min(10)); - let total = prompt.suggestions.len(); - let track_height = sb_rect.height as usize; - // Allow dragging slightly past the top/bottom; clamp - // here rather than rejecting so the thumb keeps up - // with a fast mouse. - let clamped_row = - row.clamp(sb_rect.y, sb_rect.y + sb_rect.height.saturating_sub(1)); - let click_row = clamped_row.saturating_sub(sb_rect.y) as usize; - let state = ScrollbarState::new(total, visible, prompt.scroll_offset); - prompt.scroll_offset = state.click_to_offset(track_height, click_row); + let visible = suggestions_area_visible + .unwrap_or_else(|| prompt.suggestions.len().min(MAX_VISIBLE_SUGGESTIONS)); + prompt.scroll_offset = prompt_scrollbar_offset_for_row( + prompt.suggestions.len(), + visible, + prompt.scroll_offset, + sb_rect, + row, + ); // Keep the manual-scroll latch through the drag so the // renderer doesn't pull the offset back to the selection. prompt.manual_scroll = true; diff --git a/crates/fresh-editor/src/view/ui/scrollbar.rs b/crates/fresh-editor/src/view/ui/scrollbar.rs index 1b085ddd2f..520e6da5eb 100644 --- a/crates/fresh-editor/src/view/ui/scrollbar.rs +++ b/crates/fresh-editor/src/view/ui/scrollbar.rs @@ -113,8 +113,26 @@ impl ScrollbarState { return 0; } let clamped = target_thumb_top.min(max_thumb_top); - let ratio = clamped as f64 / max_thumb_top as f64; - ((ratio * max_scroll as f64).round() as usize).min(max_scroll) + // `thumb_geometry` *floors* offset → thumb row, so a rounding + // division here lands the thumb one row above the target whenever the + // exact quotient has a fractional part. Take the smallest offset that + // reaches the row (ceiling division) instead, and keep the offset + // below it as a candidate: when there are fewer scroll positions than + // track rows, not every row is reachable and the nearest one wins. + let hi = (clamped * max_scroll) + .div_ceil(max_thumb_top) + .min(max_scroll); + let lo = hi.saturating_sub(1); + let thumb_top_of = |offset: usize| { + Self::new(self.total_items, self.visible_items, offset) + .thumb_geometry(track_height) + .0 + }; + if thumb_top_of(lo).abs_diff(clamped) < thumb_top_of(hi).abs_diff(clamped) { + lo + } else { + hi + } } /// Compute the scroll offset for a drag that preserves the cursor's @@ -505,29 +523,72 @@ mod tests { #[test] fn test_offset_for_thumb_top_round_trip() { // For every reachable thumb row, `offset_for_thumb_top` must - // produce an offset whose rendered thumb top matches that row — - // i.e. it really is the inverse of `thumb_geometry`. + // produce an offset whose rendered thumb top matches that row + // *exactly* — it really is the inverse of `thumb_geometry`. The + // last two cases are the prompt-dropdown shape (10 visible rows on + // a 10-row track), where a rounding inverse used to land the thumb + // a row above the row the user clicked. let cases = [ (200_usize, 50_usize, 20_usize), (1000, 30, 25), (50, 10, 15), + (30, 10, 10), + (14, 10, 10), ]; for (total, visible, track) in cases { let probe = ScrollbarState::new(total, visible, 0); let (_, thumb_size) = probe.thumb_geometry(track); let max_thumb_top = track.saturating_sub(thumb_size); + let max_scroll = total - visible; + // Every row is reachable only when there are at least as many + // scroll positions as thumb rows; otherwise the mapping can + // only pick the nearest reachable row (asserted below). + assert!( + max_thumb_top <= max_scroll, + "case (total={total} visible={visible} track={track}) is not exactly invertible" + ); for target in 0..=max_thumb_top { let offset = probe.offset_for_thumb_top(track, target); let placed = ScrollbarState::new(total, visible, offset); let (got_top, _) = placed.thumb_geometry(track); - assert!( - got_top.abs_diff(target) <= 1, + assert_eq!( + got_top, target, "thumb landed at {got_top}, expected {target} (total={total} visible={visible} track={track})" ); } } } + #[test] + fn test_offset_for_thumb_top_picks_nearest_when_rows_unreachable() { + // 11 items in a 10-row viewport: only two scroll positions exist, + // so most track rows can't be hit. The mapping must still pick the + // closest reachable thumb row rather than always rounding down. + let total = 11; + let visible = 10; + let track = 10; + let probe = ScrollbarState::new(total, visible, 0); + let (_, thumb_size) = probe.thumb_geometry(track); + let max_thumb_top = track - thumb_size; + for target in 0..=max_thumb_top { + let offset = probe.offset_for_thumb_top(track, target); + let (got_top, _) = ScrollbarState::new(total, visible, offset).thumb_geometry(track); + let best = (0..=(total - visible)) + .map(|o| { + ScrollbarState::new(total, visible, o) + .thumb_geometry(track) + .0 + }) + .min_by_key(|top| top.abs_diff(target)) + .unwrap(); + assert_eq!( + got_top.abs_diff(target), + best.abs_diff(target), + "target row {target}: landed at {got_top}, nearest reachable was {best}" + ); + } + } + #[test] fn test_offset_for_thumb_top_clamps_to_max() { let state = ScrollbarState::new(200, 50, 0); diff --git a/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs b/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs index b88f74652f..4867dd9e40 100644 --- a/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs +++ b/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs @@ -7,7 +7,10 @@ //! to clicks like every other scrollbar in the editor. use crate::common::harness::EditorTestHarness; -use crossterm::event::{KeyCode, KeyModifiers}; +use crossterm::event::{KeyCode, KeyModifiers, MouseButton, MouseEvent, MouseEventKind}; + +/// The scrollbar sits on the popup's right border, i.e. the last column. +const SCROLLBAR_COL: u16 = 99; /// The popup's suggestion rows: full-width popup rows start with the `│` /// left border glyph (the top/bottom borders start with `┌`/`└`). @@ -17,6 +20,51 @@ fn suggestion_rows(harness: &EditorTestHarness, height: u16) -> Vec { .collect() } +/// Where the scrollbar thumb is drawn, as `(top, size)` in rows relative to +/// the top of the track. Read from the rendered cells only — the thumb is a +/// run of background-coloured cells inside the track. +fn thumb_span(harness: &EditorTestHarness, rows: &[u16]) -> (usize, usize) { + let thumb: Vec = rows + .iter() + .enumerate() + .filter(|(_, &y)| harness.is_scrollbar_thumb_at(SCROLLBAR_COL, y)) + .map(|(i, _)| i) + .collect(); + assert!( + !thumb.is_empty(), + "expected a scrollbar thumb on the popup border:\n{}", + harness.screen_to_string() + ); + let top = thumb[0]; + assert_eq!( + thumb.last().copied(), + Some(top + thumb.len() - 1), + "thumb rows must be contiguous, got {thumb:?}" + ); + (top, thumb.len()) +} + +/// Open a prompt whose suggestion list overflows the 10-row dropdown, and +/// return its suggestion rows. `command` is run through the palette; passing +/// `None` leaves the palette itself open. +fn open_overflowing_dropdown(harness: &mut EditorTestHarness, command: Option<&str>) -> Vec { + match command { + Some(name) => harness.run_palette_command(name).unwrap(), + None => harness + .send_key(KeyCode::Char('p'), KeyModifiers::CONTROL) + .unwrap(), + } + harness.render().unwrap(); + let rows = suggestion_rows(harness, 24); + assert_eq!( + rows.len(), + 10, + "dropdown must be full for the list to overflow:\n{}", + harness.screen_to_string() + ); + rows +} + /// Issue #1593: the command palette lists far more commands than fit, so /// every suggestion row must carry a scrollbar cell on the popup's right /// border. Before the fix that column was a plain `│` frame line, so nothing @@ -139,3 +187,114 @@ fn test_clicking_palette_scrollbar_scrolls_the_list() { harness.assert_screen_not_contains("Add Cursor Above"); } + +/// Clicking track row R must leave the thumb's top *on* row R — the thumb +/// goes where you point it. The mapping used to divide the click position by +/// the full track height instead of by the thumb's actual travel, so every +/// click landed the thumb a row (or more) above the row clicked, and the +/// bottom row of the track could never reach the end of the list. +#[test] +fn test_clicking_palette_scrollbar_lands_thumb_on_the_clicked_row() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, None); + + let (_, thumb_size) = thumb_span(&harness, &rows); + // Rows below `max_thumb_top` can't hold the thumb's top — the thumb + // would hang off the end of the track — so they clamp to the bottom. + let max_thumb_top = rows.len() - thumb_size; + + for target in 0..=max_thumb_top { + harness.mouse_click(SCROLLBAR_COL, rows[target]).unwrap(); + let (top, size) = thumb_span(&harness, &rows); + assert_eq!( + top, + target, + "clicking track row {target} put the thumb at row {top}:\n{}", + harness.screen_to_string() + ); + assert_eq!( + size, thumb_size, + "thumb size must not change while scrolling" + ); + } + + // Both extremes, explicitly: the last row of the track scrolls to the + // very end of the list, the first row back to the very start. + harness + .mouse_click(SCROLLBAR_COL, *rows.last().unwrap()) + .unwrap(); + let (top, size) = thumb_span(&harness, &rows); + assert_eq!( + top + size, + rows.len(), + "clicking the last track row must park the thumb at the bottom:\n{}", + harness.screen_to_string() + ); + harness.assert_screen_not_contains("Add Cursor Above"); + + harness.mouse_click(SCROLLBAR_COL, rows[0]).unwrap(); + assert_eq!( + thumb_span(&harness, &rows).0, + 0, + "clicking the first track row must return to the top of the list" + ); + harness.assert_screen_contains("Add Cursor Above"); +} + +/// The drag follow-up uses the same mapping as the press, so the thumb keeps +/// tracking the cursor row-for-row while the button is held. Driven as one +/// continuous gesture — press once, walk the cursor down the track and back +/// up — so it exercises the drag handler rather than a series of clicks. +#[test] +fn test_dragging_palette_scrollbar_tracks_the_cursor_row() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, None); + + let (_, thumb_size) = thumb_span(&harness, &rows); + let max_thumb_top = rows.len() - thumb_size; + + let press = |h: &mut EditorTestHarness, y: u16| { + h.send_mouse(MouseEvent { + kind: MouseEventKind::Down(MouseButton::Left), + column: SCROLLBAR_COL, + row: y, + modifiers: KeyModifiers::empty(), + }) + .unwrap(); + }; + let drag_to = |h: &mut EditorTestHarness, y: u16| { + h.send_mouse(MouseEvent { + kind: MouseEventKind::Drag(MouseButton::Left), + column: SCROLLBAR_COL, + row: y, + modifiers: KeyModifiers::empty(), + }) + .unwrap(); + h.render().unwrap(); + }; + let release = |h: &mut EditorTestHarness, y: u16| { + h.send_mouse(MouseEvent { + kind: MouseEventKind::Up(MouseButton::Left), + column: SCROLLBAR_COL, + row: y, + modifiers: KeyModifiers::empty(), + }) + .unwrap(); + h.render().unwrap(); + }; + + press(&mut harness, rows[0]); + let sweep: Vec = (0..=max_thumb_top) + .chain((0..max_thumb_top).rev()) + .collect(); + for target in sweep { + drag_to(&mut harness, rows[target]); + assert_eq!( + thumb_span(&harness, &rows).0, + target, + "dragging to track row {target} left the thumb elsewhere:\n{}", + harness.screen_to_string() + ); + } + release(&mut harness, rows[0]); +} From 7081481585274be37d6cfd4411863e2f261c1c0f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 19:35:43 +0000 Subject: [PATCH 4/4] fix: make the mouse wheel scroll prompt dropdowns without moving the selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Editor-wide rule (and what VS Code does): the wheel scrolls the view, it never moves the selection. Prompt suggestion dropdowns broke it — the command palette, Select Locale, Set Language, quick open and every other bottom-anchored list walked `selected_suggestion` on each wheel tick and rewrote the prompt input to match, so the wheel silently retargeted what Enter would commit. Once #2954 let the scrollbar pin the viewport, that also produced the jump the maintainer reported: scroll the list with the scrollbar, then wheel, and the list leapt somewhere else. The wheel cleared the manual-scroll latch (on the grounds that it had "moved the selection"), so the renderer's keep-selection-visible pass yanked the offset back to a selection that had not visibly moved. `handle_prompt_scroll` now defers to `Prompt::scroll_results`, the same view-only scroll the Live Grep overlay has used since #2119, sized by the row count the renderer reported rather than a hard-coded 10. The selection stays where it is and may scroll out of sight; keyboard navigation still clears the latch and brings it back into view, so the latch now means one thing only — "the viewport is user-positioned, stop following the selection" — rather than papering over the coupling. `Popup::scroll_by` had the same coupling for `List` popups, dragging the selection along to keep it inside the viewport on every wheel tick and scrollbar click; it scrolls the view only now too. The keyboard paths (`select_next`, `page_down`, `select_last`, …) are unchanged: they move the selection and scroll the view to follow it. Refs #623 Refs #1593 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D6SkGXXTcJsytTjF1TBf8Y --- crates/fresh-editor/src/app/mouse_input.rs | 7 +- .../fresh-editor/src/app/prompt_lifecycle.rs | 53 +++-- crates/fresh-editor/src/view/popup.rs | 23 +-- .../issue_623_prompt_dropdown_scrollbar.rs | 191 ++++++++++++++++++ 4 files changed, 228 insertions(+), 46 deletions(-) diff --git a/crates/fresh-editor/src/app/mouse_input.rs b/crates/fresh-editor/src/app/mouse_input.rs index 951768e74f..07f5b6cf70 100644 --- a/crates/fresh-editor/src/app/mouse_input.rs +++ b/crates/fresh-editor/src/app/mouse_input.rs @@ -571,7 +571,9 @@ impl Editor { // under the pointer — the preview when over it, otherwise the // result list (without moving the selection). See issue #2119. } else if self.handle_prompt_scroll(delta) { - // bottom-anchored prompt consumed the scroll (moves selection) + // bottom-anchored prompt dropdown consumed the scroll: it scrolls + // its list without moving the selection, like every other wheel + // surface in the editor. } else if self.is_file_open_active() && self.is_mouse_over_file_browser(col, row) && self.handle_file_open_scroll(delta) @@ -625,7 +627,8 @@ impl Editor { /// result list *without* moving the selection. /// /// Bottom-anchored prompts (command palette, file finder) are left to - /// `handle_prompt_scroll`, which keeps their wheel-moves-selection UX. + /// `handle_prompt_scroll`, which scrolls their dropdown the same + /// selection-preserving way. fn handle_overlay_prompt_scroll(&mut self, col: u16, row: u16, delta: i32) -> bool { if !self.overlay_prompt_active() { return false; diff --git a/crates/fresh-editor/src/app/prompt_lifecycle.rs b/crates/fresh-editor/src/app/prompt_lifecycle.rs index 1eb984e35a..67dcb6a631 100644 --- a/crates/fresh-editor/src/app/prompt_lifecycle.rs +++ b/crates/fresh-editor/src/app/prompt_lifecycle.rs @@ -15,7 +15,7 @@ use crate::input::keybindings::KeyContext; use crate::input::quick_open::{BufferInfo, QuickOpenContext}; use crate::services::async_bridge::AsyncMessage; use crate::services::plugins::PluginManager; -use crate::view::prompt::{Prompt, PromptType}; +use crate::view::prompt::{Prompt, PromptType, MAX_VISIBLE_SUGGESTIONS}; use super::file_open; use super::window::Window; @@ -967,41 +967,34 @@ impl Editor { } } - /// Handle mouse wheel scroll in prompt with suggestions. + /// Handle mouse wheel scroll over a prompt's suggestion list (command + /// palette, Select Locale, quick open, every other bottom-anchored + /// dropdown). + /// + /// The wheel scrolls the **view only** — it never moves the selection. + /// That is the editor-wide rule (and what VS Code does): the highlighted + /// entry may scroll out of sight, and pressing Enter still commits it. + /// Wheeling used to walk `selected_suggestion` instead, which also + /// rewrote the prompt input under the user and — once the scrollbar + /// could latch the offset — made the list jump, because the wheel + /// released the latch and the renderer snapped the view back to a + /// selection that had never visibly moved. + /// /// Returns true if scroll was handled, false if no prompt is active or has no suggestions. pub fn handle_prompt_scroll(&mut self, delta: i32) -> bool { + // Scroll by what the renderer actually drew (`suggestions_area` is + // `(inner_rect, scroll_start_idx, visible_count, total_count)`), so + // the offset can't run past the end of the list. Read before + // borrowing the prompt: `active_window_mut()` is a method call, so + // the compiler can't see the two are disjoint sub-fields. + let visible_rows = self.active_chrome().suggestions_area.map(|(_, _, v, _)| v); if let Some(ref mut prompt) = self.active_window_mut().prompt { if prompt.suggestions.is_empty() { return false; } - - let current = prompt.selected_suggestion.unwrap_or(0); - let len = prompt.suggestions.len(); - - // Calculate new position based on scroll direction - // delta < 0 = scroll up, delta > 0 = scroll down - let new_selected = if delta < 0 { - // Scroll up - move selection up (decrease index) - current.saturating_sub((-delta) as usize) - } else { - // Scroll down - move selection down (increase index) - (current + delta as usize).min(len.saturating_sub(1)) - }; - - prompt.selected_suggestion = Some(new_selected); - // The wheel moved the selection, so re-engage the renderer's - // keep-selection-visible behaviour (clears any latch a - // scrollbar click/drag set). - prompt.manual_scroll = false; - - // Update input to match selected suggestion for non-plugin prompts - if !matches!(prompt.prompt_type, PromptType::Plugin { .. }) { - if let Some(suggestion) = prompt.suggestions.get(new_selected) { - prompt.input = suggestion.get_value().to_string(); - prompt.cursor_pos = prompt.input.len(); - } - } - + let visible = visible_rows + .unwrap_or_else(|| prompt.suggestions.len().min(MAX_VISIBLE_SUGGESTIONS)); + prompt.scroll_results(delta, visible); return true; } false diff --git a/crates/fresh-editor/src/view/popup.rs b/crates/fresh-editor/src/view/popup.rs index 8ba3bda8a3..7b663e324d 100644 --- a/crates/fresh-editor/src/view/popup.rs +++ b/crates/fresh-editor/src/view/popup.rs @@ -586,8 +586,15 @@ impl Popup { } } - /// Scroll by a delta amount (positive = down, negative = up) - /// Used for mouse wheel scrolling + /// Scroll by a delta amount (positive = down, negative = up). + /// + /// Used for mouse-wheel scrolling and for the scrollbar handlers, so it + /// moves the **view only**: a `List` popup's selection stays on whatever + /// entry it was on, even when that entry scrolls off-screen. Dragging the + /// selection along with the viewport made the wheel silently retarget + /// what Enter would commit; the keyboard paths (`select_next`, + /// `page_down`, …) are the ones that move the selection, and they scroll + /// the view to follow it. pub fn scroll_by(&mut self, delta: i32) { let content_len = self.wrapped_item_count(); let visible = self.visible_height(); @@ -600,18 +607,6 @@ impl Popup { // Scroll down self.scroll_offset = (self.scroll_offset + delta as usize).min(max_scroll); } - - // For list popups, adjust selection to stay visible - if let PopupContent::List { items, selected } = &mut self.content { - let visible_start = self.scroll_offset; - let visible_end = (self.scroll_offset + visible).min(items.len()); - - if *selected < visible_start { - *selected = visible_start; - } else if *selected >= visible_end { - *selected = visible_end.saturating_sub(1); - } - } } /// Get the total number of items/lines in the popup diff --git a/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs b/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs index 4867dd9e40..a7134070e6 100644 --- a/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs +++ b/crates/fresh-editor/tests/e2e/issue_623_prompt_dropdown_scrollbar.rs @@ -44,6 +44,35 @@ fn thumb_span(harness: &EditorTestHarness, rows: &[u16]) -> (usize, usize) { (top, thumb.len()) } +/// A dropdown row's text, with runs of padding collapsed: the column widths +/// are computed from the *visible* entries, so scrolling reflows them and +/// only the words identify the entry. +fn entry_text(harness: &EditorTestHarness, y: u16) -> String { + harness + .get_row_text(y) + .split_whitespace() + .collect::>() + .join(" ") +} + +/// Text of the highlighted suggestion row, or `None` when the selection has +/// scrolled out of the viewport. The selection is a background colour rather +/// than a glyph, so it's found as the one row whose background differs from +/// the rest of the list. +fn selected_row_text(harness: &EditorTestHarness, rows: &[u16]) -> Option { + use std::collections::HashMap; + let bg_at = |y: u16| harness.get_cell_style(2, y).and_then(|s| s.bg); + let mut counts: HashMap<_, usize> = HashMap::new(); + for &y in rows { + *counts.entry(bg_at(y)).or_default() += 1; + } + let unselected = counts.into_iter().max_by_key(|&(_, n)| n)?.0; + rows.iter() + .copied() + .find(|&y| bg_at(y) != unselected) + .map(|y| entry_text(harness, y)) +} + /// Open a prompt whose suggestion list overflows the 10-row dropdown, and /// return its suggestion rows. `command` is run through the palette; passing /// `None` leaves the palette itself open. @@ -298,3 +327,165 @@ fn test_dragging_palette_scrollbar_tracks_the_cursor_row() { } release(&mut harness, rows[0]); } + +/// The mouse wheel scrolls the VIEW only: it must never move the selection. +/// +/// Wheeling used to walk `selected_suggestion` instead, which rewrote the +/// prompt input under the user and — once a scrollbar click could pin the +/// viewport — made the list visibly jump, because the wheel released that +/// pin and the renderer snapped back to a selection that had not moved on +/// screen. Asserted on the palette, which is the surface the report used. +#[test] +fn test_wheel_over_palette_scrolls_view_without_moving_selection() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, None); + + // Put the selection a few rows down so it stays on screen after the + // wheel — that's what makes "the same entry is still highlighted" an + // observation rather than a vacuous absence. + for _ in 0..3 { + harness.send_key(KeyCode::Down, KeyModifiers::NONE).unwrap(); + } + harness.render().unwrap(); + let selected_before = selected_row_text(&harness, &rows).expect("an entry must be highlighted"); + let first_before = entry_text(&harness, rows[0]); + + harness.mouse_scroll_down(50, rows[5]).unwrap(); + + assert_ne!( + entry_text(&harness, rows[0]), + first_before, + "the wheel must scroll the list:\n{}", + harness.screen_to_string() + ); + assert_eq!( + selected_row_text(&harness, &rows).as_deref(), + Some(selected_before.as_str()), + "the wheel must leave the selection on the same entry:\n{}", + harness.screen_to_string() + ); + + // Scrolling back restores the view, selection still untouched. + harness.mouse_scroll_up(50, rows[5]).unwrap(); + assert_eq!(entry_text(&harness, rows[0]), first_before); + assert_eq!( + selected_row_text(&harness, &rows).as_deref(), + Some(selected_before.as_str()) + ); +} + +/// Same rule on the Select Locale picker (issue #623's own dropdown): a +/// different `PromptType`, the same shared suggestions renderer. +#[test] +fn test_wheel_over_select_locale_does_not_move_selection() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, Some("Select Locale")); + + for _ in 0..2 { + harness.send_key(KeyCode::Down, KeyModifiers::NONE).unwrap(); + } + harness.render().unwrap(); + let selected_before = selected_row_text(&harness, &rows).expect("a locale must be highlighted"); + let first_before = entry_text(&harness, rows[0]); + + harness.mouse_scroll_down(50, rows[5]).unwrap(); + + assert_ne!( + entry_text(&harness, rows[0]), + first_before, + "the wheel must scroll the locale list:\n{}", + harness.screen_to_string() + ); + assert_eq!( + selected_row_text(&harness, &rows).as_deref(), + Some(selected_before.as_str()), + "the wheel must leave the selected locale alone:\n{}", + harness.screen_to_string() + ); +} + +/// And on a third prompt-driven list — Set Language, whose suggestions come +/// from the grammar catalogue rather than from the command registry. +#[test] +fn test_wheel_over_set_language_does_not_move_selection() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, Some("Set Language")); + + for _ in 0..3 { + harness.send_key(KeyCode::Down, KeyModifiers::NONE).unwrap(); + } + harness.render().unwrap(); + let selected_before = + selected_row_text(&harness, &rows).expect("a language must be highlighted"); + let first_before = entry_text(&harness, rows[0]); + + harness.mouse_scroll_down(50, rows[5]).unwrap(); + + assert_ne!( + entry_text(&harness, rows[0]), + first_before, + "the wheel must scroll the language list:\n{}", + harness.screen_to_string() + ); + assert_eq!( + selected_row_text(&harness, &rows).as_deref(), + Some(selected_before.as_str()), + "the wheel must leave the selected language alone:\n{}", + harness.screen_to_string() + ); +} + +/// The wheel may scroll the selection clean off the list — that's correct +/// (VS Code does the same), and the selection must survive it: scrolling +/// back brings the very same entry back, still highlighted. +#[test] +fn test_wheel_may_scroll_selection_out_of_view_without_losing_it() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, None); + + let selected_before = selected_row_text(&harness, &rows).expect("an entry must be highlighted"); + + // Four notches of 3 rows: the first entry is far off the top now. + for _ in 0..4 { + harness.mouse_scroll_down(50, rows[5]).unwrap(); + } + assert_eq!( + selected_row_text(&harness, &rows), + None, + "the selection scrolled out of view, so no row is highlighted:\n{}", + harness.screen_to_string() + ); + + for _ in 0..4 { + harness.mouse_scroll_up(50, rows[5]).unwrap(); + } + assert_eq!( + selected_row_text(&harness, &rows).as_deref(), + Some(selected_before.as_str()), + "scrolling back must reveal the same selected entry:\n{}", + harness.screen_to_string() + ); +} + +/// Keyboard navigation still re-engages keep-the-selection-visible +/// scrolling: after wheeling the selection off screen, an arrow key brings +/// the view back to it. This is what keeps the manual-scroll latch honest +/// now that the wheel no longer touches the selection. +#[test] +fn test_arrow_key_after_wheel_brings_the_selection_back_into_view() { + let mut harness = EditorTestHarness::new(100, 24).unwrap(); + let rows = open_overflowing_dropdown(&mut harness, None); + + for _ in 0..4 { + harness.mouse_scroll_down(50, rows[5]).unwrap(); + } + assert_eq!(selected_row_text(&harness, &rows), None); + + harness.send_key(KeyCode::Down, KeyModifiers::NONE).unwrap(); + harness.render().unwrap(); + assert!( + selected_row_text(&harness, &rows).is_some(), + "an arrow key must scroll the selection back into view:\n{}", + harness.screen_to_string() + ); +}