Skip to content

Commit

Permalink
Narrowing conditions for which we defensively dup-unparent-and-root t…
Browse files Browse the repository at this point in the history
…ext nodes in the expectation they may be merged. Closes #595.

Note that this simply omits the case where we are inserting a previous
text sibling, and our case was duping and unparenting the *next* text
sibling.

It's not totally clear to me, based on looking at the libxml2 code,
that this is necessary if the pivot node is not a text node. That is,
we may be being *too* defensive here.

We should probably schedule some work to rethink all the kludges we
have around node reparenting, because I'm pretty sure we can simplify
(or decompose) `reparent_node_with` with a little thought.
  • Loading branch information
flavorjones committed Feb 20, 2012
1 parent 9a4421a commit b0cd85f
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 1 deletion.
2 changes: 1 addition & 1 deletion ext/nokogiri/xml_node.c
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ static VALUE reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_rep
}
}

if (reparentee->type == XML_TEXT_NODE && pivot->next && pivot->next->type == XML_TEXT_NODE) {
if (prf != xmlAddPrevSibling && reparentee->type == XML_TEXT_NODE && pivot->next && pivot->next->type == XML_TEXT_NODE) {
/*
* libxml merges text nodes in a right-to-left fashion, meaning that if
* there are two text nodes who would be adjacent, the right (or following,
Expand Down
32 changes: 32 additions & 0 deletions test/xml/test_node_reparenting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,22 @@ class TestNodeReparenting < Nokogiri::TestCase
replacee.add_previous_sibling "foo <p></p> bar"
assert_equal "foo <p></p> bartext node", xml.root.children.to_html
end

describe "with a text node before" do
it "should not defensively dup the 'before' text node" do
xml = Nokogiri::XML %Q(<root>before<p></p>after</root>)
pivot = xml.at_css("p")
before = xml.root.children.first
after = xml.root.children.last
pivot.add_previous_sibling("x")

assert_equal "after", after.content
assert !after.parent.nil?, "unrelated node should not be affected"

assert_equal "before", before.content
assert !before.parent.nil?, "no need to reparent"
end
end
end

describe "#add_next_sibling" do
Expand All @@ -255,6 +271,22 @@ class TestNodeReparenting < Nokogiri::TestCase
replacee.add_next_sibling "foo <p></p> bar"
assert_equal "text nodefoo <p></p> bar", xml.root.children.to_html
end

describe "with a text node after" do
it "should defensively dup the 'after' text node and not touch the 'before' text node" do
xml = Nokogiri::XML %Q(<root>before<p></p>after</root>)
pivot = xml.at_css("p")
before = xml.root.children.first
after = xml.root.children.last
pivot.add_next_sibling("x")

assert_equal "before", before.content
assert !before.parent.nil?, "unrelated node should not be affected"

assert_equal "after", after.content
assert_nil after.parent
end
end
end

describe "#replace" do
Expand Down

0 comments on commit b0cd85f

Please sign in to comment.