Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions packages/forms/src/directives/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
});
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to set _pendingChange to false after this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, you're right! Should be fixed.

control._pendingChange = false;
}
});
}
Expand Down Expand Up @@ -212,4 +215,4 @@ export function selectValueAccessor(
export function removeDir<T>(list: T[], el: T): void {
const index = list.indexOf(el);
if (index > -1) list.splice(index, 1);
}
}
10 changes: 8 additions & 2 deletions packages/forms/src/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,9 @@ export class FormControl extends AbstractControl {
/** @internal */
_pendingValue: any;

/** @internal */
_pendingChange: any;

constructor(
formState: any = null,
validatorOrOpts?: ValidatorFn|ValidatorFn[]|AbstractControlOptions|null,
Expand Down Expand Up @@ -801,6 +804,7 @@ export class FormControl extends AbstractControl {
this.markAsPristine(options);
this.markAsUntouched(options);
this.setValue(this.value, options);
this._pendingChange = false;
}

/**
Expand Down Expand Up @@ -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;
}
Expand Down
108 changes: 108 additions & 0 deletions packages/forms/test/reactive_integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'});
Expand Down Expand Up @@ -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');
Expand Down
131 changes: 131 additions & 0 deletions packages/forms/test/template_integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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();
Expand Down