Add modal tab tracking (#1403)

* add modal tab tracking

* prettier

* sort by tabindex

* sort by tabindex

* add a dialog test case for shadow roots

* add a changelog note

* add a changelog note

* prettier + test fixes

* prettier + test fixes

docs/pages/resources/changelog.md

@@ -16,6 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 - Added tests for `<sl-qr-code>` [#1416]
 - Added support for pressing [[Space]] to select/toggle selected `<sl-menu-item>` elements [#1429]
+- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `offsetParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot [#1403]
 - Fixed a bug in `valueAsDate` on `<sl-input>` where it would always set `type="date"` for the underlying `<input>` element. It now falls back to the native browser implementation for the in-memory input. This may cause unexpected behavior if you're using `valueAsDate` on any input elements that aren't `type="date"`. [#1399]
 - Fixed a bug in `<sl-qr-code>` where the `background` attribute was never passed to the QR code [#1416]
 - Fixed a bug in `<sl-dropdown>` where aria attributes were incorrectly applied to the default `<slot>` causing Lighthouse errors [#1417]

src/components/dialog/dialog.test.ts

@@ -1,6 +1,7 @@
 import '../../../dist/shoelace.js';
 // cspell:dictionaries lorem-ipsum
-import { expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { aTimeout, elementUpdated, expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { LitElement } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
 import type SlDialog from './dialog';
@@ -146,4 +147,124 @@ describe('<sl-dialog>', () => {
 
     expect(el.open).to.be.false;
   });
+
+  // https://github.com/shoelace-style/shoelace/issues/1382
+  it('should properly cycle through tabbable elements when sl-dialog is used in a shadowRoot', async () => {
+    class AContainer extends LitElement {
+      get dialog() {
+        return this.shadowRoot?.querySelector('sl-dialog');
+      }
+
+      openDialog() {
+        this.dialog?.show();
+      }
+
+      render() {
+        return html`
+          <h1>Dialog Example</h1>
+          <sl-dialog label="Dialog" class="dialog-overview">
+            Lorem ipsum dolor sit amet, consectetur adipiscing elit.
+            <br />
+            <label><input type="checkbox" />A</label>
+            <label><input type="checkbox" />B</label>
+            <button>Button</button>
+          </sl-dialog>
+
+          <sl-button @click=${this.openDialog}>Open Dialog</sl-button>
+        `;
+      }
+    }
+
+    if (!window.customElements.get('a-container')) {
+      window.customElements.define('a-container', AContainer);
+    }
+
+    const testCase = await fixture(html`
+      <div>
+        <a-container></a-container>
+
+        <p>
+          Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches
+          things outside the dialog.
+        </p>
+      </div>
+    `);
+
+    const container = testCase.querySelector('a-container');
+
+    if (!container) {
+      throw Error('Could not find <a-container> element.');
+    }
+
+    await elementUpdated(container);
+    const dialog = container.shadowRoot?.querySelector('sl-dialog');
+
+    if (!dialog) {
+      throw Error('Could not find <sl-dialog> element.');
+    }
+
+    const closeButton = dialog.shadowRoot?.querySelector('sl-icon-button');
+    const checkbox1 = dialog.querySelector("input[type='checkbox']");
+    const checkbox2 = dialog.querySelectorAll("input[type='checkbox']")[1];
+    const button = dialog.querySelector('button');
+
+    // Opens modal.
+    const openModalButton = container.shadowRoot?.querySelector('sl-button');
+
+    if (openModalButton) openModalButton.click();
+
+    // Test tab cycling
+    await pressTab();
+
+    expect(container.shadowRoot?.activeElement).to.equal(dialog);
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(button);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    // Test Shift+Tab cycling
+
+    // I found these timeouts were needed for WebKit locally.
+    await aTimeout(10);
+    await sendKeys({ down: 'Shift' });
+    await aTimeout(10);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(button);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2);
+
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
+
+    // End shift+tab cycling
+    await sendKeys({ up: 'Shift' });
+  });
 });
+
+// We wait 50ms just to give the browser some time to figure out the current focus.
+// 50 was the magic number I found locally :shrug:
+async function pressTab() {
+  await aTimeout(50);
+  await sendKeys({ press: 'Tab' });
+  await aTimeout(50);
+}

src/components/dialog/dialog.ts

@@ -104,6 +104,7 @@ export default class SlDialog extends ShoelaceElement {
 
   disconnectedCallback() {
     super.disconnectedCallback();
+    this.modal.deactivate();
     unlockBodyScrolling(this);
   }
 
@@ -269,7 +270,7 @@ export default class SlDialog extends ShoelaceElement {
           aria-hidden=${this.open ? 'false' : 'true'}
           aria-label=${ifDefined(this.noHeader ? this.label : undefined)}
           aria-labelledby=${ifDefined(!this.noHeader ? 'title' : undefined)}
-          tabindex="0"
+          tabindex="-1"
         >
           ${!this.noHeader
             ? html`
@@ -292,8 +293,10 @@ export default class SlDialog extends ShoelaceElement {
                 </header>
               `
             : ''}
-
+          ${
-          <slot part="body" class="dialog__body"></slot>
+            '' /* The tabindex="-1" is here because the body is technically scrollable if overflowing. However, if there's no focusable elements inside, you won't actually be able to scroll it via keyboard. */
+          }
+          <slot part="body" class="dialog__body" tabindex="-1"></slot>
 
           <footer part="footer" class="dialog__footer">
             <slot name="footer"></slot>

src/internal/modal.ts

@@ -1,10 +1,11 @@
-import { getTabbableBoundary } from './tabbable.js';
+import { getTabbableElements } from './tabbable.js';
 
 let activeModals: HTMLElement[] = [];
 
 export default class Modal {
   element: HTMLElement;
   tabDirection: 'forward' | 'backward' = 'forward';
+  currentFocus: HTMLElement | null;
 
   constructor(element: HTMLElement) {
     this.element = element;
@@ -22,6 +23,7 @@ export default class Modal {
 
   deactivate() {
     activeModals = activeModals.filter(modal => modal !== this.element);
+    this.currentFocus = null;
     document.removeEventListener('focusin', this.handleFocusIn);
     document.removeEventListener('keydown', this.handleKeyDown);
     document.removeEventListener('keyup', this.handleKeyUp);
@@ -34,11 +36,14 @@ export default class Modal {
 
   checkFocus() {
     if (this.isActive()) {
+      const tabbableElements = getTabbableElements(this.element);
       if (!this.element.matches(':focus-within')) {
-        const { start, end } = getTabbableBoundary(this.element);
+        const start = tabbableElements[0];
+        const end = tabbableElements[tabbableElements.length - 1];
         const target = this.tabDirection === 'forward' ? start : end;
 
         if (typeof target?.focus === 'function') {
+          this.currentFocus = target;
           target.focus({ preventScroll: true });
         }
       }
@@ -49,13 +54,45 @@ export default class Modal {
     this.checkFocus();
   }
 
-  handleKeyDown(event: KeyboardEvent) {
+  get currentFocusIndex() {
-    if (event.key === 'Tab' && event.shiftKey) {
+    return getTabbableElements(this.element).findIndex(el => el === this.currentFocus);
-      this.tabDirection = 'backward';
+  }
 
-      // Ensure focus remains trapped after the key is pressed
+  handleKeyDown(event: KeyboardEvent) {
-      requestAnimationFrame(() => this.checkFocus());
+    if (event.key !== 'Tab') return;
+
+    if (event.shiftKey) {
+      this.tabDirection = 'backward';
+    } else {
+      this.tabDirection = 'forward';
     }
+
+    event.preventDefault();
+
+    const tabbableElements = getTabbableElements(this.element);
+    const start = tabbableElements[0];
+    let focusIndex = this.currentFocusIndex;
+
+    if (focusIndex === -1) {
+      this.currentFocus = start;
+      this.currentFocus.focus({ preventScroll: true });
+      return;
+    }
+
+    const addition = this.tabDirection === 'forward' ? 1 : -1;
+
+    if (focusIndex + addition >= tabbableElements.length) {
+      focusIndex = 0;
+    } else if (this.currentFocusIndex + addition < 0) {
+      focusIndex = tabbableElements.length - 1;
+    } else {
+      focusIndex += addition;
+    }
+
+    this.currentFocus = tabbableElements[focusIndex];
+    this.currentFocus?.focus({ preventScroll: true });
+
+    setTimeout(() => this.checkFocus());
   }
 
   handleKeyUp() {

src/internal/tabbable.ts

@@ -1,3 +1,5 @@
+import { offsetParent } from 'composed-offset-position';
+
 /** Determines if the specified element is tabbable using heuristics inspired by https://github.com/focus-trap/tabbable */
 function isTabbable(el: HTMLElement) {
   const tag = el.tagName.toLowerCase();
@@ -23,7 +25,8 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  if (el.offsetParent === null) {
+  // offsetParent() is added because otherwise it misses elements in Safari
+  if (el.offsetParent === null && offsetParent(el) === null) {
     return false;
   }
 
@@ -56,10 +59,20 @@ function isTabbable(el: HTMLElement) {
  * element because it short-circuits after finding the first and last ones.
  */
 export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
+  const tabbableElements = getTabbableElements(root);
+
+  // Find the first and last tabbable elements
+  const start = tabbableElements[0] ?? null;
+  const end = tabbableElements[tabbableElements.length - 1] ?? null;
+
+  return { start, end };
+}
+
+export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   const allElements: HTMLElement[] = [];
 
   function walk(el: HTMLElement | ShadowRoot) {
-    if (el instanceof HTMLElement) {
+    if (el instanceof Element) {
       allElements.push(el);
 
       if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') {
@@ -73,9 +86,10 @@ export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
   // Collect all elements including the root
   walk(root);
 
-  // Find the first and last tabbable elements
+  return allElements.filter(isTabbable).sort((a, b) => {
-  const start = allElements.find(el => isTabbable(el)) ?? null;
+    // Make sure we sort by tabindex.
-  const end = allElements.reverse().find(el => isTabbable(el)) ?? null;
+    const aTabindex = Number(a.getAttribute('tabindex')) || 0;
-
+    const bTabindex = Number(b.getAttribute('tabindex')) || 0;
-  return { start, end };
+    return bTabindex - aTabindex;
+  });
 }