gpui: Round scroll_max
to two decimal places (#34832)
Follow up to #31836 After enabling rounding in the Taffy layout engine, we frequently run into cases where the bounds produced by Taffy and ours slightly differ after 5 or more decimal places. This leads to cases where containers become scrollable for less than 0.0000x Pixels. In case this happens for e.g. hover popovers, we render a scrollbar due to the container being technically scrollable, even though the scroll amount here will in practice never be visible. This change fixes this by rounding the `scroll_max` by which we clamp the current scroll position to two decimal places. We don't benefit from the additional floating point precision here at all and it stops such containers from becoming scrollable altogether. Furthermore, we now store the `scroll_max` instead of the `padded_content_size` as the former gives a much better idea on whether the corresponding container is scrollable or not. | `main` | After these changes | | -- | -- | | <img width="610" height="316" alt="main" src="https://github.com/user-attachments/assets/ffcc0322-6d6e-4f79-a916-bd3c57fe4211" /> | <img width="610" height="316" alt="scroll_max_rounded" src="https://github.com/user-attachments/assets/5fe530f5-2e21-4aaa-81f4-e5c53ab73e4f" /> | Release Notes: - Fixed an issue where scrollbars would appear in containers where no scrolling was possible.
This commit is contained in:
parent
8eca7f32e2
commit
5b3e371812
5 changed files with 55 additions and 33 deletions
|
@ -1664,6 +1664,11 @@ impl Interactivity {
|
||||||
window: &mut Window,
|
window: &mut Window,
|
||||||
_cx: &mut App,
|
_cx: &mut App,
|
||||||
) -> Point<Pixels> {
|
) -> Point<Pixels> {
|
||||||
|
fn round_to_two_decimals(pixels: Pixels) -> Pixels {
|
||||||
|
const ROUNDING_FACTOR: f32 = 100.0;
|
||||||
|
(pixels * ROUNDING_FACTOR).round() / ROUNDING_FACTOR
|
||||||
|
}
|
||||||
|
|
||||||
if let Some(scroll_offset) = self.scroll_offset.as_ref() {
|
if let Some(scroll_offset) = self.scroll_offset.as_ref() {
|
||||||
let mut scroll_to_bottom = false;
|
let mut scroll_to_bottom = false;
|
||||||
let mut tracked_scroll_handle = self
|
let mut tracked_scroll_handle = self
|
||||||
|
@ -1678,8 +1683,16 @@ impl Interactivity {
|
||||||
let rem_size = window.rem_size();
|
let rem_size = window.rem_size();
|
||||||
let padding = style.padding.to_pixels(bounds.size.into(), rem_size);
|
let padding = style.padding.to_pixels(bounds.size.into(), rem_size);
|
||||||
let padding_size = size(padding.left + padding.right, padding.top + padding.bottom);
|
let padding_size = size(padding.left + padding.right, padding.top + padding.bottom);
|
||||||
|
// The floating point values produced by Taffy and ours often vary
|
||||||
|
// slightly after ~5 decimal places. This can lead to cases where after
|
||||||
|
// subtracting these, the container becomes scrollable for less than
|
||||||
|
// 0.00000x pixels. As we generally don't benefit from a precision that
|
||||||
|
// high for the maximum scroll, we round the scroll max to 2 decimal
|
||||||
|
// places here.
|
||||||
let padded_content_size = self.content_size + padding_size;
|
let padded_content_size = self.content_size + padding_size;
|
||||||
let scroll_max = (padded_content_size - bounds.size).max(&Size::default());
|
let scroll_max = (padded_content_size - bounds.size)
|
||||||
|
.map(round_to_two_decimals)
|
||||||
|
.max(&Default::default());
|
||||||
// Clamp scroll offset in case scroll max is smaller now (e.g., if children
|
// Clamp scroll offset in case scroll max is smaller now (e.g., if children
|
||||||
// were removed or the bounds became larger).
|
// were removed or the bounds became larger).
|
||||||
let mut scroll_offset = scroll_offset.borrow_mut();
|
let mut scroll_offset = scroll_offset.borrow_mut();
|
||||||
|
@ -1692,7 +1705,7 @@ impl Interactivity {
|
||||||
}
|
}
|
||||||
|
|
||||||
if let Some(mut scroll_handle_state) = tracked_scroll_handle {
|
if let Some(mut scroll_handle_state) = tracked_scroll_handle {
|
||||||
scroll_handle_state.padded_content_size = padded_content_size;
|
scroll_handle_state.max_offset = scroll_max;
|
||||||
}
|
}
|
||||||
|
|
||||||
*scroll_offset
|
*scroll_offset
|
||||||
|
@ -2936,7 +2949,7 @@ impl ScrollAnchor {
|
||||||
struct ScrollHandleState {
|
struct ScrollHandleState {
|
||||||
offset: Rc<RefCell<Point<Pixels>>>,
|
offset: Rc<RefCell<Point<Pixels>>>,
|
||||||
bounds: Bounds<Pixels>,
|
bounds: Bounds<Pixels>,
|
||||||
padded_content_size: Size<Pixels>,
|
max_offset: Size<Pixels>,
|
||||||
child_bounds: Vec<Bounds<Pixels>>,
|
child_bounds: Vec<Bounds<Pixels>>,
|
||||||
scroll_to_bottom: bool,
|
scroll_to_bottom: bool,
|
||||||
overflow: Point<Overflow>,
|
overflow: Point<Overflow>,
|
||||||
|
@ -2965,6 +2978,11 @@ impl ScrollHandle {
|
||||||
*self.0.borrow().offset.borrow()
|
*self.0.borrow().offset.borrow()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Get the maximum scroll offset.
|
||||||
|
pub fn max_offset(&self) -> Size<Pixels> {
|
||||||
|
self.0.borrow().max_offset
|
||||||
|
}
|
||||||
|
|
||||||
/// Get the top child that's scrolled into view.
|
/// Get the top child that's scrolled into view.
|
||||||
pub fn top_item(&self) -> usize {
|
pub fn top_item(&self) -> usize {
|
||||||
let state = self.0.borrow();
|
let state = self.0.borrow();
|
||||||
|
@ -2999,11 +3017,6 @@ impl ScrollHandle {
|
||||||
self.0.borrow().child_bounds.get(ix).cloned()
|
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<Pixels> {
|
|
||||||
self.0.borrow().padded_content_size
|
|
||||||
}
|
|
||||||
|
|
||||||
/// scroll_to_item scrolls the minimal amount to ensure that the child is
|
/// scroll_to_item scrolls the minimal amount to ensure that the child is
|
||||||
/// fully visible
|
/// fully visible
|
||||||
pub fn scroll_to_item(&self, ix: usize) {
|
pub fn scroll_to_item(&self, ix: usize) {
|
||||||
|
|
|
@ -411,9 +411,9 @@ impl ListState {
|
||||||
self.0.borrow_mut().set_offset_from_scrollbar(point);
|
self.0.borrow_mut().set_offset_from_scrollbar(point);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Returns the size of items we have measured.
|
/// Returns the maximum scroll offset according to the items we have measured.
|
||||||
/// This value remains constant while dragging to prevent the scrollbar from moving away unexpectedly.
|
/// This value remains constant while dragging to prevent the scrollbar from moving away unexpectedly.
|
||||||
pub fn content_size_for_scrollbar(&self) -> Size<Pixels> {
|
pub fn max_offset_for_scrollbar(&self) -> Size<Pixels> {
|
||||||
let state = self.0.borrow();
|
let state = self.0.borrow();
|
||||||
let bounds = state.last_layout_bounds.unwrap_or_default();
|
let bounds = state.last_layout_bounds.unwrap_or_default();
|
||||||
|
|
||||||
|
@ -421,7 +421,7 @@ impl ListState {
|
||||||
.scrollbar_drag_start_height
|
.scrollbar_drag_start_height
|
||||||
.unwrap_or_else(|| state.items.summary().height);
|
.unwrap_or_else(|| state.items.summary().height);
|
||||||
|
|
||||||
Size::new(bounds.size.width, height)
|
Size::new(Pixels::ZERO, Pixels::ZERO.max(height - bounds.size.height))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Returns the current scroll offset adjusted for the scrollbar
|
/// Returns the current scroll offset adjusted for the scrollbar
|
||||||
|
|
|
@ -46,9 +46,16 @@ impl TerminalScrollHandle {
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ScrollableHandle for TerminalScrollHandle {
|
impl ScrollableHandle for TerminalScrollHandle {
|
||||||
fn content_size(&self) -> Size<Pixels> {
|
fn max_offset(&self) -> Size<Pixels> {
|
||||||
let state = self.state.borrow();
|
let state = self.state.borrow();
|
||||||
size(Pixels::ZERO, state.total_lines as f32 * state.line_height)
|
size(
|
||||||
|
Pixels::ZERO,
|
||||||
|
state
|
||||||
|
.total_lines
|
||||||
|
.checked_sub(state.viewport_lines)
|
||||||
|
.unwrap_or(0) as f32
|
||||||
|
* state.line_height,
|
||||||
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
fn offset(&self) -> Point<Pixels> {
|
fn offset(&self) -> Point<Pixels> {
|
||||||
|
|
|
@ -29,8 +29,8 @@ impl ThumbState {
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ScrollableHandle for UniformListScrollHandle {
|
impl ScrollableHandle for UniformListScrollHandle {
|
||||||
fn content_size(&self) -> Size<Pixels> {
|
fn max_offset(&self) -> Size<Pixels> {
|
||||||
self.0.borrow().base_handle.content_size()
|
self.0.borrow().base_handle.max_offset()
|
||||||
}
|
}
|
||||||
|
|
||||||
fn set_offset(&self, point: Point<Pixels>) {
|
fn set_offset(&self, point: Point<Pixels>) {
|
||||||
|
@ -47,8 +47,8 @@ impl ScrollableHandle for UniformListScrollHandle {
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ScrollableHandle for ListState {
|
impl ScrollableHandle for ListState {
|
||||||
fn content_size(&self) -> Size<Pixels> {
|
fn max_offset(&self) -> Size<Pixels> {
|
||||||
self.content_size_for_scrollbar()
|
self.max_offset_for_scrollbar()
|
||||||
}
|
}
|
||||||
|
|
||||||
fn set_offset(&self, point: Point<Pixels>) {
|
fn set_offset(&self, point: Point<Pixels>) {
|
||||||
|
@ -73,8 +73,8 @@ impl ScrollableHandle for ListState {
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ScrollableHandle for ScrollHandle {
|
impl ScrollableHandle for ScrollHandle {
|
||||||
fn content_size(&self) -> Size<Pixels> {
|
fn max_offset(&self) -> Size<Pixels> {
|
||||||
self.padded_content_size()
|
self.max_offset()
|
||||||
}
|
}
|
||||||
|
|
||||||
fn set_offset(&self, point: Point<Pixels>) {
|
fn set_offset(&self, point: Point<Pixels>) {
|
||||||
|
@ -91,7 +91,10 @@ impl ScrollableHandle for ScrollHandle {
|
||||||
}
|
}
|
||||||
|
|
||||||
pub trait ScrollableHandle: Any + Debug {
|
pub trait ScrollableHandle: Any + Debug {
|
||||||
fn content_size(&self) -> Size<Pixels>;
|
fn content_size(&self) -> Size<Pixels> {
|
||||||
|
self.viewport().size + self.max_offset()
|
||||||
|
}
|
||||||
|
fn max_offset(&self) -> Size<Pixels>;
|
||||||
fn set_offset(&self, point: Point<Pixels>);
|
fn set_offset(&self, point: Point<Pixels>);
|
||||||
fn offset(&self) -> Point<Pixels>;
|
fn offset(&self) -> Point<Pixels>;
|
||||||
fn viewport(&self) -> Bounds<Pixels>;
|
fn viewport(&self) -> Bounds<Pixels>;
|
||||||
|
@ -149,17 +152,17 @@ impl ScrollbarState {
|
||||||
|
|
||||||
fn thumb_range(&self, axis: ScrollbarAxis) -> Option<Range<f32>> {
|
fn thumb_range(&self, axis: ScrollbarAxis) -> Option<Range<f32>> {
|
||||||
const MINIMUM_THUMB_SIZE: Pixels = px(25.);
|
const MINIMUM_THUMB_SIZE: Pixels = px(25.);
|
||||||
let content_size = self.scroll_handle.content_size().along(axis);
|
let max_offset = self.scroll_handle.max_offset().along(axis);
|
||||||
let viewport_size = self.scroll_handle.viewport().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 {
|
if max_offset.is_zero() || viewport_size.is_zero() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
|
let content_size = viewport_size + max_offset;
|
||||||
let visible_percentage = viewport_size / content_size;
|
let visible_percentage = viewport_size / content_size;
|
||||||
let thumb_size = MINIMUM_THUMB_SIZE.max(viewport_size * visible_percentage);
|
let thumb_size = MINIMUM_THUMB_SIZE.max(viewport_size * visible_percentage);
|
||||||
if thumb_size > viewport_size {
|
if thumb_size > viewport_size {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
let max_offset = content_size - viewport_size;
|
|
||||||
let current_offset = self
|
let current_offset = self
|
||||||
.scroll_handle
|
.scroll_handle
|
||||||
.offset()
|
.offset()
|
||||||
|
@ -307,7 +310,7 @@ impl Element for Scrollbar {
|
||||||
|
|
||||||
let compute_click_offset =
|
let compute_click_offset =
|
||||||
move |event_position: Point<Pixels>,
|
move |event_position: Point<Pixels>,
|
||||||
item_size: Size<Pixels>,
|
max_offset: Size<Pixels>,
|
||||||
event_type: ScrollbarMouseEvent| {
|
event_type: ScrollbarMouseEvent| {
|
||||||
let viewport_size = padded_bounds.size.along(axis);
|
let viewport_size = padded_bounds.size.along(axis);
|
||||||
|
|
||||||
|
@ -323,7 +326,7 @@ impl Element for Scrollbar {
|
||||||
- thumb_offset)
|
- thumb_offset)
|
||||||
.clamp(px(0.), viewport_size - thumb_size);
|
.clamp(px(0.), viewport_size - thumb_size);
|
||||||
|
|
||||||
let max_offset = (item_size.along(axis) - viewport_size).max(px(0.));
|
let max_offset = max_offset.along(axis);
|
||||||
let percentage = if viewport_size > thumb_size {
|
let percentage = if viewport_size > thumb_size {
|
||||||
thumb_start / (viewport_size - thumb_size)
|
thumb_start / (viewport_size - thumb_size)
|
||||||
} else {
|
} else {
|
||||||
|
@ -347,7 +350,7 @@ impl Element for Scrollbar {
|
||||||
} else {
|
} else {
|
||||||
let click_offset = compute_click_offset(
|
let click_offset = compute_click_offset(
|
||||||
event.position,
|
event.position,
|
||||||
scroll.content_size(),
|
scroll.max_offset(),
|
||||||
ScrollbarMouseEvent::GutterClick,
|
ScrollbarMouseEvent::GutterClick,
|
||||||
);
|
);
|
||||||
scroll.set_offset(scroll.offset().apply_along(axis, |_| click_offset));
|
scroll.set_offset(scroll.offset().apply_along(axis, |_| click_offset));
|
||||||
|
@ -373,7 +376,7 @@ impl Element for Scrollbar {
|
||||||
ThumbState::Dragging(drag_state) if event.dragging() => {
|
ThumbState::Dragging(drag_state) if event.dragging() => {
|
||||||
let drag_offset = compute_click_offset(
|
let drag_offset = compute_click_offset(
|
||||||
event.position,
|
event.position,
|
||||||
scroll.content_size(),
|
scroll.max_offset(),
|
||||||
ScrollbarMouseEvent::ThumbDrag(drag_state),
|
ScrollbarMouseEvent::ThumbDrag(drag_state),
|
||||||
);
|
);
|
||||||
scroll.set_offset(scroll.offset().apply_along(axis, |_| drag_offset));
|
scroll.set_offset(scroll.offset().apply_along(axis, |_| drag_offset));
|
||||||
|
|
|
@ -18,7 +18,7 @@ use futures::{StreamExt, stream::FuturesUnordered};
|
||||||
use gpui::{
|
use gpui::{
|
||||||
Action, AnyElement, App, AsyncWindowContext, ClickEvent, ClipboardItem, Context, Corner, Div,
|
Action, AnyElement, App, AsyncWindowContext, ClickEvent, ClipboardItem, Context, Corner, Div,
|
||||||
DragMoveEvent, Entity, EntityId, EventEmitter, ExternalPaths, FocusHandle, FocusOutEvent,
|
DragMoveEvent, Entity, EntityId, EventEmitter, ExternalPaths, FocusHandle, FocusOutEvent,
|
||||||
Focusable, KeyContext, MouseButton, MouseDownEvent, NavigationDirection, Pixels, Point,
|
Focusable, IsZero, KeyContext, MouseButton, MouseDownEvent, NavigationDirection, Pixels, Point,
|
||||||
PromptLevel, Render, ScrollHandle, Subscription, Task, WeakEntity, WeakFocusHandle, Window,
|
PromptLevel, Render, ScrollHandle, Subscription, Task, WeakEntity, WeakFocusHandle, Window,
|
||||||
actions, anchored, deferred, prelude::*,
|
actions, anchored, deferred, prelude::*,
|
||||||
};
|
};
|
||||||
|
@ -46,8 +46,8 @@ use theme::ThemeSettings;
|
||||||
use ui::{
|
use ui::{
|
||||||
ButtonSize, Color, ContextMenu, ContextMenuEntry, ContextMenuItem, DecoratedIcon, IconButton,
|
ButtonSize, Color, ContextMenu, ContextMenuEntry, ContextMenuItem, DecoratedIcon, IconButton,
|
||||||
IconButtonShape, IconDecoration, IconDecorationKind, IconName, IconSize, Indicator, Label,
|
IconButtonShape, IconDecoration, IconDecorationKind, IconName, IconSize, Indicator, Label,
|
||||||
PopoverMenu, PopoverMenuHandle, ScrollableHandle, Tab, TabBar, TabPosition, Tooltip,
|
PopoverMenu, PopoverMenuHandle, Tab, TabBar, TabPosition, Tooltip, prelude::*,
|
||||||
prelude::*, right_click_menu,
|
right_click_menu,
|
||||||
};
|
};
|
||||||
use util::{ResultExt, debug_panic, maybe, truncate_and_remove_front};
|
use util::{ResultExt, debug_panic, maybe, truncate_and_remove_front};
|
||||||
|
|
||||||
|
@ -2865,10 +2865,9 @@ impl Pane {
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
.children(pinned_tabs.len().ne(&0).then(|| {
|
.children(pinned_tabs.len().ne(&0).then(|| {
|
||||||
let content_width = self.tab_bar_scroll_handle.content_size().width;
|
let max_scroll = self.tab_bar_scroll_handle.max_offset().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
|
// 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;
|
let is_scrollable = !max_scroll.is_zero();
|
||||||
let is_scrolled = self.tab_bar_scroll_handle.offset().x < px(0.);
|
let is_scrolled = self.tab_bar_scroll_handle.offset().x < px(0.);
|
||||||
let has_active_unpinned_tab = self.active_item_index >= self.pinned_tab_count;
|
let has_active_unpinned_tab = self.active_item_index >= self.pinned_tab_count;
|
||||||
h_flex()
|
h_flex()
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue