refactor: update popover to use global overlay listeners (#9972)

Author: web-padawan

packages/overlay/src/vaadin-overlay-mixin.js

@@ -237,8 +237,25 @@ export const OverlayMixin = (superClass) =>
       }
     }
 
+    /**
+     * Whether to add global listeners for closing on outside click.
+     * By default, listeners are not added for a modeless overlay.
+     *
+     * @return {boolean}
+     * @protected
+     */
+    _shouldAddGlobalListeners() {
+      return !this.modeless;
+    }
+
     /** @private */
     _addGlobalListeners() {
+      if (this.__hasGlobalListeners) {
+        return;
+      }
+
+      this.__hasGlobalListeners = true;
+
       document.addEventListener('mousedown', this._boundMouseDownListener);
       document.addEventListener('mouseup', this._boundMouseUpListener);
       // Firefox leaks click to document on contextmenu even if prevented
@@ -248,6 +265,12 @@ export const OverlayMixin = (superClass) =>
 
     /** @private */
     _removeGlobalListeners() {
+      if (!this.__hasGlobalListeners) {
+        return;
+      }
+
+      this.__hasGlobalListeners = false;
+
       document.removeEventListener('mousedown', this._boundMouseDownListener);
       document.removeEventListener('mouseup', this._boundMouseUpListener);
       document.documentElement.removeEventListener('click', this._boundOutsideClickListener, true);
@@ -281,13 +304,20 @@ export const OverlayMixin = (superClass) =>
 
     /** @private */
     _modelessChanged(modeless) {
+      if (this.opened) {
+        // Add / remove listeners if modeless is changed while opened
+        if (this._shouldAddGlobalListeners()) {
+          this._addGlobalListeners();
+        } else {
+          this._removeGlobalListeners();
+        }
+      }
+
       if (!modeless) {
         if (this.opened) {
-          this._addGlobalListeners();
           this._enterModalState();
         }
       } else {
-        this._removeGlobalListeners();
         this._exitModalState();
       }
       setOverlayStateAttribute(this, 'modeless', modeless);
@@ -326,7 +356,7 @@ export const OverlayMixin = (superClass) =>
 
         document.addEventListener('keydown', this._boundKeydownListener);
 
-        if (!this.modeless) {
+        if (this._shouldAddGlobalListeners()) {
           this._addGlobalListeners();
         }
       } else if (wasOpened) {
@@ -341,7 +371,7 @@ export const OverlayMixin = (superClass) =>
 
         document.removeEventListener('keydown', this._boundKeydownListener);
 
-        if (!this.modeless) {
+        if (this._shouldAddGlobalListeners()) {
           this._removeGlobalListeners();
         }
       }
@@ -522,7 +552,7 @@ export const OverlayMixin = (superClass) =>
       }
 
       // Only close modeless overlay on Esc press when it contains focus
-      if (this.modeless && !event.composedPath().includes(this.$.overlay)) {
+      if (!this._shouldAddGlobalListeners() && !event.composedPath().includes(this.$.overlay)) {
         return;
       }
 

packages/overlay/test/interactions.test.js

@@ -98,6 +98,13 @@ describe('interactions', () => {
         expect(overlay.opened).to.be.false;
       });
 
+      it('should not close on outside click when modeless', () => {
+        overlay.modeless = true;
+        click(parent);
+
+        expect(overlay.opened).to.be.true;
+      });
+
       it('should close on backdrop click', () => {
         overlay.withBackdrop = true;
 
@@ -151,6 +158,30 @@ describe('interactions', () => {
 
         expect(overlay.opened).to.be.true;
       });
+
+      it('should not fire the event on outside click when modeless set to true', () => {
+        overlay.modeless = true;
+
+        const spy = sinon.spy();
+        overlay.addEventListener('vaadin-overlay-outside-click', spy);
+
+        click(parent);
+
+        expect(spy).to.be.not.called;
+      });
+
+      it('should not fire the event on outside click when modeless set back to false', () => {
+        overlay.modeless = true;
+
+        const spy = sinon.spy();
+        overlay.addEventListener('vaadin-overlay-outside-click', spy);
+
+        overlay.modeless = false;
+
+        click(parent);
+
+        expect(spy.calledOnce).to.be.true;
+      });
     });
 
     describe('vaadin-overlay-close event', () => {

packages/popover/src/vaadin-popover-overlay.js

@@ -75,6 +75,17 @@ class PopoverOverlay extends PopoverOverlayMixin(
   get _modalRoot() {
     return this.owner;
   }
+
+  /**
+   * Override method from `OverlayMixin` to always add outside
+   * click listener so that it can be used by modeless popover.
+   * @return {boolean}
+   * @protected
+   * @override
+   */
+  _shouldAddGlobalListeners() {
+    return true;
+  }
 }
 
 defineCustomElement(PopoverOverlay);

packages/popover/src/vaadin-popover.js

@@ -461,7 +461,6 @@ class Popover extends PopoverPositionMixin(
 
     this.__generatedId = `vaadin-popover-${generateUniqueId()}`;
 
-    this.__onGlobalClick = this.__onGlobalClick.bind(this);
     this.__onGlobalKeyDown = this.__onGlobalKeyDown.bind(this);
     this.__onTargetClick = this.__onTargetClick.bind(this);
     this.__onTargetFocusIn = this.__onTargetFocusIn.bind(this);
@@ -574,16 +573,12 @@ class Popover extends PopoverPositionMixin(
     if (!this.id) {
       this.id = this.__generatedId;
     }
-
-    document.documentElement.addEventListener('click', this.__onGlobalClick, true);
   }
 
   /** @protected */
   disconnectedCallback() {
     super.disconnectedCallback();
 
-    document.documentElement.removeEventListener('click', this.__onGlobalClick, true);
-
     // Automatically close popover when it is removed from DOM
     // Avoid closing if the popover is just moved in the DOM
     queueMicrotask(() => {
@@ -655,23 +650,6 @@ class Popover extends PopoverPositionMixin(
     }
   }
 
-  /**
-   * Overlay's global outside click listener doesn't work when
-   * the overlay is modeless, so we use a separate listener.
-   * @private
-   */
-  __onGlobalClick(event) {
-    if (
-      this.opened &&
-      !this.modal &&
-      !event.composedPath().some((el) => el === this._overlayElement || el === this.target) &&
-      !this.noCloseOnOutsideClick &&
-      isLastOverlay(this._overlayElement)
-    ) {
-      this._openedStateController.close(true);
-    }
-  }
-
   /** @private */
   __onTargetClick() {
     if (this.__hasTrigger('click')) {
@@ -692,17 +670,11 @@ class Popover extends PopoverPositionMixin(
    * @private
    */
   __onGlobalKeyDown(event) {
-    // Modal popover uses overlay logic for Esc key and focus trap.
+    // Modal popover uses overlay logic focus trap.
     if (this.modal) {
       return;
     }
 
-    if (event.key === 'Escape' && !this.noCloseOnEsc && this.opened && isLastOverlay(this._overlayElement)) {
-      // Prevent closing parent overlay (e.g. dialog)
-      event.stopPropagation();
-      this._openedStateController.close(true);
-    }
-
     // Include popover content in the Tab order after the target.
     if (event.key === 'Tab') {
       if (event.shiftKey) {

packages/popover/test/basic.test.js

@@ -300,6 +300,31 @@ describe('popover', () => {
       expect(overlay.opened).to.be.true;
     });
 
+    it('should not close on outside click if overlay close event is prevented', async () => {
+      target.click();
+      await oneEvent(overlay, 'vaadin-overlay-open');
+
+      document.addEventListener('vaadin-overlay-close', (e) => e.preventDefault(), { once: true });
+
+      outsideClick();
+      await nextRender();
+      expect(overlay.opened).to.be.true;
+    });
+
+    it('should not close on outside click if overlay close event is prevented when modal', async () => {
+      popover.modal = true;
+      await nextUpdate(popover);
+
+      target.click();
+      await oneEvent(overlay, 'vaadin-overlay-open');
+
+      document.addEventListener('vaadin-overlay-close', (e) => e.preventDefault(), { once: true });
+
+      outsideClick();
+      await nextRender();
+      expect(overlay.opened).to.be.true;
+    });
+
     it('should close overlay when popover is detached', async () => {
       target.click();
       await oneEvent(overlay, 'vaadin-overlay-open');
@@ -320,14 +345,6 @@ describe('popover', () => {
       expect(overlay.opened).to.be.true;
     });
 
-    it('should remove document click listener when popover is detached', async () => {
-      const spy = sinon.spy(document.documentElement, 'removeEventListener');
-      popover.remove();
-      await nextRender();
-      expect(spy).to.be.called;
-      expect(spy.firstCall.args[0]).to.equal('click');
-    });
-
     describe('Escape press', () => {
       beforeEach(async () => {
         target.click();
@@ -368,6 +385,25 @@ describe('popover', () => {
         await nextRender();
         expect(overlay.opened).to.be.true;
       });
+
+      it('should not close on global Escape press if overlay close event was prevented', async () => {
+        document.addEventListener('vaadin-overlay-close', (e) => e.preventDefault(), { once: true });
+
+        esc(document.body);
+        await nextRender();
+        expect(overlay.opened).to.be.true;
+      });
+
+      it('should not close on global Escape press if overlay close event was prevented when modal', async () => {
+        popover.modal = true;
+        await nextUpdate(popover);
+
+        document.addEventListener('vaadin-overlay-close', (e) => e.preventDefault(), { once: true });
+
+        esc(document.body);
+        await nextRender();
+        expect(overlay.opened).to.be.true;
+      });
     });
 
     describe('nested popovers', () => {