Skip to content

Commit b75e90f

Browse files
crisbetodylhunn
crisbeto
authored and
dylhunn
committed
fix(forms): incorrectly keeping track of ngModel with ngFor inside a form (#40459)
When an `NgModel` is created within a `form`, it receives an `NgControl` based on its `name`, but the control doesn't get swapped out if the name changes. This can lead to problems if the `NgModel` is part of an `ngFor`, because the name can change based on its position in the list and a new control can be defined with the same name, leading us to having multiple directives pointing to the same control. For example, if we start off with a list like : ``` [0, 1, 2]; -> [NgModel(0), NgModel(1), NgModel(2)] ``` Then we remove the second item: ``` [0, 2]; -> [NgModel(0), NgModel(2)] ``` And finally, if we decide to add an item to the end of the list, we'll already have a control for index 2, causing the list to look like: ``` [0, 2, 3]; -> [NgModel(0), NgModel(2), NgModel(2)] ``` These changes fix the issue by removing the old control when the `name` of the directive changes. Fixes #38465. Fixes #37920. PR Close #40459
1 parent 3919ee3 commit b75e90f

File tree

4 files changed

+141
-12
lines changed

4 files changed

+141
-12
lines changed

‎packages/forms/src/directives/ng_form.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {Form} from './form_interface';
1616
import {NgControl} from './ng_control';
1717
import {NgModel} from './ng_model';
1818
import {NgModelGroup} from './ng_model_group';
19-
import {removeListItem, setUpControl, setUpFormContainer, syncPendingControls} from './shared';
19+
import {setUpControl, setUpFormContainer, syncPendingControls} from './shared';
2020
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from './validators';
2121

2222
export const formDirectiveProvider: any = {
@@ -104,7 +104,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit {
104104
*/
105105
public readonly submitted: boolean = false;
106106

107-
private _directives: NgModel[] = [];
107+
private _directives = new Set<NgModel>();
108108

109109
/**
110110
* @description
@@ -191,7 +191,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit {
191191
<FormControl>container.registerControl(dir.name, dir.control);
192192
setUpControl(dir.control, dir);
193193
dir.control.updateValueAndValidity({emitEvent: false});
194-
this._directives.push(dir);
194+
this._directives.add(dir);
195195
});
196196
}
197197

@@ -217,7 +217,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit {
217217
if (container) {
218218
container.removeControl(dir.name);
219219
}
220-
removeListItem(this._directives, dir);
220+
this._directives.delete(dir);
221221
});
222222
}
223223

@@ -324,8 +324,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit {
324324
}
325325
}
326326

327-
/** @internal */
328-
_findContainer(path: string[]): FormGroup {
327+
private _findContainer(path: string[]): FormGroup {
329328
path.pop();
330329
return path.length ? <FormGroup>this.form.get(path) : this.form;
331330
}

‎packages/forms/src/directives/ng_model.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,20 @@ export class NgModel extends NgControl implements OnChanges, OnDestroy {
220220
/** @nodoc */
221221
ngOnChanges(changes: SimpleChanges) {
222222
this._checkForErrors();
223-
if (!this._registered) this._setUpControl();
223+
if (!this._registered || 'name' in changes) {
224+
if (this._registered) {
225+
this._checkName();
226+
if (this.formDirective) {
227+
// We can't call `formDirective.removeControl(this)`, because the `name` has already been
228+
// changed. We also can't reset the name temporarily since the logic in `removeControl`
229+
// is inside a promise and it won't run immediately. We work around it by giving it an
230+
// object with the same shape instead.
231+
const oldName = changes['name'].previousValue;
232+
this.formDirective.removeControl({name: oldName, path: this._getPath(oldName)});
233+
}
234+
}
235+
this._setUpControl();
236+
}
224237
if ('isDisabled' in changes) {
225238
this._updateDisabled(changes);
226239
}
@@ -242,7 +255,7 @@ export class NgModel extends NgControl implements OnChanges, OnDestroy {
242255
* Each index is the string name of the control on that level.
243256
*/
244257
override get path(): string[] {
245-
return this._parent ? controlPath(this.name, this._parent) : [this.name];
258+
return this._getPath(this.name);
246259
}
247260

248261
/**
@@ -333,4 +346,8 @@ export class NgModel extends NgControl implements OnChanges, OnDestroy {
333346
this._changeDetectorRef?.markForCheck();
334347
});
335348
}
349+
350+
private _getPath(controlName: string): string[] {
351+
return this._parent ? controlPath(controlName, this._parent) : [controlName];
352+
}
336353
}

‎packages/forms/src/directives/shared.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -298,9 +298,9 @@ export function isBuiltInAccessor(valueAccessor: ControlValueAccessor): boolean
298298
return Object.getPrototypeOf(valueAccessor.constructor) === BuiltInControlValueAccessor;
299299
}
300300

301-
export function syncPendingControls(form: FormGroup, directives: NgControl[]): void {
301+
export function syncPendingControls(form: FormGroup, directives: Set<NgControl>|NgControl[]): void {
302302
form._syncPendingControls();
303-
directives.forEach(dir => {
303+
directives.forEach((dir: NgControl) => {
304304
const control = dir.control as FormControl;
305305
if (control.updateOn === 'submit' && control._pendingChange) {
306306
dir.viewToModelUpdate(control._pendingValue);

‎packages/forms/test/template_integration_spec.ts‎

Lines changed: 115 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {ɵgetDOM as getDOM} from '@angular/common';
9+
import {CommonModule, ɵgetDOM as getDOM} from '@angular/common';
1010
import {Component, Directive, forwardRef, Input, Type, ViewChild} from '@angular/core';
1111
import {ComponentFixture, fakeAsync, TestBed, tick, waitForAsync} from '@angular/core/testing';
1212
import {AbstractControl, AsyncValidator, COMPOSITION_BUFFER_MODE, ControlValueAccessor, FormControl, FormsModule, MaxLengthValidator, MaxValidator, MinLengthValidator, MinValidator, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgForm, NgModel, Validator} from '@angular/forms';
@@ -20,7 +20,7 @@ import {NgModelCustomComp, NgModelCustomWrapper} from './value_accessor_integrat
2020
describe('template-driven forms integration tests', () => {
2121
function initTest<T>(component: Type<T>, ...directives: Type<any>[]): ComponentFixture<T> {
2222
TestBed.configureTestingModule(
23-
{declarations: [component, ...directives], imports: [FormsModule]});
23+
{declarations: [component, ...directives], imports: [FormsModule, CommonModule]});
2424
return TestBed.createComponent(component);
2525
}
2626

@@ -317,6 +317,119 @@ import {NgModelCustomComp, NgModelCustomWrapper} from './value_accessor_integrat
317317
const form = fixture.debugElement.query(By.css('form'));
318318
expect(form.nativeElement.hasAttribute('novalidate')).toEqual(false);
319319
});
320+
321+
it('should keep track of the ngModel value when together used with an ngFor inside a form',
322+
fakeAsync(() => {
323+
@Component({
324+
template: `
325+
<form>
326+
<div *ngFor="let item of items; index as i">
327+
<input [(ngModel)]="item.value" name="name-{{i}}">
328+
</div>
329+
</form>
330+
`
331+
})
332+
class App {
333+
private _counter = 0;
334+
items: {value: string}[] = [];
335+
336+
add(amount: number) {
337+
for (let i = 0; i < amount; i++) {
338+
this.items.push({value: `${this._counter++}`});
339+
}
340+
}
341+
342+
remove(index: number) {
343+
this.items.splice(index, 1);
344+
}
345+
}
346+
347+
const getValues = () =>
348+
fixture.debugElement.queryAll(By.css('input')).map(el => el.nativeElement.value);
349+
const fixture = initTest(App);
350+
fixture.componentInstance.add(3);
351+
fixture.detectChanges();
352+
tick();
353+
expect(getValues()).toEqual(['0', '1', '2']);
354+
355+
fixture.componentInstance.remove(1);
356+
fixture.detectChanges();
357+
tick();
358+
expect(getValues()).toEqual(['0', '2']);
359+
360+
fixture.componentInstance.add(1);
361+
fixture.detectChanges();
362+
tick();
363+
expect(getValues()).toEqual(['0', '2', '3']);
364+
365+
fixture.componentInstance.items[1].value = '1';
366+
fixture.detectChanges();
367+
tick();
368+
expect(getValues()).toEqual(['0', '1', '3']);
369+
370+
fixture.componentInstance.items[2].value = '2';
371+
fixture.detectChanges();
372+
tick();
373+
expect(getValues()).toEqual(['0', '1', '2']);
374+
}));
375+
376+
it('should keep track of the ngModel value when together used with an ngFor inside an ngModelGroup',
377+
fakeAsync(() => {
378+
@Component({
379+
template: `
380+
<form>
381+
<ng-container ngModelGroup="group">
382+
<div *ngFor="let item of items; index as i">
383+
<input [(ngModel)]="item.value" name="name-{{i}}">
384+
</div>
385+
</ng-container>
386+
</form>
387+
`
388+
})
389+
class App {
390+
private _counter = 0;
391+
group = {};
392+
items: {value: string}[] = [];
393+
394+
add(amount: number) {
395+
for (let i = 0; i < amount; i++) {
396+
this.items.push({value: `${this._counter++}`});
397+
}
398+
}
399+
400+
remove(index: number) {
401+
this.items.splice(index, 1);
402+
}
403+
}
404+
405+
const getValues = () =>
406+
fixture.debugElement.queryAll(By.css('input')).map(el => el.nativeElement.value);
407+
const fixture = initTest(App);
408+
fixture.componentInstance.add(3);
409+
fixture.detectChanges();
410+
tick();
411+
expect(getValues()).toEqual(['0', '1', '2']);
412+
413+
fixture.componentInstance.remove(1);
414+
fixture.detectChanges();
415+
tick();
416+
expect(getValues()).toEqual(['0', '2']);
417+
418+
fixture.componentInstance.add(1);
419+
fixture.detectChanges();
420+
tick();
421+
expect(getValues()).toEqual(['0', '2', '3']);
422+
423+
fixture.componentInstance.items[1].value = '1';
424+
fixture.detectChanges();
425+
tick();
426+
expect(getValues()).toEqual(['0', '1', '3']);
427+
428+
fixture.componentInstance.items[2].value = '2';
429+
fixture.detectChanges();
430+
tick();
431+
expect(getValues()).toEqual(['0', '1', '2']);
432+
}));
320433
});
321434

322435
describe('name and ngModelOptions', () => {

0 commit comments

Comments
 (0)