fix(forms): updateOn should check if change occurred (#20358)

Fixes #20259.

PR Close #20358

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;
     }
   });
 }

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,11 +851,13 @@ 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();
+      if (this._pendingChange) {
+        this.setValue(this._pendingValue, {onlySelf: true, emitModelToViewChange: false});
         return true;
       }
+    }
     return false;
   }
 

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');

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: `
+    <form>
+      <input name="async" [ngModel]="name" (ngModelChange)="log()" 
+             [ngModelOptions]="options">
+    </form>
+  `
+})
+class NgModelChangesForm {
+  name: string;
+  events: string[] = [];
+  options: any;
+
+  log() { this.events.push('fired'); }
+}
+
 function sortedClassList(el: HTMLElement) {
   const l = getDOM().classList(el);
   l.sort();