Skip to content

Commit 9b4e511

Browse files
crisbetoandrewseguin
crisbeto
authored and
andrewseguin
committed
fix(popover-edit): incorrectly caching values for more than one control (#16057)
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 e02e9a6 commit 9b4e511

File tree

2 files changed

+55
-36
lines changed

2 files changed

+55
-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: 49 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,60 @@ 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+
clearLeftoverTimers();
749+
}));
750+
732751
it('resets the lens to previously submitted value', fakeAsync(() => {
733752
component.openLens();
734753

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

738757
component.clickSubmitButton();
739758
fixture.detectChanges();
740759

741760
component.openLens();
742761

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

746765
component.clickRevertButton();
747766

748-
expect(component.getInput()!.value).toBe('Hydragon');
767+
expect(component.getNameInput()!.value).toBe('Hydragon');
749768
clearLeftoverTimers();
750769
}));
751770

752771
it('closes the lens on escape', fakeAsync(() => {
753772
component.openLens();
754773

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

758777
expect(component.lensIsOpen()).toBe(false);
@@ -762,7 +781,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
762781
it('does not close the lens on click within lens', fakeAsync(() => {
763782
component.openLens();
764783

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

767786
expect(component.lensIsOpen()).toBe(true);
768787
clearLeftoverTimers();
@@ -771,8 +790,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
771790
it('closes the lens on outside click', fakeAsync(() => {
772791
component.openLens();
773792

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

@@ -786,8 +805,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
786805
component.clickOutBehavior = 'submit';
787806
component.openLens();
788807

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

@@ -801,8 +820,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
801820
component.clickOutBehavior = 'noop';
802821
component.openLens();
803822

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

@@ -814,7 +833,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
814833
it('sets focus on the first input in the lens', fakeAsync(() => {
815834
component.openLens();
816835

817-
expect(document.activeElement).toBe(component.getInput());
836+
expect(document.activeElement).toBe(component.getNameInput());
818837
clearLeftoverTimers();
819838
}));
820839

0 commit comments

Comments
 (0)