Skip to content

Commit

Permalink
fix(forms): handle standalone <form> tag correctly in `NgControlSta…
Browse files Browse the repository at this point in the history
…tusGroup` directive (#40344)

The `NgControlStatusGroup` directive is shared between template-driven and reactive form modules. In cases when
only reactive forms module is present, the `NgControlStatusGroup` directive is still activated on all `<form>`
elements, but if there is no other reactive directive applied (such as `formGroup`), corresponding `ControlContainer`
token is missing, thus causing exceptions (since `NgControlStatusGroup` directive relies on it to determine the
status). This commit updates the logic to handle the case when no `ControlContainer` is present (effectively making
directive logic a noop in this case).

Alternative approach (more risky) worth considering in the future is to split the `NgControlStatusGroup` into
2 directives with different set of selectors and include them into template-driven and reactive modules separately.
The downside is that these directives might be activated simultaneously on the same element (e.g. `<form>`),
effectively doing the work twice.

Resolves #38391.

PR Close #40344
  • Loading branch information
AndrewKushnir authored and atscott committed Jan 8, 2021
1 parent fcbf714 commit b3f322f
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 11 deletions.
22 changes: 11 additions & 11 deletions packages/forms/src/directives/ng_control_status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,39 +6,39 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Directive, Self} from '@angular/core';
import {Directive, Optional, Self} from '@angular/core';

import {AbstractControlDirective} from './abstract_control_directive';
import {ControlContainer} from './control_container';
import {NgControl} from './ng_control';

export class AbstractControlStatus {
private _cd: AbstractControlDirective;
private _cd: AbstractControlDirective|null;

constructor(cd: AbstractControlDirective) {
constructor(cd: AbstractControlDirective|null) {
this._cd = cd;
}

get ngClassUntouched(): boolean {
return this._cd.control ? this._cd.control.untouched : false;
return this._cd?.control?.untouched ?? false;
}
get ngClassTouched(): boolean {
return this._cd.control ? this._cd.control.touched : false;
return this._cd?.control?.touched ?? false;
}
get ngClassPristine(): boolean {
return this._cd.control ? this._cd.control.pristine : false;
return this._cd?.control?.pristine ?? false;
}
get ngClassDirty(): boolean {
return this._cd.control ? this._cd.control.dirty : false;
return this._cd?.control?.dirty ?? false;
}
get ngClassValid(): boolean {
return this._cd.control ? this._cd.control.valid : false;
return this._cd?.control?.valid ?? false;
}
get ngClassInvalid(): boolean {
return this._cd.control ? this._cd.control.invalid : false;
return this._cd?.control?.invalid ?? false;
}
get ngClassPending(): boolean {
return this._cd.control ? this._cd.control.pending : false;
return this._cd?.control?.pending ?? false;
}
}

Expand Down Expand Up @@ -99,7 +99,7 @@ export class NgControlStatus extends AbstractControlStatus {
host: ngControlStatusHost
})
export class NgControlStatusGroup extends AbstractControlStatus {
constructor(@Self() cd: ControlContainer) {
constructor(@Optional() @Self() cd: ControlContainer) {
super(cd);
}
}
50 changes: 50 additions & 0 deletions packages/forms/test/reactive_integration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec';
return TestBed.createComponent(component);
}

function initReactiveFormsTest<T>(
component: Type<T>, ...directives: Type<any>[]): ComponentFixture<T> {
TestBed.configureTestingModule(
{declarations: [component, ...directives], imports: [ReactiveFormsModule]});
return TestBed.createComponent(component);
}

// Helper method that attaches a spy to a `validate` function on a Validator class.
function validatorSpyOn(validatorClass: any) {
return spyOn(validatorClass.prototype, 'validate').and.callThrough();
Expand Down Expand Up @@ -705,6 +712,49 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec';
});

describe('setting status classes', () => {
it('should not assign status on standalone <form> element', () => {
@Component({
selector: 'form-comp',
template: `
<form></form>
`
})
class FormComp {
}

const fixture = initReactiveFormsTest(FormComp);
fixture.detectChanges();

const form = fixture.debugElement.query(By.css('form')).nativeElement;
// Expect no classes added to the <form> element since it has no
// reactive directives attached and only ReactiveForms module is used.
expect(sortedClassList(form)).toEqual([]);
});

it('should not assign status on standalone <form> element with form control inside', () => {
@Component({
selector: 'form-comp',
template: `
<form>
<input type="text" [formControl]="control">
</form>
`
})
class FormComp {
control = new FormControl('abc');
}
const fixture = initReactiveFormsTest(FormComp);
fixture.detectChanges();

const form = fixture.debugElement.query(By.css('form')).nativeElement;
// Expect no classes added to the <form> element since it has no
// reactive directives attached and only ReactiveForms module is used.
expect(sortedClassList(form)).toEqual([]);

const input = fixture.debugElement.query(By.css('input')).nativeElement;
expect(sortedClassList(input)).toEqual(['ng-pristine', 'ng-untouched', 'ng-valid']);
});

it('should work with single fields', () => {
const fixture = initTest(FormControlComp);
const control = new FormControl('', Validators.required);
Expand Down

0 comments on commit b3f322f

Please sign in to comment.