refactor: close popover on global Escape press by default (#7434)

vaadin · May 24, 2024 · c38731d · c38731d
1 parent 04107da
commit c38731d
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 32 deletions.
diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js
@@ -48,6 +48,7 @@ class Popover extends PopoverPositionMixin(
         type: Boolean,
         value: false,
         notify: true,
+        observer: '__openedChanged',
       },
 
       /**
@@ -140,6 +141,7 @@ class Popover extends PopoverPositionMixin(
   constructor() {
     super();
     this.__onGlobalClick = this.__onGlobalClick.bind(this);
+    this.__onGlobalKeyDown = this.__onGlobalKeyDown.bind(this);
     this.__onTargetClick = this.__onTargetClick.bind(this);
     this.__onTargetKeydown = this.__onTargetKeydown.bind(this);
     this.__onTargetFocusIn = this.__onTargetFocusIn.bind(this);
@@ -246,6 +248,15 @@ class Popover extends PopoverPositionMixin(
     target.removeEventListener('focusout', this.__onTargetFocusOut);
   }
 
+  /** @private */
+  __openedChanged(opened, oldOpened) {
+    if (opened) {
+      document.addEventListener('keydown', this.__onGlobalKeyDown, true);
+    } else if (oldOpened) {
+      document.removeEventListener('keydown', this.__onGlobalKeyDown, true);
+    }
+  }
+
   /**
    * Overlay's global outside click listener doesn't work when
    * the overlay is modeless, so we use a separate listener.
@@ -273,14 +284,21 @@ class Popover extends PopoverPositionMixin(
     }
   }
 
-  /** @private */
-  __onTargetKeydown(event) {
-    if (event.key === 'Escape' && !this.noCloseOnEsc && this.opened && !this.__isManual) {
+  /**
+   * Overlay's global Escape press listener doesn't work when
+   * the overlay is modeless, so we use a separate listener.
+   * @private
+   */
+  __onGlobalKeyDown(event) {
+    if (event.key === 'Escape' && !this.modal && !this.noCloseOnEsc && this.opened && !this.__isManual) {
       // Prevent closing parent overlay (e.g. dialog)
       event.stopPropagation();
       this.opened = false;
     }
+  }
 
+  /** @private */
+  __onTargetKeydown(event) {
     // Prevent restoring focus after target blur on Tab key
     if (event.key === 'Tab' && this.__shouldRestoreFocus) {
       this.__shouldRestoreFocus = false;

diff --git a/packages/popover/test/basic.test.js b/packages/popover/test/basic.test.js
@@ -234,35 +234,13 @@ describe('popover', () => {
         await nextRender();
       });
 
-      it('should not close overlay on global Escape press by default', async () => {
+      it('should close overlay on global Escape press by default', async () => {
         esc(document.body);
         await nextRender();
-        expect(overlay.opened).to.be.true;
-      });
-
-      it('should close overlay on global Escape press when modal is true', async () => {
-        popover.modal = true;
-        await nextUpdate(popover);
-
-        esc(document.body);
-        await nextRender();
-        expect(overlay.opened).to.be.false;
-      });
-
-      it('should close overlay on internal Escape press by default', async () => {
-        esc(overlay.$.overlay);
-        await nextRender();
-        expect(overlay.opened).to.be.false;
-      });
-
-      it('should close overlay on target Escape press by default', async () => {
-        esc(target);
-        await nextRender();
         expect(overlay.opened).to.be.false;
       });
 
       it('should not close on global Escape press if noCloseOnEsc is true', async () => {
-        popover.modal = true;
         popover.noCloseOnEsc = true;
         await nextUpdate(popover);
 
@@ -271,20 +249,21 @@ describe('popover', () => {
         expect(overlay.opened).to.be.true;
       });
 
-      it('should not close overlay on internal Escape if noCloseOnEsc is true', async () => {
-        popover.noCloseOnEsc = true;
+      it('should close overlay on global Escape press when modal is true', async () => {
+        popover.modal = true;
         await nextUpdate(popover);
 
-        esc(overlay.$.overlay);
+        esc(document.body);
         await nextRender();
-        expect(overlay.opened).to.be.true;
+        expect(overlay.opened).to.be.false;
       });
 
-      it('should not close overlay on target Escape if noCloseOnEsc is true', async () => {
+      it('should not close on global Escape press if noCloseOnEsc is true when moodal', async () => {
+        popover.modal = true;
         popover.noCloseOnEsc = true;
         await nextUpdate(popover);
 
-        esc(target);
+        esc(document.body);
         await nextRender();
         expect(overlay.opened).to.be.true;
       });