From 45ee00467237ecdc28d02ab9e632a954082f97d5 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sat, 8 Aug 2026 19:19:32 -0700 Subject: [PATCH] fix(site-builder): discriminate formatHtml whitespace by block/inline boundary, not newline Round 3 fixed a phantom-space idempotency bug by dropping any boundary whitespace containing a newline, but that also stripped ordinary hand-wrapped text like "hello\n", merging words on a single pass -- directly contradicting this formatter's own "does not reflow text" contract. The correct discriminator is what the whitespace borders, not whether it contains a newline: a run between two inline-level things (text, , , ...) is always significant and must survive regardless of newlines, while a run touching a block-tag boundary carries no rendered meaning and is always dropped. Since this formatter's own emitted indentation is only ever inserted next to a block tag, that rule also resolves the original phantom-space bug without any newline special-casing. tokenize() now peeks each upcoming tag's name once (reused for both the preceding text run's decision and the tag's own processing) so pushTextToken can see what's on both sides of a whitespace run. Co-Authored-By: Claude Opus 5 (1M context) --- craft/src/utils/format-html.test.ts | 41 +++++++++++++++ craft/src/utils/format-html.ts | 82 ++++++++++++++++++++--------- 2 files changed, 97 insertions(+), 26 deletions(-) diff --git a/craft/src/utils/format-html.test.ts b/craft/src/utils/format-html.test.ts index 81c62f8..3f17658 100644 --- a/craft/src/utils/format-html.test.ts +++ b/craft/src/utils/format-html.test.ts @@ -109,3 +109,44 @@ describe('formatHtml - mismatched close recovery', () => { expect(formatHtml(strayClose)).toBe(strayClose); }); }); + +// Regression coverage from round 3 of code review: the previous fix for the +// phantom-space idempotency bug used "does this boundary whitespace contain +// a newline" as its drop/keep rule, which also stripped hand-wrapped text +// like "hello\n" -- a single-pass content change, not a +// repeated-pass artifact. The correct discriminator is what the whitespace +// borders: whitespace between two inline-level things (text, , ...) +// is always significant and must survive regardless of newlines; whitespace +// touching a block-tag boundary carries no rendered meaning and is always +// dropped, regardless of newlines. +describe('formatHtml - inline whitespace vs block-boundary whitespace', () => { + test('a newline between text and an inline tag is kept as a single space', () => { + expect(formatHtml('

hello\nworld

')).toBe( + '

hello world

', + ); + }); + + test('a literal space in the same position is kept unchanged (pin)', () => { + expect(formatHtml('

hello world

')).toBe( + '

hello world

', + ); + }); + + test('a newline at a block-tag boundary is still dropped', () => { + expect(formatHtml('
\n
')).toBe('
'); + }); + + test('is idempotent across inline-boundary whitespace, block-boundary whitespace, and the case that originally exposed the phantom space', () => { + const wrapped = formatHtml('

hello\nworld

'); + expect(formatHtml(wrapped)).toBe(wrapped); + + const sameLine = formatHtml('

hello world

'); + expect(formatHtml(sameLine)).toBe(sameLine); + + const blockGap = formatHtml('
\n
'); + expect(formatHtml(blockGap)).toBe(blockGap); + + const phantomSpaceCase = formatHtml('

one

two

three

'); + expect(formatHtml(phantomSpaceCase)).toBe(phantomSpaceCase); + }); +}); diff --git a/craft/src/utils/format-html.ts b/craft/src/utils/format-html.ts index d2d3249..f3eaab0 100644 --- a/craft/src/utils/format-html.ts +++ b/craft/src/utils/format-html.ts @@ -42,6 +42,11 @@ const RAW_TEXT_TAGS = new Set(['pre', 'script', 'style']); const INDENT = ' '; +/** True for a real, known block-level tag name (see BLOCK_TAGS); '' (no tag) is not block. */ +function isBlockTag(tag: string): boolean { + return tag !== '' && BLOCK_TAGS.has(tag); +} + interface Token { /** Raw text of the token. Text runs have interior whitespace collapsed. */ text: string; @@ -53,28 +58,40 @@ interface Token { /** * Turn a raw run of text (between two tags) into a text token, collapsing * interior whitespace to single spaces. A run that is nothing but - * whitespace is dropped entirely -- it's structural gap between tags, not - * content. + * whitespace is dropped entirely UNLESS it sits between two inline-level + * things, in which case it's a real (if content-free) word gap -- e.g. + * `a b` -- and collapses to one significant space. * * For a run with real content, its leading/trailing whitespace is boiled - * down to at most one space each, and -- critically for idempotency -- - * that boundary space is kept only if it does NOT contain a newline. A - * same-line boundary space (`"hello "` before ``) is a hand-typed - * word separator and must survive. A boundary run that spans a newline - * (`"\n one\n "`, produced when this formatter puts an element's text - * on its own indented line) is formatting whitespace this function itself - * introduced; re-parsing formatted output must drop it completely; keeping - * even one space there would let re-formatting keep inserting a phantom - * space around content that had none in the original source. + * down to at most one space each, kept only on sides that border something + * inline. The discriminator is deliberately NOT "does this whitespace + * contain a newline" -- that would also strip a hand-wrapped + * `"hello\n"`, turning it into "helloworld" and rewriting the + * user's markup. It's "what does this boundary sit next to": whitespace + * between two inline-level things (text, ``, ``, ...) is always + * significant in HTML and must survive regardless of newlines; whitespace + * touching a block-tag boundary carries no rendered meaning and is always + * dropped, regardless of newlines. Because this formatter's own emitted + * indentation always sits at a block boundary (an element's own line is + * only ever created next to another block tag), that side of the rule is + * also what keeps repeated formatting from accumulating phantom spaces. + * + * `prevIsBlock`/`nextIsBlock` describe whatever sits immediately before/ + * after this run: true for a block tag or "nothing there" (start/end of + * document, or an unterminated tag), false for an inline tag. */ -function pushTextToken(tokens: Token[], raw: string): void { +function pushTextToken(tokens: Token[], raw: string, prevIsBlock: boolean, nextIsBlock: boolean): void { const core = raw.trim(); - if (!core) return; - const leading = /^\s*/.exec(raw)![0]; - const trailing = /\s*$/.exec(raw)![0]; - const leadSpace = leading && !leading.includes('\n') ? ' ' : ''; - const trailSpace = trailing && !trailing.includes('\n') ? ' ' : ''; + if (!core) { + if (!prevIsBlock && !nextIsBlock) { + tokens.push({ text: ' ', tag: '', kind: 'text' }); + } + return; + } + + const leadSpace = /^\s/.test(raw) && !prevIsBlock ? ' ' : ''; + const trailSpace = /\s$/.test(raw) && !nextIsBlock ? ' ' : ''; const text = leadSpace + core.replace(/\s+/g, ' ') + trailSpace; tokens.push({ text, tag: '', kind: 'text' }); @@ -109,29 +126,42 @@ function tokenize(src: string): Token[] { const tokens: Token[] = []; let i = 0; + // Whatever tag most recently landed in `tokens` (undefined at the very + // start of the document, which counts as a block-like boundary). + const prevIsBlock = (): boolean => + tokens.length === 0 || isBlockTag(tokens[tokens.length - 1].tag); + while (i < src.length) { const lt = src.indexOf('<', i); if (lt === -1) { - pushTextToken(tokens, src.slice(i)); + // Nothing more to tokenize after this -- end of document is a + // block-like boundary too. + pushTextToken(tokens, src.slice(i), prevIsBlock(), true); break; } + // Peek the upcoming tag's name once, up front: it decides both the + // trailing-space behaviour of the text run before it (if any) and, + // below, how this tag itself is tokenized -- no need to re-scan it. + const gt = findTagEnd(src, lt); + const raw = gt === -1 ? '' : src.slice(lt, gt + 1); + const nameMatch = raw ? /^<\/?\s*([a-zA-Z][a-zA-Z0-9-]*)/.exec(raw) : null; + const tag = nameMatch ? nameMatch[1].toLowerCase() : ''; + // An unterminated tag never really opens/closes anything, so treat it + // like the end of the document for the preceding text run's purposes. + const nextIsBlock = gt === -1 || isBlockTag(tag); + if (lt > i) { - pushTextToken(tokens, src.slice(i, lt)); + pushTextToken(tokens, src.slice(i, lt), prevIsBlock(), nextIsBlock); } - const gt = findTagEnd(src, lt); if (gt === -1) { // Unterminated '<' -- emit the remainder as text rather than looping. - pushTextToken(tokens, src.slice(lt)); + pushTextToken(tokens, src.slice(lt), prevIsBlock(), true); break; } - const raw = src.slice(lt, gt + 1); - const nameMatch = /^<\/?\s*([a-zA-Z][a-zA-Z0-9-]*)/.exec(raw); - const tag = nameMatch ? nameMatch[1].toLowerCase() : ''; - // 
/