diff --git a/craft/src/utils/format-html.test.ts b/craft/src/utils/format-html.test.ts index fc9cb3a..81c62f8 100644 --- a/craft/src/utils/format-html.test.ts +++ b/craft/src/utils/format-html.test.ts @@ -81,3 +81,31 @@ describe('formatHtml - raw content and quoted attributes', () => { ); }); }); + +// Regression coverage from round 2 of code review: a close tag whose +// innermost open frame doesn't match it must not wedge the stack for the +// rest of the document. An optional end tag skipped by the author (e.g. +// an unclosed

) must self-drain against its real ancestor close tag, +// while a close tag with no opener anywhere on the stack still has nothing +// to pair with and is left exactly where it is. +describe('formatHtml - mismatched close recovery', () => { + test('an unclosed

before drains against the ancestor close instead of wedging the stack', () => { + const src = '

one

two

three

'; + expect(formatHtml(src)).toBe( + '
\n

\n one\n

two

\n
\n

three

', + ); + }); + + test('a close tag with no opener anywhere on the stack is left in place', () => { + const src = '

x

'; + expect(formatHtml(src)).toBe('
\n

x

\n \n
'); + }); + + test('is idempotent across an unclosed

and a stray close with no opener', () => { + const unclosedP = formatHtml('

one

two

three

'); + expect(formatHtml(unclosedP)).toBe(unclosedP); + + const strayClose = formatHtml('

x

'); + expect(formatHtml(strayClose)).toBe(strayClose); + }); +}); diff --git a/craft/src/utils/format-html.ts b/craft/src/utils/format-html.ts index f16e311..d2d3249 100644 --- a/craft/src/utils/format-html.ts +++ b/craft/src/utils/format-html.ts @@ -50,10 +50,34 @@ interface Token { kind: 'open' | 'close' | 'void' | 'text' | 'verbatim'; } -/** Collapse a text run's whitespace to single spaces; drop it if it is all whitespace. */ +/** + * 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. + * + * 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. + */ function pushTextToken(tokens: Token[], raw: string): void { - const collapsed = raw.replace(/\s+/g, ' '); - if (collapsed.trim()) tokens.push({ text: collapsed, tag: '', kind: 'text' }); + 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') ? ' ' : ''; + const text = leadSpace + core.replace(/\s+/g, ' ') + trailSpace; + + tokens.push({ text, tag: '', kind: 'text' }); } /** @@ -152,6 +176,24 @@ interface Frame { committed: boolean; } +/** + * Emit whatever a still-open frame needs, without ever fabricating a + * closing tag for it: its own open-tag line if it was never committed, and + * any inline content it was holding. Used both for elements that are + * genuinely never closed anywhere in the document, and for elements + * implicitly closed by an ancestor's close tag (e.g. an optional end tag + * like `

` that the author skipped) -- either way, no synthetic close tag + * is written; only indentation for content that really was opened. + */ +function implicitlyClose(lines: string[], frame: Frame): void { + if (!frame.committed) { + lines.push(INDENT.repeat(frame.depth) + frame.text); + } + if (frame.inline) { + lines.push(INDENT.repeat(frame.depth + 1) + frame.inline); + } +} + export function formatHtml(src: string): string { if (!src || !src.trim()) return ''; @@ -194,21 +236,37 @@ export function formatHtml(src: string): string { } if (token.kind === 'close') { - const top = stack[stack.length - 1]; + // Find the nearest still-open frame with this tag name, anywhere on + // the stack -- not just the top. HTML permits skipping optional end + // tags (e.g. `

`), so the element a close tag pairs with is not + // always the innermost open element. + let matchIndex = -1; + for (let k = stack.length - 1; k >= 0; k -= 1) { + if (stack[k].tag === token.tag) { + matchIndex = k; + break; + } + } - if (!top || top.tag !== token.tag) { - // Stray close: either nothing is open, or the innermost open frame - // is a *different* element. Don't fabricate a pairing by popping - // it anyway -- that would misattribute a real element's close tag - // to the wrong element and leave the stack silently desynced for - // everything after it. Emit this close in place instead, and leave - // whatever is genuinely open on the stack for later resolution. + if (matchIndex === -1) { + // Genuinely stray: no frame anywhere was opened with this tag + // name, so there is nothing to pair it with. Don't fabricate a + // pairing -- emit it in place and leave the stack untouched. if (stack.length === 0) flushRootInline(); lines.push(INDENT.repeat(stack.length) + token.text); continue; } - stack.pop(); + // Everything above the match was opened but never explicitly closed + // in the source (e.g. a skipped `

`). Draining them here -- with + // no synthetic close tag -- keeps the stack from staying wedged for + // the rest of the document, restoring the self-draining property + // without fabricating markup. + while (stack.length - 1 > matchIndex) { + implicitlyClose(lines, stack.pop()!); + } + + const top = stack.pop()!; if (!top.committed) { // Nothing block-level ever interrupted this element: merge the // open tag, its inline content, and the close tag onto one line. @@ -241,13 +299,7 @@ export function formatHtml(src: string): string { // Unbalanced opens: nothing ever closed them. Flush what's left rather // than silently dropping content. while (stack.length) { - const top = stack.pop()!; - if (!top.committed) { - lines.push(INDENT.repeat(top.depth) + top.text); - } - if (top.inline) { - lines.push(INDENT.repeat(top.depth + 1) + top.inline); - } + implicitlyClose(lines, stack.pop()!); } flushRootInline();