fix: correct overlay closure order or operations for manual override

adobe · Aug 27, 2020 · 0b7a8c4 · 0b7a8c4
1 parent 8e1fc9e
commit 0b7a8c4
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 12 deletions.
diff --git a/packages/dropdown/src/Dropdown.ts b/packages/dropdown/src/Dropdown.ts
@@ -179,7 +179,9 @@ export class DropdownBase extends Focusable {
             }
             return;
         }
-        this.setValueFromItem(target);
+        if (target.value) {
+            this.setValueFromItem(target);
+        }
     }
 
     public onKeydown(event: KeyboardEvent): void {
@@ -194,11 +196,13 @@ export class DropdownBase extends Focusable {
         this.open = true;
     }
 
-    public setValueFromItem(item: MenuItem): void {
+    public async setValueFromItem(item: MenuItem): Promise<void> {
         const oldSelectedItemText = this.selectedItemText;
         const oldValue = this.value;
         this.selectedItemText = item.itemText;
         this.value = item.value;
+        this.open = false;
+        await this.updateComplete;
         const applyDefault = this.dispatchEvent(
             new Event('change', {
                 cancelable: true,
@@ -207,6 +211,7 @@ export class DropdownBase extends Focusable {
         if (!applyDefault) {
             this.selectedItemText = oldSelectedItemText;
             this.value = oldValue;
+            this.open = true;
             return;
         }
         const parentElement = item.parentElement as Element;
@@ -218,7 +223,6 @@ export class DropdownBase extends Focusable {
             selectedItem.selected = false;
         }
         item.selected = true;
-        this.open = false;
     }
 
     public toggle(): void {
@@ -244,6 +248,8 @@ export class DropdownBase extends Focusable {
         }
 
         delete this.placeholder;
+
+        this.menuStateResolver();
     }
 
     private async openMenu(): Promise<void> {
@@ -295,8 +301,6 @@ export class DropdownBase extends Focusable {
             this.closeOverlay();
             delete this.closeOverlay;
         }
-
-        this.menuStateResolver();
     }
 
     protected get buttonContent(): TemplateResult[] {
@@ -382,7 +386,10 @@ export class DropdownBase extends Focusable {
         if (changedProperties.has('disabled') && this.disabled) {
             this.open = false;
         }
-        if (changedProperties.has('open')) {
+        if (
+            changedProperties.has('open') &&
+            typeof changedProperties.get('open') !== 'undefined'
+        ) {
             this.menuStatePromise = new Promise(
                 (res) => (this.menuStateResolver = res)
             );

diff --git a/packages/dropdown/test/dropdown.test.ts b/packages/dropdown/test/dropdown.test.ts
@@ -252,6 +252,40 @@ describe('Dropdown', () => {
         expect(el.open).to.be.true;
         expect(secondItem.selected).to.be.false;
     });
+
+    it('can throw focus after `change`', async () => {
+        const el = await dropdownFixture();
+        const input = document.createElement('input');
+        document.body.append(input);
+
+        await elementUpdated(el);
+
+        const secondItem = el.querySelector(
+            'sp-menu-item:nth-of-type(2)'
+        ) as MenuItem;
+        const button = el.button as HTMLButtonElement;
+
+        button.click();
+        await elementUpdated(el);
+
+        expect(el.open).to.be.true;
+        expect(el.selectedItemText).to.equal('');
+        expect(el.value).to.equal('');
+        expect(secondItem.selected).to.be.false;
+
+        el.addEventListener('change', (): void => {
+            input.focus();
+        });
+
+        secondItem.click();
+        await elementUpdated(el);
+
+        expect(el.open).to.be.false;
+        expect(el.value, 'value changed').to.equal('option-2');
+        expect(secondItem.selected, 'selected changed').to.be.true;
+        await waitUntil(() => document.activeElement === input, 'focus throw');
+        input.remove();
+    });
     it('opens on ArrowDown', async () => {
         const el = await dropdownFixture();
 

diff --git a/packages/overlay/src/ActiveOverlay.ts b/packages/overlay/src/ActiveOverlay.ts
@@ -325,6 +325,11 @@ export class ActiveOverlay extends LitElement {
 
         this.returnOverlayContent();
         this.state = 'disposed';
+
+        if (this.willNotifyClosed) {
+            this.overlayContent.dispatchEvent(new Event('sp-overlay-closed'));
+            this.willNotifyClosed = false;
+        }
     }
 
     private stealOverlayContent(element: HTMLElement): void {
@@ -356,6 +361,8 @@ export class ActiveOverlay extends LitElement {
         this.stealOverlayContentResolver();
     }
 
+    private willNotifyClosed = false;
+
     private returnOverlayContent(): void {
         /* istanbul ignore if */
         if (!this.overlayContent) return;
@@ -378,9 +385,7 @@ export class ActiveOverlay extends LitElement {
                     this.overlayContent,
                     this.placeholder
                 );
-                this.overlayContent.dispatchEvent(
-                    new Event('sp-overlay-closed')
-                );
+                this.willNotifyClosed = true;
             }
         }
 

diff --git a/packages/overlay/src/overlay-stack.ts b/packages/overlay/src/overlay-stack.ts
@@ -307,9 +307,6 @@ export class OverlayStack {
             await overlay.hide(animated);
             if (overlay.state != 'dispose') return;
 
-            overlay.remove();
-            overlay.dispose();
-
             const index = this.overlays.indexOf(overlay);
             /* istanbul ignore else */
             if (index >= 0) {
@@ -335,6 +332,9 @@ export class OverlayStack {
                 }
                 overlay.tabbingAway = false;
             }
+
+            overlay.remove();
+            overlay.dispose();
         }
     }