refactor: set aria-modal and tabindex on the dialog (#10024)

Author: web-padawan

packages/crud/src/vaadin-crud-dialog.js

@@ -48,11 +48,30 @@ class CrudDialogOverlay extends OverlayMixin(DirMixin(ThemableMixin(PolylitMixin
     return this.owner;
   }
 
+  /**
+   * Override method from OverlayFocusMixin to use dialog as focus trap root
+   * @protected
+   * @override
+   */
+  get _focusTrapRoot() {
+    // Do not use `owner` since that points to `vaadin-crud`
+    return this.getRootNode().host;
+  }
+
+  /**
+   * Override method from OverlayFocusMixin to not set `aria-hidden`
+   * @protected
+   * @override
+   */
+  get _useAriaHidden() {
+    return false;
+  }
+
   /** @protected */
   render() {
     return html`
       <div part="backdrop" id="backdrop" ?hidden="${!this.withBackdrop}"></div>
-      <div part="overlay" id="overlay" tabindex="0">
+      <div part="overlay" id="overlay">
         <section id="resizerContainer" class="resizer-container">
           <header part="header">
             <slot name="header"></slot>
@@ -99,13 +118,18 @@ class CrudDialog extends DialogBaseMixin(ThemePropertyMixin(PolylitMixin(LitElem
       :host([opened]),
       :host([opening]),
       :host([closing]) {
-        display: contents !important;
+        display: block !important;
+        position: absolute;
       }
 
       :host,
       :host([hidden]) {
         display: none !important;
       }
+
+      :host(:focus) ::part(overlay) {
+        outline: var(--vaadin-focus-ring-width) solid var(--vaadin-focus-ring-color);
+      }
     `;
   }
 

packages/crud/test/a11y.test.js

@@ -15,14 +15,14 @@ describe('a11y', () => {
 
   function focusRestorationTests(testId, createFixture) {
     describe(`focus restoration - ${testId}`, () => {
-      let grid, form, overlay, newButton, saveButton, cancelButton, editButtons;
+      let grid, form, dialog, newButton, saveButton, cancelButton, editButtons;
 
       describe('create item', () => {
         beforeEach(async () => {
           crud = createFixture();
           crud.items = [{ title: 'Item 1' }];
           await nextRender();
-          overlay = getDialogEditor(crud).$.overlay;
+          dialog = getDialogEditor(crud);
           form = crud.querySelector('vaadin-crud-form');
           newButton = crud.querySelector('[slot=new-button]');
           saveButton = crud.querySelector('[slot=save-button]');
@@ -34,7 +34,7 @@ describe('a11y', () => {
           newButton.focus();
           newButton.click();
           await nextRender();
-          expect(getDeepActiveElement()).to.equal(overlay.$.overlay);
+          expect(getDeepActiveElement()).to.equal(dialog);
         });
 
         it('should restore focus to previous element on new dialog close', async () => {
@@ -327,73 +327,4 @@ describe('a11y', () => {
       expect(dialog.getAttribute('aria-label')).to.equal('Edit item');
     });
   });
-
-  describe('modal dialog', () => {
-    let dialog, grid, header, form, newButton, saveButton, cancelButton, deleteButton, sibling;
-
-    beforeEach(async () => {
-      crud = fixtureSync('<vaadin-crud></vaadin-crud>');
-      crud.items = [{ title: 'Item 1' }];
-
-      dialog = getDialogEditor(crud);
-      grid = crud.querySelector('[slot="grid"]');
-      header = crud.querySelector('[slot="header"]');
-      form = crud.querySelector('[slot="form"]');
-      newButton = crud.querySelector('[slot="new-button"]');
-      saveButton = crud.querySelector('[slot="save-button"]');
-      cancelButton = crud.querySelector('[slot="cancel-button"]');
-      deleteButton = crud.querySelector('[slot="delete-button"]');
-
-      sibling = fixtureSync('<button></button>');
-      await nextRender();
-    });
-
-    it('should hide all elements outside of the dialog when opened', async () => {
-      crud._newButton.click();
-      await nextRender();
-
-      // Sibling elements of CRUD must be hidden
-      expect(sibling.parentElement.getAttribute('aria-hidden')).to.equal('true');
-
-      // Hierarchy to dialog must not be hidden
-      [crud.parentElement, crud, dialog, dialog.$.overlay].forEach((el) => {
-        expect(el.hasAttribute('aria-hidden')).to.be.false;
-      });
-
-      // Elements slotted into the dialog must not be hidden
-      [header, form, saveButton, cancelButton, deleteButton].forEach((el) => {
-        expect(el.hasAttribute('aria-hidden')).to.be.false;
-      });
-
-      // Elements not slotted into the dialog must be hidden
-      [grid, newButton].forEach((el) => {
-        expect(el.getAttribute('aria-hidden')).to.equal('true');
-      });
-    });
-
-    it('should restore visibility of elements outside of the dialog when closed', async () => {
-      crud._newButton.click();
-      await nextRender();
-
-      // Close the dialog
-      await sendKeys({ press: 'Escape' });
-
-      [
-        sibling.parentElement,
-        crud.parentElement,
-        crud,
-        dialog,
-        dialog.$.overlay,
-        header,
-        form,
-        saveButton,
-        cancelButton,
-        deleteButton,
-        grid,
-        newButton,
-      ].forEach((el) => {
-        expect(el.hasAttribute('aria-hidden')).to.be.false;
-      });
-    });
-  });
 });

packages/crud/test/dom/__snapshots__/crud.test.snap.js

@@ -281,9 +281,11 @@ snapshots["vaadin-crud shadow default"] =
   </div>
 </div>
 <vaadin-crud-dialog
+  aria-modal="true"
   exportparts="backdrop, overlay, header, content, footer"
   id="dialog"
   role="dialog"
+  tabindex="0"
 >
   <slot
     name="header"

packages/dialog/src/vaadin-dialog-base-mixin.js

@@ -105,6 +105,8 @@ export const DialogBaseMixin = (superClass) =>
       if (!this.hasAttribute('role')) {
         this.role = 'dialog';
       }
+
+      this.setAttribute('tabindex', '0');
     }
 
     /** @protected */
@@ -114,6 +116,14 @@ export const DialogBaseMixin = (superClass) =>
       if (props.has('overlayRole')) {
         this.role = this.overlayRole || 'dialog';
       }
+
+      if (props.has('modeless')) {
+        if (!this.modeless) {
+          this.setAttribute('aria-modal', 'true');
+        } else {
+          this.removeAttribute('aria-modal');
+        }
+      }
     }
 
     /** @private */

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

@@ -33,11 +33,29 @@ export class DialogOverlay extends DialogOverlayMixin(
     return dialogOverlayStyles;
   }
 
+  /**
+   * 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;
+  }
+
   /** @protected */
   render() {
     return html`
       <div id="backdrop" part="backdrop" ?hidden="${!this.withBackdrop}"></div>
-      <div part="overlay" id="overlay" tabindex="0">
+      <div part="overlay" id="overlay">
         <section id="resizerContainer" class="resizer-container">
           <header part="header">
             <div part="title"><slot name="title"></slot></div>

packages/dialog/src/vaadin-dialog.js

@@ -101,13 +101,19 @@ class Dialog extends DialogSizeMixin(
       :host([opened]),
       :host([opening]),
       :host([closing]) {
-        display: contents !important;
+        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);
+      }
     `;
   }
 

packages/dialog/test/dialog.test.js

@@ -1,5 +1,14 @@
 import { expect } from '@vaadin/chai-plugins';
-import { aTimeout, click, esc, fixtureSync, listenOnce, nextRender, nextUpdate } from '@vaadin/testing-helpers';
+import {
+  aTimeout,
+  click,
+  esc,
+  fixtureSync,
+  listenOnce,
+  nextRender,
+  nextUpdate,
+  oneEvent,
+} from '@vaadin/testing-helpers';
 import sinon from 'sinon';
 import '../src/vaadin-dialog.js';
 import { getDeepActiveElement } from '@vaadin/a11y-base/src/focus-utils.js';
@@ -36,14 +45,15 @@ describe('vaadin-dialog', () => {
     });
 
     ['opened', 'opening', 'closing'].forEach((state) => {
-      it(`should use display: contents when ${state} attribute is set`, () => {
+      it(`should use display: block when ${state} attribute is set`, () => {
         dialog.setAttribute(state, '');
-        expect(getComputedStyle(dialog).display).to.equal('contents');
+        expect(getComputedStyle(dialog).display).to.equal('block');
       });
     });
 
     it('should use display: none when hidden while opened', async () => {
       dialog.opened = true;
+      await oneEvent(dialog.$.overlay, 'vaadin-overlay-open');
       dialog.hidden = true;
       await nextRender();
       expect(getComputedStyle(dialog).display).to.equal('none');
@@ -235,13 +245,13 @@ describe('vaadin-dialog', () => {
 
     it('should move focus to the dialog on open', async () => {
       dialog.opened = true;
-      await nextRender();
-      expect(getDeepActiveElement()).to.equal(overlay.$.overlay);
+      await oneEvent(overlay, 'vaadin-overlay-open');
+      expect(getDeepActiveElement()).to.equal(dialog);
     });
 
     it('should restore focus on dialog close', async () => {
       dialog.opened = true;
-      await nextRender();
+      await oneEvent(overlay, 'vaadin-overlay-open');
       dialog.opened = false;
       await nextRender();
       expect(getDeepActiveElement()).to.equal(button);

packages/dialog/test/dom/__snapshots__/dialog.test.snap.js

@@ -3,8 +3,10 @@ export const snapshots = {};
 
 snapshots["vaadin-dialog host"] = 
 `<vaadin-dialog
+  aria-modal="true"
   opened=""
   role="dialog"
+  tabindex="0"
   with-backdrop=""
 >
   content
@@ -115,7 +117,6 @@ snapshots["vaadin-dialog overlay"] =
 <div
   id="overlay"
   part="overlay"
-  tabindex="0"
 >
   <section
     class="resizer-container"