Skip to content

fix(compiler): preserve &ngsp; between sibling control flow blocks#69583

Open
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/compiler_55791
Open

fix(compiler): preserve &ngsp; between sibling control flow blocks#69583
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/compiler_55791

Conversation

@arturovt

Copy link
Copy Markdown
Contributor

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 #55791

@pullapprove pullapprove Bot requested a review from atscott June 30, 2026 12:51
@angular-robot angular-robot Bot added the area: compiler Issues related to `ngc`, Angular's template compiler label Jun 30, 2026
@ngbot ngbot Bot added this to the Backlog milestone Jun 30, 2026

@JeanMeche JeanMeche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**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:

  • WhitespaceVisitor actually only converts &ngsp; to a plain space. It leaves   as \u00A0 (non-breaking space).
  • The only reason   gets caught is because JavaScript's String.prototype.trim() intrinsically strips \u00A0.
  • Because findConnectedBlocks relies purely on .trim().length === 0, if a user explicitly writes @if (c) {} &ngsp; @else {} or @if (c) {}   @else {}, those text nodes are pushed to pendingTextNodes and then silently deleted when the @else block is found.
  • Inconsistency: If a user types explicit text like A between @if and @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 @if block eagerly consumes the @else block into its single t.IfBlock AST representation, the main visitChildren loop processes the IfBlock first.
  • It then proceeds to process the comments that were physically located between the @if and @else blocks because they were never added to processedNodes.
  • As a result, any comment placed between the branches is structurally reordered to appear after the entire IfBlock in the AST.
  • Impact: If a user writes an <!-- i18n: meaning | description --> comment specifically to document the @else block, that comment will be completely disjointed from the @else branch and will incorrectly attach itself to whatever element comes after the entire conditional block.

@arturovt

Copy link
Copy Markdown
Contributor Author

@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.

@JeanMeche

Copy link
Copy Markdown
Member

It looks like the presubits breakages are legit.

Here is a component that regressed, an additional new line was added.

<p (click)="click()">Hello {{name}} <mat-icon>face</mat-icon></p>
@if (note()) {
  <p>Note: {{note()}}</p>
}

Fwiw, it had preservedWhitespace: true.

`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 `&nbsp;`) 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
@arturovt arturovt force-pushed the fix/compiler_55791 branch from bbf1ce4 to 4852638 Compare July 7, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: compiler Issues related to `ngc`, Angular's template compiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ngsp is not working properly with control flow syntax

2 participants