From d201cd2c2bdb418fd1b595320855c35eb91e1e5b Mon Sep 17 00:00:00 2001 From: SkyZeroZx <73321943+SkyZeroZx@users.noreply.github.com> Date: Wed, 10 Sep 2025 18:48:44 -0500 Subject: [PATCH] feat(forms): Prevents marking fields as touched/dirty when state is hidden/readonly/disabled (#63633) Ensures fields that are hidden, disabled, or readonly cannot be marked as touched or dirty, improving form state integrity PR Close #63633 --- packages/forms/signals/src/field/state.ts | 31 ++- .../signals/test/node/field_node.spec.ts | 263 ++++++++++++++++++ 2 files changed, 284 insertions(+), 10 deletions(-) diff --git a/packages/forms/signals/src/field/state.ts b/packages/forms/signals/src/field/state.ts index 8225c270ea5..a799ef2a5a3 100644 --- a/packages/forms/signals/src/field/state.ts +++ b/packages/forms/signals/src/field/state.ts @@ -37,7 +37,6 @@ export class FieldNodeState { * Marks this specific field as touched. */ markAsTouched(): void { - // TODO: should this be noop for fields that are hidden/disabled/readonly this.selfTouched.set(true); } @@ -45,7 +44,6 @@ export class FieldNodeState { * Marks this specific field as dirty. */ markAsDirty(): void { - // TODO: should this be noop for fields that are hidden/disabled/readonly this.selfDirty.set(true); } @@ -72,13 +70,14 @@ export class FieldNodeState { * Whether this field is considered dirty. * * A field is considered dirty if one of the following is true: - * - It was directly dirtied + * - It was directly dirtied and is interactive * - One of its children is considered dirty */ readonly dirty: Signal = computed(() => { + const selfDirtyValue = this.selfDirty() && !this.isNonInteractive(); return reduceChildren( this.node, - this.selfDirty(), + selfDirtyValue, (child, value) => value || child.nodeState.dirty(), shortCircuitTrue, ); @@ -88,17 +87,18 @@ export class FieldNodeState { * Whether this field is considered touched. * * A field is considered touched if one of the following is true: - * - It was directly touched + * - It was directly touched and is interactive * - One of its children is considered touched */ - readonly touched: Signal = computed(() => - reduceChildren( + readonly touched: Signal = computed(() => { + const selfTouchedValue = this.selfTouched() && !this.isNonInteractive(); + return reduceChildren( this.node, - this.selfTouched(), + selfTouchedValue, (child, value) => value || child.nodeState.touched(), shortCircuitTrue, - ), - ); + ); + }); /** * The reasons for this field's disablement. This includes disabled reasons for any parent field @@ -156,4 +156,15 @@ export class FieldNodeState { return `${parent.name()}.${this.node.structure.keyInParent()}`; }); + + /** Whether this field is considered non-interactive. + * + * A field is considered non-interactive if one of the following is true: + * - It is hidden + * - It is disabled + * - It is readonly + */ + private readonly isNonInteractive = computed( + () => this.hidden() || this.disabled() || this.readonly(), + ); } diff --git a/packages/forms/signals/test/node/field_node.spec.ts b/packages/forms/signals/test/node/field_node.spec.ts index 0b30f3caf49..ef24ea611e1 100644 --- a/packages/forms/signals/test/node/field_node.spec.ts +++ b/packages/forms/signals/test/node/field_node.spec.ts @@ -15,6 +15,7 @@ import { disabled, FieldPath, form, + hidden, readonly, required, REQUIRED, @@ -177,6 +178,137 @@ describe('FieldNode', () => { expect(f().dirty()).toBe(false); expect(f.b).toBeUndefined(); }); + + it('should not be marked as dirty when is readonly', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + readonly(p); + }, + {injector: TestBed.inject(Injector)}, + ); + + expect(f().dirty()).toBe(false); + + f().markAsDirty(); + expect(f().dirty()).toBe(false); + }); + it('should not be marked as dirty when is disabled', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + disabled(p); + }, + {injector: TestBed.inject(Injector)}, + ); + + expect(f().dirty()).toBe(false); + + f().markAsDirty(); + expect(f().dirty()).toBe(false); + }); + + it('should not be marked as dirty when is hidden', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + hidden(p, () => true); + }, + {injector: TestBed.inject(Injector)}, + ); + + expect(f().dirty()).toBe(false); + + f().markAsDirty(); + + expect(f().dirty()).toBe(false); + }); + + it('should be marked as dirty when not readonly', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + {injector: TestBed.inject(Injector)}, + ); + + expect(f().dirty()).toBe(false); + + f().markAsDirty(); + expect(f().dirty()).toBe(true); + }); + + it('should be marked as dirty when not disabled', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + {injector: TestBed.inject(Injector)}, + ); + + expect(f().dirty()).toBe(false); + + f().markAsDirty(); + expect(f().dirty()).toBe(true); + }); + + it('should be marked as dirty when not hidden', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + {injector: TestBed.inject(Injector)}, + ); + + expect(f().dirty()).toBe(false); + + f().markAsDirty(); + expect(f().dirty()).toBe(true); + }); + + it('should become pristine when field becomes non-interactive after being marked dirty', () => { + const isReadonly = signal(false); + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + readonly(p, isReadonly); + }, + {injector: TestBed.inject(Injector)}, + ); + + // Initially interactive and not dirty + expect(f().readonly()).toBe(false); + expect(f().dirty()).toBe(false); + + // Mark as dirty while interactive + f().markAsDirty(); + expect(f().dirty()).toBe(true); + + // Make non-interactive, should become pristine + isReadonly.set(true); + expect(f().readonly()).toBe(true); + expect(f().dirty()).toBe(false); + + // Make interactive again, should still be dirty + isReadonly.set(false); + expect(f().readonly()).toBe(false); + expect(f().dirty()).toBe(true); + }); }); describe('touched', () => { @@ -255,6 +387,137 @@ describe('FieldNode', () => { f().reset(); expect(f().touched()).toBe(false); }); + + it('should not be marked as touched when is readonly', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + readonly(p); + }, + {injector: TestBed.inject(Injector)}, + ); + + expect(f().touched()).toBe(false); + + f().markAsTouched(); + expect(f().touched()).toBe(false); + }); + it('should not be marked as touched when is disabled', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + disabled(p); + }, + {injector: TestBed.inject(Injector)}, + ); + + expect(f().touched()).toBe(false); + + f().markAsTouched(); + expect(f().touched()).toBe(false); + }); + + it('should not be marked as touched when is hidden', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + hidden(p, () => true); + }, + {injector: TestBed.inject(Injector)}, + ); + + expect(f().touched()).toBe(false); + + f().markAsTouched(); + + expect(f().touched()).toBe(false); + }); + + it('should be marked as touched when not readonly', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + {injector: TestBed.inject(Injector)}, + ); + + expect(f().touched()).toBe(false); + + f().markAsTouched(); + expect(f().touched()).toBe(true); + }); + + it('should be marked as touched when not disabled', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + {injector: TestBed.inject(Injector)}, + ); + + expect(f().touched()).toBe(false); + + f().markAsTouched(); + expect(f().touched()).toBe(true); + }); + + it('should be marked as touched when not hidden', () => { + const f = form( + signal({ + a: 1, + b: 2, + }), + {injector: TestBed.inject(Injector)}, + ); + + expect(f().touched()).toBe(false); + + f().markAsTouched(); + expect(f().touched()).toBe(true); + }); + + it('should become untouched when field becomes non-interactive after being marked touched', () => { + const isHidden = signal(false); + const f = form( + signal({ + a: 1, + b: 2, + }), + (p) => { + hidden(p, isHidden); + }, + {injector: TestBed.inject(Injector)}, + ); + + // Initially interactive and not touched + expect(f().hidden()).toBe(false); + expect(f().touched()).toBe(false); + + // Mark as touched while interactive + f().markAsTouched(); + expect(f().touched()).toBe(true); + + // Make non-interactive, should become untouched + isHidden.set(true); + expect(f().hidden()).toBe(true); + expect(f().touched()).toBe(false); + + // Make interactive again, should still be touched + isHidden.set(false); + expect(f().hidden()).toBe(false); + expect(f().touched()).toBe(true); + }); }); describe('arrays', () => {