refactor: change to be modeless by default, add modal

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

@@ -57,7 +57,7 @@ class PopoverOverlay extends PopoverOverlayMixin(DirMixin(ThemableMixin(PolylitM
   render() {
     return html`
       <div id="backdrop" part="backdrop" hidden ?hidden="${!this.withBackdrop}"></div>
-      <div part="overlay" id="overlay">
+      <div part="overlay" id="overlay" tabindex="0">
         <div part="content" id="content"><slot></slot></div>
       </div>
     `;

packages/popover/src/vaadin-popover.d.ts

@@ -33,28 +33,23 @@ declare class Popover extends PopoverPositionMixin(
   renderer: PopoverRenderer | null | undefined;
 
   /**
-   * When true, opening the overlay moves focus to the first focusable child,
-   * or to the overlay part with tabindex if there are no focusable children.
-   *
-   * @attr {boolean} focus-trap
-   */
-  focusTrap: boolean;
-
-  /**
-   * When true, the backdrop is not shown and clicks on background elements
-   * are allowed. Also, modeless popover does not close on Escape press.
+   * When true, the popover prevents interacting with background elements
+   * by setting `pointer-events` style on the document body to `none`.
+   * This also enables trapping focus inside the overlay.
    */
-  modeless: boolean;
+  modal: boolean;
 
   /**
    * Set to true to disable closing popover overlay on outside click.
+   * Closing on outside click only works when the popover is modal.
    *
    * @attr {boolean} no-close-on-outside-click
    */
   noCloseOnOutsideClick: boolean;
 
   /**
    * Set to true to disable closing popover overlay on Escape press.
+   * Closing on Escape press only works when the popover is modal.
    *
    * @attr {boolean} no-close-on-esc
    */

packages/popover/src/vaadin-popover.js

@@ -50,27 +50,18 @@ class Popover extends PopoverPositionMixin(
       },
 
       /**
-       * When true, opening the overlay moves focus to the first focusable child,
-       * or to the overlay part with tabindex if there are no focusable children.
-       *
-       * @attr {boolean} focus-trap
-       */
-      focusTrap: {
-        type: Boolean,
-        value: false,
-      },
-
-      /**
-       * When true, the backdrop is not shown and clicks on background elements
-       * are allowed. Also, modeless popover does not close on Escape press.
+       * When true, the popover prevents interacting with background elements
+       * by setting `pointer-events` style on the document body to `none`.
+       * This also enables trapping focus inside the overlay.
        */
-      modeless: {
+      modal: {
         type: Boolean,
         value: false,
       },
 
       /**
        * Set to true to disable closing popover overlay on outside click.
+       * Closing on outside click only works when the popover is modal.
        *
        * @attr {boolean} no-close-on-outside-click
        */
@@ -81,6 +72,7 @@ class Popover extends PopoverPositionMixin(
 
       /**
        * Set to true to disable closing popover overlay on Escape press.
+       * Closing on Escape press only works when the popover is modal.
        *
        * @attr {boolean} no-close-on-esc
        */
@@ -126,8 +118,8 @@ class Popover extends PopoverPositionMixin(
         .positionTarget="${this.target}"
         .position="${effectivePosition}"
         .opened="${this._opened}"
-        .modeless="${this.modeless}"
-        .focusTrap="${this.focusTrap}"
+        .modeless="${!this.modal}"
+        .focusTrap="${this.modal}"
         .withBackdrop="${!this.modeless && this.withBackdrop}"
         ?no-horizontal-overlap="${this.__computeNoHorizontalOverlap(effectivePosition)}"
         ?no-vertical-overlap="${this.__computeNoVerticalOverlap(effectivePosition)}"

packages/popover/test/basic.test.js

@@ -117,24 +117,24 @@ describe('popover', () => {
   });
 
   describe('overlay properties', () => {
-    it('should propagate focusTrap property to the overlay', async () => {
-      popover.focusTrap = true;
-      await nextUpdate(popover);
-      expect(overlay.focusTrap).to.be.true;
+    it('should set modeless on the overlay by default', () => {
+      expect(overlay.modeless).to.be.true;
+    });
 
-      popover.focusTrap = false;
+    it('should set modeless on the overlay to false when modal is true', async () => {
+      popover.modal = true;
       await nextUpdate(popover);
-      expect(overlay.focusTrap).to.be.false;
+      expect(overlay.modeless).to.be.false;
     });
 
-    it('should propagate modeless property to the overlay', async () => {
-      popover.modeless = true;
-      await nextUpdate(popover);
+    it('should not set focusTrap on the overlay by default', () => {
       expect(overlay.modeless).to.be.true;
+    });
 
-      popover.modeless = false;
+    it('should set focusTrap on the overlay to true when modal is true', async () => {
+      popover.modal = true;
       await nextUpdate(popover);
-      expect(overlay.modeless).to.be.false;
+      expect(overlay.focusTrap).to.be.true;
     });
 
     it('should propagate withBackdrop property to the overlay', async () => {
@@ -146,13 +146,6 @@ describe('popover', () => {
       await nextUpdate(popover);
       expect(overlay.withBackdrop).to.be.false;
     });
-
-    it('should not propagate withBackdrop when modeless is true', async () => {
-      popover.modeless = true;
-      popover.withBackdrop = true;
-      await nextUpdate(popover);
-      expect(overlay.withBackdrop).to.be.false;
-    });
   });
 
   describe('interactions', () => {
@@ -179,7 +172,19 @@ describe('popover', () => {
       expect(overlay.opened).to.be.false;
     });
 
-    it('should close overlay on outside click by default', async () => {
+    it('should not close overlay on outside click by default', async () => {
+      target.click();
+      await nextRender();
+
+      outsideClick();
+      await nextRender();
+      expect(overlay.opened).to.be.true;
+    });
+
+    it('should close overlay on outside click when modal is true', async () => {
+      popover.modal = true;
+      await nextUpdate(popover);
+
       target.click();
       await nextRender();
 
@@ -189,7 +194,9 @@ describe('popover', () => {
     });
 
     it('should not close on outside click if noCloseOnOutsideClick is true', async () => {
+      popover.modal = true;
       popover.noCloseOnOutsideClick = true;
+      await nextUpdate(popover);
 
       target.click();
       await nextRender();
@@ -199,7 +206,19 @@ describe('popover', () => {
       expect(overlay.opened).to.be.true;
     });
 
-    it('should close overlay on Escape press by default', async () => {
+    it('should not close overlay on Escape press by default', async () => {
+      target.click();
+      await nextRender();
+
+      esc(document.body);
+      await nextRender();
+      expect(overlay.opened).to.be.true;
+    });
+
+    it('should close overlay on Escape press when modal is true', async () => {
+      popover.modal = true;
+      await nextUpdate(popover);
+
       target.click();
       await nextRender();
 
@@ -209,7 +228,9 @@ describe('popover', () => {
     });
 
     it('should not close on Escape press if noCloseOnEsc is true', async () => {
+      popover.modal = true;
       popover.noCloseOnEsc = true;
+      await nextUpdate(popover);
 
       target.click();
       await nextRender();

packages/popover/test/typings/popover.types.ts

@@ -23,8 +23,7 @@ assertType<HTMLElement | undefined>(popover.target);
 assertType<PopoverPosition>(popover.position);
 assertType<PopoverRenderer | null | undefined>(popover.renderer);
 assertType<string>(popover.overlayClass);
-assertType<boolean>(popover.modeless);
-assertType<boolean>(popover.focusTrap);
+assertType<boolean>(popover.modal);
 assertType<boolean>(popover.withBackdrop);
 assertType<boolean>(popover.noCloseOnEsc);
 assertType<boolean>(popover.noCloseOnOutsideClick);