dioxus icon indicating copy to clipboard operation
dioxus copied to clipboard

Add the `onresize` event handler to Element

Open ASR-ASU opened this issue 1 year ago • 4 comments

This PR add the capability to handle the resize event for a given Element:

#[component]
fn MyComponent() {
    rsx! {
        div {
            onresized: move |cx| {
                let data = cx.data();

                let border_box_size = data.get_border_box_size();
                ::tracing::error!("border_box_size={:?}", border_box_size);

                let content_box_size = data.get_content_box_size();
                ::tracing::error!("content_box_size={:?}", content_box_size);
            }
        }
    }
}

It should give an answer to https://github.com/DioxusLabs/dioxus/issues/1346.

ASR-ASU avatar Jun 05 '24 20:06 ASR-ASU

Thanks for the PR! Sorry this languished.

Did a review with Evan today and we had some thoughts:

  • We think the name should be onresize not onresized. Our onmounted event should have been named onmount and will need to be corrected in the next dioxus version, so it'll be good if we merge this one with the right name.
  • The impl mostly looks good, but I think it'd be better to implement the special-casing logic in NewEventListener instead of the binding logic. This should let us dump all implementation for this directly into the .js files instead of the glue code as well as centralize logic for all listeners.
  • It would be ideal to not require a signature change on the initialize method. Is there some way we can emit the event using regular means without having to pass in the closure from the initializer?
  • Not required for this PR but we should mention somewhere how this might impact performance. When a window is being resized, all these handlers will fire which might cause lag/flickering. We could also mention in the docs how you might disable/enable this using an Option<EventHandler> which should also save performance.

Since you already have the knowledge for this PR, maybe you want to follow this up with onvisible using a similar approach? Both resize and visible will eventually need to take some options struct to make them more efficient, but rsx! doesn't have syntax for that just yet, so implementing them like how you've done should be fine for now.

jkelleyrtp avatar Jun 19 '24 06:06 jkelleyrtp

Thanks for your review.

We think the name should be onresize not onresized. Our onmounted event should have been named onmount and will need to be corrected in the next dioxus version, so it'll be good if we merge this one with the right name.

I was not aware of this change. I will rename onresized.

The impl mostly looks good, but I think it'd be better to implement the special-casing logic in NewEventListener instead of the binding logic. This should let us dump all implementation for this directly into the .js files instead of the glue code as well as centralize logic for all listeners.

Ok.

It would be ideal to not require a signature change on the initialize method. Is there some way we can emit the event using regular means without having to pass in the closure from the initializer?

Indeed. I'll make you a suggestion.

Not required for this PR but we should mention somewhere how this might impact performance. When a window is being resized, all these handlers will fire which might cause lag/flickering. We could also mention in the docs how you might disable/enable this using an Option which should also save performance.

I agree that we should mention that in the documentation. However, I don’t think I understood how an Option could help to save performance: if I'm not mistaken, a new Observer is only created, and resize events raised, when a component declares a onresized handler.

Since you already have the knowledge for this PR, maybe you want to follow this up with onvisible using a similar approach? Both resize and visible will eventually need to take some options struct to make them more efficient, but rsx! doesn't have syntax for that just yet, so implementing them like how you've done should be fine for now.

Good idea. Sure. Before that, we should clarify what are your expectations for the options structs. Is it related to your previous comment?

ASR-ASU avatar Jun 22 '24 08:06 ASR-ASU

For now, the application must define which field (block_size or inlineSize) corresponds to the width or the height of the observed component. Moving the special-casing logic to the ts could be an opportunity to manage this directly in the interpreter crate (using the Window.getComputedStyle() - cf. https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle), avoiding any potential confusion for developers using dioxus. However, calling the Window.getComputedStyle() function every time a resize event is fired might reduce performance. What do you think?

ASR-ASU avatar Jun 22 '24 11:06 ASR-ASU

I agree that we should mention that in the documentation. However, I don’t think I understood how an Option could help to save performance: if I'm not mistaken, a new Observer is only created, and resize events raised, when a component declares a onresized handler.

For performance, I'm just worried about dumping tons of onresize events into the VirtualDom when the window resizes. If many elements are listening to this event, we'll have a lot of processing. I'm not sure what the actual performance here is like - if it's actually bad - but we might want to provide some nudges in our doc-comments outlining that it could lead to lag/flickering if used too liberally.

For now, the application must define which field (block_size or inlineSize) corresponds to the width or the height of the observed component. Moving the special-casing logic to the ts could be an opportunity to manage this directly in the interpreter crate (using the Window.getComputedStyle() - cf. https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle), avoiding any potential confusion for developers using dioxus. However, calling the Window.getComputedStyle() function every time a resize event is fired might reduce performance. What do you think?

For web, let's try and keep it "lazy" and for desktop It hink it's okay to call getComputedStyle on the element.

Since you already have the knowledge for this PR, maybe you want to follow this up with onvisible using a similar approach? Both resize and visible will eventually need to take some options struct to make them more efficient, but rsx! doesn't have syntax for that just yet, so implementing them like how you've done should be fine for now.

ResizeObservers in JS have room for an options object that AFAIK is not actually used for anything. The onvisible impl will likely rely on IntersectionObserver which does have options (percent visibility...) which we won't be able to plumb into the observer using the declarative approach. Maybe one day we'll implement the imperative analog with all the options, but for both resized and visible we won't be able to pass in any extra constructor options with our current rsx! syntax.

jkelleyrtp avatar Jun 24 '24 17:06 jkelleyrtp