From dc4707ee2acf5d4a8469440958e0bf9d0014a1c9 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Wed, 23 Aug 2023 13:01:37 -0500 Subject: [PATCH] WIP fix event bubbling --- packages/core/src/arena.rs | 39 +++------- packages/core/src/create.rs | 44 ++++++++--- packages/core/src/diff.rs | 99 ++++++++++++------------ packages/core/src/virtual_dom.rs | 6 +- packages/core/tests/event_propagation.rs | 64 +++++++++++++++ packages/core/tests/fuzzing.rs | 3 +- 6 files changed, 161 insertions(+), 94 deletions(-) create mode 100644 packages/core/tests/event_propagation.rs diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index c41454b50..53695d9d7 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -1,5 +1,3 @@ -use std::ptr::NonNull; - use crate::{ innerlude::DirtyScope, nodes::RenderReturn, nodes::VNode, virtual_dom::VirtualDom, AttributeValue, DynamicNode, ScopeId, @@ -21,13 +19,13 @@ pub struct ElementId(pub usize); #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] pub struct BubbleId(pub usize); -#[derive(Debug, Clone)] -pub struct ElementRef { +#[derive(Debug, Clone, Copy)] +pub struct ElementRef<'a> { // the pathway of the real element inside the template pub(crate) path: ElementPath, // The actual template - pub(crate) template: NonNull>, + pub(crate) template: &'a VNode<'a>, // The scope the element belongs to pub(crate) scope: ScopeId, @@ -35,7 +33,7 @@ pub struct ElementRef { #[derive(Clone, Copy, Debug)] pub struct ElementPath { - path: &'static [u8], + pub(crate) path: &'static [u8], } impl VirtualDom { @@ -43,22 +41,11 @@ impl VirtualDom { ElementId(self.elements.insert(None)) } - pub(crate) fn next_element_ref(&mut self, template: &VNode, path: &'static [u8]) -> BubbleId { - self.next_reference(template, ElementPath { path }) - } - - fn next_reference(&mut self, template: &VNode, path: ElementPath) -> BubbleId { - let entry = self.element_refs.vacant_entry(); - let id = entry.key(); - let scope = self.runtime.current_scope_id().unwrap_or(ScopeId(0)); - - entry.insert(ElementRef { - // We know this is non-null because it comes from a reference - template: unsafe { NonNull::new_unchecked(template as *const _ as *mut _) }, - path, - scope, - }); - BubbleId(id) + pub(crate) fn next_element_ref(&mut self, element_ref: ElementRef) -> BubbleId { + BubbleId( + self.element_refs + .insert(unsafe { std::mem::transmute(element_ref) }), + ) } pub(crate) fn reclaim(&mut self, el: ElementId) { @@ -77,15 +64,13 @@ impl VirtualDom { self.elements.try_remove(el.0).map(|_| ()) } - pub(crate) fn set_template(&mut self, el: ElementId, node: &VNode, path: &'static [u8]) { + pub(crate) fn set_template(&mut self, el: ElementId, element_ref: ElementRef) { match self.elements[el.0] { Some(bubble_id) => { - self.element_refs[bubble_id.0].path = ElementPath { path }; - self.element_refs[bubble_id.0].template = - unsafe { NonNull::new_unchecked(node as *const _ as *mut _) }; + self.element_refs[bubble_id.0] = unsafe { std::mem::transmute(element_ref) }; } None => { - self.elements[el.0] = Some(self.next_reference(node, ElementPath { path })); + self.elements[el.0] = Some(self.next_element_ref(element_ref)); } } } diff --git a/packages/core/src/create.rs b/packages/core/src/create.rs index f8ae3add3..e89942f36 100644 --- a/packages/core/src/create.rs +++ b/packages/core/src/create.rs @@ -1,6 +1,7 @@ use crate::any_props::AnyProps; -use crate::diff::BoundaryRef; -use crate::innerlude::{BorrowedAttributeValue, VComponent, VPlaceholder, VText}; +use crate::innerlude::{ + BorrowedAttributeValue, ElementPath, ElementRef, VComponent, VPlaceholder, VText, +}; use crate::mutations::Mutation; use crate::mutations::Mutation::*; use crate::nodes::VNode; @@ -182,7 +183,14 @@ impl<'b> VirtualDom { use DynamicNode::*; match &template.dynamic_nodes[idx] { node @ Component { .. } | node @ Fragment(_) => { - self.create_dynamic_node(template, idx, node, idx) + let template_ref = ElementRef { + path: ElementPath { + path: &template.template.get().node_paths[idx], + }, + template: &template, + scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), + }; + self.create_dynamic_node(template_ref, node) } Placeholder(VPlaceholder { id }) => { let id = self.set_slot(id); @@ -266,7 +274,14 @@ impl<'b> VirtualDom { .map(|sorted_index| dynamic_nodes[sorted_index].0); for idx in reversed_iter { - let m = self.create_dynamic_node(template, idx, &template.dynamic_nodes[idx], idx); + let boundary_ref = ElementRef { + path: ElementPath { + path: &template.template.get().node_paths[idx], + }, + template, + scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), + }; + let m = self.create_dynamic_node(boundary_ref, &template.dynamic_nodes[idx]); if m > 0 { // The path is one shorter because the top node is the root let path = &template.template.get().node_paths[idx][1..]; @@ -315,7 +330,12 @@ impl<'b> VirtualDom { match &attribute.value { AttributeValue::Listener(_) => { let path = &template.template.get().attr_paths[idx]; - self.set_template(id, template, path); + let element_ref = ElementRef { + path: ElementPath { path }, + template, + scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), + }; + self.set_template(id, element_ref); self.mutations.push(NewEventListener { // all listeners start with "on" name: &unbounded_name[2..], @@ -446,19 +466,19 @@ impl<'b> VirtualDom { pub(crate) fn create_dynamic_node( &mut self, - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, node: &'b DynamicNode<'b>, ) -> usize { use DynamicNode::*; match node { Text(text) => self.create_dynamic_text(parent, text), Placeholder(place) => self.create_placeholder(place, parent), - Component(component) => self.create_component_node(parent, component), + Component(component) => self.create_component_node(Some(parent), component), Fragment(frag) => frag.iter().map(|child| self.create(child)).sum(), } } - fn create_dynamic_text(&mut self, parent: BoundaryRef<'b>, text: &'b VText<'b>) -> usize { + fn create_dynamic_text(&mut self, parent: ElementRef<'b>, text: &'b VText<'b>) -> usize { // Allocate a dynamic element reference for this text node let new_id = self.next_element(); @@ -471,7 +491,7 @@ impl<'b> VirtualDom { // Add the mutation to the list self.mutations.push(HydrateText { id: new_id, - path: &parent.parent.template.get().node_paths[parent.dyn_node_idx][1..], + path: &parent.path.path[1..], value, }); @@ -482,7 +502,7 @@ impl<'b> VirtualDom { pub(crate) fn create_placeholder( &mut self, placeholder: &VPlaceholder, - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) -> usize { // Allocate a dynamic element reference for this text node let id = self.next_element(); @@ -492,7 +512,7 @@ impl<'b> VirtualDom { // Assign the ID to the existing node in the template self.mutations.push(AssignId { - path: &parent.parent.template.get().node_paths[parent.dyn_node_idx][1..], + path: &parent.path.path[1..], id, }); @@ -502,7 +522,7 @@ impl<'b> VirtualDom { pub(super) fn create_component_node( &mut self, - parent: BoundaryRef<'b>, + parent: Option>, component: &'b VComponent<'b>, ) -> usize { use RenderReturn::*; diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 80ccbc315..29a135be6 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -1,7 +1,10 @@ use crate::{ any_props::AnyProps, arena::ElementId, - innerlude::{BorrowedAttributeValue, DirtyScope, VComponent, VPlaceholder, VText}, + innerlude::{ + BorrowedAttributeValue, DirtyScope, ElementPath, ElementRef, VComponent, VPlaceholder, + VText, + }, mutations::Mutation, nodes::RenderReturn, nodes::{DynamicNode, VNode}, @@ -81,7 +84,10 @@ impl<'b> VirtualDom { if let Some(&template) = map.get(&byte_index) { right_template.template.set(template); if template != left_template.template.get() { - return self.replace(left_template, [right_template], todo!()); + let parent = left_template.parent.take().map(|parent_id| unsafe { + std::mem::transmute(self.element_refs[parent_id.0]) + }); + return self.replace(left_template, [right_template], parent); } } } @@ -99,7 +105,7 @@ impl<'b> VirtualDom { // If the templates are different by name, we need to replace the entire template if templates_are_different(left_template, right_template) { - return self.light_diff_templates(left_template, right_template, todo!()); + return self.light_diff_templates(left_template, right_template); } // If the templates are the same, we can diff the attributes and children @@ -129,9 +135,12 @@ impl<'b> VirtualDom { .zip(right_template.dynamic_nodes.iter()) .enumerate() .for_each(|(dyn_node_idx, (left_node, right_node))| { - let current_ref = BoundaryRef { - parent: right_template, - dyn_node_idx, + let current_ref = ElementRef { + template: right_template.into(), + path: ElementPath { + path: left_template.template.get().node_paths[dyn_node_idx], + }, + scope: self.runtime.scope_stack.borrow().last().copied().unwrap(), }; self.diff_dynamic_node(left_node, right_node, current_ref); }); @@ -150,13 +159,13 @@ impl<'b> VirtualDom { &mut self, left_node: &'b DynamicNode<'b>, right_node: &'b DynamicNode<'b>, - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { match (left_node, right_node) { (Text(left), Text(right)) => self.diff_vtext(left, right), (Fragment(left), Fragment(right)) => self.diff_non_empty_fragment(left, right, parent), (Placeholder(left), Placeholder(right)) => right.id.set(left.id.get()), - (Component(left), Component(right)) => self.diff_vcomponent(left, right, parent), + (Component(left), Component(right)) => self.diff_vcomponent(left, right, Some(parent)), (Placeholder(left), Fragment(right)) => self.replace_placeholder(left, *right, parent), (Fragment(left), Placeholder(right)) => self.node_to_placeholder(left, right), _ => todo!("This is an usual custom case for dynamic nodes. We don't know how to handle it yet."), @@ -179,7 +188,7 @@ impl<'b> VirtualDom { &mut self, left: &'b VComponent<'b>, right: &'b VComponent<'b>, - parent: BoundaryRef<'b>, + parent: Option>, ) { if std::ptr::eq(left, right) { return; @@ -224,7 +233,7 @@ impl<'b> VirtualDom { &mut self, right: &'b VComponent<'b>, left: &'b VComponent<'b>, - parent: BoundaryRef<'b>, + parent: Option>, ) { let m = self.create_component_node(parent, right); @@ -280,17 +289,16 @@ impl<'b> VirtualDom { /// Component { ..props } /// } /// ``` - fn light_diff_templates( - &mut self, - left: &'b VNode<'b>, - right: &'b VNode<'b>, - parent: BoundaryRef<'b>, - ) { + fn light_diff_templates(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { + let parent = left + .parent + .take() + .map(|parent_id| unsafe { std::mem::transmute(self.element_refs[parent_id.0]) }); match matching_components(left, right) { None => self.replace(left, [right], parent), Some(components) => components .into_iter() - .for_each(|(idx, l, r)| self.diff_vcomponent(l, r, parent)), + .for_each(|(l, r)| self.diff_vcomponent(l, r, parent)), } } @@ -312,7 +320,7 @@ impl<'b> VirtualDom { &mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>], - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { let new_is_keyed = new[0].key.is_some(); let old_is_keyed = old[0].key.is_some(); @@ -344,7 +352,7 @@ impl<'b> VirtualDom { &mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>], - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { use std::cmp::Ordering; @@ -385,7 +393,7 @@ impl<'b> VirtualDom { &mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>], - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { if cfg!(debug_assertions) { let mut keys = rustc_hash::FxHashSet::default(); @@ -464,7 +472,7 @@ impl<'b> VirtualDom { &mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>], - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) -> Option<(usize, usize)> { let mut left_offset = 0; @@ -523,7 +531,7 @@ impl<'b> VirtualDom { &mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>], - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { /* 1. Map the old keys into a numerical ordering based on indices. @@ -581,7 +589,7 @@ impl<'b> VirtualDom { if shared_keys.is_empty() { if old.get(0).is_some() { self.remove_nodes(&old[1..]); - self.replace(&old[0], new, parent); + self.replace(&old[0], new, Some(parent)); } else { // I think this is wrong - why are we appending? // only valid of the if there are no trailing elements @@ -762,7 +770,7 @@ impl<'b> VirtualDom { fn create_children( &mut self, nodes: impl IntoIterator>, - parent: BoundaryRef<'b>, + parent: Option>, ) -> usize { nodes.into_iter().fold(0, |acc, child| { self.assign_boundary_ref(parent, child); @@ -774,9 +782,9 @@ impl<'b> VirtualDom { &mut self, new: &'b [VNode<'b>], before: &'b VNode<'b>, - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { - let m = self.create_children(new, parent); + let m = self.create_children(new, Some(parent)); let id = self.find_first_element(before); self.mutations.push(Mutation::InsertBefore { id, m }) } @@ -785,9 +793,9 @@ impl<'b> VirtualDom { &mut self, new: &'b [VNode<'b>], after: &'b VNode<'b>, - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { - let m = self.create_children(new, parent); + let m = self.create_children(new, Some(parent)); let id = self.find_last_element(after); self.mutations.push(Mutation::InsertAfter { id, m }) } @@ -797,9 +805,9 @@ impl<'b> VirtualDom { &mut self, l: &'b VPlaceholder, r: impl IntoIterator>, - parent: BoundaryRef<'b>, + parent: ElementRef<'b>, ) { - let m = self.create_children(r, parent); + let m = self.create_children(r, Some(parent)); let id = l.id.get().unwrap(); self.mutations.push(Mutation::ReplaceWith { id, m }); self.reclaim(id); @@ -809,7 +817,7 @@ impl<'b> VirtualDom { &mut self, left: &'b VNode<'b>, right: impl IntoIterator>, - parent: BoundaryRef<'b>, + parent: Option>, ) { let m = self.create_children(right, parent); @@ -1031,13 +1039,15 @@ impl<'b> VirtualDom { } } - pub(crate) fn assign_boundary_ref(&mut self, parent: BoundaryRef, child: &VNode<'_>) { - // assign the parent - let path = parent.parent.template.get().node_paths[parent.dyn_node_idx]; - - child - .parent - .set(Some(self.next_element_ref(parent.parent, path))); + pub(crate) fn assign_boundary_ref( + &mut self, + parent: Option>, + child: &'b VNode<'b>, + ) { + if let Some(parent) = parent { + // assign the parent of the child + child.parent.set(Some(self.next_element_ref(parent))); + } } } @@ -1060,7 +1070,7 @@ fn templates_are_different(left_template: &VNode, right_template: &VNode) -> boo fn matching_components<'a>( left: &'a VNode<'a>, right: &'a VNode<'a>, -) -> Option, &'a VComponent<'a>)>> { +) -> Option, &'a VComponent<'a>)>> { let left_template = left.template.get(); let right_template = right.template.get(); if left_template.roots.len() != right_template.roots.len() { @@ -1072,8 +1082,7 @@ fn matching_components<'a>( .roots .iter() .zip(right_template.roots.iter()) - .enumerate() - .map(|(idx, (l, r))| { + .map(|(l, r)| { let (l, r) = match (l, r) { (TemplateNode::Dynamic { id: l }, TemplateNode::Dynamic { id: r }) => (l, r), _ => return None, @@ -1084,7 +1093,7 @@ fn matching_components<'a>( _ => return None, }; - Some((idx, l, r)) + Some((l, r)) }) .collect() } @@ -1116,9 +1125,3 @@ fn is_dyn_node_only_child(node: &VNode, idx: usize) -> bool { _ => false, } } - -// A parent that can be used for bubbling -pub(crate) struct BoundaryRef<'b> { - pub parent: &'b VNode<'b>, - pub dyn_node_idx: usize, -} diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index faf1ecf44..48287a7b7 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -183,7 +183,7 @@ pub struct VirtualDom { pub(crate) templates: FxHashMap>>, // Every element is actually a dual reference - one to the template and the other to the dynamic node in that template - pub(crate) element_refs: Slab, + pub(crate) element_refs: Slab>, // The element ids that are used in the renderer pub(crate) elements: Slab>, @@ -353,13 +353,11 @@ impl VirtualDom { | | | <-- no, broke early | <-- no, broke early */ - println!("calling {:?}", element); let element = match self.elements.get(element.0) { Some(Some(el)) => el, _ => return, }; let mut parent_path = self.element_refs.get(element.0).cloned(); - println!("parent path {:?}", parent_path); let mut listeners = vec![]; // We will clone this later. The data itself is wrapped in RC to be used in callbacks if required @@ -374,7 +372,6 @@ impl VirtualDom { while let Some(el_ref) = parent_path { // safety: we maintain references of all vnodes in the element slab let template = el_ref.template; - let template = unsafe { template.as_ref() }; let node_template = template.template.get(); let target_path = el_ref.path; @@ -425,7 +422,6 @@ impl VirtualDom { if let Some(el_ref) = parent_path { // safety: we maintain references of all vnodes in the element slab let template = el_ref.template; - let template = unsafe { template.as_ref() }; let node_template = template.template.get(); let target_path = el_ref.path; diff --git a/packages/core/tests/event_propagation.rs b/packages/core/tests/event_propagation.rs new file mode 100644 index 000000000..1fbf82c4a --- /dev/null +++ b/packages/core/tests/event_propagation.rs @@ -0,0 +1,64 @@ +use dioxus::prelude::*; +use dioxus_core::ElementId; +use std::{rc::Rc, sync::Mutex}; + +static CLICKS: Mutex = Mutex::new(0); + +#[test] +fn events_propagate() { + let mut dom = VirtualDom::new(app); + _ = dom.rebuild(); + + // Top-level click is registered + dom.handle_event("click", Rc::new(MouseData::default()), ElementId(1), true); + assert_eq!(*CLICKS.lock().unwrap(), 1); + + // break reference.... + for _ in 0..5 { + dom.mark_dirty(ScopeId(0)); + _ = dom.rebuild(); + } + + // Lower click is registered + dom.handle_event("click", Rc::new(MouseData::default()), ElementId(2), true); + assert_eq!(*CLICKS.lock().unwrap(), 3); + + // break reference.... + for _ in 0..5 { + dom.mark_dirty(ScopeId(0)); + _ = dom.rebuild(); + } + + // Stop propagation occurs + dom.handle_event("click", Rc::new(MouseData::default()), ElementId(2), true); + assert_eq!(*CLICKS.lock().unwrap(), 3); +} + +fn app(cx: Scope) -> Element { + render! { + div { + onclick: move |_| { + println!("top clicked"); + *CLICKS.lock().unwrap() += 1; + }, + + problematic_child {} + } + } +} + +fn problematic_child(cx: Scope) -> Element { + render! { + button { + onclick: move |evt| { + let mut clicks = CLICKS.lock().unwrap(); + + if *clicks == 3 { + evt.stop_propagation(); + } else { + *clicks += 1; + } + } + } + } +} diff --git a/packages/core/tests/fuzzing.rs b/packages/core/tests/fuzzing.rs index 1523c4e22..95c3ed9bf 100644 --- a/packages/core/tests/fuzzing.rs +++ b/packages/core/tests/fuzzing.rs @@ -271,10 +271,9 @@ fn create_random_element(cx: Scope) -> Element { ) .into_boxed_str(), )); - // println!("{template:#?}"); let node = VNode { key: None, - parent: None, + parent: Default::default(), template: Cell::new(template), root_ids: bumpalo::collections::Vec::new_in(cx.bump()).into(), dynamic_nodes: {