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

nodes inserted at position 0 have broken events #47

Closed
vwpix opened this issue Nov 12, 2021 · 6 comments
Closed

nodes inserted at position 0 have broken events #47

vwpix opened this issue Nov 12, 2021 · 6 comments
Assignees

Comments

@vwpix
Copy link

vwpix commented Nov 12, 2021

the new closures created at view() don't get passed to the corresponding elements
instead, every new node acts like the original first node, or panic on keyed nodes

i made an example code for this problem, also a video preview to illustrate it better

use {
    sauron::{
        html::text,
        prelude::*,
        Cmd, Application, Node, Program,
    },
};

struct Container {
    nodes: Vec<u32>,
    last: String
}
enum Msg {
    InsertNode,
    AddNode,
    ClickNode(u32)
}

impl Application<Msg> for Container {
    fn view(&self) -> Node<Msg> {
        let buttons = self.nodes.iter()
            .map(|&x|button([on_click(move |_|Msg::ClickNode(x))],[text(x)])).collect::<Vec<_>>();
        div([],[
            text(&self.last),
            button([on_click(|_|Msg::InsertNode)],[text("add item (broken)")]),
            button([on_click(|_|Msg::AddNode)],[text("add item (working)")]),
            div([id("buttons")],buttons)
        ])
    }
    fn update(&mut self, msg: Msg) -> Cmd<Self, Msg> {
        match msg {
            Msg::InsertNode => self.nodes.insert(0,self.nodes.len() as u32),
            Msg::AddNode => self.nodes.push(self.nodes.len() as u32),
            Msg::ClickNode(pos) => self.last = pos.to_string()
        }
        Cmd::none()
    }
}

#[wasm_bindgen(start)]
pub fn main() {
    Program::mount_to_body(
        Container{
            nodes: vec![], 
            last: String::default()
        }
    );
}

video preview
only tested this for position 0 so im not sure about the rest

@ivanceras
Copy link
Owner

Thank you @vwpix for a very detailed report on the issue. We will be working on this.

@ivanceras
Copy link
Owner

So, after some fiddling I realize that this is one of the case where the DOM diffing fails.
The code which deals with diffing elements in the DOM by comparing their attributes and event listener to see if the element can be reused or not. In rust, 2 closures are not equal even if they have the same exact code. This is not suitable for the dom-diffing algorithm as we want to be able to re-use the elements as much as possible otherwise, it will re-create the element for each view call which would makes the whole system very sluggish.
That said, we try to compare closures based only on their function signatures, in order to aggressively match old elements to new elements as much as possible.

This will be the behavior of the framework for now, while I haven't found a better way to solution. For now, you can use the work around to mark the element with replace(true) which instruct the diffing algorithm to re-create the element at every view call.

        let buttons = self
            .nodes
            .iter()
            .map(|&x| {
                button(
                    [
                        id(format!("btn_{}", x)),
                        replace(true),
                        on_click(move |_| Msg::ClickNode(x)),
                    ],
                    [text(x)],
                )
            })
            .collect::<Vec<_>>();

@ivanceras
Copy link
Owner

Also, I couldn't re-create the crash error: closure invoked recursively in any browser: firefox,chrome,webkit. I'm on linux.

Hint: you can rewrite your view like this to make it cleaner.

   fn view(&self) -> Node<Msg> {
        div(
            [],
            [
                text(&self.last),
                button(
                    [on_click(|_| Msg::InsertNode)],
                    [text("add item (broken)")],
                ),
                button(
                    [on_click(|_| Msg::AddNode)],
                    [text("add item (working)")],
                ),
                div(
                    [id("buttons")],
                    self.nodes.iter().map(|&x| {
                        button(
                            [
                                id(format!("btn_{}", x)),
                                replace(true),
                                on_click(move |_| Msg::ClickNode(x)),
                            ],
                            [text(x)],
                        )
                    }),
                ),
            ],
        )
    }

@vwpix
Copy link
Author

vwpix commented Nov 13, 2021

thank u for looking into it, and the workaround tip will be useful for now

Also, I couldn't re-create the crash error: closure invoked recursively in any browser: firefox,chrome,webkit. I'm on linux.

the crash show up if i add the "key(x)" attribute to the generated buttons , tested in latest firefox nightly and chromium

edit:
now i found another (related) bug, if i use "replace" combined with "key", it shows the same crash but the broken buttons don't get added, and it breaks the entire app after that
using replace alone works fine

@ivanceras
Copy link
Owner

After hours of hard-work revising and improving the diffing algorithm for keyed elements and tracking down the bug for the closure invoked recursively or destroyed already error, this issue is now solved.

I created a repo for issues like this, which contain the actual code for the use-case.

Thank you @vwpix again, for filing this issue, this has solved multiple long-standing bug in sauron and in mt-dom code.

@ivanceras ivanceras self-assigned this Mar 10, 2022
@ivanceras
Copy link
Owner

crate version where this is fixed:

  • sauron 0.49.2
  • mt-dom 0.19.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants