cloud-hosting-platform/site-builder
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(site-builder): discriminate formatHtml whitespace by block/inline boundary, not newline

Browse Source 
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<strong>", 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,
<strong>, <a>, ...) 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) <noreply@anthropic.com>
This commit is contained in:
shadowdao
2026-08-08 19:19:32 -07:00
co-authored by Claude Opus 5
parent 536e4e9f86
commit 45ee004672
2 changed files with 97 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
craft/src/utils/format-html.test.ts
+41
View File
@@ -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<strong>" -- 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, <strong>, ...)
// 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('<p>hello\n<strong>world</strong></p>')).toBe(
'<p>hello <strong>world</strong></p>',
);
});
test('a literal space in the same position is kept unchanged (pin)', () => {
expect(formatHtml('<p>hello <strong>world</strong></p>')).toBe(
'<p>hello <strong>world</strong></p>',
);
});
test('a newline at a block-tag boundary is still dropped', () => {
expect(formatHtml('<div>\n</div>')).toBe('<div></div>');
});
test('is idempotent across inline-boundary whitespace, block-boundary whitespace, and the case that originally exposed the phantom space', () => {
const wrapped = formatHtml('<p>hello\n<strong>world</strong></p>');
expect(formatHtml(wrapped)).toBe(wrapped);
const sameLine = formatHtml('<p>hello <strong>world</strong></p>');
expect(formatHtml(sameLine)).toBe(sameLine);
const blockGap = formatHtml('<div>\n</div>');
expect(formatHtml(blockGap)).toBe(blockGap);
const phantomSpaceCase = formatHtml('<div><p>one<p>two</p></div><p>three</p>');
expect(formatHtml(phantomSpaceCase)).toBe(phantomSpaceCase);
});
});
craft/src/utils/format-html.ts
+56 -26
View File
@@ -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.
* `<span>a</span> <span>b</span>` -- 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 `<strong>`) 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<strong>"`, 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, `<strong>`, `<a>`, ...) 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() : '';
// <pre>/<script>/<style> swallow everything up to their closing tag,
// untouched -- their content is never scanned as markup. If no closing
// tag exists, swallow to the end of the document rather than risk
@@ -223,7 +253,7 @@ export function formatHtml(src: string): string {
};
for (const token of tokens) {
const isBlock = token.tag !== '' && BLOCK_TAGS.has(token.tag);
const isBlock = isBlockTag(token.tag);
if (!isBlock) {
// Inline tag or text -- accumulate against the innermost open element.