From 0d73e43a9254d7a48250296701f39ff0ed7f515d Mon Sep 17 00:00:00 2001 From: Will Temple Date: Sun, 22 Mar 2026 01:20:11 -0400 Subject: [PATCH] Fix scrollbar regression --- lib/ruin_app/src/lib.rs | 473 +++++++++++++++++++++++++++++++++++++++- lib/ui/src/layout.rs | 65 +++++- 2 files changed, 533 insertions(+), 5 deletions(-) diff --git a/lib/ruin_app/src/lib.rs b/lib/ruin_app/src/lib.rs index 99178da..e15593c 100644 --- a/lib/ruin_app/src/lib.rs +++ b/lib/ruin_app/src/lib.rs @@ -1824,11 +1824,12 @@ fn focused_element_for_pointer( interaction_tree: &InteractionTree, event: &PointerEvent, ) -> Option { - interaction_tree - .hit_path(event.position) + let hit_path = interaction_tree.hit_path(event.position); + hit_path .iter() .rev() .find_map(|target| target.focusable.then_some(target.element_id).flatten()) + .or_else(|| hit_path.iter().rev().find_map(|target| target.element_id)) } fn element_contains_element( @@ -1869,7 +1870,6 @@ fn key_handler_for_focus<'a>( let focused_element = focused_element?; focused_ancestor_chain(&interaction_tree.root, focused_element)? .into_iter() - .rev() .find_map(|element_id| handlers.get(&element_id)) } @@ -1933,6 +1933,7 @@ struct SignalInner { #[cfg(test)] mod tests { use super::*; + use ruin_ui::{KeyboardModifiers, Point, UiRuntime, WindowSpec}; #[derive(Clone, Debug, PartialEq, Eq)] struct NamedValue(&'static str); @@ -1989,4 +1990,470 @@ mod tests { assert_eq!(*seen_value.borrow(), Some(NamedValue("inner"))); } + + #[test] + fn key_dispatch_prefers_the_nearest_focused_ancestor_handler() { + let outer_id = ElementId::new(41); + let inner_id = ElementId::new(42); + let root = Element::column().pointer_events(false).child( + Element::column() + .id(outer_id) + .width(160.0) + .height(120.0) + .focusable(true) + .child( + Element::column() + .id(inner_id) + .width(120.0) + .height(80.0) + .focusable(true), + ), + ); + let snapshot = ruin_ui::layout_snapshot(1, UiSize::new(200.0, 120.0), &root); + let outer_hits = Rc::new(StdCell::new(0usize)); + let inner_hits = Rc::new(StdCell::new(0usize)); + + let mut handlers = HashMap::::new(); + handlers.insert( + outer_id, + Rc::new({ + let outer_hits = Rc::clone(&outer_hits); + move |_, _| { + outer_hits.set(outer_hits.get() + 1); + true + } + }), + ); + handlers.insert( + inner_id, + Rc::new({ + let inner_hits = Rc::clone(&inner_hits); + move |_, _| { + inner_hits.set(inner_hits.get() + 1); + true + } + }), + ); + + let handler = key_handler_for_focus(&handlers, Some(inner_id), &snapshot.interaction_tree) + .expect("focused element should resolve a key handler"); + let _ = handler( + &KeyboardEvent::new( + 0, + KeyboardEventKind::Pressed, + KeyboardKey::ArrowDown, + KeyboardModifiers::default(), + None, + ), + &snapshot.interaction_tree, + ); + + assert_eq!(inner_hits.get(), 1); + assert_eq!(outer_hits.get(), 0); + } + + #[test] + fn scroll_box_arrow_keys_work_after_clicking_text_content() { + let offset_slot = Rc::new(RefCell::new(None::>)); + let render = render_with_context(Rc::new(RenderState::default()), { + let offset_slot = Rc::clone(&offset_slot); + move || { + let offset = use_signal(|| 0.0_f32); + *offset_slot.borrow_mut() = Some(offset.clone()); + scroll_box() + .height(120.0) + .offset_y(offset) + .children(text().children( + "line 01\nline 02\nline 03\nline 04\nline 05\nline 06\nline 07\nline 08\nline 09\nline 10\nline 11\nline 12", + )) + } + }); + let offset = offset_slot + .borrow() + .clone() + .expect("scroll signal should have been captured"); + let scrollbox_id = render + .view + .element + .id + .expect("scroll box should receive an element id"); + let snapshot = + ruin_ui::layout_snapshot(1, UiSize::new(260.0, 160.0), render.view.element()); + let focused = focused_element_for_pointer( + &snapshot.interaction_tree, + &PointerEvent::new( + 1, + Point::new(12.0, 12.0), + PointerEventKind::Down { + button: PointerButton::Primary, + }, + ), + ); + + assert_eq!(focused, Some(scrollbox_id)); + + render.view.bindings.dispatch_key( + focused, + &KeyboardEvent::new( + 0, + KeyboardEventKind::Pressed, + KeyboardKey::ArrowDown, + KeyboardModifiers::default(), + None, + ), + &snapshot.interaction_tree, + ); + + assert!(offset.get() > 0.0); + } + + #[test] + fn scroll_box_thumb_drag_updates_offset_signal() { + let offset_slot = Rc::new(RefCell::new(None::>)); + let render = render_with_context(Rc::new(RenderState::default()), { + let offset_slot = Rc::clone(&offset_slot); + move || { + let offset = use_signal(|| 0.0_f32); + *offset_slot.borrow_mut() = Some(offset.clone()); + scroll_box() + .height(120.0) + .offset_y(offset) + .children(text().children( + "line 01\nline 02\nline 03\nline 04\nline 05\nline 06\nline 07\nline 08\nline 09\nline 10\nline 11\nline 12", + )) + } + }); + let offset = offset_slot + .borrow() + .clone() + .expect("scroll signal should have been captured"); + let scrollbox_id = render + .view + .element + .id + .expect("scroll box should receive an element id"); + let snapshot = + ruin_ui::layout_snapshot(1, UiSize::new(260.0, 160.0), render.view.element()); + let metrics = snapshot + .interaction_tree + .scroll_metrics_for_element(scrollbox_id) + .expect("scroll metrics should exist for the scroll box"); + let thumb = metrics + .scrollbar_thumb + .expect("overflowing scroll box should expose a scrollbar thumb"); + let thumb_center = Point::new( + thumb.origin.x + (thumb.size.width * 0.5), + thumb.origin.y + (thumb.size.height * 0.5), + ); + let hovered_targets = snapshot.interaction_tree.hit_path(thumb_center); + let handler = scroll_handler_for_event( + &render.view.bindings.on_scroll, + Some(scrollbox_id), + &hovered_targets, + ) + .expect("scroll box should resolve its scroll handler") + .clone(); + + handler( + &RoutedPointerEvent { + kind: RoutedPointerEventKind::Down { + button: PointerButton::Primary, + }, + target: hovered_targets + .last() + .cloned() + .expect("thumb center should hit the scroll box"), + pointer_id: 1, + position: thumb_center, + }, + &snapshot.interaction_tree, + ); + handler( + &RoutedPointerEvent { + kind: RoutedPointerEventKind::Move, + target: hovered_targets + .last() + .cloned() + .expect("thumb center should hit the scroll box"), + pointer_id: 1, + position: Point::new(thumb_center.x, thumb_center.y + 24.0), + }, + &snapshot.interaction_tree, + ); + + assert!(offset.get() > 0.0); + } + + #[test] + fn live_input_path_scrolls_a_scroll_box_rendered_inside_a_branch() { + let offset_slot = Rc::new(RefCell::new(None::>)); + let render = render_with_context(Rc::new(RenderState::default()), { + let offset_slot = Rc::clone(&offset_slot); + move || match true { + true => { + let offset = use_signal(|| 0.0_f32); + *offset_slot.borrow_mut() = Some(offset.clone()); + column() + .background(surfaces::raised()) + .gap(10.0) + .children(( + text().children(("bytes = ", 4096)), + scroll_box() + .height(420.0) + .offset_y(offset.clone()) + .padding(12.0) + .background(surfaces::canvas()) + .border_radius(10.0) + .border((2.0, colors::muted())) + .children( + text() + .color(colors::muted()) + .font_family(TextFontFamily::Monospace) + .children( + "line 01\nline 02\nline 03\nline 04\nline 05\nline 06\nline 07\nline 08\nline 09\nline 10\nline 11\nline 12\nline 13\nline 14\nline 15\nline 16\nline 17\nline 18\nline 19\nline 20\nline 21\nline 22\nline 23\nline 24", + ), + ), + )) + } + false => View::from_element(Element::column()), + } + }); + let offset = offset_slot + .borrow() + .clone() + .expect("scroll signal should have been captured"); + let snapshot = + ruin_ui::layout_snapshot(1, UiSize::new(1080.0, 760.0), render.view.element()); + let interaction_tree = RefCell::new(Some(snapshot.interaction_tree.clone())); + let bindings = RefCell::new(render.view.bindings.clone()); + let mut pointer_router = PointerRouter::new(); + let mut input_state = InputState::new(); + let window = UiRuntime::headless() + .create_window(WindowSpec::new("scrollbox-test")) + .expect("headless window should be created"); + + MountedApp::::handle_pointer_event( + &window, + &interaction_tree, + &bindings, + &mut pointer_router, + &mut input_state, + PointerEvent::new( + 1, + Point::new(24.0, 64.0), + PointerEventKind::Down { + button: PointerButton::Primary, + }, + ), + ) + .expect("pointer down should succeed"); + MountedApp::::handle_keyboard_event( + &interaction_tree, + &bindings, + &RefCell::new(Vec::new()), + &input_state, + KeyboardEvent::new( + 0, + KeyboardEventKind::Pressed, + KeyboardKey::ArrowDown, + KeyboardModifiers::default(), + None, + ), + ) + .expect("keyboard event should succeed"); + + assert!(offset.get() > 0.0); + } + + #[test] + fn scroll_box_stays_interactive_when_it_appears_on_a_later_render() { + let state = Rc::new(RenderState::default()); + let ready_slot = Rc::new(RefCell::new(None::>)); + let offset_slot = Rc::new(RefCell::new(None::>)); + + let render_once = + |state: Rc, + ready_slot: Rc>>>, + offset_slot: Rc>>>| { + render_with_context(state, move || { + let ready = use_signal(|| false); + let offset = use_signal(|| 0.0_f32); + *ready_slot.borrow_mut() = Some(ready.clone()); + *offset_slot.borrow_mut() = Some(offset.clone()); + + if ready.get() { + column() + .background(surfaces::raised()) + .gap(10.0) + .children(( + text().children(("bytes = ", 4096)), + scroll_box() + .height(420.0) + .offset_y(offset.clone()) + .padding(12.0) + .background(surfaces::canvas()) + .border_radius(10.0) + .border((2.0, colors::muted())) + .children( + text() + .color(colors::muted()) + .font_family(TextFontFamily::Monospace) + .children( + "line 01\nline 02\nline 03\nline 04\nline 05\nline 06\nline 07\nline 08\nline 09\nline 10\nline 11\nline 12\nline 13\nline 14\nline 15\nline 16\nline 17\nline 18\nline 19\nline 20\nline 21\nline 22\nline 23\nline 24", + ), + ), + )) + } else { + text().children("Loading file contents...") + } + }) + }; + + let _initial = render_once( + Rc::clone(&state), + Rc::clone(&ready_slot), + Rc::clone(&offset_slot), + ); + let ready = ready_slot + .borrow() + .clone() + .expect("ready signal should have been captured"); + let offset = offset_slot + .borrow() + .clone() + .expect("offset signal should have been captured"); + let _ = ready.set(true); + + let render = render_once(state, ready_slot, Rc::clone(&offset_slot)); + let snapshot = + ruin_ui::layout_snapshot(1, UiSize::new(1080.0, 760.0), render.view.element()); + let interaction_tree = RefCell::new(Some(snapshot.interaction_tree.clone())); + let bindings = RefCell::new(render.view.bindings.clone()); + let mut pointer_router = PointerRouter::new(); + let mut input_state = InputState::new(); + let window = UiRuntime::headless() + .create_window(WindowSpec::new("scrollbox-transition-test")) + .expect("headless window should be created"); + + MountedApp::::handle_pointer_event( + &window, + &interaction_tree, + &bindings, + &mut pointer_router, + &mut input_state, + PointerEvent::new( + 1, + Point::new(24.0, 64.0), + PointerEventKind::Down { + button: PointerButton::Primary, + }, + ), + ) + .expect("pointer down should succeed after branch switch"); + MountedApp::::handle_keyboard_event( + &interaction_tree, + &bindings, + &RefCell::new(Vec::new()), + &input_state, + KeyboardEvent::new( + 0, + KeyboardEventKind::Pressed, + KeyboardKey::ArrowDown, + KeyboardModifiers::default(), + None, + ), + ) + .expect("keyboard event should succeed after branch switch"); + + assert!(offset.get() > 0.0); + } + + #[test] + fn live_input_path_scrolls_with_real_cargo_lock_contents() { + let offset_slot = Rc::new(RefCell::new(None::>)); + let render = render_with_context(Rc::new(RenderState::default()), { + let offset_slot = Rc::clone(&offset_slot); + move || { + let offset = use_signal(|| 0.0_f32); + *offset_slot.borrow_mut() = Some(offset.clone()); + column().background(surfaces::raised()).gap(10.0).children(( + text().children(("bytes = ", include_str!("../../../Cargo.lock").len())), + scroll_box() + .height(420.0) + .offset_y(offset.clone()) + .padding(12.0) + .background(surfaces::canvas()) + .border_radius(10.0) + .border((2.0, colors::muted())) + .children( + text() + .color(colors::muted()) + .font_family(TextFontFamily::Monospace) + .children(include_str!("../../../Cargo.lock")), + ), + )) + } + }); + let offset = offset_slot + .borrow() + .clone() + .expect("scroll signal should have been captured"); + let snapshot = + ruin_ui::layout_snapshot(1, UiSize::new(1080.0, 760.0), render.view.element()); + let interaction_tree = RefCell::new(Some(snapshot.interaction_tree.clone())); + let bindings = RefCell::new(render.view.bindings.clone()); + let mut pointer_router = PointerRouter::new(); + let mut input_state = InputState::new(); + let window = UiRuntime::headless() + .create_window(WindowSpec::new("scrollbox-cargo-lock-test")) + .expect("headless window should be created"); + + MountedApp::::handle_pointer_event( + &window, + &interaction_tree, + &bindings, + &mut pointer_router, + &mut input_state, + PointerEvent::new( + 1, + Point::new(24.0, 64.0), + PointerEventKind::Scroll { + delta: Point::new(0.0, 48.0), + }, + ), + ) + .expect("wheel event should succeed"); + + assert!(offset.get() > 0.0); + } + + #[test] + fn rerendered_scroll_box_element_carries_the_updated_offset() { + let state = Rc::new(RenderState::default()); + let offset_slot = Rc::new(RefCell::new(None::>)); + let render_once = + |state: Rc, offset_slot: Rc>>>| { + render_with_context(state, move || { + let offset = use_signal(|| 0.0_f32); + *offset_slot.borrow_mut() = Some(offset.clone()); + scroll_box() + .height(120.0) + .offset_y(offset.clone()) + .children( + text().children("line 01\nline 02\nline 03\nline 04\nline 05\nline 06"), + ) + }) + }; + + let _initial = render_once(Rc::clone(&state), Rc::clone(&offset_slot)); + let offset = offset_slot + .borrow() + .clone() + .expect("offset signal should have been captured"); + let _ = offset.set(96.0); + let render = render_once(state, offset_slot); + let debug = format!("{:?}", render.view.element()); + + assert!(debug.contains("offset_y: 96.0"), "{debug}"); + } } diff --git a/lib/ui/src/layout.rs b/lib/ui/src/layout.rs index 3616867..49b3455 100644 --- a/lib/ui/src/layout.rs +++ b/lib/ui/src/layout.rs @@ -313,9 +313,11 @@ fn layout_element( perf_stats, ); let provisional_content_height = content_size.height.max(viewport_rect.size.height); + let requested_offset_y = scroll_box.offset_y.max(0.0); let provisional_max_offset_y = (provisional_content_height - viewport_rect.size.height).max(0.0); - let mut offset_y = scroll_box.offset_y.max(0.0).min(provisional_max_offset_y); + let mut offset_y = requested_offset_y.min(provisional_max_offset_y); + let child_scene_start = scene.items.len(); if viewport_rect.size.width > 0.0 && viewport_rect.size.height > 0.0 { scene.push_clip(viewport_rect, 0.0); @@ -345,7 +347,31 @@ fn layout_element( .max(actual_content_height.ceil()) .max(viewport_rect.size.height); let max_offset_y = (content_height - viewport_rect.size.height).max(0.0); - offset_y = offset_y.min(max_offset_y); + let corrected_offset_y = requested_offset_y.min(max_offset_y); + if (corrected_offset_y - offset_y).abs() > f32::EPSILON + && viewport_rect.size.width > 0.0 + && viewport_rect.size.height > 0.0 + { + offset_y = corrected_offset_y; + scene.items.truncate(child_scene_start); + scene.push_clip(viewport_rect, 0.0); + interaction.children = layout_container_children( + element, + Rect::new( + viewport_rect.origin.x, + viewport_rect.origin.y - offset_y, + viewport_rect.size.width, + content_height, + ), + &interaction.path, + scene, + text_system, + perf_stats, + ); + scene.pop_clip(); + } else { + offset_y = corrected_offset_y; + } interaction.scroll_metrics = Some(ScrollMetrics { viewport_rect, content_height, @@ -1701,6 +1727,41 @@ mod tests { assert!(text_bottom >= viewport_bottom - 1.0); } + #[test] + fn scroll_box_preserves_requested_offset_for_direct_text_children() { + let scrollbox_id = ElementId::new(23); + let root = Element::column().child( + Element::scroll_box(240.0) + .id(scrollbox_id) + .width(320.0) + .height(120.0) + .padding(Edges::all(8.0)) + .child(Element::text( + "line 01\nline 02\nline 03\nline 04\nline 05\nline 06\nline 07\nline 08\nline 09\nline 10\nline 11\nline 12\nline 13\nline 14\nline 15\nline 16\nline 17\nline 18\nline 19\nline 20\nline 21\nline 22\nline 23\nline 24\nline 25\nline 26", + TextStyle::new(16.0, Color::rgb(0xFF, 0xFF, 0xFF)).with_line_height(20.0), + )), + ); + + let snapshot = layout_snapshot(1, UiSize::new(360.0, 220.0), &root); + let scroll_metrics = snapshot + .interaction_tree + .scroll_metrics_for_element(scrollbox_id) + .expect("scroll box should expose scroll metrics"); + let visible_text = snapshot + .scene + .items + .iter() + .find_map(|item| match item { + DisplayItem::Text(text) => Some(text), + _ => None, + }) + .expect("scroll box should emit direct text children"); + + assert!(scroll_metrics.max_offset_y > 240.0); + assert_eq!(scroll_metrics.offset_y, 240.0); + assert!(visible_text.origin.y < scroll_metrics.viewport_rect.origin.y); + } + #[test] fn interaction_tree_hit_test_returns_deepest_pointer_target() { let root = Element::column()