Fix double-processing of highlighted code causing span tags to render as text (#1661) - #1668
Merged
Merged
Conversation
…der as text (lsegal#1661) When {yard:include_tags} embeds already-highlighted <pre class="example code"> blocks into a page, parse_codeblocks re-processes them. The rescue clause in html_syntax_highlight_ruby_ripper checked /^<span\s+class=/ to detect pre-highlighted content, but the ^ anchor only matches spans at the start of a line. Highlighted code with leading whitespace (e.g., indented `def foo`) starts with spaces, so the check failed and h(source) HTML-escaped all the span tags into visible text. Fix 1: Change the rescue check from /^<span\s+class=/ to /<span[\s>]/ so it detects spans anywhere in the source and also handles style= spans (e.g., from Commonmarker). Fix 2: Guard parse_codeblocks against re-processing already-highlighted blocks by skipping html_syntax_highlight when the code block content already contains <span> tags. This also prevents CGI.unescapeHTML from corrupting escaped entities inside spans. Also add require in html_syntax_highlight_helper_spec.rb so the spec loads correctly when run in isolation (html_equals_string was only available via other specs in full suite).
Owner
|
This is really good stuff! |
- Extract /<span[\s>]/ to ALREADY_HIGHLIGHTED_RE constant in HtmlSyntaxHighlightHelper, used by both the rescue clause and the parse_codeblocks guard so the pattern stays in sync - Add explanatory comment on the parse_codeblocks guard documenting the CGI.unescapeHTML motivation and the known false-positive edge case for :html markup containing literal <span> tags in source - Add two unit tests directly targeting the rescue clause in html_syntax_highlight_ruby_ripper — one without and one with leading whitespace — so Fix 1 (the regex change) has its own coverage independent of the parse_codeblocks guard - Drop `if HAVE_RIPPER` from the two html_helper_spec regression tests; they exercise the parse_codeblocks guard (Fix 2), not Ripper itself, so the guard is misleading and prevents coverage on non-Ripper runs
dduugg
marked this pull request as ready for review
April 8, 2026 20:59
Contributor
Author
|
@lsegal Thanks! It's ready for review. |
Owner
|
Thanks for the PR! |
lsegal
added a commit
that referenced
this pull request
Apr 13, 2026
References: #1601, #1675, #1656, #1665, #1666, #1655, #1582, #1674, #1673, #1664, #1652, #1622, #1672, #1116, #1671, #1547, #1547, #1670, #1669, #1661, #1668, #1661, #1661, #1660, #1639, #1627, #1636, #1528, #1, #1434, #1385, #1294, #1019, #1007, #955, #929, #623, #474, #467, #456, #467, #443, #467, #478, #479, #467, #477, #467, #458, #467, #397, #467, #461, #467, #458, #467, #457, #467, #465, #467, #464, #467, #466, #467, #446, #467, #445, #467, #432, #467
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
{yard:include_tags}(or similar tag embedding) resolves, it returns HTML containing already-highlighted<pre class="example code"><code>HIGHLIGHTED</code></pre>blocks. These blocks are then re-processed byparse_codeblocks, which callshtml_syntax_highlight_ruby_ripperon the already-highlighted HTML.Ripper fails to parse HTML as Ruby and falls into the rescue clause:
The
^anchor means "start of line". For highlighted code with leading whitespace (e.g., indenteddef fooproduces<span class='kw'>def</span> ...), the first characters are spaces, not<span. The regex fails,h(source)is called, and all span tags get HTML-escaped into visible text —<span class='kw'>appears literally on the page.Fixes #1661.
Changes
lib/yard/templates/helpers/html_syntax_highlight_helper.rbALREADY_HIGHLIGHTED_RE = /<span[\s>]/so both fix sites reference the same definition/^<span\s+class=/toALREADY_HIGHLIGHTED_RE: removes the line-start anchor so spans are detected anywhere in the source; also handlesstyle=spans (e.g., from Commonmarker highlighting)lib/yard/templates/helpers/html_helper.rbparse_codeblocksusingALREADY_HIGHLIGHTED_REto skip re-highlighting when the code block content already contains<span>tags. This prevents the double-processing entirely and also preventsCGI.unescapeHTMLfrom corrupting<→<inside span contents when they're returned by the rescue path.CGI.unescapeHTMLand noting the known edge case: code blocks in:htmlmarkup that contain a literal<span>in the Ruby source being documented will have highlighting suppressed (uncommon; old behavior was to corrupt the output anyway)spec/templates/helpers/html_syntax_highlight_helper_spec.rbrequireforspec/templates/spec_helper.rbso the spec works when run in isolation (previouslyhtml_equals_stringwas only available when the full suite loaded other template specs first)html_syntax_highlight_ruby_ripperwith pre-highlighted content — one without leading whitespace, one with — so the regex fix has targeted coverage independent of theparse_codeblocksguardspec/templates/helpers/html_helper_spec.rbparse_codeblocksguard (not Ripper itself), so drop theif HAVE_RIPPERguard that was preventing them from running on non-Ripper CI configurationsTest plan
bundle exec rspec spec/templates/helpers/— all passbundle exec rspec spec/templates/helpers/html_syntax_highlight_helper_spec.rb— passes in isolation (was broken before)bundle exec rspec— full suite, 0 failures