From cc93bbabceef9cfb25fd5daf305f565ee18089f6 Mon Sep 17 00:00:00 2001 From: Mykola Humanov Date: Tue, 5 Nov 2024 17:40:09 +0200 Subject: [PATCH 1/5] src/node/node_ref.rs: `NodeRef::append_child`, `Node::prepend_child` also detach a `child` from its previous parent --- src/dom_tree.rs | 30 ++++++++++++++---------------- src/node/node_ref.rs | 13 ++++++++++--- tests/node-manipulation.rs | 27 +++++++++++++++++++++++++++ tests/selection-manipulation.rs | 1 - 4 files changed, 51 insertions(+), 20 deletions(-) diff --git a/src/dom_tree.rs b/src/dom_tree.rs index 8280875..92091aa 100644 --- a/src/dom_tree.rs +++ b/src/dom_tree.rs @@ -372,32 +372,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; } } diff --git a/src/node/node_ref.rs b/src/node/node_ref.rs index 45a4120..81b95a0 100644 --- a/src/node/node_ref.rs +++ b/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(&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(&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); } } diff --git a/tests/node-manipulation.rs b/tests/node-manipulation.rs index 6b9120e..0638cce 100644 --- a/tests/node-manipulation.rs +++ b/tests/node-manipulation.rs @@ -31,6 +31,33 @@ 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 contents = r#" + + + +
+

It's

+ Wonderful +
+ + "#; + + let doc = Document::from(contents); + let p_sel = doc.select_single("p"); + let p_node = p_sel.nodes().first().unwrap(); + + let span_sel = doc.select_single("span"); + let span_node = span_sel.nodes().first().unwrap(); + + p_node.append_child(span_node); + + assert!(doc.select("#main > #first > span").exists()); + assert!(!doc.select("#main > span").exists()); +} + #[cfg_attr(not(target_arch = "wasm32"), test)] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn test_append_element_html() { diff --git a/tests/selection-manipulation.rs b/tests/selection-manipulation.rs index 29ddbd6..991954d 100644 --- a/tests/selection-manipulation.rs +++ b/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() { From 742e96204dc316c104e897d0f90dee328b913de0 Mon Sep 17 00:00:00 2001 From: Mykola Humanov Date: Tue, 5 Nov 2024 19:12:53 +0200 Subject: [PATCH 2/5] src/dom_tree.rs: revise `Tree:append_child_of` --- src/dom_tree.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/dom_tree.rs b/src/dom_tree.rs index 92091aa..7f1f58a 100644 --- a/src/dom_tree.rs +++ b/src/dom_tree.rs @@ -312,7 +312,19 @@ 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) { @@ -320,13 +332,7 @@ impl Tree { } } - 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); From 0cd9e0625d89fc9b802cd41f383ca6774416fac4 Mon Sep 17 00:00:00 2001 From: Mykola Humanov Date: Tue, 5 Nov 2024 19:16:01 +0200 Subject: [PATCH 3/5] src/dom_tree.rs: revise `Tree:prepend_child_of` --- src/dom_tree.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/dom_tree.rs b/src/dom_tree.rs index 7f1f58a..3a1aa18 100644 --- a/src/dom_tree.rs +++ b/src/dom_tree.rs @@ -343,7 +343,18 @@ 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) { @@ -351,13 +362,7 @@ impl Tree { } } - 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); From bb66d49b7996389d07c271104cea1cdec9b4987c Mon Sep 17 00:00:00 2001 From: Mykola Humanov Date: Tue, 5 Nov 2024 20:10:15 +0200 Subject: [PATCH 4/5] tests/node-manipulation.rs: add `test_prepend_existing_element` --- CHANGELOG.md | 2 +- src/dom_tree.rs | 16 ++++++--------- tests/data.rs | 3 ++- tests/node-manipulation.rs | 41 ++++++++++++++++++++++---------------- 4 files changed, 33 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c757dbf..e922d34 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/dom_tree.rs b/src/dom_tree.rs index 3a1aa18..d426c88 100644 --- a/src/dom_tree.rs +++ b/src/dom_tree.rs @@ -430,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); } } diff --git a/tests/data.rs b/tests/data.rs index 78c9434..00c264d 100644 --- a/tests/data.rs +++ b/tests/data.rs @@ -61,7 +61,8 @@ pub static REPLACEMENT_CONTENTS: &str = r#"

-

Something

+

Something

+

About

"#; diff --git a/tests/node-manipulation.rs b/tests/node-manipulation.rs index 0638cce..0f7b751 100644 --- a/tests/node-manipulation.rs +++ b/tests/node-manipulation.rs @@ -34,28 +34,35 @@ fn test_create_element() { #[cfg_attr(not(target_arch = "wasm32"), test)] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn test_append_existing_element() { - let contents = r#" - - - -
-

It's

- Wonderful -
- - "#; + let doc = Document::from(REPLACEMENT_CONTENTS); + let origin_sel = doc.select_single("p#origin"); + let origin_node = origin_sel.nodes().first().unwrap(); - let doc = Document::from(contents); - let p_sel = doc.select_single("p"); - let p_node = p_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_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("span"); + let span_sel = doc.select_single(" #after-origin span"); let span_node = span_sel.nodes().first().unwrap(); - p_node.append_child(span_node); + origin_node.prepend_child(span_node); - assert!(doc.select("#main > #first > span").exists()); - assert!(!doc.select("#main > span").exists()); + assert_eq!(doc.select_single("#origin").text(), "AboutSomething".into()); } #[cfg_attr(not(target_arch = "wasm32"), test)] From ab576e481cd26a0cb2c7e8d160f1bb061f6867a4 Mon Sep 17 00:00:00 2001 From: Mykola Humanov Date: Tue, 5 Nov 2024 20:24:34 +0200 Subject: [PATCH 5/5] tests/node-manipulation.rs: add tests for appending and prepending child nodes --- tests/data.rs | 2 +- tests/node-manipulation.rs | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/data.rs b/tests/data.rs index 00c264d..c04017a 100644 --- a/tests/data.rs +++ b/tests/data.rs @@ -62,7 +62,7 @@ pub static REPLACEMENT_CONTENTS: &str = r#"

Something

-

About

+

AboutMe

"#; diff --git a/tests/node-manipulation.rs b/tests/node-manipulation.rs index 0f7b751..95cf518 100644 --- a/tests/node-manipulation.rs +++ b/tests/node-manipulation.rs @@ -48,6 +48,24 @@ fn test_append_existing_element() { 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() { @@ -65,6 +83,25 @@ fn test_prepend_existing_element() { 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() {