Switch to owned listener type (#4289)

* Switch to owned listener type

* remove html event workaround

* fix event conversion

* keep track of the origin scope

* fix clippy

* fix docs

* Use "ListenerCallback" instead of "ListenerCb"

* Add a small example

---------

Co-authored-by: Jonathan Kelley <jkelleyrtp@gmail.com>

packages/core/src/events.rs

@@ -1,6 +1,8 @@
-use crate::{properties::SuperFrom, runtime::RuntimeGuard, Runtime, ScopeId};
+use crate::{
+    prelude::current_scope_id, properties::SuperFrom, runtime::RuntimeGuard, Runtime, ScopeId,
+};
 use generational_box::GenerationalBox;
-use std::{cell::RefCell, marker::PhantomData, panic::Location, rc::Rc};
+use std::{any::Any, cell::RefCell, marker::PhantomData, panic::Location, rc::Rc};
 
 /// A wrapper around some generic data that handles the event's state
 ///
@@ -67,6 +69,17 @@ impl<T: ?Sized> Event<T> {
         }
     }
 
+    /// Convert this event into a boxed event with a dynamic type
+    pub fn into_any(self) -> Event<dyn Any>
+    where
+        T: Sized,
+    {
+        Event {
+            data: self.data as Rc<dyn Any>,
+            metadata: self.metadata,
+        }
+    }
+
     /// Prevent this event from continuing to bubble up the tree to parent elements.
     ///
     /// # Example
@@ -376,6 +389,27 @@ impl<
     }
 }
 
+impl<
+        Function: FnMut(Event<T>) -> Spawn + 'static,
+        T: 'static,
+        Spawn: SpawnIfAsync<Marker> + 'static,
+        Marker,
+    > SuperFrom<Function, MarkerWrapper<Marker>> for ListenerCallback<T>
+{
+    fn super_from(input: Function) -> Self {
+        ListenerCallback::new(input)
+    }
+}
+
+// ListenerCallback<T> can be created from Callback<Event<T>>
+impl<T: 'static> SuperFrom<Callback<Event<T>>> for ListenerCallback<T> {
+    fn super_from(input: Callback<Event<T>>) -> Self {
+        // https://github.com/rust-lang/rust-clippy/issues/15072
+        #[allow(clippy::redundant_closure)]
+        ListenerCallback::new(move |event| input(event))
+    }
+}
+
 #[doc(hidden)]
 pub struct UnitClosure<Marker>(PhantomData<Marker>);
 
@@ -471,14 +505,6 @@ impl<Args: 'static, Ret: 'static> Callback<Args, Ret> {
         Self { callback, origin }
     }
 
-    /// Leak a new reference to the [`Callback`] that will not be dropped unless the callback is dropped manually
-    pub(crate) fn leak_reference(&self) -> generational_box::BorrowResult<Callback<Args, Ret>> {
-        Ok(Callback {
-            callback: self.callback.leak_reference()?,
-            origin: self.origin,
-        })
-    }
-
     /// Call this callback with the appropriate argument type
     ///
     /// This borrows the callback using a RefCell. Recursively calling a callback will cause a panic.
@@ -559,3 +585,87 @@ impl<Args: 'static, Ret: 'static> std::ops::Deref for Callback<Args, Ret> {
         reference_to_closure as &_
     }
 }
