fix: only close the topmost popover on esc and outside click (#7480)

vaadin · Jun 10, 2024 · fcd342c · fcd342c
1 parent 9ccd96c
commit fcd342c
Show file tree

Hide file tree

Showing 3 changed files with 134 additions and 2 deletions.
diff --git a/integration/tests/dialog-popover.test.js b/integration/tests/dialog-popover.test.js
@@ -71,3 +71,66 @@ describe('popover in dialog', () => {
     });
   });
 });
+
+describe('dialog in popover', () => {
+  let popover, target, button, dialog;
+
+  beforeEach(async () => {
+    [target, popover] = fixtureSync(`
+      <div>
+        <button id="target">Open popover</button>
+        <vaadin-popover for="target"></vaadin-popover>
+      </div>
+    `).children;
+
+    popover.renderer = (root) => {
+      root.innerHTML = `
+        <button>Open dialog</button>
+        <vaadin-dialog></vaadin-dialog>
+      `;
+      [button, dialog] = root.children;
+
+      button.addEventListener('click', () => {
+        dialog.opened = true;
+      });
+
+      dialog.renderer = (dialogRoot) => {
+        dialogRoot.textContent = 'Dialog content';
+      };
+    };
+
+    await nextRender();
+    target.click();
+    await nextRender();
+  });
+
+  ['modal', 'modeless'].forEach((type) => {
+    describe(`${type} popover`, () => {
+      beforeEach(async () => {
+        if (type === 'modal') {
+          popover.modal = true;
+          await nextUpdate(popover);
+        }
+
+        button.focus();
+        button.click();
+        await nextRender();
+      });
+
+      it(`should not close the ${type} popover when closing a child dialog on Escape`, async () => {
+        await sendKeys({ press: 'Escape' });
+
+        expect(dialog.opened).to.be.false;
+        expect(popover.opened).to.be.true;
+      });
+
+      it(`should close the ${type} popover on subsequent Escape after the child dialog is closed`, async () => {
+        await sendKeys({ press: 'Escape' });
+
+        await sendKeys({ press: 'Escape' });
+
+        expect(popover.opened).to.be.false;
+      });
+    });
+  });
+});
diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js
@@ -12,6 +12,7 @@ import { ElementMixin } from '@vaadin/component-base/src/element-mixin.js';
 import { OverlayClassMixin } from '@vaadin/component-base/src/overlay-class-mixin.js';
 import { PolylitMixin } from '@vaadin/component-base/src/polylit-mixin.js';
 import { generateUniqueId } from '@vaadin/component-base/src/unique-id-utils.js';
+import { isLastOverlay } from '@vaadin/overlay/src/vaadin-overlay-stack-mixin.js';
 import { ThemePropertyMixin } from '@vaadin/vaadin-themable-mixin/vaadin-theme-property-mixin.js';
 import { PopoverPositionMixin } from './vaadin-popover-position-mixin.js';
 import { PopoverTargetMixin } from './vaadin-popover-target-mixin.js';
@@ -500,7 +501,8 @@ class Popover extends PopoverPositionMixin(
       !this.__isManual &&
       !this.modal &&
       !event.composedPath().some((el) => el === this._overlayElement || el === this.target) &&
-      !this.noCloseOnOutsideClick
+      !this.noCloseOnOutsideClick &&
+      isLastOverlay(this._overlayElement)
     ) {
       this._openedStateController.close(true);
     }
@@ -526,7 +528,14 @@ class Popover extends PopoverPositionMixin(
    * @private
    */
   __onGlobalKeyDown(event) {
-    if (event.key === 'Escape' && !this.modal && !this.noCloseOnEsc && this.opened && !this.__isManual) {
+    if (
+      event.key === 'Escape' &&
+      !this.modal &&
+      !this.noCloseOnEsc &&
+      this.opened &&
+      !this.__isManual &&
+      isLastOverlay(this._overlayElement)
+    ) {
       // Prevent closing parent overlay (e.g. dialog)
       event.stopPropagation();
       this._openedStateController.close(true);

diff --git a/packages/popover/test/basic.test.js b/packages/popover/test/basic.test.js
@@ -269,6 +269,66 @@ describe('popover', () => {
       });
     });
 
+    describe('nested popovers', () => {
+      let secondPopover, secondTarget;
+
+      function nestedRenderer(root) {
+        root.innerHTML = `
+          <button id="second-target">Second target</button>
+          <vaadin-popover for="second-target"></vaadin-popover>
+        `;
+        [secondTarget, secondPopover] = root.children;
+      }
+
+      beforeEach(async () => {
+        popover.renderer = nestedRenderer;
+
+        // Open the first popover
+        target.click();
+        await nextRender();
+
+        // Open the second popover
+        secondTarget.click();
+        await nextRender();
+
+        // Expect both popovers to be opened
+        expect(popover.opened).to.be.true;
+        expect(secondPopover.opened).to.be.true;
+      });
+
+      it('should close the topmost overlay on global Escape press', async () => {
+        esc(document.body);
+        await nextRender();
+
+        // Expect only the second popover to be closed
+        expect(popover.opened).to.be.true;
+        expect(secondPopover.opened).to.be.false;
+
+        esc(document.body);
+        await nextRender();
+
+        // Expect both popovers to be closed
+        expect(popover.opened).to.be.false;
+        expect(secondPopover.opened).to.be.false;
+      });
+
+      it('should close the topmost overlay on outside click', async () => {
+        outsideClick();
+        await nextRender();
+
+        // Expect only the second popover to be closed
+        expect(popover.opened).to.be.true;
+        expect(secondPopover.opened).to.be.false;
+
+        outsideClick();
+        await nextRender();
+
+        // Expect both popovers to be closed
+        expect(popover.opened).to.be.false;
+        expect(secondPopover.opened).to.be.false;
+      });
+    });
+
     describe('backdrop', () => {
       beforeEach(async () => {
         popover.withBackdrop = true;