fix(ui): reworked file preview placement towards better HTML validity (#9181)

### What?

- fixes HTML nodes placement for inline previews
- (breaking?) disallows file previews in headings (`<h1>`, …), striked out (`<del>`), `<summary>` and other environments
- allows them in `<span>`, `<em>` and `<strong>` environments, but without extra formatting
- allows them in `<div>`, `<li>`, `<th>`, `<td>` and `<details>` (not in `<summary>`) environments following the parent’s formatting
- improves overall HTML validity, but only to the extend of the direct parent nodes (but not a big issue, as modern browsers tend to ignore invalid HTML and still parse it)
- fix #9136

See examples of strangely formatted file previews at https://v13.next.forgejo.org/mahlzahn/test/issues/4.

### How is it implemented?

For links in `<p>`, `<span>`, `<em>` and `<strong>` parent nodes it inserts the file preview and following text as siblings to the **parent** node.
`<em>before LINK after</em>`
→ `<em>before </em>PREVIEW<em> after</em>`

For links in `<div>`, `<li>`, `<th>`, `<td>` and `<details>` parent nodes it inserts the file preview and following text as siblings to the **text** node.
`<div>before LINK after</div>`
→ `<div>before PREVIEW after</div>`

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9181
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Robert Wolff <mahlzahn@posteo.de>
Co-committed-by: Robert Wolff <mahlzahn@posteo.de>

modules/markup/html.go

@@ -1078,12 +1078,15 @@ func filePreviewPatternProcessor(ctx *RenderContext, node *html.Node) {
 
 	next := node.NextSibling
 	for node != nil && node != next {
+		if node.Parent == nil || node.Parent.Type != html.ElementNode {
+			node = node.NextSibling
+			continue
+		}
 		previews := NewFilePreviews(ctx, node, locale)
 		if previews == nil {
 			node = node.NextSibling
 			continue
 		}
-
 		offset := 0
 		for _, preview := range previews {
 			previewNode := preview.CreateHTML(locale)
@@ -1091,23 +1094,41 @@ func filePreviewPatternProcessor(ctx *RenderContext, node *html.Node) {
 			// Specialized version of replaceContent, so the parent paragraph element is not destroyed from our div
 			before := node.Data[:(preview.start - offset)]
 			after := node.Data[(preview.end - offset):]
-			afterPrefix := "<p>"
-			offset = preview.end - len(afterPrefix)
-			node.Data = before
-			nextSibling := node.NextSibling
-			node.Parent.InsertBefore(&html.Node{
-				Type: html.RawNode,
-				Data: "</p>",
-			}, nextSibling)
-			node.Parent.InsertBefore(previewNode, nextSibling)
 			afterNode := &html.Node{
-				Type: html.RawNode,
+				Type: html.TextNode,
-				Data: afterPrefix + after,
+				Data: after,
+			}
+			matched := true
+			switch node.Parent.Data {
+			case "div", "li", "td", "th", "details":
+				nextSibling := node.NextSibling
+				node.Parent.InsertBefore(previewNode, nextSibling)
+				node.Parent.InsertBefore(afterNode, nextSibling)
+			case "p", "span", "em", "strong":
+				nextSibling := node.Parent.NextSibling
+				node.Parent.Parent.InsertBefore(previewNode, nextSibling)
+				afterPNode := &html.Node{
+					Type: html.ElementNode,
+					Data: node.Parent.Data,
+					Attr: node.Parent.Attr,
+				}
+				afterPNode.AppendChild(afterNode)
+				node.Parent.Parent.InsertBefore(afterPNode, nextSibling)
+				siblingNode := node.NextSibling
+				if siblingNode != nil {
+					node.NextSibling = nil
+					siblingNode.PrevSibling = nil
+					afterPNode.AppendChild(siblingNode)
+				}
+			default:
+				matched = false
+			}
+			if matched {
+				offset = preview.end
+				node.Data = before
+				node = afterNode
 			}
-			node.Parent.InsertBefore(afterNode, nextSibling)
-			node = afterNode
 		}
-
 		node = node.NextSibling
 	}
 }

modules/markup/html_test.go

@@ -1181,32 +1181,75 @@ func TestRender_FilePreview(t *testing.T) {
 	})
 
 	commitFileURL = util.URLJoin(markup.TestRepoURL, "src", "commit", "eeb243c3395e1921c5d90e73bd739827251fc99d", "path", "to", "file%20%23.txt")
+	commitFileURLFirstLine := commitFileURL + "#L1"
+	filePreviewBox := `<div class="file-preview-box">` +
+		`<div class="header">` +
+		`<div>` +
+		`<a href="http://localhost:3000/gogits/gogs/src/commit/eeb243c3395e1921c5d90e73bd739827251fc99d/path/to/file%20%23.txt#L1" class="muted" rel="nofollow">path/to/file #.txt</a>` +
+		`</div>` +
+		`<span class="text grey">` +
+		`Line 1 in <a href="http://localhost:3000/gogits/gogs/src/commit/eeb243c3395e1921c5d90e73bd739827251fc99d" class="text black" rel="nofollow">eeb243c</a>` +
+		`</span>` +
+		`</div>` +
+		`<div class="ui table">` +
+		`<table class="file-preview">` +
+		`<tbody>` +
+		`<tr>` +
+		`<td class="lines-num"><span data-line-number="1"></span></td>` +
+		`<td class="lines-code chroma"><code class="code-inner">A` + "\n" + `</code></td>` +
+		`</tr>` +
+		`</tbody>` +
+		`</table>` +
+		`</div>` +
+		`</div>`
+	linkRendered := `<a href="` + commitFileURLFirstLine + `" rel="nofollow"><code>eeb243c339/path/to/file%20%23.txt (L1)</code></a>`
 
 	t.Run("file with strange characters in name", func(t *testing.T) {
 		testRender(
-			commitFileURL+"#L1",
+			commitFileURLFirstLine,
-			`<p></p>`+
+			`<p></p>`+filePreviewBox+`<p></p>`,
-				`<div class="file-preview-box">`+
+			localMetas,
-				`<div class="header">`+
+		)
-				`<div>`+
+	})
-				`<a href="http://localhost:3000/gogits/gogs/src/commit/eeb243c3395e1921c5d90e73bd739827251fc99d/path/to/file%20%23.txt#L1" class="muted" rel="nofollow">path/to/file #.txt</a>`+
+
-				`</div>`+
+	t.Run("file preview with stuff before and after", func(t *testing.T) {
-				`<span class="text grey">`+
+		testRender(
-				`Line 1 in <a href="http://localhost:3000/gogits/gogs/src/commit/eeb243c3395e1921c5d90e73bd739827251fc99d" class="text black" rel="nofollow">eeb243c</a>`+
+			":frog: before"+commitFileURLFirstLine+" :frog: after",
-				`</span>`+
+			`<p><span class="emoji" aria-label="frog" data-alias="frog">🐸</span> before</p>`+
-				`</div>`+
+				filePreviewBox+
-				`<div class="ui table">`+
+				`<p> <span class="emoji" aria-label="frog" data-alias="frog">🐸</span> after</p>`,
-				`<table class="file-preview">`+
+			localMetas,
-				`<tbody>`+
+		)
-				`<tr>`+
+	})
-				`<td class="lines-num"><span data-line-number="1"></span></td>`+
+
-				`<td class="lines-code chroma"><code class="code-inner">A`+"\n"+`</code></td>`+
+	t.Run("file preview in <div>, <li>, <details> (not in <summary>) environments", func(t *testing.T) {
-				`</tr>`+
+		testRender(
-				`</tbody>`+
+			"<div>"+commitFileURLFirstLine+"</div>\n"+
-				`</table>`+
+				"<ul><li>"+commitFileURLFirstLine+"</li></ul>\n"+
-				`</div>`+
+				"<details><summary>"+commitFileURLFirstLine+"</summary>"+commitFileURLFirstLine+"</details>",
-				`</div>`+
+			`<div>`+filePreviewBox+`</div>`+"\n"+
-				`<p></p>`,
+				`<ul><li>`+filePreviewBox+`</li></ul>`+"\n"+
+				`<details><summary>`+linkRendered+`</summary>`+filePreviewBox+`</details>`,
+			localMetas,
+		)
+	})
+
+	t.Run("file preview in <span>, <em> and <strong> environments", func(t *testing.T) {
+		testRender(
+			"<div><span>"+commitFileURLFirstLine+"</span> <em>"+commitFileURLFirstLine+"</em> <strong>"+commitFileURLFirstLine+"</strong></div>",
+			`<div><span></span>`+filePreviewBox+`<span></span> `+
+				`<em></em>`+filePreviewBox+`<em></em> `+
+				`<strong></strong>`+filePreviewBox+`<strong></strong></div>`,
+			localMetas,
+		)
+	})
+
+	t.Run("no file preview in heading, striked out, code environments", func(t *testing.T) {
+		testRender(
+			"<h1>"+commitFileURLFirstLine+"</h1>\n<del>"+commitFileURLFirstLine+"</del>\n<code>"+commitFileURLFirstLine+"</code>",
+			`<h1>`+linkRendered+`</h1>`+"\n"+
+				`<del>`+linkRendered+`</del>`+"\n"+
+				`<code>`+commitFileURLFirstLine+`</code>`,
 			localMetas,
 		)
 	})