From 69c53c3e03c7a8bb2260496c9aa32925cd2d59b4 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Sat, 11 Nov 2017 02:54:36 -0800 Subject: [PATCH] fix(forms): updateOn should check if change occurred (#20358) Fixes #20259. PR Close #20358 --- packages/forms/src/directives/shared.ts | 9 +- packages/forms/src/model.ts | 10 +- .../forms/test/reactive_integration_spec.ts | 108 +++++++++++++++ .../forms/test/template_integration_spec.ts | 131 ++++++++++++++++++ 4 files changed, 253 insertions(+), 5 deletions(-) diff --git a/packages/forms/src/directives/shared.ts b/packages/forms/src/directives/shared.ts index 1f66036a99..c261a9a44d 100644 --- a/packages/forms/src/directives/shared.ts +++ b/packages/forms/src/directives/shared.ts @@ -82,6 +82,7 @@ export function cleanUpControl(control: FormControl, dir: NgControl) { function setUpViewChangePipeline(control: FormControl, dir: NgControl): void { dir.valueAccessor !.registerOnChange((newValue: any) => { control._pendingValue = newValue; + control._pendingChange = true; control._pendingDirty = true; if (control.updateOn === 'change') updateControl(control, dir); @@ -92,7 +93,7 @@ function setUpBlurPipeline(control: FormControl, dir: NgControl): void { dir.valueAccessor !.registerOnTouched(() => { control._pendingTouched = true; - if (control.updateOn === 'blur') updateControl(control, dir); + if (control.updateOn === 'blur' && control._pendingChange) updateControl(control, dir); if (control.updateOn !== 'submit') control.markAsTouched(); }); } @@ -101,6 +102,7 @@ function updateControl(control: FormControl, dir: NgControl): void { dir.viewToModelUpdate(control._pendingValue); if (control._pendingDirty) control.markAsDirty(); control.setValue(control._pendingValue, {emitModelToViewChange: false}); + control._pendingChange = false; } function setUpModelChangePipeline(control: FormControl, dir: NgControl): void { @@ -171,8 +173,9 @@ export function syncPendingControls(form: FormGroup, directives: NgControl[]): v form._syncPendingControls(); directives.forEach(dir => { const control = dir.control as FormControl; - if (control.updateOn === 'submit') { + if (control.updateOn === 'submit' && control._pendingChange) { dir.viewToModelUpdate(control._pendingValue); + control._pendingChange = false; } }); } @@ -212,4 +215,4 @@ export function selectValueAccessor( export function removeDir(list: T[], el: T): void { const index = list.indexOf(el); if (index > -1) list.splice(index, 1); -} \ No newline at end of file +} diff --git a/packages/forms/src/model.ts b/packages/forms/src/model.ts index cf45ac857f..348898318f 100644 --- a/packages/forms/src/model.ts +++ b/packages/forms/src/model.ts @@ -708,6 +708,9 @@ export class FormControl extends AbstractControl { /** @internal */ _pendingValue: any; + /** @internal */ + _pendingChange: any; + constructor( formState: any = null, validatorOrOpts?: ValidatorFn|ValidatorFn[]|AbstractControlOptions|null, @@ -801,6 +804,7 @@ export class FormControl extends AbstractControl { this.markAsPristine(options); this.markAsUntouched(options); this.setValue(this.value, options); + this._pendingChange = false; } /** @@ -847,10 +851,12 @@ export class FormControl extends AbstractControl { /** @internal */ _syncPendingControls(): boolean { if (this.updateOn === 'submit') { - this.setValue(this._pendingValue, {onlySelf: true, emitModelToViewChange: false}); if (this._pendingDirty) this.markAsDirty(); if (this._pendingTouched) this.markAsTouched(); - return true; + if (this._pendingChange) { + this.setValue(this._pendingValue, {onlySelf: true, emitModelToViewChange: false}); + return true; + } } return false; } diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts index 22ba1848f4..03a683f6b7 100644 --- a/packages/forms/test/reactive_integration_spec.ts +++ b/packages/forms/test/reactive_integration_spec.ts @@ -947,6 +947,55 @@ export function main() { sub.unsubscribe(); }); + it('should not emit valueChanges or statusChanges on blur if value unchanged', () => { + const fixture = initTest(FormControlComp); + const control = new FormControl('', {validators: Validators.required, updateOn: 'blur'}); + fixture.componentInstance.control = control; + fixture.detectChanges(); + const values: string[] = []; + + const sub = + merge(control.valueChanges, control.statusChanges).subscribe(val => values.push(val)); + + const input = fixture.debugElement.query(By.css('input')).nativeElement; + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + expect(values).toEqual( + [], 'Expected no valueChanges or statusChanges if value unchanged.'); + + input.value = 'Nancy'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + expect(values).toEqual([], 'Expected no valueChanges or statusChanges on input.'); + + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + expect(values).toEqual( + ['Nancy', 'VALID'], + 'Expected valueChanges and statusChanges on blur if value changed.'); + + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + expect(values).toEqual( + ['Nancy', 'VALID'], + 'Expected valueChanges and statusChanges not to fire again on blur unless value changed.'); + + input.value = 'Bess'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + expect(values).toEqual( + ['Nancy', 'VALID'], + 'Expected valueChanges and statusChanges not to fire on input after blur.'); + + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + expect(values).toEqual( + ['Nancy', 'VALID', 'Bess', 'VALID'], + 'Expected valueChanges and statusChanges to fire again on blur if value changed.'); + + sub.unsubscribe(); + }); + it('should mark as pristine properly if pending dirty', () => { const fixture = initTest(FormControlComp); const control = new FormControl('', {updateOn: 'blur'}); @@ -1272,6 +1321,65 @@ export function main() { sub.unsubscribe(); }); + it('should not emit valueChanges or statusChanges on submit if value unchanged', () => { + const fixture = initTest(FormGroupComp); + const control = + new FormControl('', {validators: Validators.required, updateOn: 'submit'}); + const formGroup = new FormGroup({login: control}); + fixture.componentInstance.form = formGroup; + fixture.detectChanges(); + + const values: string[] = []; + const streams = merge( + control.valueChanges, control.statusChanges, formGroup.valueChanges, + formGroup.statusChanges); + const sub = streams.subscribe(val => values.push(val)); + + const form = fixture.debugElement.query(By.css('form')).nativeElement; + dispatchEvent(form, 'submit'); + fixture.detectChanges(); + expect(values).toEqual( + [], 'Expected no valueChanges or statusChanges if value unchanged.'); + + const input = fixture.debugElement.query(By.css('input')).nativeElement; + input.value = 'Nancy'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + expect(values).toEqual([], 'Expected no valueChanges or statusChanges on input.'); + + dispatchEvent(form, 'submit'); + fixture.detectChanges(); + expect(values).toEqual( + ['Nancy', 'VALID', {login: 'Nancy'}, 'VALID'], + 'Expected valueChanges and statusChanges on submit if value changed.'); + + dispatchEvent(form, 'submit'); + fixture.detectChanges(); + expect(values).toEqual( + ['Nancy', 'VALID', {login: 'Nancy'}, 'VALID'], + 'Expected valueChanges and statusChanges not to fire again if value unchanged.'); + + input.value = 'Bess'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + + expect(values).toEqual( + ['Nancy', 'VALID', {login: 'Nancy'}, 'VALID'], + 'Expected valueChanges and statusChanges not to fire on input after submit.'); + + dispatchEvent(form, 'submit'); + fixture.detectChanges(); + + expect(values).toEqual( + [ + 'Nancy', 'VALID', {login: 'Nancy'}, 'VALID', 'Bess', 'VALID', {login: 'Bess'}, + 'VALID' + ], + 'Expected valueChanges and statusChanges to fire again on submit if value changed.'); + + sub.unsubscribe(); + }); + it('should not run validation for onChange controls on submit', () => { const validatorSpy = jasmine.createSpy('validator'); const groupValidatorSpy = jasmine.createSpy('groupValidatorSpy'); diff --git a/packages/forms/test/template_integration_spec.ts b/packages/forms/test/template_integration_spec.ts index 1ff07fe41d..261e4361a6 100644 --- a/packages/forms/test/template_integration_spec.ts +++ b/packages/forms/test/template_integration_spec.ts @@ -484,6 +484,64 @@ export function main() { sub.unsubscribe(); })); + it('should not fire ngModelChange event on blur unless value has changed', fakeAsync(() => { + const fixture = initTest(NgModelChangesForm); + fixture.componentInstance.name = 'Carson'; + fixture.componentInstance.options = {updateOn: 'blur'}; + fixture.detectChanges(); + tick(); + + expect(fixture.componentInstance.events) + .toEqual([], 'Expected ngModelChanges not to fire.'); + + const input = fixture.debugElement.query(By.css('input')).nativeElement; + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual([], 'Expected ngModelChanges not to fire if value unchanged.'); + + input.value = 'Carson'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + tick(); + + expect(fixture.componentInstance.events) + .toEqual([], 'Expected ngModelChanges not to fire on input.'); + + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual( + ['fired'], 'Expected ngModelChanges to fire once blurred if value changed.'); + + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual( + ['fired'], + 'Expected ngModelChanges not to fire again on blur unless value changed.'); + + input.value = 'Bess'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + tick(); + + expect(fixture.componentInstance.events) + .toEqual(['fired'], 'Expected ngModelChanges not to fire on input after blur.'); + + dispatchEvent(input, 'blur'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual( + ['fired', 'fired'], + 'Expected ngModelChanges to fire again on blur if value changed.'); + + })); + }); describe('submit', () => { @@ -764,6 +822,62 @@ export function main() { sub.unsubscribe(); })); + it('should not fire ngModelChange event on submit unless value has changed', + fakeAsync(() => { + const fixture = initTest(NgModelChangesForm); + fixture.componentInstance.name = 'Carson'; + fixture.componentInstance.options = {updateOn: 'submit'}; + fixture.detectChanges(); + tick(); + + const formEl = fixture.debugElement.query(By.css('form')).nativeElement; + dispatchEvent(formEl, 'submit'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual([], 'Expected ngModelChanges not to fire if value unchanged.'); + + const input = fixture.debugElement.query(By.css('input')).nativeElement; + input.value = 'Carson'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + tick(); + + expect(fixture.componentInstance.events) + .toEqual([], 'Expected ngModelChanges not to fire on input.'); + + dispatchEvent(formEl, 'submit'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual( + ['fired'], 'Expected ngModelChanges to fire once submitted if value changed.'); + + dispatchEvent(formEl, 'submit'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual( + ['fired'], + 'Expected ngModelChanges not to fire again on submit unless value changed.'); + + input.value = 'Bess'; + dispatchEvent(input, 'input'); + fixture.detectChanges(); + tick(); + + expect(fixture.componentInstance.events) + .toEqual(['fired'], 'Expected ngModelChanges not to fire on input after submit.'); + + dispatchEvent(formEl, 'submit'); + fixture.detectChanges(); + + expect(fixture.componentInstance.events) + .toEqual( + ['fired', 'fired'], + 'Expected ngModelChanges to fire again on submit if value changed.'); + })); + }); describe('ngFormOptions', () => { @@ -1670,6 +1784,23 @@ class NgAsyncValidator implements AsyncValidator { class NgModelAsyncValidation { } +@Component({ + selector: 'ng-model-changes-form', + template: ` +
+ +
+ ` +}) +class NgModelChangesForm { + name: string; + events: string[] = []; + options: any; + + log() { this.events.push('fired'); } +} + function sortedClassList(el: HTMLElement) { const l = getDOM().classList(el); l.sort();