Skip to content

Commit adea7dc

Browse files
crisbetocrisbeto
committed
fix(popover-edit): incorrectly caching values for more than one control
Currently the edit popover uses the first `valueChange` event to know when to cache the value of its form so that it can revert to it if the user presess the `revert` button. This breaks down if the form has more than one control, because it'll emit a `valueChange` for each control that gets added. These changes fix the issue by using `NgZone` to wait for the form to be initialized.
1 parent 5b3e846 commit adea7dc

File tree

2 files changed

+54
-36
lines changed

2 files changed

+54
-36
lines changed

‎src/cdk-experimental/popover-edit/edit-ref.ts

Lines changed: 6 additions & 6 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 {Injectable, OnDestroy, Self} from '@angular/core';
9+
import {Injectable, OnDestroy, Self, NgZone} from '@angular/core';
1010
import {ControlContainer} from '@angular/forms';
1111
import {Observable, Subject} from 'rxjs';
1212
import {take} from 'rxjs/operators';
@@ -41,7 +41,8 @@ export class EditRef<FormValue> implements OnDestroy {
4141

4242
constructor(
4343
@Self() private readonly _form: ControlContainer,
44-
private readonly _editEventDispatcher: EditEventDispatcher) {
44+
private readonly _editEventDispatcher: EditEventDispatcher,
45+
private readonly _ngZone: NgZone) {
4546
this._editEventDispatcher.setActiveEditRef(this);
4647
}
4748

@@ -51,11 +52,10 @@ export class EditRef<FormValue> implements OnDestroy {
5152
* applicable.
5253
*/
5354
init(previousFormValue: FormValue|undefined): void {
54-
// Wait for either the first value to be set, then override it with
55-
// the previously entered value, if any.
56-
this._form.valueChanges!.pipe(take(1)).subscribe(() => {
55+
// Wait for the zone to stabilize before caching the initial value.
56+
// This ensures that all form controls have been initialized.
57+
this._ngZone.onStable.pipe(take(1)).subscribe(() => {
5758
this.updateRevertValue();
58-
5959
if (previousFormValue) {
6060
this.reset(previousFormValue);
6161
}

‎src/cdk-experimental/popover-edit/popover-edit.spec.ts

Lines changed: 48 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const NAME_EDIT_TEMPLATE = `
2727
[cdkEditControlIgnoreSubmitUnlessValid]="ignoreSubmitUnlessValid"
2828
[cdkEditControlClickOutBehavior]="clickOutBehavior">
2929
<input [ngModel]="element.name" name="name" required>
30+
<input [ngModel]="element.weight" name="weight">
3031
<br>
3132
<button class="submit" type="submit">Confirm</button>
3233
<button class="revert" cdkEditRevert>Revert</button>
@@ -131,12 +132,16 @@ abstract class BaseTestComponent {
131132
return document.querySelector('.cdk-overlay-connected-position-bounding-box');
132133
}
133134

134-
getInput() {
135-
return document.querySelector('input') as HTMLInputElement|null;
135+
getNameInput() {
136+
return document.querySelector('input[name="name"]') as HTMLInputElement|null;
137+
}
138+
139+
getWeightInput() {
140+
return document.querySelector('input[name="weight"]') as HTMLInputElement|null;
136141
}
137142

138143
lensIsOpen() {
139-
return !!this.getInput();
144+
return !!this.getNameInput();
140145
}
141146

142147
getSubmitButton() {
@@ -602,7 +607,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
602607
it('shows a lens with the value from the table', fakeAsync(() => {
603608
component.openLens();
604609

605-
expect(component.getInput()!.value).toBe('Hydrogen');
610+
expect(component.getNameInput()!.value).toBe('Hydrogen');
606611
clearLeftoverTimers();
607612
}));
608613

@@ -654,8 +659,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
654659
it('updates the form and submits, closing the lens', fakeAsync(() => {
655660
component.openLens();
656661

657-
component.getInput()!.value = 'Hydragon';
658-
component.getInput()!.dispatchEvent(new Event('input'));
662+
component.getNameInput()!.value = 'Hydragon';
663+
component.getNameInput()!.dispatchEvent(new Event('input'));
659664

660665
component.clickSubmitButton();
661666
fixture.detectChanges();
@@ -667,8 +672,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
667672
it('does not close the lens on submit when form is invalid', fakeAsync(() => {
668673
component.openLens();
669674

670-
component.getInput()!.value = '';
671-
component.getInput()!.dispatchEvent(new Event('input'));
675+
component.getNameInput()!.value = '';
676+
component.getNameInput()!.dispatchEvent(new Event('input'));
672677

673678
component.clickSubmitButton();
674679

@@ -681,8 +686,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
681686
component.ignoreSubmitUnlessValid = false;
682687
component.openLens();
683688

684-
component.getInput()!.value = '';
685-
component.getInput()!.dispatchEvent(new Event('input'));
689+
component.getNameInput()!.value = '';
690+
component.getNameInput()!.dispatchEvent(new Event('input'));
686691

687692
component.clickSubmitButton();
688693

@@ -702,8 +707,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
702707
it('closes and reopens a lens with modified value persisted', fakeAsync(() => {
703708
component.openLens();
704709

705-
component.getInput()!.value = 'Hydragon';
706-
component.getInput()!.dispatchEvent(new Event('input'));
710+
component.getNameInput()!.value = 'Hydragon';
711+
component.getNameInput()!.dispatchEvent(new Event('input'));
707712

708713
component.clickCloseButton();
709714
fixture.detectChanges();
@@ -713,46 +718,59 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
713718

714719
component.openLens();
715720

716-
expect(component.getInput()!.value).toBe('Hydragon');
721+
expect(component.getNameInput()!.value).toBe('Hydragon');
717722
clearLeftoverTimers();
718723
}));
719724

720725
it('resets the lens to original value', fakeAsync(() => {
721726
component.openLens();
722727

723-
component.getInput()!.value = 'Hydragon';
724-
component.getInput()!.dispatchEvent(new Event('input'));
728+
component.getNameInput()!.value = 'Hydragon';
729+
component.getNameInput()!.dispatchEvent(new Event('input'));
725730

726731
component.clickRevertButton();
727732

728-
expect(component.getInput()!.value).toBe('Hydrogen');
733+
expect(component.getNameInput()!.value).toBe('Hydrogen');
729734
clearLeftoverTimers();
730735
}));
731736

737+
it('should not reset the values when clicking revert without making changes',
738+
fakeAsync(() => {
739+
component.openLens();
740+
741+
expect(component.getNameInput()!.value).toBe('Hydrogen');
742+
expect(component.getWeightInput()!.value).toBe('1.007');
743+
744+
component.clickRevertButton();
745+
746+
expect(component.getNameInput()!.value).toBe('Hydrogen');
747+
expect(component.getWeightInput()!.value).toBe('1.007');
748+
}));
749+
732750
it('resets the lens to previously submitted value', fakeAsync(() => {
733751
component.openLens();
734752

735-
component.getInput()!.value = 'Hydragon';
736-
component.getInput()!.dispatchEvent(new Event('input'));
753+
component.getNameInput()!.value = 'Hydragon';
754+
component.getNameInput()!.dispatchEvent(new Event('input'));
737755

738756
component.clickSubmitButton();
739757
fixture.detectChanges();
740758

741759
component.openLens();
742760

743-
component.getInput()!.value = 'Hydragon X';
744-
component.getInput()!.dispatchEvent(new Event('input'));
761+
component.getNameInput()!.value = 'Hydragon X';
762+
component.getNameInput()!.dispatchEvent(new Event('input'));
745763

746764
component.clickRevertButton();
747765

748-
expect(component.getInput()!.value).toBe('Hydragon');
766+
expect(component.getNameInput()!.value).toBe('Hydragon');
749767
clearLeftoverTimers();
750768
}));
751769

752770
it('closes the lens on escape', fakeAsync(() => {
753771
component.openLens();
754772

755-
component.getInput()!.dispatchEvent(
773+
component.getNameInput()!.dispatchEvent(
756774
new KeyboardEvent('keyup', {bubbles: true, key: 'Escape'}));
757775

758776
expect(component.lensIsOpen()).toBe(false);
@@ -762,7 +780,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
762780
it('does not close the lens on click within lens', fakeAsync(() => {
763781
component.openLens();
764782

765-
component.getInput()!.dispatchEvent(new Event('click', {bubbles: true}));
783+
component.getNameInput()!.dispatchEvent(new Event('click', {bubbles: true}));
766784

767785
expect(component.lensIsOpen()).toBe(true);
768786
clearLeftoverTimers();
@@ -771,8 +789,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
771789
it('closes the lens on outside click', fakeAsync(() => {
772790
component.openLens();
773791

774-
component.getInput()!.value = 'Hydragon';
775-
component.getInput()!.dispatchEvent(new Event('input'));
792+
component.getNameInput()!.value = 'Hydragon';
793+
component.getNameInput()!.dispatchEvent(new Event('input'));
776794
document.body.dispatchEvent(new Event('click', {bubbles: true}));
777795
fixture.detectChanges();
778796

@@ -786,8 +804,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
786804
component.clickOutBehavior = 'submit';
787805
component.openLens();
788806

789-
component.getInput()!.value = 'Hydragon';
790-
component.getInput()!.dispatchEvent(new Event('input'));
807+
component.getNameInput()!.value = 'Hydragon';
808+
component.getNameInput()!.dispatchEvent(new Event('input'));
791809
document.body.dispatchEvent(new Event('click', {bubbles: true}));
792810
fixture.detectChanges();
793811

@@ -801,8 +819,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
801819
component.clickOutBehavior = 'noop';
802820
component.openLens();
803821

804-
component.getInput()!.value = 'Hydragon';
805-
component.getInput()!.dispatchEvent(new Event('input'));
822+
component.getNameInput()!.value = 'Hydragon';
823+
component.getNameInput()!.dispatchEvent(new Event('input'));
806824
document.body.dispatchEvent(new Event('click', {bubbles: true}));
807825
fixture.detectChanges();
808826

@@ -814,7 +832,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
814832
it('sets focus on the first input in the lens', fakeAsync(() => {
815833
component.openLens();
816834

817-
expect(document.activeElement).toBe(component.getInput());
835+
expect(document.activeElement).toBe(component.getNameInput());
818836
clearLeftoverTimers();
819837
}));
820838

0 commit comments

Comments
 (0)