From 99c8f1ecd57161e11c7b0e9d28db021296753fbf Mon Sep 17 00:00:00 2001 From: David Wilkins Date: Sun, 28 Jan 2018 16:56:27 -0800 Subject: [PATCH 1/5] Fixes #178 - Send context to parseHtml --- manipulation.go | 137 +++++++++++++++++++++++++++++++++++++------ manipulation_test.go | 51 ++++++++++++++++ type_test.go | 4 ++ 3 files changed, 174 insertions(+), 18 deletions(-) diff --git a/manipulation.go b/manipulation.go index b7f2fe5..257f207 100644 --- a/manipulation.go +++ b/manipulation.go @@ -39,8 +39,14 @@ func (s *Selection) AfterSelection(sel *Selection) *Selection { // AfterHtml parses the html and inserts it after the set of matched elements. // // This follows the same rules as Selection.Append. -func (s *Selection) AfterHtml(html string) *Selection { - return s.AfterNodes(parseHtml(html)...) +func (s *Selection) AfterHtml(htmlStr string) *Selection { + return s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + for _, n := range nodes { + if node.Parent != nil { + node.Parent.InsertBefore(n, node.NextSibling) + } + } + }) } // AfterNodes inserts the nodes after each element in the set of matched elements. @@ -85,8 +91,12 @@ func (s *Selection) AppendSelection(sel *Selection) *Selection { } // AppendHtml parses the html and appends it to the set of matched elements. -func (s *Selection) AppendHtml(html string) *Selection { - return s.AppendNodes(parseHtml(html)...) +func (s *Selection) AppendHtml(htmlStr string) *Selection { + return s.eachNodeHtml(htmlStr, false, func(node *html.Node, nodes []*html.Node) { + for _, n := range nodes { + node.AppendChild(n) + } + }) } // AppendNodes appends the specified nodes to each node in the set of matched elements. @@ -123,8 +133,14 @@ func (s *Selection) BeforeSelection(sel *Selection) *Selection { // BeforeHtml parses the html and inserts it before the set of matched elements. // // This follows the same rules as Selection.Append. -func (s *Selection) BeforeHtml(html string) *Selection { - return s.BeforeNodes(parseHtml(html)...) +func (s *Selection) BeforeHtml(htmlStr string) *Selection { + return s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + for _, n := range nodes { + if node.Parent != nil { + node.Parent.InsertBefore(n, node) + } + } + }) } // BeforeNodes inserts the nodes before each element in the set of matched elements. @@ -184,8 +200,12 @@ func (s *Selection) PrependSelection(sel *Selection) *Selection { } // PrependHtml parses the html and prepends it to the set of matched elements. -func (s *Selection) PrependHtml(html string) *Selection { - return s.PrependNodes(parseHtml(html)...) +func (s *Selection) PrependHtml(htmlStr string) *Selection { + return s.eachNodeHtml(htmlStr, false, func(node *html.Node, nodes []*html.Node) { + for _, n := range nodes { + node.InsertBefore(n, node.FirstChild) + } + }) } // PrependNodes prepends the specified nodes to each node in the set of @@ -261,8 +281,15 @@ func (s *Selection) ReplaceWithSelection(sel *Selection) *Selection { // It returns the removed elements. // // This follows the same rules as Selection.Append. -func (s *Selection) ReplaceWithHtml(html string) *Selection { - return s.ReplaceWithNodes(parseHtml(html)...) +func (s *Selection) ReplaceWithHtml(htmlStr string) *Selection { + s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + for _, n := range nodes { + if node.Parent != nil { + node.Parent.InsertBefore(n, node.NextSibling) + } + } + }) + return s.Remove() } // ReplaceWithNodes replaces each element in the set of matched elements with @@ -277,8 +304,17 @@ func (s *Selection) ReplaceWithNodes(ns ...*html.Node) *Selection { // SetHtml sets the html content of each element in the selection to // specified html string. -func (s *Selection) SetHtml(html string) *Selection { - return setHtmlNodes(s, parseHtml(html)...) +func (s *Selection) SetHtml(htmlStr string) *Selection { + for _, context := range s.Nodes { + for c := context.FirstChild; c != nil; c = context.FirstChild { + context.RemoveChild(c) + } + } + return s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + for _, n := range nodes { + node.AppendChild(n) + } + }) } // SetText sets the content of each element in the selection to specified content. @@ -334,8 +370,23 @@ func (s *Selection) WrapSelection(sel *Selection) *Selection { // most child of the given HTML. // // It returns the original set of elements. -func (s *Selection) WrapHtml(html string) *Selection { - return s.wrapNodes(parseHtml(html)...) +func (s *Selection) WrapHtml(htmlStr string) *Selection { + nodesMap := make(map[html.NodeType][]*html.Node) + var parent *html.Node + for _, context := range s.Nodes { + if context.Parent != nil { + parent = context.Parent + } else { + parent = &html.Node{Type: html.ElementNode} + } + nodes, found := nodesMap[parent.Type] + if !found { + nodes = parseHtmlWithContext(htmlStr, parent) + nodesMap[parent.Type] = nodes + } + newSingleSelection(context, s.document).wrapAllNodes(cloneNodes(nodes)...) + } + return s } // WrapNode wraps each element in the set of matched elements inside the inner- @@ -387,8 +438,18 @@ func (s *Selection) WrapAllSelection(sel *Selection) *Selection { // document. // // It returns the original set of elements. -func (s *Selection) WrapAllHtml(html string) *Selection { - return s.wrapAllNodes(parseHtml(html)...) +func (s *Selection) WrapAllHtml(htmlStr string) *Selection { + var context *html.Node + var nodes []*html.Node + if len(s.Nodes) > 0 { + context = s.Nodes[0] + if context.Parent != nil { + nodes = parseHtmlWithContext(htmlStr, context) + } else { + nodes = parseHtml(htmlStr) + } + } + return s.wrapAllNodes(nodes...) } func (s *Selection) wrapAllNodes(ns ...*html.Node) *Selection { @@ -457,8 +518,17 @@ func (s *Selection) WrapInnerSelection(sel *Selection) *Selection { // cloned before being inserted into the document. // // It returns the original set of elements. -func (s *Selection) WrapInnerHtml(html string) *Selection { - return s.wrapInnerNodes(parseHtml(html)...) +func (s *Selection) WrapInnerHtml(htmlStr string) *Selection { + nodesMap := make(map[html.NodeType][]*html.Node) + for _, context := range s.Nodes { + nodes, found := nodesMap[context.Type] + if !found { + nodes = parseHtmlWithContext(htmlStr, context) + nodesMap[context.Type] = nodes + } + newSingleSelection(context, s.document).wrapInnerNodes(cloneNodes(nodes)...) + } + return s } // WrapInnerNode wraps an HTML structure, matched by the given selector, around @@ -498,6 +568,16 @@ func parseHtml(h string) []*html.Node { return nodes } +func parseHtmlWithContext(h string, context *html.Node) []*html.Node { + // Errors are only returned when the io.Reader returns any error besides + // EOF, but strings.Reader never will + nodes, err := html.ParseFragment(strings.NewReader(h), context) + if err != nil { + panic("goquery: failed to parse HTML: " + err.Error()) + } + return nodes +} + func setHtmlNodes(s *Selection, ns ...*html.Node) *Selection { for _, n := range s.Nodes { for c := n.FirstChild; c != nil; c = n.FirstChild { @@ -577,3 +657,24 @@ func (s *Selection) manipulateNodes(ns []*html.Node, reverse bool, return s } + +func (s *Selection) eachNodeHtml(htmlStr string, parent bool, fn func(n *html.Node, nodes []*html.Node)) *Selection { + nodesMap := make(map[html.NodeType][]*html.Node) + var context *html.Node + for _, n := range s.Nodes { + if parent { + context = n.Parent + } else { + context = n + } + if context != nil { + nodes, found := nodesMap[context.Type] + if !found { + nodes = parseHtmlWithContext(htmlStr, context) + nodesMap[context.Type] = nodes + } + fn(n, cloneNodes(nodes)) + } + } + return s +} diff --git a/manipulation_test.go b/manipulation_test.go index c5f5022..fcf0d1f 100644 --- a/manipulation_test.go +++ b/manipulation_test.go @@ -56,6 +56,13 @@ func TestAfterHtml(t *testing.T) { printSel(t, doc.Selection) } +func TestAfterHtmlContext(t *testing.T) { + doc := DocBClone() + doc.Find("table tr td").AfterHtml("Test") + assertLength(t, doc.Find("table tr td").Nodes, 14) + printSel(t, doc.Selection) +} + func TestAppend(t *testing.T) { doc := Doc2Clone() doc.Find("#main").Append("#nf6") @@ -113,6 +120,15 @@ func TestAppendHtml(t *testing.T) { printSel(t, doc.Selection) } +func TestAppendHtmlContext(t *testing.T) { + doc := DocBClone() + doc.Find("table tr").AppendHtml("new node") + + assertLength(t, doc.Find("table td").Nodes, 11) + assertLength(t, doc.Find("table tr td.new-node:last-child").Nodes, 4) + printSel(t, doc.Selection) +} + func TestBefore(t *testing.T) { doc := Doc2Clone() doc.Find("#main").Before("#nf6") @@ -151,6 +167,14 @@ func TestBeforeHtml(t *testing.T) { printSel(t, doc.Selection) } +func TestBeforeHtmlContext(t *testing.T) { + doc := DocBClone() + doc.Find("table tr td:first-child").BeforeHtml("new node") + + assertLength(t, doc.Find("table td.new-node:first-child").Nodes, 3) + printSel(t, doc.Selection) +} + func TestEmpty(t *testing.T) { doc := Doc2Clone() s := doc.Find("#main").Empty() @@ -218,6 +242,15 @@ func TestPrependHtml(t *testing.T) { printSel(t, doc.Selection) } +func TestPrependHtmlContext(t *testing.T) { + doc := DocBClone() + doc.Find("table tr").PrependHtml("new node") + + assertLength(t, doc.Find("table td").Nodes, 11) + assertLength(t, doc.Find("table tr td.new-node:first-child").Nodes, 4) + printSel(t, doc.Selection) +} + func TestRemove(t *testing.T) { doc := Doc2Clone() doc.Find("#nf1").Remove() @@ -278,6 +311,15 @@ func TestReplaceWithHtml(t *testing.T) { printSel(t, doc.Selection) } +func TestReplaceWithHtmlContext(t *testing.T) { + doc := DocBClone() + doc.Find("table th").ReplaceWithHtml("TestReplace") + + assertLength(t, doc.Find("table th").Nodes, 0) + assertLength(t, doc.Find("table tr:first-child td").Nodes, 6) + printSel(t, doc.Selection) +} + func TestSetHtml(t *testing.T) { doc := Doc2Clone() q := doc.Find("#main, #foot") @@ -313,6 +355,15 @@ func TestSetHtmlEmpty(t *testing.T) { printSel(t, doc.Selection) } +func TestSetHtmlContext(t *testing.T) { + doc := DocBClone() + doc.Find("table tr").SetHtml("Test") + + assertLength(t, doc.Find("table th").Nodes, 0) + assertLength(t, doc.Find("table td.new-node").Nodes, 4) + printSel(t, doc.Selection) +} + func TestSetText(t *testing.T) { doc := Doc2Clone() q := doc.Find("#main, #foot") diff --git a/type_test.go b/type_test.go index 1e82d5e..d263163 100644 --- a/type_test.go +++ b/type_test.go @@ -53,6 +53,10 @@ func DocB() *Document { return docB } +func DocBClone() *Document { + return CloneDocument(DocB()) +} + func DocW() *Document { if docW == nil { docW = loadDoc("gowiki.html") From d89086ead1a84b8176c983497d9add8a97759685 Mon Sep 17 00:00:00 2001 From: thiemok Date: Mon, 28 Sep 2020 15:37:51 +0200 Subject: [PATCH 2/5] fix(manipulation): address issues from code review of #235 * SetHml remains unchanged, see comment on the isParent param of eachNodeHtml * Removed unused functions * Added documentation on eachNodeHtml * Fixed node parsing cache keys and added test * Refactored some tests for clarity --- manipulation.go | 29 ++++----- manipulation_test.go | 138 ++++++++++++++++++++++++++++++++++++++----- type_test.go | 12 ++-- utilities.go | 18 ++++-- 4 files changed, 157 insertions(+), 40 deletions(-) diff --git a/manipulation.go b/manipulation.go index 257f207..407a963 100644 --- a/manipulation.go +++ b/manipulation.go @@ -578,18 +578,6 @@ func parseHtmlWithContext(h string, context *html.Node) []*html.Node { return nodes } -func setHtmlNodes(s *Selection, ns ...*html.Node) *Selection { - for _, n := range s.Nodes { - for c := n.FirstChild; c != nil; c = n.FirstChild { - n.RemoveChild(c) - } - for _, c := range ns { - n.AppendChild(cloneNode(c)) - } - } - return s -} - // Get the first child that is an ElementNode func getFirstChildEl(n *html.Node) *html.Node { c := n.FirstChild @@ -658,22 +646,27 @@ func (s *Selection) manipulateNodes(ns []*html.Node, reverse bool, return s } -func (s *Selection) eachNodeHtml(htmlStr string, parent bool, fn func(n *html.Node, nodes []*html.Node)) *Selection { - nodesMap := make(map[html.NodeType][]*html.Node) +// eachNodeHtml parses the given html string and inserts the resulting nodes in the dom with the mergeFn. +// The parsed nodes are inserted for each element of the selection. +// isParent can be used to indicate that the elements of the selection should be treated as the parent for the parsed html. +// A cache is used to avoid parsing the html multiple times should the elements of the selection result in the same context. +func (s *Selection) eachNodeHtml(htmlStr string, isParent bool, mergeFn func(n *html.Node, nodes []*html.Node)) *Selection { + // cache to avoid parsing the html for the same context multiple times + nodeCache := make(map[string][]*html.Node) var context *html.Node for _, n := range s.Nodes { - if parent { + if isParent { context = n.Parent } else { context = n } if context != nil { - nodes, found := nodesMap[context.Type] + nodes, found := nodeCache[nodeName(context)] if !found { nodes = parseHtmlWithContext(htmlStr, context) - nodesMap[context.Type] = nodes + nodeCache[nodeName(context)] = nodes } - fn(n, cloneNodes(nodes)) + mergeFn(n, cloneNodes(nodes)) } } return s diff --git a/manipulation_test.go b/manipulation_test.go index fcf0d1f..3f7535a 100644 --- a/manipulation_test.go +++ b/manipulation_test.go @@ -57,9 +57,19 @@ func TestAfterHtml(t *testing.T) { } func TestAfterHtmlContext(t *testing.T) { - doc := DocBClone() + doc := loadString(` + + + + + + + + `, + t) doc.Find("table tr td").AfterHtml("") - assertLength(t, doc.Find("table tr td").Nodes, 14) + assertLength(t, doc.Find("table tr td").Nodes, 2) printSel(t, doc.Selection) } @@ -121,11 +131,21 @@ func TestAppendHtml(t *testing.T) { } func TestAppendHtmlContext(t *testing.T) { - doc := DocBClone() + doc := loadString(` + + +
Before
Test
+ + + + + `, + t) doc.Find("table tr").AppendHtml("") - assertLength(t, doc.Find("table td").Nodes, 11) - assertLength(t, doc.Find("table tr td.new-node:last-child").Nodes, 4) + assertLength(t, doc.Find("table td").Nodes, 2) + assertClass(t, doc.Find("table td").Last(), "new-node") printSel(t, doc.Selection) } @@ -168,10 +188,20 @@ func TestBeforeHtml(t *testing.T) { } func TestBeforeHtmlContext(t *testing.T) { - doc := DocBClone() + doc := loadString(` + + +
Before
new node
+ + + + + `, + t) doc.Find("table tr td:first-child").BeforeHtml("") - assertLength(t, doc.Find("table td.new-node:first-child").Nodes, 3) + assertClass(t, doc.Find("table td").First(), "new-node") printSel(t, doc.Selection) } @@ -243,11 +273,21 @@ func TestPrependHtml(t *testing.T) { } func TestPrependHtmlContext(t *testing.T) { - doc := DocBClone() + doc := loadString(` + + +
Before
new node
+ + + + + `, + t) doc.Find("table tr").PrependHtml("") - assertLength(t, doc.Find("table td").Nodes, 11) - assertLength(t, doc.Find("table tr td.new-node:first-child").Nodes, 4) + assertLength(t, doc.Find("table td").Nodes, 2) + assertClass(t, doc.Find("table tr td").First(), "new-node") printSel(t, doc.Selection) } @@ -312,11 +352,21 @@ func TestReplaceWithHtml(t *testing.T) { } func TestReplaceWithHtmlContext(t *testing.T) { - doc := DocBClone() + doc := loadString(` + + +
Before
new node
+ + + + + `, + t) doc.Find("table th").ReplaceWithHtml("") assertLength(t, doc.Find("table th").Nodes, 0) - assertLength(t, doc.Find("table tr:first-child td").Nodes, 6) + assertLength(t, doc.Find("table tr td").Nodes, 2) printSel(t, doc.Selection) } @@ -356,11 +406,21 @@ func TestSetHtmlEmpty(t *testing.T) { } func TestSetHtmlContext(t *testing.T) { - doc := DocBClone() + doc := loadString(` + + +
Before
TestReplace
+ + + + + `, + t) doc.Find("table tr").SetHtml("") assertLength(t, doc.Find("table th").Nodes, 0) - assertLength(t, doc.Find("table td.new-node").Nodes, 4) + assertLength(t, doc.Find("table td.new-node").Nodes, 1) printSel(t, doc.Selection) } @@ -562,3 +622,53 @@ func TestWrapInnerHtml(t *testing.T) { printSel(t, doc.Selection) } + +func TestParsingRespectsVaryingContext(t *testing.T) { + docA := loadString(` + + + + + `, + t) + docTable := loadString(` + + +
Before
Test
+ + `, + t) + docBoth := loadString(` + + +
+ + + `, + t) + + sA := docA.Find(".x").AppendHtml("Hello") + sTable := docTable.Find(".x").AppendHtml("Hello") + sBoth := docBoth.Find(".x").AppendHtml("Hello") + + oA, _ := sA.Html() + oTable, _ := sTable.Html() + + if oA == oTable { + t.Errorf("Expected inner html of and to not be equal, but got %s and %s", oA, oTable) + } + + oBothTable, _ := sBoth.First().Html() + if oBothTable != oTable { + t.Errorf("Expected inner html of
and
in doc containing both tags to be equal, but got %s and %s", + oTable, + oBothTable) + } + + oBothA, _ := sBoth.Last().Html() + if oBothA != oA { + t.Errorf("Expected inner html of and in doc containing both tags to be equal, but got %s and %s", + oA, + oBothA) + } +} diff --git a/type_test.go b/type_test.go index d263163..b57cbad 100644 --- a/type_test.go +++ b/type_test.go @@ -53,10 +53,6 @@ func DocB() *Document { return docB } -func DocBClone() *Document { - return CloneDocument(DocB()) -} - func DocW() *Document { if docW == nil { docW = loadDoc("gowiki.html") @@ -125,6 +121,14 @@ func loadDoc(page string) *Document { return NewDocumentFromNode(node) } +func loadString(doc string, t *testing.T) *Document { + d, err := NewDocumentFromReader(strings.NewReader(doc)) + if err != nil { + t.Error("Failed to parse test document") + } + return d +} + func TestNewDocument(t *testing.T) { if f, e := os.Open("./testdata/page.html"); e != nil { t.Error(e.Error()) diff --git a/utilities.go b/utilities.go index b4c061a..3e11b1d 100644 --- a/utilities.go +++ b/utilities.go @@ -36,12 +36,22 @@ func NodeName(s *Selection) string { if s.Length() == 0 { return "" } - switch n := s.Get(0); n.Type { + return nodeName(s.Get(0)) +} + +// nodeName returns the node name of the given html node. +// See NodeName for additional details on behaviour. +func nodeName(node *html.Node) string { + if node == nil { + return "" + } + + switch node.Type { case html.ElementNode, html.DoctypeNode: - return n.Data + return node.Data default: - if n.Type >= 0 && int(n.Type) < len(nodeNames) { - return nodeNames[n.Type] + if node.Type >= 0 && int(node.Type) < len(nodeNames) { + return nodeNames[node.Type] } return "" } From 5d7b0271f0444fb5ee2c1385f857c659c098038f Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Wed, 7 Oct 2020 10:54:01 -0400 Subject: [PATCH 3/5] add tests to confirm erroneous behaviour --- manipulation.go | 2 +- manipulation_test.go | 109 ++++++++++++++++++++++++------------------- type_test.go | 2 +- 3 files changed, 64 insertions(+), 49 deletions(-) diff --git a/manipulation.go b/manipulation.go index 407a963..2df68aa 100644 --- a/manipulation.go +++ b/manipulation.go @@ -372,8 +372,8 @@ func (s *Selection) WrapSelection(sel *Selection) *Selection { // It returns the original set of elements. func (s *Selection) WrapHtml(htmlStr string) *Selection { nodesMap := make(map[html.NodeType][]*html.Node) - var parent *html.Node for _, context := range s.Nodes { + var parent *html.Node if context.Parent != nil { parent = context.Parent } else { diff --git a/manipulation_test.go b/manipulation_test.go index 3f7535a..a08d3bd 100644 --- a/manipulation_test.go +++ b/manipulation_test.go @@ -57,19 +57,21 @@ func TestAfterHtml(t *testing.T) { } func TestAfterHtmlContext(t *testing.T) { - doc := loadString(` + doc := loadString(t, `
- - Before1 + + + +
Before
Before2
- `, - t) + `) doc.Find("table tr td").AfterHtml("Test") - assertLength(t, doc.Find("table tr td").Nodes, 2) + assertLength(t, doc.Find("table tr td").Nodes, 4) printSel(t, doc.Selection) } @@ -131,20 +133,22 @@ func TestAppendHtml(t *testing.T) { } func TestAppendHtmlContext(t *testing.T) { - doc := loadString(` + doc := loadString(t, ` - - Before1 + + + +
Before
Before2
- `, - t) + `) doc.Find("table tr").AppendHtml("new node") - assertLength(t, doc.Find("table td").Nodes, 2) + assertLength(t, doc.Find("table td").Nodes, 4) assertClass(t, doc.Find("table td").Last(), "new-node") printSel(t, doc.Selection) } @@ -188,19 +192,22 @@ func TestBeforeHtml(t *testing.T) { } func TestBeforeHtmlContext(t *testing.T) { - doc := loadString(` + doc := loadString(t, ` - - Before1 + + + +
Before
Before2
- `, - t) + `) doc.Find("table tr td:first-child").BeforeHtml("new node") + assertLength(t, doc.Find("table td").Nodes, 4) assertClass(t, doc.Find("table td").First(), "new-node") printSel(t, doc.Selection) } @@ -273,21 +280,23 @@ func TestPrependHtml(t *testing.T) { } func TestPrependHtmlContext(t *testing.T) { - doc := loadString(` + doc := loadString(t, ` - - Before1 + + + +
Before
Before2
- `, - t) - doc.Find("table tr").PrependHtml("new node") + `) + doc.Find("table tr").PrependHtml("new nodeother new node") - assertLength(t, doc.Find("table td").Nodes, 2) - assertClass(t, doc.Find("table tr td").First(), "new-node") + assertLength(t, doc.Find("table td").Nodes, 6) + assertClass(t, doc.Find("table tr td").First(), "c1") printSel(t, doc.Selection) } @@ -352,21 +361,23 @@ func TestReplaceWithHtml(t *testing.T) { } func TestReplaceWithHtmlContext(t *testing.T) { - doc := loadString(` + doc := loadString(t, ` - - Before1 + + + +
Before
Before2
- `, - t) + `) doc.Find("table th").ReplaceWithHtml("TestReplace") assertLength(t, doc.Find("table th").Nodes, 0) - assertLength(t, doc.Find("table tr td").Nodes, 2) + assertLength(t, doc.Find("table tr td").Nodes, 4) printSel(t, doc.Selection) } @@ -406,21 +417,24 @@ func TestSetHtmlEmpty(t *testing.T) { } func TestSetHtmlContext(t *testing.T) { - doc := loadString(` + doc := loadString(t, ` - - Before1 + + + +
Before
Before2
- `, - t) - doc.Find("table tr").SetHtml("Test") + `) + doc.Find("table tr").SetHtml("TestAgain") assertLength(t, doc.Find("table th").Nodes, 0) - assertLength(t, doc.Find("table td.new-node").Nodes, 1) + assertLength(t, doc.Find("table td").Nodes, 4) + assertLength(t, doc.Find("table t2").Nodes, 2) printSel(t, doc.Selection) } @@ -624,33 +638,34 @@ func TestWrapInnerHtml(t *testing.T) { } func TestParsingRespectsVaryingContext(t *testing.T) { - docA := loadString(` + docA := loadString(t, ` - `, - t) - docTable := loadString(` + `) + docTable := loadString(t, `
- `, - t) - docBoth := loadString(` + `) + docBoth := loadString(t, `
- `, - t) + `) sA := docA.Find(".x").AppendHtml("Hello") sTable := docTable.Find(".x").AppendHtml("Hello") sBoth := docBoth.Find(".x").AppendHtml("Hello") + printSel(t, docA.Selection) + printSel(t, docTable.Selection) + printSel(t, docBoth.Selection) + oA, _ := sA.Html() oTable, _ := sTable.Html() diff --git a/type_test.go b/type_test.go index b57cbad..798d4ea 100644 --- a/type_test.go +++ b/type_test.go @@ -121,7 +121,7 @@ func loadDoc(page string) *Document { return NewDocumentFromNode(node) } -func loadString(doc string, t *testing.T) *Document { +func loadString(t *testing.T, doc string) *Document { d, err := NewDocumentFromReader(strings.NewReader(doc)) if err != nil { t.Error("Failed to parse test document") From 94c2530dd898fac51c676ea4a9f915ccaa00c2c8 Mon Sep 17 00:00:00 2001 From: thiemok Date: Wed, 7 Oct 2020 20:04:45 +0200 Subject: [PATCH 4/5] fix(manipulation): fix node insertion and caching issues Node caching was still using the node type instead of the node name in a few places. Updated those to also use the node name as the cache key. ReplaceWithHtml and PrependHtml inserted parsed nodes in the wrong order. Kept track of the correct parent to fix this. Use correct parent flag for SetHtml --- manipulation.go | 20 +++++++++++--------- manipulation_test.go | 2 +- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/manipulation.go b/manipulation.go index 2df68aa..07237a9 100644 --- a/manipulation.go +++ b/manipulation.go @@ -202,8 +202,9 @@ func (s *Selection) PrependSelection(sel *Selection) *Selection { // PrependHtml parses the html and prepends it to the set of matched elements. func (s *Selection) PrependHtml(htmlStr string) *Selection { return s.eachNodeHtml(htmlStr, false, func(node *html.Node, nodes []*html.Node) { + firstChild := node.FirstChild for _, n := range nodes { - node.InsertBefore(n, node.FirstChild) + node.InsertBefore(n, firstChild) } }) } @@ -283,9 +284,10 @@ func (s *Selection) ReplaceWithSelection(sel *Selection) *Selection { // This follows the same rules as Selection.Append. func (s *Selection) ReplaceWithHtml(htmlStr string) *Selection { s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + nextSibling := node.NextSibling for _, n := range nodes { if node.Parent != nil { - node.Parent.InsertBefore(n, node.NextSibling) + node.Parent.InsertBefore(n, nextSibling) } } }) @@ -310,7 +312,7 @@ func (s *Selection) SetHtml(htmlStr string) *Selection { context.RemoveChild(c) } } - return s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + return s.eachNodeHtml(htmlStr, false, func(node *html.Node, nodes []*html.Node) { for _, n := range nodes { node.AppendChild(n) } @@ -371,7 +373,7 @@ func (s *Selection) WrapSelection(sel *Selection) *Selection { // // It returns the original set of elements. func (s *Selection) WrapHtml(htmlStr string) *Selection { - nodesMap := make(map[html.NodeType][]*html.Node) + nodesMap := make(map[string][]*html.Node) for _, context := range s.Nodes { var parent *html.Node if context.Parent != nil { @@ -379,10 +381,10 @@ func (s *Selection) WrapHtml(htmlStr string) *Selection { } else { parent = &html.Node{Type: html.ElementNode} } - nodes, found := nodesMap[parent.Type] + nodes, found := nodesMap[nodeName(parent)] if !found { nodes = parseHtmlWithContext(htmlStr, parent) - nodesMap[parent.Type] = nodes + nodesMap[nodeName(parent)] = nodes } newSingleSelection(context, s.document).wrapAllNodes(cloneNodes(nodes)...) } @@ -519,12 +521,12 @@ func (s *Selection) WrapInnerSelection(sel *Selection) *Selection { // // It returns the original set of elements. func (s *Selection) WrapInnerHtml(htmlStr string) *Selection { - nodesMap := make(map[html.NodeType][]*html.Node) + nodesMap := make(map[string][]*html.Node) for _, context := range s.Nodes { - nodes, found := nodesMap[context.Type] + nodes, found := nodesMap[nodeName(context)] if !found { nodes = parseHtmlWithContext(htmlStr, context) - nodesMap[context.Type] = nodes + nodesMap[nodeName(context)] = nodes } newSingleSelection(context, s.document).wrapInnerNodes(cloneNodes(nodes)...) } diff --git a/manipulation_test.go b/manipulation_test.go index a08d3bd..1d37054 100644 --- a/manipulation_test.go +++ b/manipulation_test.go @@ -434,7 +434,7 @@ func TestSetHtmlContext(t *testing.T) { assertLength(t, doc.Find("table th").Nodes, 0) assertLength(t, doc.Find("table td").Nodes, 4) - assertLength(t, doc.Find("table t2").Nodes, 2) + assertLength(t, doc.Find("table tr").Nodes, 2) printSel(t, doc.Selection) } From 4a687a672f0f3b30ea3a60775067215da81d8081 Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Thu, 8 Oct 2020 09:02:13 -0400 Subject: [PATCH 5/5] add test cases to check html insertion order --- manipulation.go | 3 ++- manipulation_test.go | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/manipulation.go b/manipulation.go index 07237a9..2c31770 100644 --- a/manipulation.go +++ b/manipulation.go @@ -41,9 +41,10 @@ func (s *Selection) AfterSelection(sel *Selection) *Selection { // This follows the same rules as Selection.Append. func (s *Selection) AfterHtml(htmlStr string) *Selection { return s.eachNodeHtml(htmlStr, true, func(node *html.Node, nodes []*html.Node) { + nextSibling := node.NextSibling for _, n := range nodes { if node.Parent != nil { - node.Parent.InsertBefore(n, node.NextSibling) + node.Parent.InsertBefore(n, nextSibling) } } }) diff --git a/manipulation_test.go b/manipulation_test.go index 1d37054..74dda7c 100644 --- a/manipulation_test.go +++ b/manipulation_test.go @@ -70,8 +70,9 @@ func TestAfterHtmlContext(t *testing.T) { `) - doc.Find("table tr td").AfterHtml("Test") - assertLength(t, doc.Find("table tr td").Nodes, 4) + doc.Find("table tr td").AfterHtml("TestAgain") + assertLength(t, doc.Find("table tr td").Nodes, 6) + assertClass(t, doc.Find("table tr td").Last(), "c2") printSel(t, doc.Selection) } @@ -146,10 +147,10 @@ func TestAppendHtmlContext(t *testing.T) { `) - doc.Find("table tr").AppendHtml("new node") + doc.Find("table tr").AppendHtml("new1new2") - assertLength(t, doc.Find("table td").Nodes, 4) - assertClass(t, doc.Find("table td").Last(), "new-node") + assertLength(t, doc.Find("table td").Nodes, 6) + assertClass(t, doc.Find("table td").Last(), "c2") printSel(t, doc.Selection) } @@ -205,10 +206,10 @@ func TestBeforeHtmlContext(t *testing.T) { `) - doc.Find("table tr td:first-child").BeforeHtml("new node") + doc.Find("table tr td:first-child").BeforeHtml("new1new2") - assertLength(t, doc.Find("table td").Nodes, 4) - assertClass(t, doc.Find("table td").First(), "new-node") + assertLength(t, doc.Find("table td").Nodes, 6) + assertClass(t, doc.Find("table td").First(), "c1") printSel(t, doc.Selection) } @@ -374,10 +375,11 @@ func TestReplaceWithHtmlContext(t *testing.T) { `) - doc.Find("table th").ReplaceWithHtml("TestReplace") + doc.Find("table th").ReplaceWithHtml("TestReplace") assertLength(t, doc.Find("table th").Nodes, 0) assertLength(t, doc.Find("table tr td").Nodes, 4) + assertClass(t, doc.Find("table tr td").First(), "c1") printSel(t, doc.Selection) }