From 82a7aca5a6e81f6542b67c3cfc2444c958e7e827 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Sun, 11 May 2025 21:40:45 +0200 Subject: [PATCH] ui: Account for padding of parent container during scrollbar layout (#27402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes https://github.com/zed-industries/zed/issues/23386 This PR updates the scrollbar-component to account for padding present in the parent container. Since the linked issue was opened, https://github.com/zed-industries/zed/pull/25288 improved the behaviour so that the scrollbar does allow scrolling the entire container, however the scrollbar thumb still does not go the entire way to the bottom. This can be seen here: https://github.com/user-attachments/assets/89204355-e6b8-428b-9fa9-bb614051b6fa This happens because during layouting of the scrollbar, padding of the parent container is not taken into account. The scrollbar thumb size is calculated as if no padding was present. With this change, padding is now included in the calculation, which resolves the issue: https://github.com/user-attachments/assets/1d4c62e0-4555-4332-a9ab-4e114684b4b3 The change here is to store the calculated content size during prepaint _including_ padding and use this for layouting the scrollbar. This ensures that the actual scroll max and the content size are always in sync. Furthermore, the existing `TODO`-comment is also resolved, as we now no longer look at the size of the last child but the actual parent size instead. This also removes an existing panic of the scrollbar-component in cases where the content size was 0, which was previously not accounted for (this never happened in practice so far, for example because of the padding added here: https://github.com/zed-industries/zed/blob/43712285bfa8aa75bac1ca8a7eb5146dd36239be/crates/editor/src/hover_popover.rs#L802-L809 which prevented the container size from ever being 0). --- Lastly, as I was wiring through the changes of the `content_size` I noticed that some code was duplicated during the initial layouting as well as in the click handlers. I refactored this in the second commit to use `along` where possible as well as computing the new click offset in one closure which can be passed to both event listeners. As always, should any of these changes not be wanted, feel free to let me know and I will revert these. Looking forward to your feedback 😄 Release Notes: - Fixed scrollbars sometimes not scrolling all the way to the bottom. --- crates/gpui/src/elements/div.rs | 44 ++- .../terminal_view/src/terminal_scrollbar.rs | 11 +- crates/ui/src/components/scrollbar.rs | 269 +++++++----------- crates/workspace/src/pane.rs | 6 +- 4 files changed, 126 insertions(+), 204 deletions(-) diff --git a/crates/gpui/src/elements/div.rs b/crates/gpui/src/elements/div.rs index e851c76a5bd8705babebc33fc4e59d287edefec3..bc2abc7c46b886c9ff46a9cceb9d4d8f75406b0e 100644 --- a/crates/gpui/src/elements/div.rs +++ b/crates/gpui/src/elements/div.rs @@ -1559,32 +1559,20 @@ impl Interactivity { ) -> Point { if let Some(scroll_offset) = self.scroll_offset.as_ref() { let mut scroll_to_bottom = false; - if let Some(scroll_handle) = &self.tracked_scroll_handle { - let mut state = scroll_handle.0.borrow_mut(); - state.overflow = style.overflow; - scroll_to_bottom = mem::take(&mut state.scroll_to_bottom); + let mut tracked_scroll_handle = self + .tracked_scroll_handle + .as_ref() + .map(|handle| handle.0.borrow_mut()); + if let Some(mut scroll_handle_state) = tracked_scroll_handle.as_deref_mut() { + scroll_handle_state.overflow = style.overflow; + scroll_to_bottom = mem::take(&mut scroll_handle_state.scroll_to_bottom); } let rem_size = window.rem_size(); - let padding_size = size( - style - .padding - .left - .to_pixels(bounds.size.width.into(), rem_size) - + style - .padding - .right - .to_pixels(bounds.size.width.into(), rem_size), - style - .padding - .top - .to_pixels(bounds.size.height.into(), rem_size) - + style - .padding - .bottom - .to_pixels(bounds.size.height.into(), rem_size), - ); - let scroll_max = (self.content_size + padding_size - bounds.size).max(&Size::default()); + let padding = style.padding.to_pixels(bounds.size.into(), rem_size); + let padding_size = size(padding.left + padding.right, padding.top + padding.bottom); + let padded_content_size = self.content_size + padding_size; + let scroll_max = (padded_content_size - bounds.size).max(&Size::default()); // Clamp scroll offset in case scroll max is smaller now (e.g., if children // were removed or the bounds became larger). let mut scroll_offset = scroll_offset.borrow_mut(); @@ -1596,6 +1584,10 @@ impl Interactivity { scroll_offset.y = scroll_offset.y.clamp(-scroll_max.height, px(0.)); } + if let Some(mut scroll_handle_state) = tracked_scroll_handle { + scroll_handle_state.padded_content_size = padded_content_size; + } + *scroll_offset } else { Point::default() @@ -2913,6 +2905,7 @@ impl ScrollAnchor { struct ScrollHandleState { offset: Rc>>, bounds: Bounds, + padded_content_size: Size, child_bounds: Vec>, scroll_to_bottom: bool, overflow: Point, @@ -2975,6 +2968,11 @@ impl ScrollHandle { self.0.borrow().child_bounds.get(ix).cloned() } + /// Get the size of the content with padding of the container. + pub fn padded_content_size(&self) -> Size { + self.0.borrow().padded_content_size + } + /// scroll_to_item scrolls the minimal amount to ensure that the child is /// fully visible pub fn scroll_to_item(&self, ix: usize) { diff --git a/crates/terminal_view/src/terminal_scrollbar.rs b/crates/terminal_view/src/terminal_scrollbar.rs index 5f5546aec0c78b41a06c36d69375a5d96b03d20d..18e135be2eef3b8e7ec71c070f2a60a46792a271 100644 --- a/crates/terminal_view/src/terminal_scrollbar.rs +++ b/crates/terminal_view/src/terminal_scrollbar.rs @@ -3,9 +3,9 @@ use std::{ rc::Rc, }; -use gpui::{Bounds, Point, size}; +use gpui::{Bounds, Point, Size, size}; use terminal::Terminal; -use ui::{ContentSize, Pixels, ScrollableHandle, px}; +use ui::{Pixels, ScrollableHandle, px}; #[derive(Debug)] struct ScrollHandleState { @@ -46,12 +46,9 @@ impl TerminalScrollHandle { } impl ScrollableHandle for TerminalScrollHandle { - fn content_size(&self) -> Option { + fn content_size(&self) -> Size { let state = self.state.borrow(); - Some(ContentSize { - size: size(px(0.), px(state.total_lines as f32 * state.line_height.0)), - scroll_adjustment: Some(Point::new(px(0.), px(0.))), - }) + size(Pixels::ZERO, state.total_lines as f32 * state.line_height) } fn offset(&self) -> Point { diff --git a/crates/ui/src/components/scrollbar.rs b/crates/ui/src/components/scrollbar.rs index 255b5e57728947c94465b8f8a06dd9520be2e8cf..878732140994cf95116a3c514a24699f78746a4a 100644 --- a/crates/ui/src/components/scrollbar.rs +++ b/crates/ui/src/components/scrollbar.rs @@ -3,9 +3,9 @@ use std::{any::Any, cell::Cell, fmt::Debug, ops::Range, rc::Rc, sync::Arc}; use crate::{IntoElement, prelude::*, px, relative}; use gpui::{ Along, App, Axis as ScrollbarAxis, BorderStyle, Bounds, ContentMask, Corners, Edges, Element, - ElementId, Entity, EntityId, GlobalElementId, Hitbox, Hsla, LayoutId, ListState, + ElementId, Entity, EntityId, GlobalElementId, Hitbox, Hsla, IsZero, LayoutId, ListState, MouseDownEvent, MouseMoveEvent, MouseUpEvent, Pixels, Point, ScrollHandle, ScrollWheelEvent, - Size, Style, UniformListScrollHandle, Window, point, quad, + Size, Style, UniformListScrollHandle, Window, quad, }; pub struct Scrollbar { @@ -15,11 +15,8 @@ pub struct Scrollbar { } impl ScrollableHandle for UniformListScrollHandle { - fn content_size(&self) -> Option { - Some(ContentSize { - size: self.0.borrow().last_item_size.map(|size| size.contents)?, - scroll_adjustment: None, - }) + fn content_size(&self) -> Size { + self.0.borrow().base_handle.content_size() } fn set_offset(&self, point: Point) { @@ -36,11 +33,8 @@ impl ScrollableHandle for UniformListScrollHandle { } impl ScrollableHandle for ListState { - fn content_size(&self) -> Option { - Some(ContentSize { - size: self.content_size_for_scrollbar(), - scroll_adjustment: None, - }) + fn content_size(&self) -> Size { + self.content_size_for_scrollbar() } fn set_offset(&self, point: Point) { @@ -65,27 +59,8 @@ impl ScrollableHandle for ListState { } impl ScrollableHandle for ScrollHandle { - fn content_size(&self) -> Option { - let last_children_index = self.children_count().checked_sub(1)?; - - let mut last_item = self.bounds_for_item(last_children_index)?; - let mut scroll_adjustment = None; - - if last_children_index != 0 { - // todo: PO: this is slightly wrong for horizontal scrollbar, as the last item is not necessarily the longest one. - let first_item = self.bounds_for_item(0)?; - last_item.size.height += last_item.origin.y; - last_item.size.width += last_item.origin.x; - - scroll_adjustment = Some(first_item.origin); - last_item.size.height -= first_item.origin.y; - last_item.size.width -= first_item.origin.x; - } - - Some(ContentSize { - size: last_item.size, - scroll_adjustment, - }) + fn content_size(&self) -> Size { + self.padded_content_size() } fn set_offset(&self, point: Point) { @@ -101,14 +76,8 @@ impl ScrollableHandle for ScrollHandle { } } -#[derive(Debug)] -pub struct ContentSize { - pub size: Size, - pub scroll_adjustment: Option>, -} - pub trait ScrollableHandle: Any + Debug { - fn content_size(&self) -> Option; + fn content_size(&self) -> Size; fn set_offset(&self, point: Point); fn offset(&self) -> Point; fn viewport(&self) -> Bounds; @@ -149,30 +118,26 @@ impl ScrollbarState { } fn thumb_range(&self, axis: ScrollbarAxis) -> Option> { - const MINIMUM_THUMB_SIZE: f32 = 25.; - let ContentSize { - size: main_dimension_size, - scroll_adjustment, - } = self.scroll_handle.content_size()?; - let content_size = main_dimension_size.along(axis).0; - let mut current_offset = self.scroll_handle.offset().along(axis).min(px(0.)).abs().0; - if let Some(adjustment) = scroll_adjustment.and_then(|adjustment| { - let adjust = adjustment.along(axis).0; - if adjust < 0.0 { Some(adjust) } else { None } - }) { - current_offset -= adjustment; - } - let viewport_size = self.scroll_handle.viewport().size.along(axis).0; - if content_size < viewport_size { + const MINIMUM_THUMB_SIZE: Pixels = px(25.); + let content_size = self.scroll_handle.content_size().along(axis); + let viewport_size = self.scroll_handle.viewport().size.along(axis); + if content_size.is_zero() || viewport_size.is_zero() || content_size < viewport_size { return None; } + + let max_offset = content_size - viewport_size; + let current_offset = self + .scroll_handle + .offset() + .along(axis) + .clamp(-max_offset, Pixels::ZERO) + .abs(); + let visible_percentage = viewport_size / content_size; let thumb_size = MINIMUM_THUMB_SIZE.max(viewport_size * visible_percentage); if thumb_size > viewport_size { return None; } - let max_offset = content_size - viewport_size; - current_offset = current_offset.clamp(0., max_offset); let start_offset = (current_offset / max_offset) * (viewport_size - thumb_size); let thumb_percentage_start = start_offset / viewport_size; let thumb_percentage_end = (start_offset + thumb_size) / viewport_size; @@ -247,57 +212,38 @@ impl Element for Scrollbar { window: &mut Window, cx: &mut App, ) { + const EXTRA_PADDING: Pixels = px(5.0); window.with_content_mask(Some(ContentMask { bounds }), |window| { + let axis = self.kind; let colors = cx.theme().colors(); let thumb_background = colors .surface_background .blend(colors.scrollbar_thumb_background); - let is_vertical = self.kind == ScrollbarAxis::Vertical; - let extra_padding = px(5.0); - let padded_bounds = if is_vertical { - Bounds::from_corners( - bounds.origin + point(Pixels::ZERO, extra_padding), - bounds.bottom_right() - point(Pixels::ZERO, extra_padding * 3), - ) - } else { - Bounds::from_corners( - bounds.origin + point(extra_padding, Pixels::ZERO), - bounds.bottom_right() - point(extra_padding * 3, Pixels::ZERO), - ) - }; - - let mut thumb_bounds = if is_vertical { - let thumb_offset = self.thumb.start * padded_bounds.size.height; - let thumb_end = self.thumb.end * padded_bounds.size.height; - let thumb_upper_left = point( - padded_bounds.origin.x, - padded_bounds.origin.y + thumb_offset, - ); - let thumb_lower_right = point( - padded_bounds.origin.x + padded_bounds.size.width, - padded_bounds.origin.y + thumb_end, - ); - Bounds::from_corners(thumb_upper_left, thumb_lower_right) - } else { - let thumb_offset = self.thumb.start * padded_bounds.size.width; - let thumb_end = self.thumb.end * padded_bounds.size.width; - let thumb_upper_left = point( - padded_bounds.origin.x + thumb_offset, - padded_bounds.origin.y, - ); - let thumb_lower_right = point( - padded_bounds.origin.x + thumb_end, - padded_bounds.origin.y + padded_bounds.size.height, - ); - Bounds::from_corners(thumb_upper_left, thumb_lower_right) - }; - let corners = if is_vertical { - thumb_bounds.size.width /= 1.5; - Corners::all(thumb_bounds.size.width / 2.0) - } else { - thumb_bounds.size.height /= 1.5; - Corners::all(thumb_bounds.size.height / 2.0) - }; + + let padded_bounds = Bounds::from_corners( + bounds + .origin + .apply_along(axis, |origin| origin + EXTRA_PADDING), + bounds + .bottom_right() + .apply_along(axis, |track_end| track_end - 3.0 * EXTRA_PADDING), + ); + + let thumb_offset = self.thumb.start * padded_bounds.size.along(axis); + let thumb_end = self.thumb.end * padded_bounds.size.along(axis); + + let thumb_bounds = Bounds::new( + padded_bounds + .origin + .apply_along(axis, |origin| origin + thumb_offset), + padded_bounds + .size + .apply_along(axis, |_| thumb_end - thumb_offset) + .apply_along(axis.invert(), |width| width / 1.5), + ); + + let corners = Corners::all(thumb_bounds.size.along(axis.invert()) / 2.0); + window.paint_quad(quad( thumb_bounds, corners, @@ -308,7 +254,39 @@ impl Element for Scrollbar { )); let scroll = self.state.scroll_handle.clone(); - let axis = self.kind; + + enum ScrollbarMouseEvent { + GutterClick, + ThumbDrag(Pixels), + } + + let compute_click_offset = + move |event_position: Point, + item_size: Size, + event_type: ScrollbarMouseEvent| { + let viewport_size = padded_bounds.size.along(axis); + + let thumb_size = thumb_bounds.size.along(axis); + + let thumb_offset = match event_type { + ScrollbarMouseEvent::GutterClick => thumb_size / 2., + ScrollbarMouseEvent::ThumbDrag(thumb_offset) => thumb_offset, + }; + + let thumb_start = (event_position.along(axis) + - padded_bounds.origin.along(axis) + - thumb_offset) + .clamp(px(0.), viewport_size - thumb_size); + + let max_offset = (item_size.along(axis) - viewport_size).max(px(0.)); + let percentage = if viewport_size > thumb_size { + thumb_start / (viewport_size - thumb_size) + } else { + 0. + }; + + -max_offset * percentage + }; window.on_mouse_event({ let scroll = scroll.clone(); @@ -323,39 +301,17 @@ impl Element for Scrollbar { if thumb_bounds.contains(&event.position) { let offset = event.position.along(axis) - thumb_bounds.origin.along(axis); state.drag.set(Some(offset)); - } else if let Some(ContentSize { - size: item_size, .. - }) = scroll.content_size() - { - let click_offset = { - let viewport_size = padded_bounds.size.along(axis); - - let thumb_size = thumb_bounds.size.along(axis); - let thumb_start = (event.position.along(axis) - - padded_bounds.origin.along(axis) - - (thumb_size / 2.)) - .clamp(px(0.), viewport_size - thumb_size); - - let max_offset = (item_size.along(axis) - viewport_size).max(px(0.)); - let percentage = if viewport_size > thumb_size { - thumb_start / (viewport_size - thumb_size) - } else { - 0. - }; - - -max_offset * percentage - }; - match axis { - ScrollbarAxis::Horizontal => { - scroll.set_offset(point(click_offset, scroll.offset().y)); - } - ScrollbarAxis::Vertical => { - scroll.set_offset(point(scroll.offset().x, click_offset)); - } - } + } else { + let click_offset = compute_click_offset( + event.position, + scroll.content_size(), + ScrollbarMouseEvent::GutterClick, + ); + scroll.set_offset(scroll.offset().apply_along(axis, |_| click_offset)); } } }); + window.on_mouse_event({ let scroll = scroll.clone(); move |event: &ScrollWheelEvent, phase, window, _| { @@ -367,44 +323,19 @@ impl Element for Scrollbar { } } }); + let state = self.state.clone(); - let axis = self.kind; window.on_mouse_event(move |event: &MouseMoveEvent, _, window, cx| { if let Some(drag_state) = state.drag.get().filter(|_| event.dragging()) { - if let Some(ContentSize { - size: item_size, .. - }) = scroll.content_size() - { - let drag_offset = { - let viewport_size = padded_bounds.size.along(axis); - - let thumb_size = thumb_bounds.size.along(axis); - let thumb_start = (event.position.along(axis) - - padded_bounds.origin.along(axis) - - drag_state) - .clamp(px(0.), viewport_size - thumb_size); - - let max_offset = (item_size.along(axis) - viewport_size).max(px(0.)); - let percentage = if viewport_size > thumb_size { - thumb_start / (viewport_size - thumb_size) - } else { - 0. - }; - - -max_offset * percentage - }; - match axis { - ScrollbarAxis::Horizontal => { - scroll.set_offset(point(drag_offset, scroll.offset().y)); - } - ScrollbarAxis::Vertical => { - scroll.set_offset(point(scroll.offset().x, drag_offset)); - } - }; - window.refresh(); - if let Some(id) = state.parent_id { - cx.notify(id); - } + let drag_offset = compute_click_offset( + event.position, + scroll.content_size(), + ScrollbarMouseEvent::ThumbDrag(drag_state), + ); + scroll.set_offset(scroll.offset().apply_along(axis, |_| drag_offset)); + window.refresh(); + if let Some(id) = state.parent_id { + cx.notify(id); } } else { state.drag.set(None); diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index 543ec5186b1b4b63633349e91d61f57b32a532ab..6074ebfee9067ad3a501e07ba585992bb9f83e39 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -2671,11 +2671,7 @@ impl Pane { } }) .children(pinned_tabs.len().ne(&0).then(|| { - let content_width = self - .tab_bar_scroll_handle - .content_size() - .map(|content_size| content_size.size.width) - .unwrap_or(px(0.)); + let content_width = self.tab_bar_scroll_handle.content_size().width; let viewport_width = self.tab_bar_scroll_handle.viewport().size.width; // We need to check both because offset returns delta values even when the scroll handle is not scrollable let is_scrollable = content_width > viewport_width;