refactor: set aria-modal and tabindex on the popover (#10020)

web-padawan · web-flow · commit 7ed2da4c49ed · 2025-08-18T17:31:53.000+03:00
diff --git a/packages/popover/src/vaadin-popover-overlay.js b/packages/popover/src/vaadin-popover-overlay.js
@@ -38,7 +38,7 @@ class PopoverOverlay extends PopoverOverlayMixin(
   render() {
     return html`
       <div id="backdrop" part="backdrop" hidden ?hidden="${!this.withBackdrop}"></div>
-      <div part="overlay" id="overlay" tabindex="0">
+      <div part="overlay" id="overlay">
         <div part="arrow"></div>
         <div part="content" id="content"><slot></slot></div>
       </div>
@@ -76,6 +76,24 @@ class PopoverOverlay extends PopoverOverlayMixin(
     return this.owner;
   }
 
+  /**
+   * Override method from OverlayFocusMixin to use owner as focus trap root
+   * @protected
+   * @override
+   */
+  get _focusTrapRoot() {
+    return this.owner;
+  }
+
+  /**
+   * Override method from OverlayFocusMixin to not set `aria-hidden`
+   * @protected
+   * @override
+   */
+  get _useAriaHidden() {
+    return false;
+  }
+
   /**
    * Override method from `OverlayMixin` to always add outside
    * click listener so that it can be used by modeless popover.
diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js
@@ -215,8 +215,21 @@ class Popover extends PopoverPositionMixin(
 
   static get styles() {
     return css`
-      :host {
-        display: contents;
+      :host([opened]),
+      :host([opening]),
+      :host([closing]) {
+        display: block !important;
+        position: absolute;
+        outline: none;
+      }
+
+      :host,
+      :host([hidden]) {
+        display: none !important;
+      }
+
+      :host(:focus) ::part(overlay) {
+        outline: var(--vaadin-focus-ring-width) solid var(--vaadin-focus-ring-color);
       }
     `;
   }
@@ -531,6 +544,16 @@ class Popover extends PopoverPositionMixin(
 
     this._overlayElement = this.$.overlay;
 
+    this.setAttribute('tabindex', '0');
+
+    this.addEventListener('focusin', (e) => {
+      this.__onFocusIn(e);
+    });
+
+    this.addEventListener('focusout', (e) => {
+      this.__onFocusOut(e);
+    });
+
     if (!this.hasAttribute('role')) {
       this.role = 'dialog';
     }
@@ -564,6 +587,14 @@ class Popover extends PopoverPositionMixin(
         this.removeAttribute('aria-labelledby');
       }
     }
+
+    if (props.has('modal')) {
+      if (this.modal) {
+        this.setAttribute('aria-modal', 'true');
+      } else {
+        this.removeAttribute('aria-modal');
+      }
+    }
   }
 
   /** @protected */
@@ -688,20 +719,18 @@ class Popover extends PopoverPositionMixin(
 
   /** @private */
   __onGlobalTab(event) {
-    const overlayPart = this._overlayElement.$.overlay;
-
-    // Move focus to the popover content on target element Tab
+    // Move focus to the popover on target element Tab
     if (this.target && isElementFocused(this.__getTargetFocusable())) {
       event.preventDefault();
-      overlayPart.focus();
+      this.focus();
       return;
     }
 
     // Move focus to the next element after target on content Tab
-    const lastFocusable = this.__getLastFocusable(overlayPart);
+    const lastFocusable = this.__getLastFocusable(this);
     if (lastFocusable && isElementFocused(lastFocusable)) {
       const focusable = this.__getNextBodyFocusable(this.__getTargetFocusable());
-      if (focusable && focusable !== overlayPart) {
+      if (focusable && focusable !== this) {
         event.preventDefault();
         focusable.focus();
         return;
@@ -711,7 +740,7 @@ class Popover extends PopoverPositionMixin(
     // Prevent focusing the popover content on previous element Tab
     const activeElement = getDeepActiveElement();
     const nextFocusable = this.__getNextBodyFocusable(activeElement);
-    if (nextFocusable === overlayPart && lastFocusable) {
+    if (nextFocusable === this && lastFocusable) {
       // Move focus to the last overlay focusable and do NOT prevent keydown
       // to move focus outside the popover content (e.g. to the URL bar).
       lastFocusable.focus();
@@ -720,16 +749,14 @@ class Popover extends PopoverPositionMixin(
 
   /** @private */
   __onGlobalShiftTab(event) {
-    const overlayPart = this._overlayElement.$.overlay;
-
     // Prevent restoring focus after target blur on Shift + Tab
     if (this.target && isElementFocused(this.__getTargetFocusable()) && this.__shouldRestoreFocus) {
       this.__shouldRestoreFocus = false;
       return;
     }
 
-    // Move focus back to the target on overlay content Shift + Tab
-    if (this.target && isElementFocused(overlayPart)) {
+    // Move focus back to the target on popover Shift + Tab
+    if (this.target && isElementFocused(this)) {
       event.preventDefault();
       this.__getTargetFocusable().focus();
       return;
@@ -738,7 +765,7 @@ class Popover extends PopoverPositionMixin(
     // Move focus back to the popover on next element Shift + Tab
     const nextFocusable = this.__getNextBodyFocusable(this.__getTargetFocusable());
     if (nextFocusable && isElementFocused(nextFocusable)) {
-      const lastFocusable = this.__getLastFocusable(overlayPart);
+      const lastFocusable = this.__getLastFocusable(this);
       if (lastFocusable) {
         event.preventDefault();
         lastFocusable.focus();
@@ -834,7 +861,7 @@ class Popover extends PopoverPositionMixin(
   }
 
   /** @private */
-  __onOverlayFocusIn() {
+  __onFocusIn() {
     this.__focusInside = true;
 
     // When using Tab to move focus, restoring focus is reset. However, if pressing Tab
@@ -845,7 +872,7 @@ class Popover extends PopoverPositionMixin(
   }
 
   /** @private */
-  __onOverlayFocusOut(event) {
+  __onFocusOut(event) {
     // Do not close the popover on overlay focusout if it's not the last one.
     // This covers the following cases of nested overlay based components:
     // 1. Moving focus to the nested overlay (e.g. vaadin-select, vaadin-menu-bar)
@@ -857,7 +884,6 @@ class Popover extends PopoverPositionMixin(
     if (
       (this.__hasTrigger('focus') && this.__mouseDownInside) ||
       event.relatedTarget === this.target ||
-      event.relatedTarget === this._overlayElement ||
       this.contains(event.relatedTarget)
     ) {
       return;
@@ -946,7 +972,7 @@ class Popover extends PopoverPositionMixin(
   /** @private */
   __onOverlayOpened() {
     if (this.autofocus && !this.modal) {
-      this._overlayElement.$.overlay.focus();
+      this.focus();
     }
   }
 
diff --git a/packages/popover/test/a11y.test.js b/packages/popover/test/a11y.test.js
@@ -1,15 +1,6 @@
 import { expect } from '@vaadin/chai-plugins';
 import { sendKeys } from '@vaadin/test-runner-commands';
-import {
-  esc,
-  fixtureSync,
-  focusout,
-  nextRender,
-  nextUpdate,
-  oneEvent,
-  outsideClick,
-  tab,
-} from '@vaadin/testing-helpers';
+import { fixtureSync, focusout, nextRender, nextUpdate, oneEvent, outsideClick, tab } from '@vaadin/testing-helpers';
 import sinon from 'sinon';
 import './not-animated-styles.js';
 import { getDeepActiveElement } from '@vaadin/a11y-base/src/focus-utils.js';
@@ -226,16 +217,16 @@ describe('a11y', () => {
     let spy;
 
     beforeEach(() => {
-      spy = sinon.spy(overlay.$.overlay, 'focus');
+      spy = sinon.spy(popover, 'focus');
     });
 
-    it('should not move focus to the overlay content when opened by default', async () => {
+    it('should not focus the popover when opened by default', async () => {
       target.click();
       await oneEvent(overlay, 'vaadin-overlay-open');
       expect(spy).to.not.be.called;
     });
 
-    it('should move focus to the overlay content when opened if autofocus is true', async () => {
+    it('should focus the popover when opened if autofocus is true', async () => {
       popover.autofocus = true;
       target.click();
       await oneEvent(overlay, 'vaadin-overlay-open');
@@ -255,31 +246,31 @@ describe('a11y', () => {
 
       it('should restore focus on Esc with trigger set to focus', async () => {
         const focusSpy = sinon.spy(target, 'focus');
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(focusSpy).to.be.calledOnce;
       });
 
       it('should not re-open when restoring focus on Esc with trigger set to focus', async () => {
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(popover.opened).to.be.false;
       });
 
       it('should not re-open when restoring focus on outside click with trigger set to focus', async () => {
-        overlay.$.overlay.focus();
+        popover.focus();
         outsideClick();
         await nextRender();
 
         expect(popover.opened).to.be.false;
       });
 
       it('should re-open when re-focusing after closing on outside click with trigger set to focus', async () => {
-        overlay.$.overlay.focus();
+        popover.focus();
         outsideClick();
         await nextRender();
 
@@ -302,8 +293,8 @@ describe('a11y', () => {
 
       it('should restore focus on Esc with trigger set to click', async () => {
         const focusSpy = sinon.spy(target, 'focus');
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(focusSpy).to.be.calledOnce;
@@ -321,8 +312,8 @@ describe('a11y', () => {
         const focusSpy = sinon.spy(target, 'focus');
         tab(target);
         focusout(target, overlay);
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(focusSpy).to.be.calledOnce;
@@ -340,8 +331,8 @@ describe('a11y', () => {
         await oneEvent(overlay, 'vaadin-overlay-open');
 
         const focusSpy = sinon.spy(target, 'focus');
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(focusSpy).to.not.be.called;
@@ -389,8 +380,8 @@ describe('a11y', () => {
         await nextRender();
 
         const focusSpy = sinon.spy(target, 'focus');
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(focusSpy).to.not.be.called;
@@ -400,8 +391,8 @@ describe('a11y', () => {
         mouseenter(target);
 
         const focusSpy = sinon.spy(target, 'focus');
-        overlay.$.overlay.focus();
-        esc(overlay.$.overlay);
+        popover.focus();
+        await sendKeys({ press: 'Escape' });
         await nextRender();
 
         expect(focusSpy).to.be.calledOnce;
@@ -432,10 +423,10 @@ describe('a11y', () => {
           await oneEvent(overlay, 'vaadin-overlay-open');
         });
 
-        it('should focus the overlay content part on target Tab', async () => {
+        it('should focus the popover on target Tab', async () => {
           target.focus();
 
-          const spy = sinon.spy(overlay.$.overlay, 'focus');
+          const spy = sinon.spy(popover, 'focus');
           await sendKeys({ press: 'Tab' });
 
           expect(spy).to.be.calledOnce;
@@ -483,13 +474,13 @@ describe('a11y', () => {
           expect(spy).to.be.calledOnce;
         });
 
-        it('should not focus the overlay part on the next element Tab', async () => {
+        it('should not focus the popover on the next element Tab', async () => {
           input.focus();
 
           await sendKeys({ press: 'Tab' });
 
           const activeElement = getDeepActiveElement();
-          expect(activeElement).to.not.equal(overlay.$.overlay);
+          expect(activeElement).to.not.equal(popover);
         });
 
         it('should focus previous element on target Shift Tab while opened', async () => {
@@ -543,18 +534,18 @@ describe('a11y', () => {
         expect(activeElement).to.equal(input);
       });
 
-      it('should focus the overlay content part on focusable content Shift Tab', async () => {
+      it('should focus the popover on focusable content Shift Tab', async () => {
         // Move focus to the overlay
         await sendKeys({ press: 'Tab' });
 
         // Move focus to the input inside the overlay
         await sendKeys({ press: 'Tab' });
 
-        // Move focus back to the overlay part
+        // Move focus back to the popover
         await sendKeys({ press: 'Shift+Tab' });
 
         const activeElement = getDeepActiveElement();
-        expect(activeElement).to.equal(overlay.$.overlay);
+        expect(activeElement).to.equal(popover);
       });
     });
   });
diff --git a/packages/popover/test/basic.test.js b/packages/popover/test/basic.test.js
@@ -30,8 +30,15 @@ describe('popover', () => {
   });
 
   describe('host element', () => {
-    it('should set display: contents on the host element by default', () => {
-      expect(getComputedStyle(popover).display).to.equal('contents');
+    it('should use display: none when not opened', () => {
+      expect(getComputedStyle(popover).display).to.equal('none');
+    });
+
+    ['opened', 'opening', 'closing'].forEach((state) => {
+      it(`should use display: block when ${state} attribute is set`, () => {
+        popover.setAttribute(state, '');
+        expect(getComputedStyle(popover).display).to.equal('block');
+      });
     });
 
     it('should reflect opened property to attribute', async () => {
@@ -480,7 +487,7 @@ describe('popover', () => {
         await nextUpdate(popover);
 
         target.click();
-        await nextRender();
+        await oneEvent(overlay, 'vaadin-overlay-open');
       });
 
       it('should set pointer-events on backdrop to none when non modal', () => {
diff --git a/packages/popover/test/dom/__snapshots__/popover.test.snap.js b/packages/popover/test/dom/__snapshots__/popover.test.snap.js
diff --git a/packages/popover/test/dom/popover.test.js b/packages/popover/test/dom/popover.test.js
