fix(forms): registerOnValidatorChange should be called for ngModelGroup. (#41971)

The Validator and AsyncValidator interfaces provide a callback, `registerOnValidatorChange(fn)`. `registerOnValidatorChange` is supposed to be fired at least once to register `fn` with the validator. `fn` is then called by the validator whenever its inputs change. This was previously not happening for FormGroup validators, and is now fixed.

PR Close #41971

packages/forms/src/directives/reactive_directives/form_group_directive.ts

@@ -114,7 +114,7 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
   /** @nodoc */
   ngOnDestroy() {
     if (this.form) {
-      cleanUpValidators(this.form, this, /* handleOnValidatorChange */ false);
+      cleanUpValidators(this.form, this);
 
       // Currently the `onCollectionChange` callback is rewritten each time the
       // `_registerOnCollectionChange` function is invoked. The implication is that cleanup should
@@ -348,9 +348,9 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
   }
 
   private _updateValidators() {
-    setUpValidators(this.form, this, /* handleOnValidatorChange */ false);
+    setUpValidators(this.form, this);
     if (this._oldForm) {
-      cleanUpValidators(this._oldForm, this, /* handleOnValidatorChange */ false);
+      cleanUpValidators(this._oldForm, this);
     }
   }
 

packages/forms/src/directives/shared.ts

@@ -37,7 +37,7 @@ export function setUpControl(control: FormControl, dir: NgControl): void {
     if (!dir.valueAccessor) _throwError(dir, 'No value accessor for form control with');
   }
 
-  setUpValidators(control, dir, /* handleOnValidatorChange */ true);
+  setUpValidators(control, dir);
 
   dir.valueAccessor!.writeValue(control.value);
 
@@ -79,7 +79,7 @@ export function cleanUpControl(
     dir.valueAccessor.registerOnTouched(noop);
   }
 
-  cleanUpValidators(control, dir, /* handleOnValidatorChange */ true);
+  cleanUpValidators(control, dir);
 
   if (control) {
     dir._invokeOnDestroyCallbacks();
@@ -122,12 +122,8 @@ export function setUpDisabledChangeHandler(control: FormControl, dir: NgControl)
  *
  * @param control Form control where directive validators should be setup.
  * @param dir Directive instance that contains validators to be setup.
- * @param handleOnValidatorChange Flag that determines whether directive validators should be setup
- *     to handle validator input change.
  */
-export function setUpValidators(
+export function setUpValidators(control: AbstractControl, dir: AbstractControlDirective): void {
-    control: AbstractControl, dir: AbstractControlDirective,
-    handleOnValidatorChange: boolean): void {
   const validators = getControlValidators(control);
   if (dir.validator !== null) {
     control.setValidators(mergeValidators<ValidatorFn>(validators, dir.validator));
@@ -151,11 +147,9 @@ export function setUpValidators(
   }
 
   // Re-run validation when validator binding changes, e.g. minlength=3 -> minlength=4
-  if (handleOnValidatorChange) {
+  const onValidatorChange = () => control.updateValueAndValidity();
-    const onValidatorChange = () => control.updateValueAndValidity();
+  registerOnValidatorChange<ValidatorFn>(dir._rawValidators, onValidatorChange);
-    registerOnValidatorChange<ValidatorFn>(dir._rawValidators, onValidatorChange);
+  registerOnValidatorChange<AsyncValidatorFn>(dir._rawAsyncValidators, onValidatorChange);
-    registerOnValidatorChange<AsyncValidatorFn>(dir._rawAsyncValidators, onValidatorChange);
-  }
 }
 
 /**
@@ -165,13 +159,10 @@ export function setUpValidators(
  *
  * @param control Form control from where directive validators should be removed.
  * @param dir Directive instance that contains validators to be removed.
- * @param handleOnValidatorChange Flag that determines whether directive validators should also be
- *     cleaned up to stop handling validator input change (if previously configured to do so).
  * @returns true if a control was updated as a result of this action.
  */
 export function cleanUpValidators(
-    control: AbstractControl|null, dir: AbstractControlDirective,
+    control: AbstractControl|null, dir: AbstractControlDirective): boolean {
-    handleOnValidatorChange: boolean): boolean {
   let isControlUpdated = false;
   if (control !== null) {
     if (dir.validator !== null) {
@@ -200,12 +191,10 @@ export function cleanUpValidators(
     }
   }
 
-  if (handleOnValidatorChange) {
+  // Clear onValidatorChange callbacks by providing a noop function.
-    // Clear onValidatorChange callbacks by providing a noop function.
+  const noop = () => {};
-    const noop = () => {};
+  registerOnValidatorChange<ValidatorFn>(dir._rawValidators, noop);
-    registerOnValidatorChange<ValidatorFn>(dir._rawValidators, noop);
+  registerOnValidatorChange<AsyncValidatorFn>(dir._rawAsyncValidators, noop);
-    registerOnValidatorChange<AsyncValidatorFn>(dir._rawAsyncValidators, noop);
-  }
 
   return isControlUpdated;
 }
@@ -264,7 +253,7 @@ export function setUpFormContainer(
     control: FormGroup|FormArray, dir: AbstractFormGroupDirective|FormArrayName) {
   if (control == null && (typeof ngDevMode === 'undefined' || ngDevMode))
     _throwError(dir, 'Cannot find control with');
-  setUpValidators(control, dir, /* handleOnValidatorChange */ false);
+  setUpValidators(control, dir);
 }
 
 /**
@@ -276,7 +265,7 @@ export function setUpFormContainer(
  */
 export function cleanUpFormContainer(
     control: FormGroup|FormArray, dir: AbstractFormGroupDirective|FormArrayName): boolean {
-  return cleanUpValidators(control, dir, /* handleOnValidatorChange */ false);
+  return cleanUpValidators(control, dir);
 }
 
 function _noControlError(dir: NgControl) {

packages/forms/test/reactive_integration_spec.ts

@@ -2733,6 +2733,80 @@ const ValueAccessorB = createControlValueAccessor('[cva-b]');
             expect(form.controls.pin.errors).toEqual({max: {max: -10, actual: 0}});
           });
         });
+
+        it('should fire registerOnValidatorChange for validators attached to the formGroups',
+           () => {
+             let registerOnValidatorChangeFired = 0;
+             let registerOnAsyncValidatorChangeFired = 0;
+
+             @Directive({
+               selector: '[ng-noop-validator]',
+               providers: [
+                 {provide: NG_VALIDATORS, useExisting: forwardRef(() => NoOpValidator), multi: true}
+               ]
+             })
+             class NoOpValidator implements Validator {
+               @Input() validatorInput = '';
+
+               validate(c: AbstractControl) {
+                 return null;
+               }
+
+               public registerOnValidatorChange(fn: () => void) {
+                 registerOnValidatorChangeFired++;
+               }
+             }
+
+             @Directive({
+               selector: '[ng-noop-async-validator]',
+               providers: [{
+                 provide: NG_ASYNC_VALIDATORS,
+                 useExisting: forwardRef(() => NoOpAsyncValidator),
+                 multi: true
+               }]
+             })
+             class NoOpAsyncValidator implements AsyncValidator {
+               @Input() validatorInput = '';
+
+               validate(c: AbstractControl) {
+                 return Promise.resolve(null);
+               }
+
+               public registerOnValidatorChange(fn: () => void) {
+                 registerOnAsyncValidatorChangeFired++;
+               }
+             }
+
+             @Component({
+               selector: 'ng-model-noop-validation',
+               template: `
+            <form [formGroup]="fooGroup" ng-noop-validator ng-noop-async-validator [validatorInput]="validatorInput">
+                <input type="text" formControlName="fooInput">
+            </form>
+           `
+             })
+             class NgModelNoOpValidation {
+               validatorInput = 'bar';
+
+               fooGroup = new FormGroup({
+                 fooInput: new FormControl(''),
+               });
+             }
+
+             const fixture = initTest(NgModelNoOpValidation, NoOpValidator, NoOpAsyncValidator);
+             fixture.detectChanges();
+
+             expect(registerOnValidatorChangeFired).toBe(1);
+             expect(registerOnAsyncValidatorChangeFired).toBe(1);
+
+             fixture.componentInstance.validatorInput = 'baz';
+             fixture.detectChanges();
+
+             // Changing the validator input should not cause the onValidatorChange to be called
+             // again.
+             expect(registerOnValidatorChangeFired).toBe(1);
+             expect(registerOnAsyncValidatorChangeFired).toBe(1);
+           });
       });
     });
 

