Formatter: fix attribute spacing lost on elements nested in ERB blocks - #2148
Open
vjymisal0 wants to merge 2 commits into
Open
Formatter: fix attribute spacing lost on elements nested in ERB blocks#2148vjymisal0 wants to merge 2 commits into
vjymisal0 wants to merge 2 commits into
Conversation
When formatting a content-preserving element (e.g. <pre>) whose body
contains an ERB block/conditional, the formatter falls back to
IdentityPrinter to reconstruct that ERB-wrapped subtree byte-for-byte.
Without track_whitespace: true, the parser doesn't emit a node for the
whitespace that separates a tag name from its first attribute, or the
whitespace between attributes - that gap simply isn't represented in the
AST. IdentityPrinter.visitHTMLOpenTagNode wrote the tag name and then its
children back-to-back with no separator, so any HTML element with
attributes inside an ERB block lost its attribute spacing:
<pre><% if condition %><span class="x">x</span><% end %></pre>
formatted to:
<pre><% if condition %><spanclass="x">x</span><% end %></pre>
which changes browser behavior (the elements aren't equivalent - the
formatted version creates a bogus spanclass custom element and drops
the class attribute). Confirmed against @herb-tools/formatter 0.10.3
per the report in marcoroth#2142.
Fix IdentityPrinter.visitHTMLOpenTagNode to compare each child's start
position against the end of the previously written node, restoring a
single separating space whenever the AST has left a gap. When whitespace
tracking is enabled the gap is already covered by an explicit
WhitespaceNode, so no extra space is added in that case (verified via the
printer package's own IndentPrinter/track_whitespace test suite, which
continues to pass unchanged).
Added regression tests to
javascript/packages/formatter/test/html/content-preserving-tags.test.ts
covering both a single-attribute and multi-attribute element nested in an
ERB block.
Tested via vitest run in javascript/packages/printer (116/116 passing,
matching baseline) and javascript/packages/formatter (1372/1480 passing,
2 more than baseline - the 2 new regression tests; the remaining failures
are pre-existing and unrelated, mostly CLI-binary and Tailwind-sorter
tests that need a built binary/tailwind config not available in this
sandbox).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
marcoroth
reviewed
Aug 11, 2026
marcoroth
left a comment
Owner
There was a problem hiding this comment.
Hey @vjymisal0, thanks for the pull request!
Would you mind looking at the failing tests, as they seem directly related to your changes, thank you! 🙏🏼
Author
|
Noting for the record: the 'main' CI check is failing on javascript/packages/rewriter/test/action-view-tag-helper-to-html.test.ts (expected "<div class="content">" but got "<div class="content">" — a double space), which looks directly related to this PR's attribute-spacing fix. I wasn't able to check out this branch in my current environment (the repo has filenames containing '?' that are invalid on Windows filesystems), so flagging this test failure for a fix rather than guessing at one. |
…dy prints its own whitespace IdentityPrinter's tag-open gap-filling compared node positions to decide whether to insert a separating space between children. Synthetic WhitespaceNode children inserted by autofix rules (e.g. html-no-space-in-tag, erb-no-trailing-whitespace) don't have positions that line up with the surrounding nodes, so the gap-fill logic inserted an extra space in addition to the space the WhitespaceNode itself prints, producing doubled spaces like '<div />' instead of '<div />'. WhitespaceNode children are now always skipped by the gap-fill heuristic since they already print the correct whitespace themselves.
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
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.
Summary
Fixes #2142.
When the formatter encounters a content-preserving element (e.g.
<pre>) whose body contains an ERB block or conditional, it falls back toIdentityPrinterto reconstruct that ERB-wrapped subtree byte-for-byte (javascript/packages/formatter/src/format-printer.ts→visitContentPreservingBody).IdentityPrinter.visitHTMLOpenTagNode(javascript/packages/printer/src/identity-printer.ts) wrote the tag name and then its attribute children back-to-back with no separator. Without the parser'strack_whitespace: trueoption (which the formatter does not pass), the AST simply has no node for the whitespace between a tag name and its first attribute, or between attributes — that gap isn't represented at all. So any HTML element with attributes nested inside an ERB block lost its attribute spacing:formatted to:
This isn't just a cosmetic diff —
<spanclass="x">is a different (bogus custom) element than<span class="x">, so the formatter silently changes rendered output.Root cause
Confirmed by inspecting the parsed AST directly:
Herb.parse(source)(default options, notrack_whitespace) returnsopen_tag.childrencontaining only theHTMLAttributeNodes — there is no node covering the single space between the tag name and the first attribute, or between attributes.IdentityPrinterassumed children could be printed contiguously, which only holds whentrack_whitespace: trueis used (in that mode an explicitWhitespaceNodefills the gap).Fix
IdentityPrinter.visitHTMLOpenTagNodenow tracks the end position of the last thing it wrote and compares it against each child's start position. If there's a gap (no node covers it — thetrack_whitespace: falsecase), it writes a single separating space. If the positions already line up (an explicitWhitespaceNodealready covers the gap, i.e.track_whitespace: true), nothing extra is written.How I found and verified this
@herb-tools/formatter@0.10.3+@herb-tools/node-wasm@0.10.3packages with the exact example from Formatter: Attribute spacing is lost inside ERB blocks in pre elements #2142 — confirmed the<spanclass="x">corruption.track_whitespace) to find the actual gap in node coverage described above.<div <%= attrs %>>), and interpolated attribute values, and formatting is idempotent (format(format(x)) === format(x)) in every case I tried.javascript/packages/printer:vitest run— 116/116 real assertions pass (unchanged from baseline; the 45 failing test files are pre-existing, unrelated to this change — they fail to import in this environment becauseERBEndNode.buildrequires the generatednodes.tsfrom the Ruby/C codegen pipeline, which isn't available without the full C toolchain).javascript/packages/formatter:vitest run— 1372/1480 passing, 2 more than baseline (the 2 new regression tests below). The 101 remaining failures are pre-existing and unrelated (CLI-binary tests needing a built executable, Tailwind class-sorter tests needing a Tailwind config) — verified identical failure set with and without this change.javascript/packages/formatter/test/html/content-preserving-tags.test.ts: the exact case from Formatter: Attribute spacing is lost inside ERB blocks in pre elements #2142, plus a multi-attribute variant.Test plan
cd javascript/packages/printer && vitest run— no regressions (116/116 real assertions pass, matching baseline)cd javascript/packages/formatter && vitest run test/html/content-preserving-tags.test.ts test/erb/content-preserving-blocks.test.ts test/erb/whitespace-preservation.test.ts— 94/94 pass, including the 2 new regression testscd javascript/packages/formatter && vitest run(full suite) — no new failures vs. baseline