Merge pull request #27 from niklak/fix/node-append-child

Fixed Node::append_child, Node::append_children, Node::prepend_child, and Node::prepend_children

CHANGELOG.md

@@ -19,7 +19,7 @@ All notable changes to the `dom_query` crate will be documented in this file.
 ### Fixed
 - Fixed `Selection::append_selection` to work with selections with multiple nodes and selections from another tree.
 - Fixed `Selection::replace_with_selection` to work with selections with multiple nodes and selections from another tree.
-
+- Fixed `Node::append_child`, `Node::append_children`, `Node::prepend_child`, and `Node::prepend_children`: these methods now internally remove the child/children from their previous parent node before attachment.
 
 ## [0.8.0] - 2024-11-03
 

src/dom_tree.rs

@@ -312,21 +312,27 @@ impl Tree {
     /// Appends a child node by `new_child_id` to a node by `id`. `new_child_id` must exist in the tree.
     pub fn append_child_of(&self, id: &NodeId, new_child_id: &NodeId) {
         let mut nodes = self.nodes.borrow_mut();
-        let last_child_id = nodes.get_mut(id.value).and_then(|node| node.last_child);
+
+        let Some(parent) = nodes.get_mut(id.value) else {
+            // TODO: panic or not?
+            return;
+        };
+
+        let last_child_id = parent.last_child;
+
+        if last_child_id.is_none() {
+            parent.first_child = Some(*new_child_id);
+        }
+
+        parent.last_child = Some(*new_child_id);
 
         if let Some(id) = last_child_id {
             if let Some(last_child) = nodes.get_mut(id.value) {
                 last_child.next_sibling = Some(*new_child_id);
             }
         }
 
-        if let Some(parent) = nodes.get_mut(id.value) {
-            if last_child_id.is_none() {
-                parent.first_child = Some(*new_child_id);
-            }
-
-            parent.last_child = Some(*new_child_id);
-
+        {
             if let Some(child) = nodes.get_mut(new_child_id.value) {
                 child.prev_sibling = last_child_id;
                 child.parent = Some(*id);
@@ -337,21 +343,26 @@ impl Tree {
     /// Prepend a child node by `new_child_id` to a node by `id`. `new_child_id` must exist in the tree.
     pub fn prepend_child_of(&self, id: &NodeId, new_child_id: &NodeId) {
         let mut nodes = self.nodes.borrow_mut();
-        let first_child_id = nodes.get_mut(id.value).and_then(|node| node.first_child);
+
+        let Some(parent) = nodes.get_mut(id.value) else {
+            // TODO: panic or not?
+            return;
+        };
+        let first_child_id = parent.first_child;
+
+        if first_child_id.is_none() {
+            parent.last_child = Some(*new_child_id);
+        }
+
+        parent.first_child = Some(*new_child_id);
 
         if let Some(id) = first_child_id {
             if let Some(first_child) = nodes.get_mut(id.value) {
                 first_child.prev_sibling = Some(*new_child_id);
             }
         }
 
-        if let Some(parent) = nodes.get_mut(id.value) {
-            if first_child_id.is_none() {
-                parent.last_child = Some(*new_child_id);
-            }
-
-            parent.first_child = Some(*new_child_id);
-
+        {
             if let Some(child) = nodes.get_mut(new_child_id.value) {
                 child.next_sibling = first_child_id;
                 child.parent = Some(*id);
@@ -372,32 +383,30 @@ impl Tree {
         let prev_sibling_id = node.prev_sibling;
         let next_sibling_id = node.next_sibling;
 
+        if parent_id.is_none() && prev_sibling_id.is_none() && next_sibling_id.is_none() {
+            return;
+        }
+
         node.parent = None;
         node.next_sibling = None;
         node.prev_sibling = None;
 
-        if let Some(parent_id) = parent_id {
-            if let Some(parent) = nodes.get_mut(parent_id.value) {
-                if parent.first_child == Some(*id) {
-                    parent.first_child = next_sibling_id;
-                }
+        if let Some(parent) = parent_id.and_then(|id| nodes.get_mut(id.value)) {
+            if parent.first_child == Some(*id) {
+                parent.first_child = next_sibling_id;
+            }
 
-                if parent.last_child == Some(*id) {
-                    parent.last_child = prev_sibling_id;
-                }
+            if parent.last_child == Some(*id) {
+                parent.last_child = prev_sibling_id;
             }
         }
 
-        if let Some(prev_sibling_id) = prev_sibling_id {
-            if let Some(prev_sibling) = nodes.get_mut(prev_sibling_id.value) {
-                prev_sibling.next_sibling = next_sibling_id;
-            }
+        if let Some(prev_sibling) = prev_sibling_id.and_then(|id| nodes.get_mut(id.value)) {
+            prev_sibling.next_sibling = next_sibling_id;
         }
 
-        if let Some(next_sibling_id) = next_sibling_id {
-            if let Some(next_sibling) = nodes.get_mut(next_sibling_id.value) {
-                next_sibling.prev_sibling = prev_sibling_id;
-            };
+        if let Some(next_sibling) = next_sibling_id.and_then(|id| nodes.get_mut(id.value)) {
+            next_sibling.prev_sibling = prev_sibling_id;
         }
     }
 
@@ -421,18 +430,14 @@ impl Tree {
             new_sibling.next_sibling = Some(*id);
         };
 
-        if let Some(parent_id) = parent_id {
-            if let Some(parent) = nodes.get_mut(parent_id.value) {
-                if parent.first_child == Some(*id) {
-                    parent.first_child = Some(*new_sibling_id);
-                }
-            };
+        if let Some(parent) = parent_id.and_then(|id| nodes.get_mut(id.value)) {
+            if parent.first_child == Some(*id) {
+                parent.first_child = Some(*new_sibling_id);
+            }
         }
 
-        if let Some(prev_sibling_id) = prev_sibling_id {
-            if let Some(prev_sibling) = nodes.get_mut(prev_sibling_id.value) {
-                prev_sibling.next_sibling = Some(*new_sibling_id);
-            };
+        if let Some(prev_sibling) = prev_sibling_id.and_then(|id| nodes.get_mut(id.value)) {
+            prev_sibling.next_sibling = Some(*new_sibling_id);
         }
     }
 

src/node/node_ref.rs

@@ -164,7 +164,9 @@ impl<'a> NodeRef<'a> {
     /// Appends another node by id to the selected node.
     #[inline]
     pub fn append_child<P: NodeIdProver>(&self, id_provider: P) {
-        self.tree.append_child_of(&self.id, id_provider.node_id())
+        let new_child_id = id_provider.node_id();
+        self.tree.remove_from_parent(new_child_id);
+        self.tree.append_child_of(&self.id, new_child_id)
     }
 
     /// Appends another node and it's siblings to the selected node.
@@ -173,15 +175,19 @@ impl<'a> NodeRef<'a> {
         let mut next_node = self.tree.get(id_provider.node_id());
 
         while let Some(ref node) = next_node {
-            self.tree.append_child_of(&self.id, &node.id);
+            let node_id = node.id;
             next_node = node.next_sibling();
+            self.tree.remove_from_parent(&node_id);
+            self.tree.append_child_of(&self.id, &node_id);
         }
     }
 
     /// Prepend another node by id to the selected node.
     #[inline]
     pub fn prepend_child<P: NodeIdProver>(&self, id_provider: P) {
-        self.tree.prepend_child_of(&self.id, id_provider.node_id())
+        let new_child_id = id_provider.node_id();
+        self.tree.remove_from_parent(new_child_id);
+        self.tree.prepend_child_of(&self.id, new_child_id)
     }
 
     /// Prepend another node and it's siblings to the selected node.
@@ -196,6 +202,7 @@ impl<'a> NodeRef<'a> {
         while let Some(ref node) = next_node {
             let node_id = node.id;
             next_node = node.prev_sibling();
+            self.tree.remove_from_parent(&node_id);
             self.tree.prepend_child_of(&self.id, &node_id);
         }
     }

tests/data.rs

@@ -61,7 +61,8 @@ pub static REPLACEMENT_CONTENTS: &str = r#"<!DOCTYPE html>
         <body>
             <div id="main">
                 <p id="before-origin"></p>
-                <p id="origin"><span id="inline">Something</span></p><p id="after-origin"></p>
+                <p id="origin"><span id="inline">Something</span></p>
+                <p id="after-origin"><span>About</span><span>Me</span></p>
             </div>
         </body>
     </html>"#;

tests/node-manipulation.rs

@@ -31,6 +31,77 @@ fn test_create_element() {
     assert!(doc.select("#main #inline").exists());
 }
 
+#[cfg_attr(not(target_arch = "wasm32"), test)]
+#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+fn test_append_existing_element() {
+    let doc = Document::from(REPLACEMENT_CONTENTS);
+    let origin_sel = doc.select_single("p#origin");
+    let origin_node = origin_sel.nodes().first().unwrap();
+
+    assert_eq!(doc.select_single("#origin").text(), "Something".into());
+
+    let span_sel = doc.select_single(" #after-origin span");
+    let span_node = span_sel.nodes().first().unwrap();
+
+    origin_node.append_child(span_node);
+
+    assert_eq!(doc.select_single("#origin").text(), "SomethingAbout".into());
+}
+
+#[cfg_attr(not(target_arch = "wasm32"), test)]
+#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+fn test_append_existing_children() {
+    let doc = Document::from(REPLACEMENT_CONTENTS);
+    let origin_sel = doc.select_single("p#origin");
+    let origin_node = origin_sel.nodes().first().unwrap();
+
+    assert_eq!(doc.select_single("#origin").text(), "Something".into());
+
+    let span_sel = doc.select_single(" #after-origin span");
+    let span_node = span_sel.nodes().first().unwrap();
+
+    // this thing adds a child element and its sibling after existing child nodes.
+    origin_node.append_children(span_node);
+
+    assert_eq!(doc.select_single("#origin").text(), "SomethingAboutMe".into());
+}
+
+#[cfg_attr(not(target_arch = "wasm32"), test)]
+#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+fn test_prepend_existing_element() {
+    let doc = Document::from(REPLACEMENT_CONTENTS);
+    let origin_sel = doc.select_single("p#origin");
+    let origin_node = origin_sel.nodes().first().unwrap();
+
+    assert_eq!(doc.select_single("#origin").text(), "Something".into());
+
+    let span_sel = doc.select_single(" #after-origin span");
+    let span_node = span_sel.nodes().first().unwrap();
+
+    origin_node.prepend_child(span_node);
+
+    assert_eq!(doc.select_single("#origin").text(), "AboutSomething".into());
+}
+
+
+#[cfg_attr(not(target_arch = "wasm32"), test)]
+#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+fn test_prepend_existing_children() {
+    let doc = Document::from(REPLACEMENT_CONTENTS);
+    let origin_sel = doc.select_single("p#origin");
+    let origin_node = origin_sel.nodes().first().unwrap();
+
+    assert_eq!(doc.select_single("#origin").text(), "Something".into());
+
+    let span_sel = doc.select_single(" #after-origin span");
+    let span_node = span_sel.nodes().first().unwrap();
+
+    // this thing adds a child element and its sibling before existing child nodes.
+    origin_node.prepend_children(span_node);
+
+    assert_eq!(doc.select_single("#origin").text(), "AboutMeSomething".into());
+}
+
 #[cfg_attr(not(target_arch = "wasm32"), test)]
 #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
 fn test_append_element_html() {

tests/selection-manipulation.rs

@@ -185,7 +185,6 @@ fn test_replace_with_another_tree_selection() {
     assert_eq!(doc_dst.select(".ad-content .source").length(), 4)
 }
 
-
 #[cfg_attr(not(target_arch = "wasm32"), test)]
 #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
 fn test_append_tree_selection() {