+
+type AnyEventHandler = Rc<RefCell<dyn FnMut(Event<dyn Any>)>>;
+
+/// An owned callback type used in [`AttributeValue::Listener`](crate::AttributeValue::Listener).
+///
+/// This is the type that powers the `on` attributes in the `rsx!` macro, allowing you to pass event
+/// handlers to elements.
+///
+/// ```rust, ignore
+/// rsx! {
+///     button {
+///         onclick: AttributeValue::Listener(ListenerCallback::new(move |evt: Event<MouseData>| {
+///             // ...
+///         }
+///     }
+/// }
+/// ```
+pub struct ListenerCallback<T = ()> {
+    pub(crate) origin: ScopeId,
+    callback: AnyEventHandler,
+    _marker: PhantomData<T>,
+}
+
+impl<T> Clone for ListenerCallback<T> {
+    fn clone(&self) -> Self {
+        Self {
+            origin: self.origin,
+            callback: self.callback.clone(),
+            _marker: PhantomData,
+        }
+    }
+}
+
+impl<T> PartialEq for ListenerCallback<T> {
+    fn eq(&self, other: &Self) -> bool {
+        // We compare the pointers of the callbacks, since they are unique
+        Rc::ptr_eq(&self.callback, &other.callback) && self.origin == other.origin
+    }
+}
+
+impl<T> ListenerCallback<T> {
+    /// Create a new [`ListenerCallback`] from a callback
+    ///
+    /// This is expected to be called within a runtime scope. Make sure a runtime is current before
+    /// calling this method.
+    pub fn new<MaybeAsync, Marker>(mut f: impl FnMut(Event<T>) -> MaybeAsync + 'static) -> Self
+    where
+        T: 'static,
+        MaybeAsync: SpawnIfAsync<Marker>,
+    {
+        Self {
+            origin: current_scope_id().expect("ListenerCallback must be created within a scope"),
+            callback: Rc::new(RefCell::new(move |event: Event<dyn Any>| {
+                let data = event.data.downcast::<T>().unwrap();
+                f(Event {
+                    metadata: event.metadata.clone(),
+                    data,
+                })
+                .spawn();
+            })),
+            _marker: PhantomData,
+        }
+    }
+
+    /// Call the callback with an event
+    ///
+    /// This is expected to be called within a runtime scope. Make sure a runtime is current before
+    /// calling this method.
+    pub fn call(&self, event: Event<dyn Any>) {
+        let runtime = Runtime::current().expect("ListenerCallback must be called within a runtime");
+        runtime.with_scope_on_stack(self.origin, || {
+            (self.callback.borrow_mut())(event);
+        });
+    }
+
+    /// Erase the type of the callback, allowing it to be used with any type of event
+    pub fn erase(self) -> ListenerCallback {
+        ListenerCallback {
+            origin: self.origin,
+            callback: self.callback,
+            _marker: PhantomData,
+        }
+    }
+}

packages/core/src/lib.rs

@@ -79,10 +79,10 @@ pub(crate) mod innerlude {
 pub use crate::innerlude::{
     fc_to_builder, generation, schedule_update, schedule_update_any, use_hook, vdom_is_rendering,
     AnyValue, Attribute, AttributeValue, CapturedError, Component, ComponentFunction, DynamicNode,
-    Element, ElementId, Event, Fragment, HasAttributes, IntoDynNode, LaunchConfig, MarkerWrapper,
-    Mutation, Mutations, NoOpMutations, Ok, Properties, Result, Runtime, ScopeId, ScopeState,
-    SpawnIfAsync, Task, Template, TemplateAttribute, TemplateNode, VComponent, VNode, VNodeInner,
-    VPlaceholder, VText, VirtualDom, WriteMutations,
+    Element, ElementId, Event, Fragment, HasAttributes, IntoDynNode, LaunchConfig,
+    ListenerCallback, MarkerWrapper, Mutation, Mutations, NoOpMutations, Ok, Properties, Result,
+    Runtime, ScopeId, ScopeState, SpawnIfAsync, Task, Template, TemplateAttribute, TemplateNode,
+    VComponent, VNode, VNodeInner, VPlaceholder, VText, VirtualDom, WriteMutations,
 };
 
 /// The purpose of this module is to alleviate imports of many common types

packages/core/src/nodes.rs

@@ -1,11 +1,12 @@
 use dioxus_core_types::DioxusFormattable;
 
+use crate::events::ListenerCallback;
 use crate::innerlude::VProps;
 use crate::prelude::RenderError;
 use crate::{any_props::BoxedAnyProps, innerlude::ScopeState};
 use crate::{arena::ElementId, Element, Event};
 use crate::{
-    innerlude::{ElementRef, EventHandler, MountId},
+    innerlude::{ElementRef, MountId},
     properties::ComponentFunction,
 };
 use crate::{Properties, ScopeId, VirtualDom};
@@ -157,29 +158,6 @@ impl Default for VNode {
     }
 }
 
-impl Drop for VNode {
-    fn drop(&mut self) {
-        // FIXME:
-        // TODO:
-        //
-        // We have to add this drop *here* because we can't add a drop impl to AttributeValue and
-        // keep semver compatibility. Adding a drop impl means you can't destructure the value, which
-        // we need to do for enums.
-        //
-        // if dropping this will drop the last vnode (rc count is 1), then we need to drop the listeners
-        // in this template
-        if Rc::strong_count(&self.vnode) == 1 {
-            for attrs in self.vnode.dynamic_attrs.iter() {
-                for attr in attrs.iter() {
-                    if let AttributeValue::Listener(listener) = &attr.value {
-                        listener.callback.manually_drop();
-                    }
-                }
-            }
-        }
-    }
-}
-
 impl PartialEq for VNode {
     fn eq(&self, other: &Self) -> bool {
         Rc::ptr_eq(&self.vnode, &other.vnode)
@@ -810,12 +788,7 @@ impl Attribute {
             name: self.name,
             namespace: self.namespace,
             volatile: self.volatile,
-            value: match &self.value {
-                AttributeValue::Listener(listener) => {
-                    AttributeValue::Listener(listener.leak_reference().unwrap())
-                }
-                value => value.clone(),
-            },
+            value: self.value.clone(),
         }
     }
 }
@@ -839,7 +812,7 @@ pub enum AttributeValue {
     Bool(bool),
 
     /// A listener, like "onclick"
-    Listener(ListenerCb),
+    Listener(ListenerCallback),
 
     /// An arbitrary value that implements PartialEq and is static
     Any(Rc<dyn AnyValue>),
@@ -852,16 +825,8 @@ impl AttributeValue {
     /// Create a new [`AttributeValue`] with the listener variant from a callback
     ///
     /// The callback must be confined to the lifetime of the ScopeState
-    pub fn listener<T: 'static>(mut callback: impl FnMut(Event<T>) + 'static) -> AttributeValue {
-        // TODO: maybe don't use the copy-variant of EventHandler here?
-        // Maybe, create an Owned variant so we are less likely to run into leaks
-        AttributeValue::Listener(EventHandler::leak(move |event: Event<dyn Any>| {
-            let data = event.data.downcast::<T>().unwrap();
-            callback(Event {
-                metadata: event.metadata.clone(),
-                data,
-            });
-        }))
+    pub fn listener<T: 'static>(callback: impl FnMut(Event<T>) + 'static) -> AttributeValue {
+        AttributeValue::Listener(ListenerCallback::new(callback).erase())
     }
 
     /// Create a new [`AttributeValue`] with a value that implements [`AnyValue`]
@@ -870,8 +835,6 @@ impl AttributeValue {
     }
 }
 
-pub type ListenerCb = EventHandler<Event<dyn Any>>;
-
 impl std::fmt::Debug for AttributeValue {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
@@ -879,7 +842,7 @@ impl std::fmt::Debug for AttributeValue {
             Self::Float(arg0) => f.debug_tuple("Float").field(arg0).finish(),
             Self::Int(arg0) => f.debug_tuple("Int").field(arg0).finish(),
             Self::Bool(arg0) => f.debug_tuple("Bool").field(arg0).finish(),
-            Self::Listener(listener) => f.debug_tuple("Listener").field(listener).finish(),
+            Self::Listener(_) => f.debug_tuple("Listener").finish(),
             Self::Any(_) => f.debug_tuple("Any").finish(),
             Self::None => write!(f, "None"),
         }
@@ -1197,6 +1160,12 @@ impl IntoAttributeValue for Rc<dyn AnyValue> {
     }
 }
 
+impl<T> IntoAttributeValue for ListenerCallback<T> {
+    fn into_value(self) -> AttributeValue {
+        AttributeValue::Listener(self.erase())
+    }
+}
+
 impl<T: IntoAttributeValue> IntoAttributeValue for Option<T> {
     fn into_value(self) -> AttributeValue {
         match self {