Make pasted "img" tag has the same behavior as markdown image (#31235)

Fix #31230 --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
5 months ago · 9000811118
parent 4ca65fabda
commit 9000811118
5 changed files with 79 additions and 59 deletions
--- a/modules/markup/html.go
+++ b/modules/markup/html.go
@ -372,7 +372,42 @@ func postProcess(ctx *RenderContext, procs []processor, input io.Reader, output
 	return nil
 }

-func visitNode(ctx *RenderContext, procs []processor, node *html.Node) {
+func handleNodeImg(ctx *RenderContext, img *html.Node) {
+	for i, attr := range img.Attr {
+		if attr.Key != "src" {
+			continue
+		}
+
+		if attr.Val != "" && !IsFullURLString(attr.Val) && !strings.HasPrefix(attr.Val, "/") {
+			attr.Val = util.URLJoin(ctx.Links.ResolveMediaLink(ctx.IsWiki), attr.Val)
+
+			// By default, the "<img>" tag should also be clickable,
+			// because frontend use `<img>` to paste the re-scaled image into the markdown,
+			// so it must match the default markdown image behavior.
+			hasParentAnchor := false
+			for p := img.Parent; p != nil; p = p.Parent {
+				if hasParentAnchor = p.Type == html.ElementNode && p.Data == "a"; hasParentAnchor {
+					break
+				}
+			}
+			if !hasParentAnchor {
+				imgA := &html.Node{Type: html.ElementNode, Data: "a", Attr: []html.Attribute{
+					{Key: "href", Val: attr.Val},
+					{Key: "target", Val: "_blank"},
+				}}
+				parent := img.Parent
+				imgNext := img.NextSibling
+				parent.RemoveChild(img)
+				parent.InsertBefore(imgA, imgNext)
+				imgA.AppendChild(img)
+			}
+		}
+		attr.Val = camoHandleLink(attr.Val)
+		img.Attr[i] = attr
+	}
+}
+
+func visitNode(ctx *RenderContext, procs []processor, node *html.Node) *html.Node {
 	// Add user-content- to IDs and "#" links if they don't already have them
 	for idx, attr := range node.Attr {
 		val := strings.TrimPrefix(attr.Val, "#")
@ -397,21 +432,14 @@ func visitNode(ctx *RenderContext, procs []processor, node *html.Node) {
 		textNode(ctx, procs, node)
 	case html.ElementNode:
 		if node.Data == "img" {
-			for i, attr := range node.Attr {
-				if attr.Key != "src" {
-					continue
-				}
-				if len(attr.Val) > 0 && !IsFullURLString(attr.Val) && !strings.HasPrefix(attr.Val, "data:image/") {
-					attr.Val = util.URLJoin(ctx.Links.ResolveMediaLink(ctx.IsWiki), attr.Val)
-				}
-				attr.Val = camoHandleLink(attr.Val)
-				node.Attr[i] = attr
-			}
+			next := node.NextSibling
+			handleNodeImg(ctx, node)
+			return next
 		} else if node.Data == "a" {
 			// Restrict text in links to emojis
 			procs = emojiProcessors
 		} else if node.Data == "code" || node.Data == "pre" {
-			return
+			return node.NextSibling
 		} else if node.Data == "i" {
 			for _, attr := range node.Attr {
 				if attr.Key != "class" {
@ -434,11 +462,11 @@ func visitNode(ctx *RenderContext, procs []processor, node *html.Node) {
 				}
 			}
 		}
-		for n := node.FirstChild; n != nil; n = n.NextSibling {
-			visitNode(ctx, procs, n)
+		for n := node.FirstChild; n != nil; {
+			n = visitNode(ctx, procs, n)
 		}
 	}
-	// ignore everything else
+	return node.NextSibling
 }

 // textNode runs the passed node through various processors, in order to handle
@ -851,7 +879,7 @@ func issueIndexPatternProcessor(ctx *RenderContext, node *html.Node) {

 	// FIXME: the use of "mode" is quite dirty and hacky, for example: what is a "document"? how should it be rendered?
 	// The "mode" approach should be refactored to some other more clear&reliable way.
-	crossLinkOnly := (ctx.Metas["mode"] == "document" && !ctx.IsWiki)
+	crossLinkOnly := ctx.Metas["mode"] == "document" && !ctx.IsWiki

 	var (
 		found bool
--- a/modules/markup/html_internal_test.go
+++ b/modules/markup/html_internal_test.go
@ -18,8 +18,7 @@ import (

 const (
 	TestAppURL  = "http://localhost:3000/"
-	TestOrgRepo = "gogits/gogs"
-	TestRepoURL = TestAppURL + TestOrgRepo + "/"
+	TestRepoURL = TestAppURL + "test-owner/test-repo/"
 )

 // externalIssueLink an HTML link to an alphanumeric-style issue
@ -64,8 +63,8 @@ var regexpMetas = map[string]string{

 // these values should match the TestOrgRepo const above
 var localMetas = map[string]string{
-	"user": "gogits",
-	"repo": "gogs",
+	"user": "test-owner",
+	"repo": "test-repo",
 }

 func TestRender_IssueIndexPattern(t *testing.T) {
@ -362,12 +361,12 @@ func TestRender_FullIssueURLs(t *testing.T) {
 		`Look here <a href="http://localhost:3000/person/repo/issues/4" class="ref-issue">person/repo#4</a>`)
 	test("http://localhost:3000/person/repo/issues/4#issuecomment-1234",
 		`<a href="http://localhost:3000/person/repo/issues/4#issuecomment-1234" class="ref-issue">person/repo#4 (comment)</a>`)
-	test("http://localhost:3000/gogits/gogs/issues/4",
-		`<a href="http://localhost:3000/gogits/gogs/issues/4" class="ref-issue">#4</a>`)
-	test("http://localhost:3000/gogits/gogs/issues/4 test",
-		`<a href="http://localhost:3000/gogits/gogs/issues/4" class="ref-issue">#4</a> test`)
-	test("http://localhost:3000/gogits/gogs/issues/4?a=1&b=2#comment-123 test",
-		`<a href="http://localhost:3000/gogits/gogs/issues/4?a=1&amp;b=2#comment-123" class="ref-issue">#4 (comment)</a> test`)
+	test("http://localhost:3000/test-owner/test-repo/issues/4",
+		`<a href="http://localhost:3000/test-owner/test-repo/issues/4" class="ref-issue">#4</a>`)
+	test("http://localhost:3000/test-owner/test-repo/issues/4 test",
+		`<a href="http://localhost:3000/test-owner/test-repo/issues/4" class="ref-issue">#4</a> test`)
+	test("http://localhost:3000/test-owner/test-repo/issues/4?a=1&b=2#comment-123 test",
+		`<a href="http://localhost:3000/test-owner/test-repo/issues/4?a=1&amp;b=2#comment-123" class="ref-issue">#4 (comment)</a> test`)
 	test("http://localhost:3000/testOrg/testOrgRepo/pulls/2/files#issuecomment-24",
 		"http://localhost:3000/testOrg/testOrgRepo/pulls/2/files#issuecomment-24")
 	test("http://localhost:3000/testOrg/testOrgRepo/pulls/2/files",
--- a/modules/markup/html_test.go
+++ b/modules/markup/html_test.go
@ -120,8 +120,8 @@ func TestRender_CrossReferences(t *testing.T) {
 	}

 	test(
-		"gogits/gogs#12345",
-		`<p><a href="`+util.URLJoin(markup.TestAppURL, "gogits", "gogs", "issues", "12345")+`" class="ref-issue" rel="nofollow">gogits/gogs#12345</a></p>`)
+		"test-owner/test-repo#12345",
+		`<p><a href="`+util.URLJoin(markup.TestAppURL, "test-owner", "test-repo", "issues", "12345")+`" class="ref-issue" rel="nofollow">test-owner/test-repo#12345</a></p>`)
 	test(
 		"go-gitea/gitea#12345",
 		`<p><a href="`+util.URLJoin(markup.TestAppURL, "go-gitea", "gitea", "issues", "12345")+`" class="ref-issue" rel="nofollow">go-gitea/gitea#12345</a></p>`)
@ -530,43 +530,31 @@ func TestRender_ShortLinks(t *testing.T) {
 }

 func TestRender_RelativeImages(t *testing.T) {
-	setting.AppURL = markup.TestAppURL
-
-	test := func(input, expected, expectedWiki string) {
+	render := func(input string, isWiki bool, links markup.Links) string {
 		buffer, err := markdown.RenderString(&markup.RenderContext{
-			Ctx: git.DefaultContext,
-			Links: markup.Links{
-				Base:       markup.TestRepoURL,
-				BranchPath: "master",
-			},
-			Metas: localMetas,
-		}, input)
-		assert.NoError(t, err)
-		assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(buffer)))
-		buffer, err = markdown.RenderString(&markup.RenderContext{
-			Ctx: git.DefaultContext,
-			Links: markup.Links{
-				Base: markup.TestRepoURL,
-			},
+			Ctx:    git.DefaultContext,
+			Links:  links,
 			Metas:  localMetas,
-			IsWiki: true,
+			IsWiki: isWiki,
 		}, input)
 		assert.NoError(t, err)
-		assert.Equal(t, strings.TrimSpace(expectedWiki), strings.TrimSpace(string(buffer)))
+		return strings.TrimSpace(string(buffer))
 	}

-	rawwiki := util.URLJoin(markup.TestRepoURL, "wiki", "raw")
-	mediatree := util.URLJoin(markup.TestRepoURL, "media", "master")
+	out := render(`<img src="LINK">`, false, markup.Links{Base: "/test-owner/test-repo"})
+	assert.Equal(t, `<a href="/test-owner/test-repo/LINK" target="_blank" rel="nofollow noopener"><img src="/test-owner/test-repo/LINK"/></a>`, out)

-	test(
-		`<img src="Link">`,
-		`<img src="`+util.URLJoin(mediatree, "Link")+`"/>`,
-		`<img src="`+util.URLJoin(rawwiki, "Link")+`"/>`)
+	out = render(`<img src="LINK">`, true, markup.Links{Base: "/test-owner/test-repo"})
+	assert.Equal(t, `<a href="/test-owner/test-repo/wiki/raw/LINK" target="_blank" rel="nofollow noopener"><img src="/test-owner/test-repo/wiki/raw/LINK"/></a>`, out)

-	test(
-		`<img src="./icon.png">`,
-		`<img src="`+util.URLJoin(mediatree, "icon.png")+`"/>`,
-		`<img src="`+util.URLJoin(rawwiki, "icon.png")+`"/>`)
+	out = render(`<img src="LINK">`, false, markup.Links{Base: "/test-owner/test-repo", BranchPath: "test-branch"})
+	assert.Equal(t, `<a href="/test-owner/test-repo/media/test-branch/LINK" target="_blank" rel="nofollow noopener"><img src="/test-owner/test-repo/media/test-branch/LINK"/></a>`, out)
+
+	out = render(`<img src="LINK">`, true, markup.Links{Base: "/test-owner/test-repo", BranchPath: "test-branch"})
+	assert.Equal(t, `<a href="/test-owner/test-repo/wiki/raw/LINK" target="_blank" rel="nofollow noopener"><img src="/test-owner/test-repo/wiki/raw/LINK"/></a>`, out)
+
+	out = render(`<img src="/LINK">`, true, markup.Links{Base: "/test-owner/test-repo", BranchPath: "test-branch"})
+	assert.Equal(t, `<img src="/LINK"/>`, out)
 }

 func Test_ParseClusterFuzz(t *testing.T) {
@ -719,5 +707,6 @@ func TestIssue18471(t *testing.T) {
 func TestIsFullURL(t *testing.T) {
 	assert.True(t, markup.IsFullURLString("https://example.com"))
 	assert.True(t, markup.IsFullURLString("mailto:test@example.com"))
+	assert.True(t, markup.IsFullURLString("data:image/11111"))
 	assert.False(t, markup.IsFullURLString("/foo:bar"))
 }
--- a/modules/markup/renderer.go
+++ b/modules/markup/renderer.go
@ -74,7 +74,7 @@ type RenderContext struct {
 	Type             string
 	IsWiki           bool
 	Links            Links
-	Metas            map[string]string
+	Metas            map[string]string // user, repo, mode(comment/document)
 	DefaultLink      string
 	GitRepo          *git.Repository
 	Repo             gitrepo.Repository
--- a/web_src/js/features/comp/Paste.js
+++ b/web_src/js/features/comp/Paste.js
@ -100,13 +100,17 @@ async function handleClipboardImages(editor, dropzone, images, e) {
    const {uuid} = await uploadFile(img, uploadUrl);
    const {width, dppx} = await imageInfo(img);

-    const url = `/attachments/${uuid}`;
    let text;
    if (width > 0 && dppx > 1) {
      // Scale down images from HiDPI monitors. This uses the <img> tag because it's the only
      // method to change image size in Markdown that is supported by all implementations.
+      // Make the image link relative to the repo path, then the final URL is "/sub-path/owner/repo/attachments/{uuid}"
+      const url = `attachments/${uuid}`;
      text = `<img width="${Math.round(width / dppx)}" alt="${htmlEscape(name)}" src="${htmlEscape(url)}">`;
    } else {
+      // Markdown always renders the image with a relative path, so the final URL is "/sub-path/owner/repo/attachments/{uuid}"
+      // TODO: it should also use relative path for consistency, because absolute is ambiguous for "/sub-path/attachments" or "/attachments"
+      const url = `/attachments/${uuid}`;
      text = `![${name}](${url})`;
    }
    editor.replacePlaceholder(placeholder, text);