Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the onresize event handler to Element #2479

Merged
merged 23 commits into from
Aug 15, 2024

Conversation

ASR-ASU
Copy link
Contributor

@ASR-ASU ASR-ASU commented Jun 5, 2024

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 #1346.

@ASR-ASU ASR-ASU marked this pull request as draft June 6, 2024 22:17
@ASR-ASU ASR-ASU marked this pull request as ready for review June 7, 2024 19:46
@jkelleyrtp jkelleyrtp added enhancement New feature or request breaking This is a breaking change html Related to the html crate labels Jun 7, 2024
@jkelleyrtp
Copy link
Member

jkelleyrtp commented Jun 19, 2024

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 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 jkelleyrtp self-requested a review June 19, 2024 06:24
@ASR-ASU
Copy link
Contributor Author

ASR-ASU commented Jun 22, 2024

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 ASR-ASU marked this pull request as draft June 22, 2024 08:05
@ASR-ASU
Copy link
Contributor Author

ASR-ASU commented Jun 22, 2024

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 ASR-ASU changed the title Add the onresized event handler to Element Add the onresize event handler to Element Jun 22, 2024
@jkelleyrtp
Copy link
Member

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 jkelleyrtp linked an issue Jul 26, 2024 that may be closed by this pull request
@ASR-ASU ASR-ASU marked this pull request as ready for review August 4, 2024 18:52
@ealmloff ealmloff self-requested a review August 6, 2024 21:07
packages/liveview/src/element.rs Outdated Show resolved Hide resolved
packages/web/src/dom.rs Outdated Show resolved Hide resolved
packages/interpreter/src/unified_bindings.rs Outdated Show resolved Hide resolved
@ASR-ASU ASR-ASU requested a review from ealmloff August 9, 2024 18:22
Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this! I tweaked the event to make it fit a bit more closely with our other events. We have a trait on the web renderer to downcast events to the web-sys counterparts. I removed the wrapper around the observer size so we can implement that trait with a web-sys type.

I also added an example and tested on all platforms. It seems like this approach is pretty fast. I am getting ~16ms between the event being sent on desktop and the render being sent back

@ealmloff ealmloff enabled auto-merge (squash) August 15, 2024 00:59
@ealmloff ealmloff merged commit 2f49a89 into DioxusLabs:main Aug 15, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change enhancement New feature or request html Related to the html crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resize Event
3 participants