diff --git a/Cargo.toml b/Cargo.toml index a09d6f46..e78b47d0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,7 @@ exclude = ["sites", "packages/blitz-wasm/guest", ".ps-observability", ".chuzz", resolver = "2" [workspace.package] -version = "0.4.8" +version = "0.4.9" license = "MIT OR Apache-2.0" homepage = "https://github.com/pathscale/ps-blitz" repository = "https://github.com/pathscale/ps-blitz" diff --git a/packages/blitz-dom/src/document.rs b/packages/blitz-dom/src/document.rs index 5cfb750e..bd38bdf6 100644 --- a/packages/blitz-dom/src/document.rs +++ b/packages/blitz-dom/src/document.rs @@ -2954,8 +2954,53 @@ impl BaseDocument { pub fn scroll_to_node_with_events( &mut self, node_id: NodeId, + dispatch_event: F, + ) { + self.scroll_to_node_aligned_with_events(node_id, false, dispatch_event); + } + + /// Reveal a node near the centre of each scrollport and the viewport. + /// + /// Semantic input uses this placement so fixed or sticky chrome cannot + /// cover the point an automation client will drive. Ordinary DOM + /// `scrollIntoView` and fragment navigation retain their start alignment. + pub fn scroll_to_node_centered_with_events( + &mut self, + node_id: NodeId, + dispatch_event: F, + ) { + self.scroll_to_node_aligned_with_events(node_id, true, dispatch_event); + } + + fn scroll_to_node_aligned_with_events( + &mut self, + node_id: NodeId, + centered: bool, mut dispatch_event: F, ) { + // Semantic snapshots expose visible text as addressable nodes, but a + // text node does not own a Taffy layout box. `absolute_position` and + // the scrolling calculations below require one, so resolve such a + // target to its nearest layout ancestor first. Without this, asking an + // automation client to reveal a label panics the entire host at + // `Node::final_layout` instead of scrolling the label's box into view. + let mut scroll_target = Some(node_id); + let node_id = loop { + let Some(candidate) = scroll_target else { + return; + }; + let Some(node) = self.nodes.get(candidate) else { + return; + }; + if matches!( + node.data, + NodeData::Element(_) | NodeData::AnonymousBlock(_) | NodeData::Document(_) + ) { + break candidate; + } + scroll_target = node.layout_parent.get().or(node.parent); + }; + // Every scroll container between the node and the root, innermost // first. Scrolling only the viewport is not `scrollIntoView`: it does // nothing at all for a node inside a nested scroller, which is what an @@ -2994,12 +3039,24 @@ impl BaseDocument { }; let box_ = scroller.absolute_position(0.0, 0.0); let layout = scroller.final_layout(); - // Land the node at the top-left of the scrollport. `scroll_node_by` - // takes a delta and subtracts it, so the sign here matches - // `scroll_viewport_by` below. - let dx = f64::from(box_.x - target.x); - let dy = f64::from(box_.y - target.y); - let _ = layout; + let target_layout = node.final_layout(); + // `scroll_node_by` subtracts its delta from the offset. Semantic + // automation centres the target to avoid chrome; DOM behavior + // keeps its historical start alignment. + let (dx, dy) = if centered { + ( + f64::from( + box_.x + layout.size.width / 2.0 + - (target.x + target_layout.size.width / 2.0), + ), + f64::from( + box_.y + layout.size.height / 2.0 + - (target.y + target_layout.size.height / 2.0), + ), + ) + } else { + (f64::from(box_.x - target.x), f64::from(box_.y - target.y)) + }; self.scroll_node_by(container, dx, dy, &mut dispatch_event); } @@ -3010,11 +3067,25 @@ impl BaseDocument { }; let target = node.absolute_position(0.0, 0.0); let current = self.viewport_scroll; + let (desired_x, desired_y) = if centered { + let target_layout = node.final_layout(); + let scale = self.viewport.scale(); + let viewport_width = self.viewport.window_size.0 as f32 / scale; + let viewport_height = self.viewport.window_size.1 as f32 / scale; + ( + f64::from(target.x + target_layout.size.width / 2.0 - viewport_width / 2.0) + .max(0.0), + f64::from(target.y + target_layout.size.height / 2.0 - viewport_height / 2.0) + .max(0.0), + ) + } else { + (f64::from(target.x), f64::from(target.y)) + }; - // `scroll_viewport_by` subtracts the delta from the current scroll offset, so pass - // `current - target` in order to land on `target`. - let dx = current.x - target.x as f64; - let dy = current.y - target.y as f64; + // `scroll_viewport_by` subtracts the delta from the current scroll + // offset, so pass current minus the centred destination. + let dx = current.x - desired_x; + let dy = current.y - desired_y; if let Some(root) = self.try_root_element().map(|element| element.id) { self.scroll_node_by(root, dx, dy, dispatch_event); } else { @@ -3897,6 +3968,8 @@ mod control_scroll_tests { ); let spacer = mutator.create_element(qual_name!("div"), vec![style("height:400px")]); let target = mutator.create_element(qual_name!("button"), vec![style("height:40px")]); + let target_text = mutator.create_text_node("Reveal me"); + mutator.append_children(target, &[target_text]); mutator.append_children(scroller, &[spacer, target]); mutator.append_children(body, &[scroller]); mutator.append_children(html, &[body]); @@ -3912,9 +3985,15 @@ mod control_scroll_tests { doc.nodes[target].final_layout_mut().location.y = 400.0; let mut events = Vec::new(); - doc.scroll_to_node_with_events(target, |event| events.push(event)); + // A semantic client may target the exposed text rather than its + // element. Text has no `final_layout`, so this also pins the host-crash + // regression from revealing a named label. + doc.scroll_to_node_centered_with_events(target_text, |event| events.push(event)); - assert!(doc.viewport_scroll.y > 0.0); + assert!( + (doc.viewport_scroll.y - 270.0).abs() < 0.1, + "the 40px target should be centred in the 300px viewport" + ); assert!( events .iter() diff --git a/packages/blitz-dom/src/events/driver.rs b/packages/blitz-dom/src/events/driver.rs index da60368b..389c0e83 100644 --- a/packages/blitz-dom/src/events/driver.rs +++ b/packages/blitz-dom/src/events/driver.rs @@ -174,6 +174,20 @@ impl<'doc, Handler: EventHandler> EventDriver<'doc, Handler> { } pub fn handle_ui_event(&mut self, event: UiEvent) { + self.handle_ui_event_inner(event, None); + } + + /// Dispatch input to a DOM node that automation already resolved. + /// + /// Window input remains coordinate hit-tested through [`Self::handle_ui_event`]. + /// A semantic driver has already selected its target by node id, and may be + /// operating without fonts or a compositor, so hit-testing its synthetic + /// coordinate would either choose a different node or reject a flat one. + pub fn handle_ui_event_to_node(&mut self, event: UiEvent, node_id: NodeId) { + self.handle_ui_event_inner(event, Some(node_id)); + } + + fn handle_ui_event_inner(&mut self, event: UiEvent, forced_target: Option) { let doc = self.doc.inner(); let mut should_clear_hover = false; @@ -184,16 +198,22 @@ impl<'doc, Handler: EventHandler> EventDriver<'doc, Handler> { // Update document input state (hover, focus, active, etc) match &event { UiEvent::PointerMove(event) => { - hover_node_id = self.handle_pointer_move(event); + hover_node_id = forced_target + .and_then(|node_id| self.handle_pointer_move_to_node(event, node_id)) + .or_else(|| self.handle_pointer_move(event)); } UiEvent::PointerDown(event) => { - hover_node_id = self.handle_pointer_move(event); + hover_node_id = forced_target + .and_then(|node_id| self.handle_pointer_move_to_node(event, node_id)) + .or_else(|| self.handle_pointer_move(event)); let mut doc = self.doc.inner_mut(); doc.active_node(); doc.set_mousedown_node_id(hover_node_id); } UiEvent::PointerUp(event) => { - hover_node_id = self.handle_pointer_move(event); + hover_node_id = forced_target + .and_then(|node_id| self.handle_pointer_move_to_node(event, node_id)) + .or_else(|| self.handle_pointer_move(event)); let mut doc = self.doc.inner_mut(); doc.unactive_node(); @@ -202,7 +222,9 @@ impl<'doc, Handler: EventHandler> EventDriver<'doc, Handler> { } } UiEvent::PointerCancel(event) => { - hover_node_id = self.handle_pointer_move(event); + hover_node_id = forced_target + .and_then(|node_id| self.handle_pointer_move_to_node(event, node_id)) + .or_else(|| self.handle_pointer_move(event)); let mut doc = self.doc.inner_mut(); doc.unactive_node(); doc.set_mousedown_node_id(None); diff --git a/packages/blitz-script/src/document.rs b/packages/blitz-script/src/document.rs index 5526e3be..877afe8c 100644 --- a/packages/blitz-script/src/document.rs +++ b/packages/blitz-script/src/document.rs @@ -575,6 +575,23 @@ impl ScriptDocument { self.arm_timer_thread(); } + /// Deliver one automation input event to the exact semantic node selected + /// by the caller, while retaining the normal pointer event ordering, + /// compatibility mouse events, activation state and click synthesis. + pub fn handle_ui_event_to_node(&mut self, event: UiEvent, node_id: NodeId) { + let profiling_boundary = self.runtime.ctx.enter_profiling_boundary(); + let profiling = profiling_boundary.enabled(); + let handler = ScriptEventHandler { + runtime: &mut self.runtime, + profiling, + }; + let mut driver = EventDriver::new(&mut self.inner, handler); + driver.handle_ui_event_to_node(event, node_id); + + self.request_redraw(); + self.arm_timer_thread(); + } + /// The real poll. Split out so every exit path is timed by the wrapper /// above rather than by a stopwatch threaded through each early return. fn poll_inner(&mut self, task_context: Option, profiling: bool) -> bool { diff --git a/packages/blitz-script/src/dom/mod.rs b/packages/blitz-script/src/dom/mod.rs index b331b3c3..df6ac944 100644 --- a/packages/blitz-script/src/dom/mod.rs +++ b/packages/blitz-script/src/dom/mod.rs @@ -101,10 +101,16 @@ pub(crate) fn node_wrapper(ctx: &DomCtx, node_id: NodeId, _context: &mut Context }; let wrapper = JsObject::from_proto_and_data(Some(proto), NodeRef { node_id }); - ctx.state - .borrow_mut() - .node_wrappers - .insert(node_id, wrapper.downgrade()); + let connected = ctx + .doc + .borrow() + .get_node(node_id) + .is_some_and(|node| node.flags.is_in_document()); + let mut state = ctx.state.borrow_mut(); + state.node_wrappers.insert(node_id, wrapper.downgrade()); + if connected { + state.connected_wrappers.insert(node_id, wrapper.clone()); + } wrapper } @@ -150,17 +156,12 @@ pub(crate) fn mark_node_reattached(ctx: &DomCtx, node_id: NodeId) { .detached_nodes .retain(|candidate| *candidate != node_id); for id in ids { - let has_listeners = state - .node_listeners + if let Some(wrapper) = state + .node_wrappers .get(&id) - .is_some_and(|by_type| by_type.values().any(|listeners| !listeners.is_empty())); - if has_listeners - && let Some(wrapper) = state - .node_wrappers - .get(&id) - .and_then(|wrapper| wrapper.upgrade()) + .and_then(|wrapper| wrapper.upgrade()) { - state.listener_wrappers.insert(id, wrapper); + state.connected_wrappers.insert(id, wrapper); } } } @@ -253,7 +254,7 @@ pub(crate) fn sweep_detached_nodes(ctx: &DomCtx) { state.dataset_wrappers.remove(&id); state.class_list_wrappers.remove(&id); state.node_listeners.remove(&id); - state.listener_wrappers.remove(&id); + state.connected_wrappers.remove(&id); } } diff --git a/packages/blitz-script/src/dom/node.rs b/packages/blitz-script/src/dom/node.rs index 5cc2d3e1..5a955708 100644 --- a/packages/blitz-script/src/dom/node.rs +++ b/packages/blitz-script/src/dom/node.rs @@ -60,9 +60,9 @@ pub(crate) fn sync_node_listener_callbacks( let connected = node_is_connected(ctx, node_id); let mut state = ctx.state.borrow_mut(); if connected { - state.listener_wrappers.insert(node_id, wrapper); + state.connected_wrappers.insert(node_id, wrapper); } else { - state.listener_wrappers.remove(&node_id); + state.connected_wrappers.remove(&node_id); } } } @@ -113,7 +113,7 @@ pub(crate) fn root_inline_event_handlers( if carries_handler { ctx.state .borrow_mut() - .listener_wrappers + .connected_wrappers .insert(node_id, wrapper); } } @@ -163,7 +163,7 @@ pub(crate) fn unroot_detached_listener_subtree( } let mut state = ctx.state.borrow_mut(); for id in ids { - state.listener_wrappers.remove(&id); + state.connected_wrappers.remove(&id); } } diff --git a/packages/blitz-script/src/state.rs b/packages/blitz-script/src/state.rs index 2aa2bfdd..6660cba5 100644 --- a/packages/blitz-script/src/state.rs +++ b/packages/blitz-script/src/state.rs @@ -85,10 +85,13 @@ pub(crate) struct RuntimeState { pub class_list_wrappers: FxHashMap, /// Event listeners registered on nodes, keyed by node id then event type. pub node_listeners: FxHashMap, - /// Strong roots for wrappers with listeners while their nodes are in the - /// live document. Detaching a subtree removes these roots after linking - /// its wrappers together inside Boa's heap. - pub listener_wrappers: FxHashMap, + /// Strong roots for wrappers while their nodes are in the live document. + /// + /// DOM nodes own their JavaScript wrappers in a browser. Keeping that + /// relationship here preserves arbitrary expando properties such as + /// Solid's delegated `$$click` handler. Detaching a subtree removes these + /// roots after linking its wrappers together inside Boa's heap. + pub connected_wrappers: FxHashMap, /// Nodes detached from the document but not yet freed. /// /// A node is removed while script may still hold its wrapper, and whether diff --git a/packages/blitz-script/tests/inline_handlers_survive_collection.rs b/packages/blitz-script/tests/inline_handlers_survive_collection.rs index 36095684..aefef867 100644 --- a/packages/blitz-script/tests/inline_handlers_survive_collection.rs +++ b/packages/blitz-script/tests/inline_handlers_survive_collection.rs @@ -108,6 +108,70 @@ fn a_handler_assigned_after_insertion_survives_too() { ); } +/// Solid and other delegated-event runtimes keep the authored callback on an +/// expando property and install one listener on the document. The connected +/// DOM node owns that property even after application code drops its wrapper. +#[test] +fn a_delegated_expando_handler_survives_collection() { + let mut document = page( + r#"document.addEventListener('click', function (event) { + var target = event.composedPath().find(function (node) { + return node.nodeName === 'BUTTON'; + }); + if (target && target.$$click) target.$$click(event); + }); + (function () { + var button = document.createElement('button'); + button.id = 'later'; + button.$$click = function () { + document.getElementById('out').textContent = 'delegated'; + }; + document.getElementById('host').appendChild(button); + })();"#, + ); + document.execute_scripts(); + + boa_gc::force_collect(); + click(&mut document, "#later"); + + assert_eq!( + text_of(&document, "#out"), + "delegated", + "the collector took a delegated expando while its node was connected" + ); +} + +#[test] +fn delegated_handlers_survive_a_framework_remount() { + let mut document = page( + r#"document.addEventListener('click', function (event) { + var target = event.composedPath().find(function (node) { + return node.nodeName === 'BUTTON'; + }); + if (target && target.$$click) target.$$click(event); + }); + function render(label) { + var button = document.createElement('button'); + button.id = label; + button.textContent = label; + button.$$click = function () { + document.getElementById('out').textContent += label; + if (label === 'first') render('second'); + }; + document.getElementById('host').replaceChildren(button); + } + render('first');"#, + ); + document.execute_scripts(); + + boa_gc::force_collect(); + click(&mut document, "#first"); + boa_gc::force_collect(); + click(&mut document, "#second"); + + assert_eq!(text_of(&document, "#out"), "not yetfirstsecond"); +} + /// A node the page removed and let go of is not kept alive by this. Rooting /// every wrapper that ever carried a handler would leak exactly the nodes the /// weak cache exists to release.