fix focus trapping to respect the currently focused element

docs/pages/resources/changelog.md

@@ -15,6 +15,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 ## Next
 
 - Fixed a bug [in the localize dependency](https://github.com/shoelace-style/localize/issues/20) that caused underscores in language codes to throw a `RangeError`
+- Fixed a bug in the focus trapping utility used by modals that caused unexpected focus behavior. [#1583]
 - Fixed a bug in `<sl-copy-button>` that prevented exported tooltip parts from being styled [#1586]
 - Updated `@shoelace-style/localize` to 3.1.0
 

src/internal/active-elements.ts

@@ -20,3 +20,7 @@ export function* activeElements(activeElement: Element | null = document.activeE
     yield* activeElements(activeElement.shadowRoot.activeElement);
   }
 }
+
+export function getDeepestActiveElement() {
+  return [...activeElements()].pop();
+}

src/internal/modal.ts

@@ -1,4 +1,4 @@
-import { activeElements } from './active-elements.js';
+import { getDeepestActiveElement } from './active-elements.js';
 import { getTabbableElements } from './tabbable.js';
 
 let activeModals: HTMLElement[] = [];
@@ -66,22 +66,6 @@ export default class Modal {
     this.checkFocus();
   };
 
-  get currentFocusIndex() {
-    return getTabbableElements(this.element).findIndex(el => el === this.currentFocus);
-  }
-
-  // Checks if the `startElement` is already focused. This is important if the modal already has an existing focus prior
-  // to the first tab key.
-  private startElementAlreadyFocused(startElement: HTMLElement) {
-    for (const activeElement of activeElements()) {
-      if (startElement === activeElement) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
   private handleKeyDown = (event: KeyboardEvent) => {
     if (event.key !== 'Tab' || this.isExternalActivated) return;
 
@@ -94,29 +78,30 @@ export default class Modal {
     event.preventDefault();
 
     const tabbableElements = getTabbableElements(this.element);
-    const start = tabbableElements[0];
 
-    // Sometimes we programmatically focus the first element in a modal.
-    // Lets make sure the start element isn't already focused.
-    let focusIndex = this.startElementAlreadyFocused(start) ? 0 : this.currentFocusIndex;
+    // Because sometimes focus can actually be taken over from outside sources,
+    // we don't want to rely on `this.currentFocus`. Instead we check the actual `activeElement` and
+    // recurse through shadowRoots.
+    const currentActiveElement = getDeepestActiveElement();
+    let currentFocusIndex = tabbableElements.findIndex(el => el === currentActiveElement);
 
-    if (focusIndex === -1) {
-      this.currentFocus = start;
+    if (currentFocusIndex === -1) {
+      this.currentFocus = tabbableElements[0];
       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;
+    if (currentFocusIndex + addition >= tabbableElements.length) {
+      currentFocusIndex = 0;
+    } else if (currentFocusIndex + addition < 0) {
+      currentFocusIndex = tabbableElements.length - 1;
     } else {
-      focusIndex += addition;
+      currentFocusIndex += addition;
     }
 
-    this.currentFocus = tabbableElements[focusIndex];
+    this.currentFocus = tabbableElements[currentFocusIndex];
     this.currentFocus?.focus({ preventScroll: true });
 
     setTimeout(() => this.checkFocus());

src/internal/tabbable.test.ts

@@ -1,7 +1,7 @@
 import { elementUpdated, expect, fixture } from '@open-wc/testing';
 
 import '../../dist/shoelace.js';
-import { activeElements } from './active-elements.js';
+import { activeElements, getDeepestActiveElement } from './active-elements.js';
 import { html } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 
@@ -19,10 +19,6 @@ function activeElementsArray() {
   return [...activeElements()];
 }
 
-function getDeepestActiveElement() {
-  return activeElementsArray().pop();
-}
-
 window.customElements.define(
   'tab-test-1',
   class extends HTMLElement {
@@ -145,3 +141,36 @@ it('Should allow tabbing to slotted elements', async () => {
   await holdShiftKey(async () => await sendKeys({ press: tabKey }));
   expect(activeElementsArray()).to.include(focusSix);
 });
+
+it('Should account for when focus is changed from outside sources (like clicking)', async () => {
+  const dialog = await fixture(html`
+    <sl-dialog open="" label="Dialog" class="dialog-overview">
+      Lorem ipsum dolor sit amet, consectetur adipiscing elit.
+      <sl-input placeholder="tab to me"></sl-input>
+      <sl-button slot="footer" variant="primary">Close</sl-button>
+    </sl-dialog>
+  `);
+
+  const inputEl = dialog.querySelector('sl-input')!;
+  const closeButton = dialog.shadowRoot!.querySelector('sl-icon-button')!;
+  const footerButton = dialog.querySelector('sl-button')!;
+
+  expect(activeElementsArray()).to.not.include(inputEl);
+
+  // Sets focus to the input element
+  inputEl.focus();
+
+  expect(activeElementsArray()).to.include(inputEl);
+
+  await sendKeys({ press: tabKey });
+
+  expect(activeElementsArray()).not.to.include(inputEl);
+  expect(activeElementsArray()).to.include(footerButton);
+
+  // Reset focus back to input el
+  inputEl.focus();
+  expect(activeElementsArray()).to.include(inputEl);
+
+  await holdShiftKey(async () => await sendKeys({ press: tabKey }));
+  expect(activeElementsArray()).to.include(closeButton);
+});

src/internal/tabbable.ts

@@ -14,11 +14,6 @@ function isTabbable(el: HTMLElement) {
     return false;
   }
 
-  // Elements with aria-disabled are not tabbable
-  if (el.hasAttribute('aria-disabled') && el.getAttribute('aria-disabled') !== 'false') {
-    return false;
-  }
-
   // Radios without a checked attribute are not tabbable
   if (tag === 'input' && el.getAttribute('type') === 'radio' && !el.hasAttribute('checked')) {
     return false;
@@ -107,14 +102,12 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   // Collect all elements including the root
   walk(root);
 
-  return tabbableElements;
-
   // Is this worth having? Most sorts will always add increased overhead. And positive tabindexes shouldn't really be used.
   // So is it worth being right? Or fast?
-  // return tabbableElements.filter(isTabbable).sort((a, b) => {
-  //   // Make sure we sort by tabindex.
-  //   const aTabindex = Number(a.getAttribute('tabindex')) || 0;
-  //   const bTabindex = Number(b.getAttribute('tabindex')) || 0;
-  //   return bTabindex - aTabindex;
-  // });
+  return tabbableElements.sort((a, b) => {
+    // Make sure we sort by tabindex.
+    const aTabindex = Number(a.getAttribute('tabindex')) || 0;
+    const bTabindex = Number(b.getAttribute('tabindex')) || 0;
+    return bTabindex - aTabindex;
+  });
 }