Bug 2060932 - Support GitHub-style collapsible sections in comments - #2726
Conversation
Comments render markdown with cmark's safe option and every `<` escaped beforehand, so raw HTML never reaches the parser. Instead of weakening that, convert the four disclosure tags back to real elements after rendering: mark the escaped tags in text nodes (skipping pre/code so the syntax can still be documented), then re-parse so the block level elements are lifted out of the paragraph markdown wrapped them in. Only those four exact tags are recognized and they never carry attributes, so no other markup can be smuggled in. The marker characters are stripped from the input so they cannot be forged, and unbalanced tags cannot leak an unclosed element into the page.
There was a problem hiding this comment.
Pull request overview
Adds GitHub-style collapsible comment sections while retaining safe Markdown rendering.
Changes:
- Post-processes bare
<details>and<summary>tags. - Adds disclosure rendering and security tests.
- Styles disclosure elements in Markdown comments.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Bugzilla/Markdown.pm |
Expands disclosure tags after Markdown rendering. |
t/markdown.t |
Tests disclosure rendering and safeguards. |
skins/standard/global.css |
Styles collapsible sections. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@kyoshino Is this something that you could take over and finish for us? Given your past experience with markdown comments and rendering. |
|
Sure, will check it out! |
There was a problem hiding this comment.
🟡 Changes recommended
Entity-encoded marker characters can forge disclosure tags, and code literals lose their original casing.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
|
Tested quickly, locally. If the https://gist.github.com/scmx/eca72d44afee0113ceb0349dd54a84a2 
|
|
Also a header in title needs <details><summary>
# Summary (h1)
</summary>
content
</details>
|
There was a problem hiding this comment.
🟡 Changes recommended
Unicode case folding can cause unsupported tag spellings to be silently deleted.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Backslash-escaped disclosure tags are incorrectly activated, and the documented attribute policy conflicts with the implementation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Bugzilla/Markdown.pm:152
- This substitution also marks backslash-escaped tags. In GFM,
\<details>is literal text, but replacing<details>before cmark sees it prevents the backslash escape from being applied; the second pass then creates an active disclosure element (and leaves the backslash visible). Please skip tags preceded by an odd-length run of backslashes and add a regression test for escaped opening and closing tags.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
Comments render markdown with cmark's safe option and every
<escaped beforehand, so raw HTML never reaches the parser. Instead of weakening that, convert the four disclosure tags back to real elements after rendering: mark the escaped tags in text nodes (skipping pre/code so the syntax can still be documented), then re-parse so the block level elements are lifted out of the paragraph markdown wrapped them in.Only those four exact tags are recognized and they never carry attributes, so no other markup can be smuggled in. The marker characters are stripped from the input so they cannot be forged, and unbalanced tags cannot leak an unclosed element into the page.