From 108fd3ac11c600ce04f01a3dbd9c73d24eb631e9 Mon Sep 17 00:00:00 2001 From: rn23thakur Date: Fri, 18 Sep 2026 11:24:01 +0000 Subject: [PATCH] editor: Fix vertical scroll margin calculation (#63944) # Objective - Fixes an issue where setting an arbitrarily high `Vertical Scroll Margin` fails to perfectly lock the cursor in the center of the viewport (Typewriter Mode?). In certain cases one could go up/down a line before auto-scroll gets triggered, deviating from user intent. - Fixes #63862 ## Solution - **The Issue:** When `Vertical Scroll Margin` is very high, the effective margin applied above and below the cursor is derived from the viewport's free space: ```rust ((visible_lines - (target_bottom - target_top)) / 2.0).floor() ``` - **The Fix:** Instead of using the same rounded number for both the top and bottom margins, calculate them separately: 1. The top margin stays the same as before. 2. The bottom margin is calculated by subtracting the top margin from the total free screen space. - This ensures the bottom margin automatically catches whatever fraction the floor operation on the top margin drops, so the two margins always sum to the full available space. ## Testing - Did you test these changes? If so, how? Ran `cargo test -p editor`, all passed. I've also attached the results of manual testing in the Showcase section below. ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([[UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Showcase Before and after screen recording
Click to view showcase **Before:** https://private-user-images.githubusercontent.com/63847494/646999418-4dd388b8-4aba-4004-ac4f-6f1e0dad0f70.mp4?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3ODg5MTE3MTUsIm5iZiI6MTc4ODkxMTQxNSwicGF0aCI6Ii82Mzg0NzQ5NC82NDY5OTk0MTgtNGRkMzg4YjgtNGFiYS00MDA0LWFjNGYtNmYxZTBkYWQwZjcwLm1wND9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNjA5MDglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjYwOTA4VDIzNTAxNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWRhZGM5MWExMDkyMDIzZjVlYjcxYmQ4YTRmOTM3MDliNDcyOTUzZjFhYmY1ZTgwMDY0YzhlYmQ1NzllYWYyN2EmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JnJlc3BvbnNlLWNvbnRlbnQtdHlwZT12aWRlbyUyRm1wNCJ9.DG6RpiEM-781ZctlvtQeWhw3kAYJ4KS6rNsAmMU38fg **After:** https://github.com/user-attachments/assets/4bf870e0-84ba-449a-bf2a-925c4a90d5ed
Release Notes: - Fixed the cursor not staying perfectly centered when using a high Vertical Scroll Margin (Typewriter Mode). --------- Co-authored-by: Smit Barmase --- crates/editor/src/editor_tests.rs | 198 +++++++++++++++++++++++++ crates/editor/src/scroll/autoscroll.rs | 33 ++++- 2 files changed, 228 insertions(+), 3 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 7fe1e07193..fb19c9cfc5 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -3756,6 +3756,115 @@ async fn test_autoscroll(cx: &mut TestAppContext) { }); } +#[track_caller] +fn assert_cursor_row_and_scroll_top( + cx: &mut EditorTestContext, + expected_cursor_row: u32, + expected_scroll_top: f64, +) { + cx.update_editor(|editor, window, cx| { + let snapshot = editor.snapshot(window, cx); + let cursor_row = editor + .selections + .newest_display(&snapshot.display_snapshot) + .head() + .row(); + + assert_eq!(cursor_row, DisplayRow(expected_cursor_row)); + assert_eq!(snapshot.scroll_position().y, expected_scroll_top); + }); +} + +#[gpui::test] +async fn test_autoscroll_high_margin_keeps_cursor_centered(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + let mut cx = EditorTestContext::new(cx).await; + + let line_height = cx.update_editor(|editor, window, cx| { + // Let viewport space, rather than the configured margin, limit the padding. + editor.set_vertical_scroll_margin(999, cx); + editor + .style(cx) + .text + .line_height_in_pixels(window.rem_size()) + }); + let window = cx.window; + + for autoscroll in [Autoscroll::fit(), Autoscroll::newest()] { + // After allowing for the cursor row, the free space is odd, fractional, or even. + for (visible_lines, expected_top_margin) in [(6., 2.), (6.5, 2.), (7., 3.)] { + cx.simulate_window_resize(window, size(px(1000.), visible_lines * line_height)); + + // Start at zero-based row 6, away from file boundaries that could clamp scrolling. + cx.set_state("0\n1\n2\n3\n4\n5\nˇ6\n7\n8\n9\n10\n11\n12\n13\n14"); + cx.update_editor(|editor, window, cx| { + editor.set_scroll_position(gpui::Point::new(0., 0.), window, cx); + editor.request_autoscroll(autoscroll, cx); + }); + cx.run_until_parked(); + + // The cursor's viewport offset is its row minus the scroll top. + assert_cursor_row_and_scroll_top(&mut cx, 6, 6. - expected_top_margin); + + // Moving down to row 7 must scroll with the cursor, preserving that offset. + cx.update_editor(|editor, window, cx| { + editor.move_down(&MoveDown, window, cx); + editor.request_autoscroll(autoscroll, cx); + }); + cx.run_until_parked(); + assert_cursor_row_and_scroll_top(&mut cx, 7, 7. - expected_top_margin); + + // Returning to row 6 must restore the viewport, not leave room for cursor drift. + cx.update_editor(|editor, window, cx| { + editor.move_up(&MoveUp, window, cx); + editor.request_autoscroll(autoscroll, cx); + }); + cx.run_until_parked(); + assert_cursor_row_and_scroll_top(&mut cx, 6, 6. - expected_top_margin); + } + } +} + +#[gpui::test] +async fn test_autoscroll_auto_height_has_no_margin(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + let mut cx = EditorTestContext::new(cx).await; + + let line_height = cx.update_editor(|editor, window, cx| { + editor.set_mode(EditorMode::AutoHeight { + min_lines: 1, + max_lines: Some(4), + }); + + // Auto-height inputs must ignore even a nonzero configured margin. + editor.set_vertical_scroll_margin(3, cx); + editor + .style(cx) + .text + .line_height_in_pixels(window.rem_size()) + }); + let window = cx.window; + cx.simulate_window_resize(window, size(px(1000.), 4. * line_height)); + cx.set_state("ˇ0\n1\n2\n3\n4\n5\n6\n7\n8\n9"); + cx.update_editor(|editor, _, _| { + assert_eq!(editor.visible_line_count(), Some(4.)); + }); + + // Row 2 is already visible, so moving there must not reserve space below it. + cx.update_editor(|editor, window, cx| { + editor.move_down_by_lines(&MoveDownByLines { lines: 2 }, window, cx); + }); + cx.run_until_parked(); + assert_cursor_row_and_scroll_top(&mut cx, 2, 0.); + + // Row 4 is just outside the viewport: scroll only far enough to reveal it. + cx.update_editor(|editor, window, cx| { + editor.move_down_by_lines(&MoveDownByLines { lines: 2 }, window, cx); + }); + cx.run_until_parked(); + assert_cursor_row_and_scroll_top(&mut cx, 4, 1.); +} + #[gpui::test] async fn test_cursor_animation_remains_active_during_keyboard_autoscroll(cx: &mut TestAppContext) { init_test(cx, |_| {}); @@ -43705,6 +43814,95 @@ async fn test_no_duplicated_sticky_headers(cx: &mut TestAppContext) { assert_eq!(sticky_headers(5.0), vec![]); } +#[gpui::test] +async fn test_autoscroll_margin_reserves_sticky_header_space(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + update_test_editor_settings(cx, &|settings| { + settings.vertical_scroll_margin = Some(3.0); + settings.sticky_scroll = Some(settings::StickyScrollContent { + enabled: Some(true), + }); + }); + + let mut cx = EditorTestContext::new(cx).await; + cx.set_state(indoc! {" + fn demo() { + let value_01 = 1; + let value_02 = 2; + let value_03 = 3; + let value_04 = 4; + let value_05 = 5; + let value_06 = 6; + let value_07 = 7; + let value_08 = 8; + let value_09 = 9; + let value_10 = 10; + ˇlet value_11 = 11; + let value_12 = 12; + let value_13 = 13; + } + "}); + + // Use a parsed scope so the test also exercises sticky-header discovery. + cx.update_buffer(|buffer, cx| buffer.set_language(Some(rust_lang()), cx)); + cx.run_until_parked(); + + let line_height = cx.update_editor(|editor, window, cx| { + editor + .style(cx) + .text + .line_height_in_pixels(window.rem_size()) + }); + let window = cx.window; + + for visible_lines in [2., 2.5] { + cx.simulate_window_resize(window, size(px(1000.), visible_lines * line_height)); + + // Center explicitly so the starting viewport does not depend on the + // Fit calculation being tested. + cx.update_editor(|editor, window, cx| { + editor.change_selections(SelectionEffects::no_scroll(), window, cx, |selections| { + selections.select_ranges([Point::new(11, 4)..Point::new(11, 4)]); + }); + editor.scroll_cursor_center(&ScrollCursorCenter, window, cx); + }); + cx.run_until_parked(); + + cx.update_editor(|editor, window, cx| { + let snapshot = editor.snapshot(window, cx); + assert_eq!(snapshot.scroll_position().y, 10.); + assert_eq!(EditorElement::sticky_headers(editor, &snapshot).len(), 1); + + // Row 10 is covered by the header; moving there must scroll to expose it. + editor.move_up(&MoveUp, window, cx); + }); + cx.run_until_parked(); + + cx.update_editor(|editor, window, cx| { + let snapshot = editor.snapshot(window, cx); + let cursor_row = editor + .selections + .newest_display(&snapshot.display_snapshot) + .head() + .row(); + let sticky_header_count = EditorElement::sticky_headers(editor, &snapshot).len(); + + assert_eq!(cursor_row, DisplayRow(10)); + assert_eq!(snapshot.scroll_position().y, 9.); + assert_eq!(sticky_header_count, 1); + + // A scroll alone is insufficient: the full cursor row must be unobscured. + assert!( + cursor_row.as_f64() >= snapshot.scroll_position().y + sticky_header_count as f64 + ); + assert!( + cursor_row.next_row().as_f64() + <= snapshot.scroll_position().y + visible_lines as f64 + ); + }); + } +} + #[gpui::test] async fn test_autoscroll_keeps_cursor_visible_below_sticky_headers(cx: &mut TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/editor/src/scroll/autoscroll.rs b/crates/editor/src/scroll/autoscroll.rs index 74cea8461e..3efc0b3d77 100644 --- a/crates/editor/src/scroll/autoscroll.rs +++ b/crates/editor/src/scroll/autoscroll.rs @@ -235,9 +235,14 @@ impl Editor { let was_autoscrolled = match strategy { AutoscrollStrategy::Fit | AutoscrollStrategy::Newest => { - let margin = margin.min(self.scroll_manager.vertical_scroll_margin); - let target_top = (target_top - margin - visible_sticky_headers as f64).max(0.0); - let target_bottom = target_bottom + margin; + let (margin_top, margin_bottom) = self.fit_autoscroll_margins( + visible_lines, + target_bottom - target_top, + visible_sticky_headers, + ); + let target_top = (target_top - margin_top - visible_sticky_headers as f64).max(0.0); + let target_bottom = target_bottom + margin_bottom; + let start_row = scroll_position.y; let end_row = start_row + visible_lines; @@ -294,6 +299,28 @@ impl Editor { (NeedsHorizontalAutoscroll(true), was_scrolled) } + fn fit_autoscroll_margins( + &self, + visible_lines: ScrollOffset, + target_height: ScrollOffset, + visible_sticky_headers: usize, + ) -> (ScrollOffset, ScrollOffset) { + if matches!(self.mode, EditorMode::AutoHeight { .. }) { + return (0., 0.); + } + + let available_margin_space = + (visible_lines - target_height - visible_sticky_headers as f64).max(0.); + let top_space = (available_margin_space / 2.).floor(); + + // Keep the rounding remainder below the target so large margins leave no + // slack for the cursor to move without scrolling. + ( + top_space.min(self.scroll_manager.vertical_scroll_margin), + (available_margin_space - top_space).min(self.scroll_manager.vertical_scroll_margin), + ) + } + pub(crate) fn visible_sticky_header_count_for_point( &self, display_map: &DisplaySnapshot,