gpui: Snap recomputed padding to the device pixel grid before clamping scroll (#62296)
# Objective Follow-up to #62135, taking the approach suggested there: fix the error at its source in GPUI instead of hiding it in the scrollbar. At fractional rem sizes (`py_1` at an odd UI font size), a container whose content fits becomes scrollable by a sub-pixel amount, and every uniform-list picker grows a full-height scrollbar thumb for that phantom range. ## Solution Taffy lays boxes out with every authored length snapped to the device pixel grid (`to_taffy`), and the final bounds are snapped again after layout. However, `Interactivity::clamp_scroll_position` recomputed the padding from the raw style with no snapping at all, so the padded content size exceeded the snapped bounds by up to half a device pixel, far past the existing two-decimal float-noise rounding, and `scroll_max` came out positive. The fix snaps the recomputed padding with the same `pixel_snap` helper the rest of the pipeline uses, so both operands of the subtraction sit on the same grid and the phantom range never comes into existence. Two neighbouring cases are deliberately left out to keep this scoped, both are follow-up candidates: percentage-based padding resolves inside Taffy without pre-snapping, so a discrepancy is still theoretically possible there; and `ListState::max_scroll_offset` (the older `list()` element) has no rounding protection at all. ## Testing - New regression test: a 50px container with `py(4.25)` padding and a child that fits exactly. It fails on main (`max_offset` comes out at 0.5px) and passes with the fix; verified in both directions. - Full gpui suite: 241 tests pass. - Reviewers can see the original symptom by opening any picker at a UI font size that makes `py_1` fractional in device pixels: a spurious full-height scrollbar thumb appears on main and is gone with this change. The math is logical-pixel only, so the behavior is platform-independent. ## 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 - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable (one pixel-snap per padding edge per clamp, negligible against existing per-frame layout work) cc @MrSubidubi Release Notes: - Fixed containers becoming scrollable by a sub-pixel amount, with a phantom full-height scrollbar thumb, at fractional UI font sizes. Co-authored-by: MrSubidubi <finn@zed.dev>
This commit is contained in:
1 parent
946dab8c11
commit
d0699ca1d6
1 file changed
+50
-1
@@ -2346,7 +2346,14 @@ impl Interactivity {
|
||||
}
|
||||
|
||||
let rem_size = window.rem_size();
|
||||
let padding = style.padding.to_pixels(bounds.size.into(), rem_size);
|
||||
// Taffy lays the box out with the padding snapped to the device pixel
|
||||
// grid (`to_taffy`); recomputed unsnapped, e.g. py_1 at a fractional
|
||||
// rem size, it exceeds `bounds` and leaves the box scrollable by the
|
||||
// sub-pixel difference.
|
||||
let padding = style
|
||||
.padding
|
||||
.to_pixels(bounds.size.into(), rem_size)
|
||||
.map(|edge| window.pixel_snap(*edge));
|
||||
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
|
||||
@@ -5165,6 +5172,48 @@ mod tests {
|
||||
assert_eq!(focused, Some(item_b.id));
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
fn test_fractional_padding_does_not_make_a_fitting_container_scrollable(
|
||||
cx: &mut TestAppContext,
|
||||
) {
|
||||
struct PaddedContainer {
|
||||
scroll_handle: ScrollHandle,
|
||||
}
|
||||
|
||||
impl Render for PaddedContainer {
|
||||
fn render(
|
||||
&mut self,
|
||||
_window: &mut Window,
|
||||
_cx: &mut Context<Self>,
|
||||
) -> impl IntoElement {
|
||||
// 4.25px of padding snaps to 4px in layout, so a 42px child
|
||||
// fits the 50px box exactly.
|
||||
div().size_full().child(
|
||||
div()
|
||||
.id("container")
|
||||
.h(px(50.))
|
||||
.w(px(100.))
|
||||
.py(px(4.25))
|
||||
.overflow_y_scroll()
|
||||
.track_scroll(&self.scroll_handle)
|
||||
.child(div().w_full().h(px(42.))),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
let scroll_handle = ScrollHandle::new();
|
||||
let window: AnyWindowHandle = cx
|
||||
.add_window({
|
||||
let scroll_handle = scroll_handle.clone();
|
||||
move |_, _| PaddedContainer { scroll_handle }
|
||||
})
|
||||
.into();
|
||||
cx.update_window(window, |_, window, cx| window.draw(cx).clear(cx))
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(scroll_handle.max_offset().y, px(0.));
|
||||
}
|
||||
|
||||
struct ContentSizedGrid;
|
||||
|
||||
impl Render for ContentSizedGrid {
|
||||
|
||||
Reference in new issue
Block a user