packages/forms/test/template_integration_spec.ts

@@ -9,7 +9,7 @@
 import {ɵgetDOM as getDOM} from '@angular/common';
 import {Component, Directive, forwardRef, Input, Type, ViewChild} from '@angular/core';
 import {ComponentFixture, fakeAsync, TestBed, tick, waitForAsync} from '@angular/core/testing';
-import {AbstractControl, AsyncValidator, COMPOSITION_BUFFER_MODE, ControlValueAccessor, FormControl, FormsModule, MaxValidator, MinValidator, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgForm, NgModel} from '@angular/forms';
+import {AbstractControl, AsyncValidator, COMPOSITION_BUFFER_MODE, ControlValueAccessor, FormControl, FormsModule, MaxValidator, MinValidator, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgForm, NgModel, Validator} from '@angular/forms';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {dispatchEvent, sortedClassList} from '@angular/platform-browser/testing/src/browser_util';
 import {merge} from 'rxjs';
@@ -1967,6 +1967,79 @@ import {NgModelCustomComp, NgModelCustomWrapper} from './value_accessor_integrat
            expect(form.valid).toBeFalse();
            expect(form.controls.min_max.errors).toEqual({max: {max: -10, actual: 0}});
          }));
+
+      it('should call registerOnValidatorChange as a part of a formGroup setup', fakeAsync(() => {
+           let registerOnValidatorChangeFired = 0;
+           let registerOnAsyncValidatorChangeFired = 0;
+
+           @Directive({
+             selector: '[ng-noop-validator]',
+             providers: [
+               {provide: NG_VALIDATORS, useExisting: forwardRef(() => NoOpValidator), multi: true}
+             ]
+           })
+           class NoOpValidator implements Validator {
+             @Input() validatorInput = '';
+
+             validate(c: AbstractControl) {
+               return null;
+             }
+
+             public registerOnValidatorChange(fn: () => void) {
+               registerOnValidatorChangeFired++;
+             }
+           }
+
+           @Directive({
+             selector: '[ng-noop-async-validator]',
+             providers: [{
+               provide: NG_ASYNC_VALIDATORS,
+               useExisting: forwardRef(() => NoOpAsyncValidator),
+               multi: true
+             }]
+           })
+           class NoOpAsyncValidator implements AsyncValidator {
+             @Input() validatorInput = '';
+
+             validate(c: AbstractControl) {
+               return Promise.resolve(null);
+             }
+
+             public registerOnValidatorChange(fn: () => void) {
+               registerOnAsyncValidatorChangeFired++;
+             }
+           }
+
+           @Component({
+             selector: 'ng-model-noop-validation',
+             template: `
+              <form>
+                <div ngModelGroup="emptyGroup" ng-noop-validator ng-noop-async-validator [validatorInput]="validatorInput">
+                  <input name="fgInput" ngModel>
+                </div>
+              </form>
+            `
+           })
+           class NgModelNoOpValidation {
+             validatorInput = 'foo';
+             emptyGroup = {};
+           }
+
+           const fixture = initTest(NgModelNoOpValidation, NoOpValidator, NoOpAsyncValidator);
+           fixture.detectChanges();
+           tick();
+
+           expect(registerOnValidatorChangeFired).toBe(1);
+           expect(registerOnAsyncValidatorChangeFired).toBe(1);
+
+           fixture.componentInstance.validatorInput = 'bar';
+           fixture.detectChanges();
+
+           // Changing validator inputs should not cause `registerOnValidatorChange` to be invoked,
+           // since it's invoked just once during the setup phase.
+           expect(registerOnValidatorChangeFired).toBe(1);
+           expect(registerOnAsyncValidatorChangeFired).toBe(1);
+         }));
     });
 
     describe('IME events', () => {