fix(compiler): preserve &ngsp; between sibling control flow blocks#69583
fix(compiler): preserve &ngsp; between sibling control flow blocks#69583arturovt wants to merge 1 commit into
Conversation
JeanMeche
left a comment
There was a problem hiding this comment.
**AGENT: ** Here are two subtle but important issues with this implementation:
1. Silent deletion of explicitly requested non-ignorable spaces (&ngsp; and )
The PR description notes: "WhitespaceVisitor has already converted &ngsp; (and ) into a plain space character, making them indistinguishable from insignificant whitespace via .trim().length". This contains a factual error and introduces a logical inconsistency:
WhitespaceVisitoractually only converts&ngsp;to a plain space. It leaves as\u00A0(non-breaking space).- The only reason
gets caught is because JavaScript'sString.prototype.trim()intrinsically strips\u00A0. - Because
findConnectedBlocksrelies purely on.trim().length === 0, if a user explicitly writes@if (c) {} &ngsp; @else {}or@if (c) {} @else {}, those text nodes are pushed topendingTextNodesand then silently deleted when the@elseblock is found. - Inconsistency: If a user types explicit text like
Abetween@ifand@else, it correctly breaks the block connection and throws a compilation error (@else block can only be used after an @if...). Explicit spaces like&ngsp;and should arguably trigger the exact same parsing error (or be strictly preserved), rather than being mysteriously swallowed.
2. AST Reordering and i18n misattachment for skipped sibling nodes
The PR (and the existing logic it builds upon) uses continue to skip over html.Comment nodes, and now also defers whitespace nodes via pendingTextNodes. When an @else block is found, findConnectedBlocks commits the pending text to processedNodes but completely ignores the skipped comments.
- Because the outer
@ifblock eagerly consumes the@elseblock into its singlet.IfBlockAST representation, the mainvisitChildrenloop processes theIfBlockfirst. - It then proceeds to process the comments that were physically located between the
@ifand@elseblocks because they were never added toprocessedNodes. - As a result, any comment placed between the branches is structurally reordered to appear after the entire
IfBlockin the AST. - Impact: If a user writes an
<!-- i18n: meaning | description -->comment specifically to document the@elseblock, that comment will be completely disjointed from the@elsebranch and will incorrectly attach itself to whatever element comes after the entire conditional block.
|
@JeanMeche on both concerns: the silent deletion of significant whitespace between connected blocks and the comment AST reordering are pre-existing behaviors that this PR doesn't change. |
|
It looks like the presubits breakages are legit. Here is a component that regressed, an additional new line was added. Fwiw, it had |
`findConnectedBlocks` scans siblings after an `@if` to collect connected `@else`/`@else if` blocks. Whitespace-only text nodes encountered during the scan were eagerly added to `processedNodes`, marking them as "do not emit", before confirming whether a connected block actually followed. By the time `findConnectedBlocks` runs, `WhitespaceVisitor` has already converted `&ngsp;` (and ` `) into a plain space character, making them indistinguishable from insignificant whitespace via `.trim().length`. When the next sibling was a second, unrelated `@if` instead of `@else`, the scan stopped but the text node was already silently dropped. Fix by deferring the `processedNodes` insertion into a pending buffer and only committing those nodes once a connected block is confirmed to follow. Fixes angular#55791
bbf1ce4 to
4852638
Compare
findConnectedBlocksscans siblings after an@ifto collect connected@else/@else ifblocks. Whitespace-only text nodes encountered during the scan were eagerly added toprocessedNodes, marking them as "do not emit", before confirming whether a connected block actually followed.By the time
findConnectedBlocksruns,WhitespaceVisitorhas already converted&ngsp;(and ) into a plain space character, making them indistinguishable from insignificant whitespace via.trim().length. When the next sibling was a second, unrelated@ifinstead of@else, the scan stopped but the text node was already silently dropped.Fix by deferring the
processedNodesinsertion into a pending buffer and only committing those nodes once a connected block is confirmed to follow.Fixes #55791