From 342c14b7cf39d555aaabd2696b196d80c887664b Mon Sep 17 00:00:00 2001 From: Matthieu Riegler Date: Mon, 20 Apr 2026 20:22:51 +0200 Subject: [PATCH] fix(forms): Handle orphan children that recieve events Prior to this change children nodes that where both deleted and recieved an event on during the same tick were throwing errors. This commit fixes it. fixes #66711 --- goldens/public-api/forms/signals/index.api.md | 1 + .../signals/compat/src/compat_structure.ts | 13 +++- packages/forms/signals/src/api/types.ts | 5 ++ .../signals/src/directive/control_custom.ts | 12 ++- .../signals/src/directive/control_cva.ts | 15 ++-- .../signals/src/directive/control_native.ts | 10 ++- packages/forms/signals/src/field/node.ts | 4 + packages/forms/signals/src/field/structure.ts | 75 +++++++++++++------ packages/forms/signals/test/web/focus.spec.ts | 22 ++++++ 9 files changed, 121 insertions(+), 36 deletions(-) diff --git a/goldens/public-api/forms/signals/index.api.md b/goldens/public-api/forms/signals/index.api.md index ff0a6701c970..31eac86cdc7e 100644 --- a/goldens/public-api/forms/signals/index.api.md +++ b/goldens/public-api/forms/signals/index.api.md @@ -507,6 +507,7 @@ export interface ReadonlyFieldState): boolean; readonly hidden: Signal; readonly invalid: Signal; + readonly isOrphaned: Signal; readonly keyInParent: Signal; readonly max: Signal | undefined; readonly maxLength: Signal | undefined; diff --git a/packages/forms/signals/compat/src/compat_structure.ts b/packages/forms/signals/compat/src/compat_structure.ts index 7c86c0b6e066..f8b4bbc0483e 100644 --- a/packages/forms/signals/compat/src/compat_structure.ts +++ b/packages/forms/signals/compat/src/compat_structure.ts @@ -8,10 +8,10 @@ import { computed, + ɵRuntimeError as RuntimeError, Signal, signal, WritableSignal, - ɵRuntimeError as RuntimeError, } from '@angular/core'; import {RuntimeErrorCode} from '../../src/errors'; import {FormFieldManager} from '../../src/field/manager'; @@ -109,6 +109,7 @@ function getControlValueSignal(options: CompatFieldNodeOptions) { export class CompatStructure extends FieldNodeStructure { override value: WritableSignal; override keyInParent: Signal; + override isOrphaned: Signal; override root: FieldNode; override pathKeys: Signal; override readonly children = signal([]); @@ -130,7 +131,15 @@ export class CompatStructure extends FieldNodeStructure { const identityInParent = options.kind === 'child' ? options.identityInParent : undefined; const initialKeyInParent = options.kind === 'child' ? options.initialKeyInParent : undefined; - this.keyInParent = this.createKeyInParent(options, identityInParent, initialKeyInParent); + const keyState = this.createKeyInParentState(options, identityInParent, initialKeyInParent); + this.keyInParent = computed(() => { + const s = keyState(); + if (s.type === 'error') { + throw new RuntimeError(s.error, s.msg); + } + return s.key; + }); + this.isOrphaned = computed(() => keyState().type === 'error'); this.pathKeys = computed(() => this.parent ? [...this.parent.structure.pathKeys(), this.keyInParent()] : [], diff --git a/packages/forms/signals/src/api/types.ts b/packages/forms/signals/src/api/types.ts index bb25df6a46db..044504a82d54 100644 --- a/packages/forms/signals/src/api/types.ts +++ b/packages/forms/signals/src/api/types.ts @@ -462,6 +462,11 @@ export interface ReadonlyFieldState; + /** + * A signal indicating whether the field is orphaned. + */ + readonly isOrphaned: Signal; + /** * The {@link FormField} directives that bind this field to a UI control. */ diff --git a/packages/forms/signals/src/directive/control_custom.ts b/packages/forms/signals/src/directive/control_custom.ts index d4767a61e7db..89f5d37a1fcb 100644 --- a/packages/forms/signals/src/directive/control_custom.ts +++ b/packages/forms/signals/src/directive/control_custom.ts @@ -7,7 +7,7 @@ */ import type {ɵControlDirectiveHost as ControlDirectiveHost} from '@angular/core'; -import type {FormField} from './form_field'; +import {FormUiControl} from '../api/control'; import { bindingUpdated, CONTROL_BINDING_NAMES, @@ -15,15 +15,19 @@ import { createBindings, readFieldStateBindingValue, } from './bindings'; +import type {FormField} from './form_field'; import {setNativeDomProperty} from './native'; -import {FormUiControl} from '../api/control'; export function customControlCreate( host: ControlDirectiveHost, parent: FormField, ): () => void { - host.listenToCustomControlModel((value) => parent.state().controlValue.set(value)); - host.listenToCustomControlOutput('touch', () => parent.state().markAsTouched()); + host.listenToCustomControlModel((value) => { + if (!parent.state().isOrphaned()) parent.state().controlValue.set(value); + }); + host.listenToCustomControlOutput('touch', () => { + if (!parent.state().isOrphaned()) parent.state().markAsTouched(); + }); parent.registerAsBinding(host.customControl as FormUiControl); diff --git a/packages/forms/signals/src/directive/control_cva.ts b/packages/forms/signals/src/directive/control_cva.ts index 8d9cff89cfac..f97f472c1fb5 100644 --- a/packages/forms/signals/src/directive/control_cva.ts +++ b/packages/forms/signals/src/directive/control_cva.ts @@ -10,22 +10,22 @@ import { computed, signal, untracked, + type ɵControlDirectiveHost as ControlDirectiveHost, type Signal, type WritableSignal, - type ɵControlDirectiveHost as ControlDirectiveHost, } from '@angular/core'; -import {NG_VALIDATORS, type Validator, Validators, type ValidatorFn} from '@angular/forms'; -import {reactiveErrorsToSignalErrors} from '../compat/validation_errors'; +import {NG_VALIDATORS, Validators, type Validator, type ValidatorFn} from '@angular/forms'; import {type ValidationError} from '../api/rules'; +import {reactiveErrorsToSignalErrors} from '../compat/validation_errors'; import { bindingUpdated, CONTROL_BINDING_NAMES, - type ControlBindingKey, createBindings, readFieldStateBindingValue, + type ControlBindingKey, } from './bindings'; -import {setNativeDomProperty} from './native'; import type {FormField} from './form_field'; +import {setNativeDomProperty} from './native'; function isValidatorObject(v: Function | Validator): v is Validator { return typeof v === 'object' && v !== null; @@ -38,13 +38,16 @@ export function cvaControlCreate( const bindings = createBindings(); parent.controlValueAccessor!.registerOnChange((value: unknown) => { + if (parent.state().isOrphaned()) return; // Update tracking for 'controlValue' here so that when the effect runs, // `bindingUpdated` sees that the model value matches the last seen view value. // This prevents the framework from writing the same value back to the CVA (CVA loopback). bindings['controlValue'] = value; parent.state().controlValue.set(value as any); }); - parent.controlValueAccessor!.registerOnTouched(() => parent.state().markAsTouched()); + parent.controlValueAccessor!.registerOnTouched(() => { + if (!parent.state().isOrphaned()) parent.state().markAsTouched(); + }); const legacyValidators = parent.injector.get(NG_VALIDATORS, null, {optional: true, self: true}); if (legacyValidators) { diff --git a/packages/forms/signals/src/directive/control_native.ts b/packages/forms/signals/src/directive/control_native.ts index d90537886b38..c93cbf4017b0 100644 --- a/packages/forms/signals/src/directive/control_native.ts +++ b/packages/forms/signals/src/directive/control_native.ts @@ -23,8 +23,8 @@ import type {FormField} from './form_field'; import {InputValidityMonitor} from './input_validity_monitor'; import { getNativeControlValue, - isInput, inputRequiresValidityTracking, + isInput, setNativeControlValue, setNativeDomProperty, } from './native'; @@ -54,8 +54,12 @@ export function nativeControlCreate( parseErrorsSource.set(parser.errors); // Pass undefined as the raw value since the parse function doesn't care about it. - host.listenToDom('input', () => parser.setRawValue(undefined)); - host.listenToDom('blur', () => parent.state().markAsTouched()); + host.listenToDom('input', () => { + if (!parent.state().isOrphaned()) parser.setRawValue(undefined); + }); + host.listenToDom('blur', () => { + if (!parent.state().isOrphaned()) parent.state().markAsTouched(); + }); // TODO: move extraction to first update pass? if (isInput(input) && inputRequiresValidityTracking(input)) { diff --git a/packages/forms/signals/src/field/node.ts b/packages/forms/signals/src/field/node.ts index d494ab7d67a3..8aa3e49c778f 100644 --- a/packages/forms/signals/src/field/node.ts +++ b/packages/forms/signals/src/field/node.ts @@ -164,6 +164,10 @@ export class FieldNode implements FieldState { return this.structure.keyInParent; } + get isOrphaned(): Signal { + return this.structure.isOrphaned; + } + get errors(): Signal { return this.validationState.errors; } diff --git a/packages/forms/signals/src/field/structure.ts b/packages/forms/signals/src/field/structure.ts index 8caa8d0b59d6..b9df03030314 100644 --- a/packages/forms/signals/src/field/structure.ts +++ b/packages/forms/signals/src/field/structure.ts @@ -40,6 +40,17 @@ export type ChildNodeCtor = ( isArray: boolean, ) => FieldNode; +export type KeyInParentState = + | { + type: 'error'; + error: RuntimeErrorCode; + msg: string | false | null; + } + | { + type: 'key'; + key: string; + }; + /** Structural component of a `FieldNode` which tracks its path, parent, and children. */ export abstract class FieldNodeStructure { /** @@ -59,6 +70,9 @@ export abstract class FieldNodeStructure { */ abstract readonly keyInParent: Signal; + /** Whether this field currently has an associated parent node that considers it a child. */ + abstract readonly isOrphaned: Signal; + /** The field manager responsible for managing this field. */ abstract readonly fieldManager: FormFieldManager; @@ -207,30 +221,32 @@ export abstract class FieldNodeStructure { * @param initialKeyInParent The initial key in parent (only for child nodes) * @returns A signal representing the field's key in its parent */ - protected createKeyInParent( + protected createKeyInParentState( options: FieldNodeOptions, identityInParent: TrackingKey | undefined, initialKeyInParent: string | undefined, - ): Signal { + ): Signal { if (options.kind === 'root') { - return ROOT_KEY_IN_PARENT; + return computed(() => ({key: ROOT_KEY_IN_PARENT(), type: 'key'})); } if (identityInParent === undefined) { const key = initialKeyInParent!; - return computed(() => { + return computed(() => { if (this.parent!.structure.getChild(key) !== this.node) { - throw new RuntimeError( - RuntimeErrorCode.ORPHAN_FIELD_PROPERTY, - ngDevMode && + return { + type: 'error', + error: RuntimeErrorCode.ORPHAN_FIELD_PROPERTY, + msg: + ngDevMode && `Orphan field, looking for property '${key}' of ${getDebugName(this.parent!)}`, - ); + }; } - return key; + return {key, type: 'key'}; }); } else { let lastKnownKey = initialKeyInParent!; - return computed(() => { + return computed(() => { // TODO(alxhub): future perf optimization: here we depend on the parent's value, but most // changes to the value aren't structural - they aren't moving around objects and thus // shouldn't affect `keyInParent`. We currently mitigate this issue via `lastKnownKey` @@ -240,10 +256,11 @@ export abstract class FieldNodeStructure { // It should not be possible to encounter this error. It would require the parent to // change from an array field to non-array field. However, in the current implementation // a field's parent can never change. - throw new RuntimeError( - RuntimeErrorCode.ORPHAN_FIELD_ARRAY, - ngDevMode && `Orphan field, expected ${getDebugName(this.parent!)} to be an array`, - ); + return { + type: 'error', + error: RuntimeErrorCode.ORPHAN_FIELD_ARRAY, + msg: ngDevMode && `Orphan field, expected ${getDebugName(this.parent!)} to be an array`, + }; } // Check the parent value at the last known key to avoid a scan. @@ -255,7 +272,7 @@ export abstract class FieldNodeStructure { data.hasOwnProperty(this.parent!.structure.identitySymbol) && data[this.parent!.structure.identitySymbol] === identityInParent ) { - return lastKnownKey; + return {key: lastKnownKey, type: 'key'}; } // Otherwise, we need to check all the keys in the parent. @@ -266,14 +283,17 @@ export abstract class FieldNodeStructure { data.hasOwnProperty(this.parent!.structure.identitySymbol) && data[this.parent!.structure.identitySymbol] === identityInParent ) { - return (lastKnownKey = i.toString()); + lastKnownKey = i.toString(); + return {key: lastKnownKey, type: 'key'}; } } - throw new RuntimeError( - RuntimeErrorCode.ORPHAN_FIELD_NOT_FOUND, - ngDevMode && `Orphan field, can't find element in array ${getDebugName(this.parent!)}`, - ); + return { + type: 'error', + error: RuntimeErrorCode.ORPHAN_FIELD_NOT_FOUND, + msg: + ngDevMode && `Orphan field, can't find element in array ${getDebugName(this.parent!)}`, + }; }); } } @@ -436,6 +456,10 @@ export class RootFieldNodeStructure extends FieldNodeStructure { return ROOT_KEY_IN_PARENT; } + override get isOrphaned(): Signal { + return computed(() => false); + } + protected override readonly childrenMap: Signal; /** @@ -467,6 +491,7 @@ export class ChildFieldNodeStructure extends FieldNodeStructure { override readonly root: FieldNode; override readonly pathKeys: Signal; override readonly keyInParent: Signal; + override readonly isOrphaned: Signal; override readonly value: WritableSignal; override readonly childrenMap: Signal; @@ -498,7 +523,7 @@ export class ChildFieldNodeStructure extends FieldNodeStructure { this.root = this.parent.structure.root; - this.keyInParent = this.createKeyInParent( + const keyState = this.createKeyInParentState( { kind: 'child', parent, @@ -511,6 +536,14 @@ export class ChildFieldNodeStructure extends FieldNodeStructure { identityInParent, initialKeyInParent, ); + this.keyInParent = computed(() => { + const s = keyState(); + if (s.type === 'error') { + throw new RuntimeError(s.error, s.msg || false); + } + return s.key!; + }); + this.isOrphaned = computed(() => keyState().type === 'error'); this.pathKeys = computed(() => [...parent.structure.pathKeys(), this.keyInParent()]); diff --git a/packages/forms/signals/test/web/focus.spec.ts b/packages/forms/signals/test/web/focus.spec.ts index 998b6b24558d..4c6b42a6eaac 100644 --- a/packages/forms/signals/test/web/focus.spec.ts +++ b/packages/forms/signals/test/web/focus.spec.ts @@ -307,6 +307,28 @@ describe('FieldState focus behavior', () => { await act(() => fixture.componentInstance.f().focusBoundControl({preventScroll: true})); expect(receivedOptions).toEqual({preventScroll: true}); }); + + it('should handle gracefully a removal+blur on the removed element', async () => { + @Component({ + selector: 'app-root', + imports: [FormField], + template: ` + @for (field of form; track field) { + + } + `, + }) + class App { + form = form(signal(['foo'])); + } + + const fixture = TestBed.createComponent(App); + + await fixture.whenStable(); + + fixture.componentInstance.form().value.set([]); // remove + document.querySelector('input')!.dispatchEvent(new Event('blur')); + }); }); async function act(fn: () => T): Promise {