fix innerText issues in diffing; tag 0.1.13

Respo · Aug 3, 2024 · c6b0d90 · c6b0d90
1 parent 7880f6b
commit c6b0d90
Show file tree

Hide file tree

Showing 8 changed files with 117 additions and 12 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/demo_respo/src/inner_text.rs b/demo_respo/src/inner_text.rs
@@ -0,0 +1,57 @@
+//! a demo for switching inner-text and children, which might cause a bug in respo
+
+use std::fmt::Debug;
+
+use respo::{button, css::RespoStyle, div, span, ui::ui_button, util, DispatchFn, RespoElement, RespoEvent};
+use respo_state_derive::RespoState;
+use serde::{Deserialize, Serialize};
+
+use respo::states_tree::{RespoState, RespoStatesTree};
+
+use super::store::ActionOp;
+
+#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize, RespoState)]
+struct InnerTextState {
+  inner_text: bool,
+}
+
+pub fn comp_inner_text(states: &RespoStatesTree) -> Result<RespoElement<ActionOp>, String> {
+  let cursor = states.path();
+
+  let state = states.cast_branch::<InnerTextState>();
+
+  let on_inc = {
+    let cursor = cursor.to_owned();
+    let state = state.to_owned();
+    move |e, dispatch: DispatchFn<_>| -> Result<(), String> {
+      util::log!("click {:?}", e);
+      if let RespoEvent::Click { original_event, .. } = e {
+        original_event.prevent_default();
+      }
+
+      dispatch.run(ActionOp::Increment)?;
+      dispatch.run_state(
+        &cursor,
+        InnerTextState {
+          inner_text: !state.inner_text,
+        },
+      )?;
+      Ok(())
+    }
+  };
+
+  Ok(
+    div().elements([
+      div().elements([button()
+        .class(ui_button())
+        .inner_text("Switch inner text")
+        .style(RespoStyle::default().margin(4.))
+        .on_click(on_inc)]),
+      div().elements([if state.inner_text {
+        div().inner_text("inner text")
+      } else {
+        div().elements([span().inner_text("child 1"), span().inner_text("child 2")])
+      }]),
+    ]),
+  )
+}
diff --git a/demo_respo/src/main.rs b/demo_respo/src/main.rs
@@ -1,6 +1,7 @@
 extern crate console_error_panic_hook;
 
 mod counter;
+mod inner_text;
 mod panel;
 mod plugins;
 mod store;
@@ -11,7 +12,8 @@ use std::cell::{Ref, RefCell};
 use std::panic;
 use std::rc::Rc;
 
-use respo::RespoAction;
+use inner_text::comp_inner_text;
+use respo::{space, RespoAction};
 use web_sys::Node;
 
 use respo::ui::ui_global;
@@ -65,9 +67,14 @@ impl RespoApp for App {
         .style(RespoStyle::default().padding(12.0))
         .children([
           comp_counter(&states.pick("counter"), store.counted)?.to_node(),
+          space(None, Some(80)).to_node(),
           comp_panel(&states.pick("panel"))?,
           comp_todolist(&states.pick("todolist"), &store.tasks)?.to_node(),
+          space(None, Some(80)).to_node(),
           comp_plugins_demo(&states.pick("plugins-demo"))?.to_node(),
+          space(None, Some(80)).to_node(),
+          comp_inner_text(&states.pick("inner-text"))?.to_node(),
+          space(None, Some(80)).to_node(),
         ])
         .to_node(),
     )

diff --git a/respo/Cargo.toml b/respo/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "respo"
-version = "0.1.12"
+version = "0.1.13"
 edition = "2021"
 description = "a tiny virtual DOM library migrated from ClojureScript"
 license = "Apache-2.0"

diff --git a/respo/src/app/diff.rs b/respo/src/app/diff.rs
@@ -1,3 +1,4 @@
+use std::cell::RefCell;
 use std::collections::{HashMap, HashSet};
 use std::fmt::Debug;
 use std::rc::Rc;
