-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
fix: generate IDs for HTML headings without id attribute #36233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This fixes go-gitea#27383 where HTML headings like `<h1>Title</h1>` in markdown files would have empty permalink anchors (showing just `#` in the URL). The fix generates an ID from the heading's text content using the same CleanValue function used for Markdown headings, ensuring consistent behavior between: - Markdown headings: `## Title` → `id="user-content-title"` - HTML headings: `<h1>Title</h1>` → `id="user-content-title"` Fixes go-gitea#27383
65906c4 to
7cbe377
Compare
|
One difference from GitHub's implementation is that this I think the reason for this is because in issue comments you have multiple markdown documents on one page, so the generated IDs may not be unique any more. So ideally we should disable this generation mechanism on pages where there can be more than one markdown document. |
|
good point! baru sadar juga kalo di issue comments bisa duplicate ID karena multiple markdown docs di satu page. mau saya tambahin logic buat skip generation di context issue comments? atau prefer bikin follow-up PR aja? |
|
@wxiaoguang got it, jadi perlu skip ID generation untuk issue comments ya. mau saya fix di PR ini atau prefer follow-up PR terpisah? |
|
Please speak in English |
|
@wxiaoguang got it, I need to skip ID generation for issue comments. Should I fix it in this PR or would you prefer a separate follow-up PR? |
|
It should be an easy fix in this PR, see #36233 (comment) |
Skip auto-generating IDs for HTML headings in comment-like contexts (issue comments, wiki pages) where multiple markdown documents exist on one page, as this could create duplicate IDs. This matches GitHub's behavior where heading IDs are only generated for repository files (README, etc.) but not for issue comments.
|
@wxiaoguang Fixed. Now skipping heading ID generation for issue comments (when markupAllowShortIssuePattern is true). Added tests to verify the behavior. |
|
Please don't use You need a clearly defined option to control the new behavior. (no need to add a configurable option, you can hard-code it for wiki and file) |
Replace markupAllowShortIssuePattern check with a clearly defined EnableHeadingIDGeneration option for controlling HTML heading ID auto-generation. - Add EnableHeadingIDGeneration field to RenderOptions - Add WithEnableHeadingIDGeneration builder method - Enable it explicitly in repo_file.go and repo_wiki.go - Update tests to use the new explicit option
|
Thanks for the feedback! I've refactored the implementation to use a dedicated Changes:
This makes the option clearly defined and hard-coded for file and wiki pages as requested. |

Summary
This PR fixes #27383 where HTML headings like
<h1>Title</h1>in markdown files would have empty permalink anchors (showing just#in the URL).Before:
<h1>Heading without ID</h1>→ permalink shows#(empty)After:
<h1>Heading without ID</h1>→ permalink shows#heading-without-idProblem
When users write HTML headings in markdown without an
idattribute, the frontend'sanchors.tsgenerates permalink links usingheading.id. If the heading has noid, the link becomes just#, which is not useful.GitHub handles this by auto-generating IDs for HTML headings based on their text content, matching the behavior of Markdown headings.
Solution
Modified
processNodeAttrIDinmodules/markup/html_node.goto:idattributeCleanValuefunction used for Markdown headingsuser-content-prefix as expected by the frontendThis ensures consistent behavior between:
## Title→id="user-content-title"<h1>Title</h1>→id="user-content-title"Testing
Added test cases in
modules/markup/html_node_test.gocovering:All existing tests continue to pass.
Fixes #27383