Merge pull request #1220 from DioxusLabs/jk/event-bubbling-fix

Don't use boxed cell slice, use a refcell instead

packages/core/src/create.rs

@@ -88,8 +88,7 @@ impl<'b> VirtualDom {
         }
 
         // Intialize the root nodes slice
-        node.root_ids
-            .intialize(vec![ElementId(0); node.template.get().roots.len()].into_boxed_slice());
+        *node.root_ids.borrow_mut() = vec![ElementId(0); node.template.get().roots.len()];
 
         // The best renderers will have templates prehydrated and registered
         // Just in case, let's create the template using instructions anyways
@@ -328,7 +327,7 @@ impl<'b> VirtualDom {
     fn load_template_root(&mut self, template: &VNode, root_idx: usize) -> ElementId {
         // Get an ID for this root since it's a real root
         let this_id = self.next_root(template, root_idx);
-        template.root_ids.set(root_idx, this_id);
+        template.root_ids.borrow_mut()[root_idx] = this_id;
 
         self.mutations.push(LoadTemplate {
             name: template.template.get().name,

packages/core/src/diff.rs

@@ -129,12 +129,14 @@ impl<'b> VirtualDom {
             });
 
         // Make sure the roots get transferred over while we're here
-        right_template.root_ids.transfer(&left_template.root_ids);
+        *right_template.root_ids.borrow_mut() = left_template.root_ids.borrow().clone();
+
+        let root_ids = right_template.root_ids.borrow();
 
         // Update the node refs
-        for i in 0..right_template.root_ids.len() {
-            if let Some(root_id) = right_template.root_ids.get(i) {
-                self.update_template(root_id, right_template);
+        for i in 0..root_ids.len() {
+            if let Some(root_id) = root_ids.get(i) {
+                self.update_template(*root_id, right_template);
             }
         }
     }
@@ -686,7 +688,7 @@ impl<'b> VirtualDom {
                     Some(node) => node,
                     None => {
                         self.mutations.push(Mutation::PushRoot {
-                            id: node.root_ids.get(idx).unwrap(),
+                            id: node.root_ids.borrow()[idx],
                         });
                         return 1;
                     }
@@ -821,7 +823,7 @@ impl<'b> VirtualDom {
             if let Some(dy) = node.dynamic_root(idx) {
                 self.remove_dynamic_node(dy, gen_muts);
             } else {
-                let id = node.root_ids.get(idx).unwrap();
+                let id = node.root_ids.borrow()[idx];
                 if gen_muts {
                     self.mutations.push(Mutation::Remove { id });
                 }
@@ -928,7 +930,7 @@ impl<'b> VirtualDom {
 
     fn find_first_element(&self, node: &'b VNode<'b>) -> ElementId {
         match node.dynamic_root(0) {
-            None => node.root_ids.get(0).unwrap(),
+            None => node.root_ids.borrow()[0],
             Some(Text(t)) => t.id.get().unwrap(),
             Some(Fragment(t)) => self.find_first_element(&t[0]),
             Some(Placeholder(t)) => t.id.get().unwrap(),
@@ -944,7 +946,7 @@ impl<'b> VirtualDom {
 
     fn find_last_element(&self, node: &'b VNode<'b>) -> ElementId {
         match node.dynamic_root(node.template.get().roots.len() - 1) {
-            None => node.root_ids.last().unwrap(),
+            None => *node.root_ids.borrow().last().unwrap(),
             Some(Text(t)) => t.id.get().unwrap(),
             Some(Fragment(t)) => self.find_last_element(t.last().unwrap()),
             Some(Placeholder(t)) => t.id.get().unwrap(),

packages/core/src/nodes.rs

@@ -5,7 +5,7 @@ use bumpalo::boxed::Box as BumpBox;
 use bumpalo::Bump;
 use std::{
     any::{Any, TypeId},
-    cell::{Cell, RefCell, UnsafeCell},
+    cell::{Cell, RefCell},
     fmt::{Arguments, Debug},
 };
 
@@ -54,7 +54,7 @@ pub struct VNode<'a> {
 
     /// The IDs for the roots of this template - to be used when moving the template around and removing it from
     /// the actual Dom
-    pub root_ids: BoxedCellSlice,
+    pub root_ids: RefCell<Vec<ElementId>>,
 
     /// The dynamic parts of the template
     pub dynamic_nodes: &'a [DynamicNode<'a>],
@@ -63,112 +63,13 @@ pub struct VNode<'a> {
     pub dynamic_attrs: &'a [Attribute<'a>],
 }
 
-// Saftey: There is no way to get references to the internal data of this struct so no refrences will be invalidated by mutating the data with a immutable reference (The same principle behind Cell)
-#[derive(Debug, Default)]
-pub struct BoxedCellSlice(UnsafeCell<Option<Box<[ElementId]>>>);
-
-impl Clone for BoxedCellSlice {
-    fn clone(&self) -> Self {
-        Self(UnsafeCell::new(unsafe { (*self.0.get()).clone() }))
-    }
-}
-
-impl BoxedCellSlice {
-    pub fn last(&self) -> Option<ElementId> {
-        unsafe {
-            (*self.0.get())
-                .as_ref()
-                .and_then(|inner| inner.as_ref().last().copied())
-        }
-    }
-
-    pub fn get(&self, idx: usize) -> Option<ElementId> {
-        unsafe {
-            (*self.0.get())
-                .as_ref()
-                .and_then(|inner| inner.as_ref().get(idx).copied())
-        }
-    }
-
-    pub unsafe fn get_unchecked(&self, idx: usize) -> Option<ElementId> {
-        (*self.0.get())
-            .as_ref()
-            .and_then(|inner| inner.as_ref().get(idx).copied())
-    }
-
-    pub fn set(&self, idx: usize, new: ElementId) {
-        unsafe {
-            if let Some(inner) = &mut *self.0.get() {
-                inner[idx] = new;
-            }
-        }
-    }
-
-    pub fn intialize(&self, contents: Box<[ElementId]>) {
-        unsafe {
-            *self.0.get() = Some(contents);
-        }
-    }
-
-    pub fn transfer(&self, other: &Self) {
-        unsafe {
-            *self.0.get() = (*other.0.get()).clone();
-        }
-    }
-
-    pub fn take_from(&self, other: &Self) {
-        unsafe {
-            *self.0.get() = (*other.0.get()).take();
-        }
-    }
-
-    pub fn len(&self) -> usize {
-        unsafe {
-            (*self.0.get())
-                .as_ref()
-                .map(|inner| inner.len())
-                .unwrap_or(0)
-        }
-    }
-}
-
-impl<'a> IntoIterator for &'a BoxedCellSlice {
-    type Item = ElementId;
-
-    type IntoIter = BoxedCellSliceIter<'a>;
-
-    fn into_iter(self) -> Self::IntoIter {
-        BoxedCellSliceIter {
-            index: 0,
-            borrow: self,
-        }
-    }
-}
-
-pub struct BoxedCellSliceIter<'a> {
-    index: usize,
-    borrow: &'a BoxedCellSlice,
-}
-
-impl Iterator for BoxedCellSliceIter<'_> {
-    type Item = ElementId;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        let result = self.borrow.get(self.index);
-        if result.is_some() {
-            self.index += 1;
-        }
-        result
-    }
-}
-
 impl<'a> VNode<'a> {
     /// Create a template with no nodes that will be skipped over during diffing
     pub fn empty() -> Element<'a> {
         Some(VNode {
             key: None,
             parent: None,
-            root_ids: BoxedCellSlice::default(),
+            root_ids: Default::default(),
             dynamic_nodes: &[],
             dynamic_attrs: &[],
             template: Cell::new(Template {

packages/core/src/scope_arena.rs

@@ -83,12 +83,12 @@ impl VirtualDom {
             id: scope.id,
         });
 
-        if matches!(allocated, RenderReturn::Aborted(_)) {
-            if scope.suspended.get() {
+        if scope.suspended.get() {
+            if matches!(allocated, RenderReturn::Aborted(_)) {
                 self.suspended_scopes.insert(scope.id);
-            } else if !self.suspended_scopes.is_empty() {
-                _ = self.suspended_scopes.remove(&scope.id);
             }
+        } else if !self.suspended_scopes.is_empty() {
+            _ = self.suspended_scopes.remove(&scope.id);
         }
 
         // rebind the lifetime now that its stored internally

packages/core/src/virtual_dom.rs

@@ -571,12 +571,15 @@ impl VirtualDom {
     /// The mutations will be thrown out, so it's best to use this method for things like SSR that have async content
     pub async fn wait_for_suspense(&mut self) {
         loop {
+            // println!("waiting for suspense {:?}", self.suspended_scopes);
             if self.suspended_scopes.is_empty() {
                 return;
             }
 
+            // println!("waiting for suspense");
             self.wait_for_work().await;
 
+            // println!("Rendered immediately");
             _ = self.render_immediate();
         }
     }

packages/core/tests/task.rs

@@ -1,9 +1,9 @@
 //! Verify that tasks get polled by the virtualdom properly, and that we escape wait_for_work safely
 
 use dioxus::prelude::*;
-use std::time::Duration;
+use std::{sync::atomic::AtomicUsize, time::Duration};
 
-static mut POLL_COUNT: usize = 0;
+static POLL_COUNT: AtomicUsize = AtomicUsize::new(0);
 
 #[tokio::test]
 async fn it_works() {
@@ -18,7 +18,10 @@ async fn it_works() {
 
     // By the time the tasks are finished, we should've accumulated ticks from two tasks
     // Be warned that by setting the delay to too short, tokio might not schedule in the tasks
-    assert_eq!(unsafe { POLL_COUNT }, 135);
+    assert_eq!(
+        POLL_COUNT.fetch_add(0, std::sync::atomic::Ordering::Relaxed),
+        135
+    );
 }
 
 fn app(cx: Scope) -> Element {
@@ -26,14 +29,14 @@ fn app(cx: Scope) -> Element {
         cx.spawn(async {
             for x in 0..10 {
                 tokio::time::sleep(Duration::from_micros(50)).await;
-                unsafe { POLL_COUNT += x }
+                POLL_COUNT.fetch_add(x, std::sync::atomic::Ordering::Relaxed);
             }
         });
 
         cx.spawn(async {
             for x in 0..10 {
                 tokio::time::sleep(Duration::from_micros(25)).await;
-                unsafe { POLL_COUNT += x * 2 }
+                POLL_COUNT.fetch_add(x * 2, std::sync::atomic::Ordering::Relaxed);
             }
         });
     });

packages/dioxus-tui/examples/colorpicker.rs

@@ -20,7 +20,7 @@ fn app(cx: Scope) -> Element {
             background_color: "hsl({hue}, 70%, {brightness}%)",
             onmousemove: move |evt| {
                 if let RenderReturn::Ready(node) = cx.root_node() {
-                    if let Some(id) = node.root_ids.get(0){
+                    if let Some(id) = node.root_ids.borrow().get(0).cloned() {
                         let node = tui_query.get(mapping.get_node_id(id).unwrap());
                         let Size{width, height} = node.size().unwrap();
                         let pos = evt.inner().element_coordinates();

packages/web/src/rehydrate.rs

@@ -90,7 +90,7 @@ impl WebsysDom {
             // make sure we set the root node ids even if the node is not dynamic
             set_node(
                 hydrated,
-                vnode.root_ids.get(i).ok_or(VNodeNotInitialized)?,
+                *vnode.root_ids.borrow().get(i).ok_or(VNodeNotInitialized)?,
                 current_child.clone()?,
             );