From 14c54620192517c85e86ed6db67625a43f11f78d Mon Sep 17 00:00:00 2001 From: Robert Wolff Date: Mon, 15 Sep 2025 15:57:52 +0200 Subject: [PATCH] fix(ui): reworked file preview placement towards better HTML validity (#9181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What? - fixes HTML nodes placement for inline previews - (breaking?) disallows file previews in headings (`

`, …), striked out (``), `` and other environments - allows them in ``, `` and `` environments, but without extra formatting - allows them in `
`, `
  • `, ``, `` and `
    ` (not in ``) 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 `

    `, ``, `` and `` parent nodes it inserts the file preview and following text as siblings to the **parent** node. `before LINK after` → `before PREVIEW after` For links in `

    `, `
  • `, ``, `` and `
    ` parent nodes it inserts the file preview and following text as siblings to the **text** node. `
    before LINK after
    ` → `
    before PREVIEW after
    ` Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9181 Reviewed-by: Gusted Co-authored-by: Robert Wolff Co-committed-by: Robert Wolff --- modules/markup/html.go | 51 ++++++++++++++------- modules/markup/html_test.go | 89 +++++++++++++++++++++++++++---------- 2 files changed, 102 insertions(+), 38 deletions(-) diff --git a/modules/markup/html.go b/modules/markup/html.go index 58021f5cb5..83ce87aea4 100644 --- a/modules/markup/html.go +++ b/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 := "

    " - offset = preview.end - len(afterPrefix) - node.Data = before - nextSibling := node.NextSibling - node.Parent.InsertBefore(&html.Node{ - Type: html.RawNode, - Data: "

    ", - }, nextSibling) - node.Parent.InsertBefore(previewNode, nextSibling) afterNode := &html.Node{ - Type: html.RawNode, - Data: afterPrefix + after, + Type: html.TextNode, + 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 } } diff --git a/modules/markup/html_test.go b/modules/markup/html_test.go index 11a6290ca3..cd235331bc 100644 --- a/modules/markup/html_test.go +++ b/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 := `
    ` + + `
    ` + + `
    ` + + `path/to/file #.txt` + + `
    ` + + `` + + `Line 1 in eeb243c` + + `` + + `
    ` + + `
    ` + + `` + + `` + + `` + + `` + + `` + + `` + + `` + + `
    A` + "\n" + `
    ` + + `
    ` + + `
    ` + linkRendered := `eeb243c339/path/to/file%20%23.txt (L1)` t.Run("file with strange characters in name", func(t *testing.T) { testRender( - commitFileURL+"#L1", - `

    `+ - `
    `+ - `
    `+ - `
    `+ - `path/to/file #.txt`+ - `
    `+ - ``+ - `Line 1 in eeb243c`+ - ``+ - `
    `+ - `
    `+ - ``+ - ``+ - ``+ - ``+ - ``+ - ``+ - ``+ - `
    A`+"\n"+`
    `+ - `
    `+ - `
    `+ - `

    `, + commitFileURLFirstLine, + `

    `+filePreviewBox+`

    `, + localMetas, + ) + }) + + t.Run("file preview with stuff before and after", func(t *testing.T) { + testRender( + ":frog: before"+commitFileURLFirstLine+" :frog: after", + `

    🐸 before

    `+ + filePreviewBox+ + `

    🐸 after

    `, + localMetas, + ) + }) + + t.Run("file preview in
    ,
  • ,
    (not in ) environments", func(t *testing.T) { + testRender( + "
    "+commitFileURLFirstLine+"
    \n"+ + "
    • "+commitFileURLFirstLine+"
    \n"+ + "
    "+commitFileURLFirstLine+""+commitFileURLFirstLine+"
    ", + `
    `+filePreviewBox+`
    `+"\n"+ + `
    • `+filePreviewBox+`
    `+"\n"+ + `
    `+linkRendered+``+filePreviewBox+`
    `, + localMetas, + ) + }) + + t.Run("file preview in , and environments", func(t *testing.T) { + testRender( + "
    "+commitFileURLFirstLine+" "+commitFileURLFirstLine+" "+commitFileURLFirstLine+"
    ", + `
    `+filePreviewBox+` `+ + ``+filePreviewBox+` `+ + ``+filePreviewBox+`
    `, + localMetas, + ) + }) + + t.Run("no file preview in heading, striked out, code environments", func(t *testing.T) { + testRender( + "

    "+commitFileURLFirstLine+"

    \n"+commitFileURLFirstLine+"\n"+commitFileURLFirstLine+"", + `

    `+linkRendered+`

    `+"\n"+ + ``+linkRendered+``+"\n"+ + ``+commitFileURLFirstLine+``, localMetas, ) })