Skip to content

Commit 83843c7

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 dc859fe commit 83843c7

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 _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
@@ -21,6 +21,7 @@ const NAME_EDIT_TEMPLATE = `
2121
[cdkEditControlIgnoreSubmitUnlessValid]="ignoreSubmitUnlessValid"
2222
[cdkEditControlClickOutBehavior]="clickOutBehavior">
2323
<input [ngModel]="element.name" name="name" required>
24+
<input [ngModel]="element.weight" name="weight">
2425
<br>
2526
<button class="submit" type="submit">Confirm</button>
2627
<button class="revert" cdkEditRevert>Revert</button>
@@ -114,12 +115,16 @@ abstract class BaseTestComponent {
114115
return document.querySelector('.cdk-overlay-connected-position-bounding-box');
115116
}
116117

117-
getInput() {
118-
return document.querySelector('input') as HTMLInputElement|null;
118+
getNameInput() {
119+
return document.querySelector('input[name="name"]') as HTMLInputElement|null;
120+
}
121+
122+
getWeightInput() {
123+
return document.querySelector('input[name="weight"]') as HTMLInputElement|null;
119124
}
120125

121126
lensIsOpen() {
122-
return !!this.getInput();
127+
return !!this.getNameInput();
123128
}
124129

125130
getSubmitButton() {
@@ -503,7 +508,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
503508
it('shows a lens with the value from the table', fakeAsync(() => {
504509
component.openLens();
505510

506-
expect(component.getInput()!.value).toBe('Hydrogen');
511+
expect(component.getNameInput()!.value).toBe('Hydrogen');
507512
}));
508513

509514
it('positions the lens at the top left corner and spans the full width of the cell',
@@ -552,8 +557,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
552557
it('updates the form and submits, closing the lens', fakeAsync(() => {
553558
component.openLens();
554559

555-
component.getInput()!.value = 'Hydragon';
556-
component.getInput()!.dispatchEvent(new Event('input'));
560+
component.getNameInput()!.value = 'Hydragon';
561+
component.getNameInput()!.dispatchEvent(new Event('input'));
557562

558563
component.clickSubmitButton();
559564
fixture.detectChanges();
@@ -565,8 +570,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
565570
it('does not close the lens on submit when form is invalid', fakeAsync(() => {
566571
component.openLens();
567572

568-
component.getInput()!.value = '';
569-
component.getInput()!.dispatchEvent(new Event('input'));
573+
component.getNameInput()!.value = '';
574+
component.getNameInput()!.dispatchEvent(new Event('input'));
570575

571576
component.clickSubmitButton();
572577

@@ -578,8 +583,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
578583
component.ignoreSubmitUnlessValid = false;
579584
component.openLens();
580585

581-
component.getInput()!.value = '';
582-
component.getInput()!.dispatchEvent(new Event('input'));
586+
component.getNameInput()!.value = '';
587+
component.getNameInput()!.dispatchEvent(new Event('input'));
583588

584589
component.clickSubmitButton();
585590

@@ -597,8 +602,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
597602
it('closes and reopens a lens with modified value persisted', fakeAsync(() => {
598603
component.openLens();
599604

600-
component.getInput()!.value = 'Hydragon';
601-
component.getInput()!.dispatchEvent(new Event('input'));
605+
component.getNameInput()!.value = 'Hydragon';
606+
component.getNameInput()!.dispatchEvent(new Event('input'));
602607

603608
component.clickCloseButton();
604609
fixture.detectChanges();
@@ -608,43 +613,56 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
608613

609614
component.openLens();
610615

611-
expect(component.getInput()!.value).toBe('Hydragon');
616+
expect(component.getNameInput()!.value).toBe('Hydragon');
612617
}));
613618

614619
it('resets the lens to original value', fakeAsync(() => {
615620
component.openLens();
616621

617-
component.getInput()!.value = 'Hydragon';
618-
component.getInput()!.dispatchEvent(new Event('input'));
622+
component.getNameInput()!.value = 'Hydragon';
623+
component.getNameInput()!.dispatchEvent(new Event('input'));
619624

620625
component.clickRevertButton();
621626

622-
expect(component.getInput()!.value).toBe('Hydrogen');
627+
expect(component.getNameInput()!.value).toBe('Hydrogen');
623628
}));
624629

630+
it('should not reset the values when clicking revert without making changes',
631+
fakeAsync(() => {
632+
component.openLens();
633+
634+
expect(component.getNameInput()!.value).toBe('Hydrogen');
635+
expect(component.getWeightInput()!.value).toBe('1.007');
636+
637+
component.clickRevertButton();
638+
639+
expect(component.getNameInput()!.value).toBe('Hydrogen');
640+
expect(component.getWeightInput()!.value).toBe('1.007');
641+
}));
642+
625643
it('resets the lens to previously submitted value', fakeAsync(() => {
626644
component.openLens();
627645

628-
component.getInput()!.value = 'Hydragon';
629-
component.getInput()!.dispatchEvent(new Event('input'));
646+
component.getNameInput()!.value = 'Hydragon';
647+
component.getNameInput()!.dispatchEvent(new Event('input'));
630648

631649
component.clickSubmitButton();
632650
fixture.detectChanges();
633651

634652
component.openLens();
635653

636-
component.getInput()!.value = 'Hydragon X';
637-
component.getInput()!.dispatchEvent(new Event('input'));
654+
component.getNameInput()!.value = 'Hydragon X';
655+
component.getNameInput()!.dispatchEvent(new Event('input'));
638656

639657
component.clickRevertButton();
640658

641-
expect(component.getInput()!.value).toBe('Hydragon');
659+
expect(component.getNameInput()!.value).toBe('Hydragon');
642660
}));
643661

644662
it('closes the lens on escape', fakeAsync(() => {
645663
component.openLens();
646664

647-
component.getInput()!.dispatchEvent(
665+
component.getNameInput()!.dispatchEvent(
648666
new KeyboardEvent('keyup', {bubbles: true, key: 'Escape'}));
649667

650668
expect(component.lensIsOpen()).toBe(false);
@@ -653,16 +671,16 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
653671
it('does not close the lens on click within lens', fakeAsync(() => {
654672
component.openLens();
655673

656-
component.getInput()!.dispatchEvent(new Event('click', {bubbles: true}));
674+
component.getNameInput()!.dispatchEvent(new Event('click', {bubbles: true}));
657675

658676
expect(component.lensIsOpen()).toBe(true);
659677
}));
660678

661679
it('closes the lens on outside click', fakeAsync(() => {
662680
component.openLens();
663681

664-
component.getInput()!.value = 'Hydragon';
665-
component.getInput()!.dispatchEvent(new Event('input'));
682+
component.getNameInput()!.value = 'Hydragon';
683+
component.getNameInput()!.dispatchEvent(new Event('input'));
666684
document.body.dispatchEvent(new Event('click', {bubbles: true}));
667685
fixture.detectChanges();
668686

@@ -676,8 +694,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
676694
component.clickOutBehavior = 'submit';
677695
component.openLens();
678696

679-
component.getInput()!.value = 'Hydragon';
680-
component.getInput()!.dispatchEvent(new Event('input'));
697+
component.getNameInput()!.value = 'Hydragon';
698+
component.getNameInput()!.dispatchEvent(new Event('input'));
681699
document.body.dispatchEvent(new Event('click', {bubbles: true}));
682700
fixture.detectChanges();
683701

@@ -690,8 +708,8 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
690708
component.clickOutBehavior = 'noop';
691709
component.openLens();
692710

693-
component.getInput()!.value = 'Hydragon';
694-
component.getInput()!.dispatchEvent(new Event('input'));
711+
component.getNameInput()!.value = 'Hydragon';
712+
component.getNameInput()!.dispatchEvent(new Event('input'));
695713
document.body.dispatchEvent(new Event('click', {bubbles: true}));
696714
fixture.detectChanges();
697715

@@ -702,7 +720,7 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
702720
it('sets focus on the first input in the lens', fakeAsync(() => {
703721
component.openLens();
704722

705-
expect(document.activeElement).toBe(component.getInput());
723+
expect(document.activeElement).toBe(component.getNameInput());
706724
}));
707725

708726
it('returns focus to the edited cell after closing', fakeAsync(() => {

0 commit comments

Comments
 (0)