@@ -109,7 +110,8 @@ where
         });
         collect_effects_outside_in_as(new_tree, coord, dom_path, RespoEffectType::Mounted, changes)?;
       } else {
-        diff_attrs(attrs, old_attrs, coord, dom_path, changes);
+        let reset_inner = RefCell::new(false);
+        diff_attrs(attrs, old_attrs, coord, dom_path, changes, &reset_inner);
         diff_style(
           &HashMap::from_iter(style.0.to_owned()),
           &HashMap::from_iter(old_style.0.to_owned()),
@@ -119,7 +121,12 @@ where
         );
 
         diff_event(event, old_event, coord, dom_path, changes);
-        diff_children(children, old_children, coord, dom_path, changes)?;
+        if *reset_inner.borrow() {
+          // children is empty after innerHTML or innerText changed
+          diff_children(children, &[], coord, dom_path, changes)?;
+        } else {
+          diff_children(children, old_children, coord, dom_path, changes)?;
+        }
       }
     }
     (RespoNode::Referenced(new_cell), RespoNode::Referenced(old_cell)) => {
@@ -147,6 +154,7 @@ fn diff_attrs<T>(
   coord: &[RespoCoord],
   dom_path: &[u32],
   changes: &mut Vec<DomChange<T>>,
+  reset_inner: &RefCell<bool>,
 ) where
   T: Debug + Clone,
 {
@@ -156,15 +164,24 @@ fn diff_attrs<T>(
     if old_attrs.contains_key(key) {
       if &old_attrs[key] != value {
         added.insert(key.to_owned(), value.to_owned());
+        if inner_changed(key) {
+          *reset_inner.borrow_mut() = true;
+        }
       }
     } else {
       added.insert(key.to_owned(), value.to_owned());
+      if inner_changed(key) {
+        *reset_inner.borrow_mut() = true;
+      }
     }
   }
 
   for key in old_attrs.keys() {
     if !new_attrs.contains_key(key) {
       removed.insert(key.to_owned());
+      if inner_changed(key) {
+        *reset_inner.borrow_mut() = true;
+      }
     }
   }
 
@@ -178,6 +195,11 @@ fn diff_attrs<T>(
   }
 }
 
+/// changed innerHTML or innerText, which resets children values
+fn inner_changed(key: &Rc<str>) -> bool {
+  key == &"innerHTML".into() || key == &"innerText".into() || key == &"inner-text".into()
+}
+
 fn diff_style<T>(
   new_style: &HashMap<Rc<str>, String>,
   old_style: &HashMap<Rc<str>, String>,

diff --git a/respo/src/app/patch.rs b/respo/src/app/patch.rs
@@ -199,7 +199,10 @@ where
                 .expect("get node")
                 .children()
                 .item(*idx)
-                .ok_or_else(|| format!("child to remove not found at {}", &idx))?;
+                .ok_or_else(|| {
+                  util::warn_log!("child not found at {:?}", coord);
+                  format!("child to remove not found at {}", &idx)
+                })?;
               target.remove_child(&child).expect("child removed");
             }
             ChildDomOp::InsertAfter(idx, k, node) => {

diff --git a/respo/src/app/renderer.rs b/respo/src/app/renderer.rs
@@ -3,14 +3,15 @@ use crate::node::dom_change::RespoCoord;
 use crate::node::{
   DispatchFn, DomChange, RespoComponent, RespoEffectType, RespoElement, RespoEventMark, RespoEventMarkFn, RespoListenerFn, RespoNode,
 };
+use crate::warn_log;
 use std::cell::RefCell;
 use std::fmt::Debug;
 use std::rc::Rc;
 use std::sync::RwLock;
 
 use wasm_bindgen::{JsCast, JsValue};
 use web_sys::console::{error_1, warn_1};
-use web_sys::{HtmlElement, HtmlLabelElement, HtmlTextAreaElement, Node};
+use web_sys::{HtmlElement, HtmlInputElement, HtmlLabelElement, HtmlTextAreaElement, Node};
 
 use crate::app::diff::{collect_effects_outside_in_as, diff_tree};
 use crate::app::patch::{attach_event, patch_tree};
@@ -236,20 +237,28 @@ where
       children,
     }) => {
       let element = document.create_element(name)?;
+      let mut inner_set = false;
       for (key, value) in attrs {
         let key = key.as_ref();
         match key {
           "style" => warn_1(&"style is handled outside attrs".into()),
-          "innerText" => element.dyn_ref::<HtmlElement>().expect("into html element").set_inner_text(value),
-          "innerHTML" => element.set_inner_html(value),
+          "innerText" => {
+            inner_set = true;
+            element.dyn_ref::<HtmlElement>().expect("into html element").set_inner_text(value)
+          }
+          "innerHTML" => {
+            inner_set = true;
+            element.set_inner_html(value)
+          }
           "htmlFor" => element
             .dyn_ref::<HtmlLabelElement>()
             .expect("into label element")
             .set_html_for(value),
-          "value" if &**name == "textarea" || &**name == "input" => element
+          "value" if &**name == "textarea" => element
             .dyn_ref::<HtmlTextAreaElement>()
-            .expect("into html element")
+            .expect("into textarea element")
             .set_value(value),
+          "value" if &**name == "input" => element.dyn_ref::<HtmlInputElement>().expect("into input element").set_value(value),
           _ => {
             element.set_attribute(key, value)?;
           }
@@ -258,6 +267,13 @@ where
       if !style.is_empty() {
         element.set_attribute("style", &style.to_string())?;
       }
+      if inner_set && !children.is_empty() {
+        warn_log!(
+          "innerText or innerHTML is set, it's conflicted with children: {} {:?}",
+          inner_set,
+          children
+        );
+      }
       for (k, child) in children {
         let mut next_coord = coord.to_owned();
         next_coord.push(RespoCoord::Key(k.to_owned()));

diff --git a/respo/src/app/util.rs b/respo/src/app/util.rs
@@ -41,7 +41,7 @@ pub fn raf_loop_slow(interval: i32, mut cb: Box<dyn FnMut() -> Result<(), String
   *g.borrow_mut() = Some(Closure::wrap(Box::new(move || {
     if let Err(e) = cb() {
       crate::warn_log!(
-        "Failure in slow loop, program has to stop since inconsistent states. Details: {}",
+        "Failure in slow loop, program has to stop since inconsistent DOM states. Details: {}",
         e
       );
     }