fix dialog focus trapping behavior (#1813)

* fix dialog focus trapping behavior

* add changelog entry

* prettier

* remove duplicate 'disabled' check in tabbable

* fix dialog stuff

* prettier

* fix logic around checking active elements

* prettier

* prettier

* remove cusrtom-elements.mjs

---------

Co-authored-by: Cory LaViska <cory@abeautifulsite.net>

docs/pages/resources/changelog.md

@@ -23,6 +23,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 - Fixed a bug in `<sl-input>` and `<sl-textarea>` that made it work differently from `<input>` and `<textarea>` when using defaults [#1746]
 - Fixed a bug in `<sl-select>` that prevented it from closing when tabbing to another select inside a shadow root [#1763]
 - Fixed a bug in `<sl-spinner>` that caused the animation to appear strange in certain circumstances [#1787]
+- Fixed a bug in `<sl-dialog>` with focus trapping [#1813]
 - Fixed a bug that caused form controls to submit even after they were removed from the DOM [#1823]
 - Fixed a bug that caused empty `<sl-radio-group>` elements to log an error in the console [#1795]
 - Fixed a bug that caused modal scroll locking to conflict with the `scrollbar-gutter` property [#1805]

src/components/dialog/dialog.test.ts

@@ -211,7 +211,7 @@ describe('<sl-dialog>', () => {
     // Opens modal.
     const openModalButton = container.shadowRoot?.querySelector('sl-button');
 
-    if (openModalButton) openModalButton.click();
+    openModalButton!.click();
 
     // Test tab cycling
     await pressTab();

src/components/select/select.component.ts

@@ -409,6 +409,10 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon
   }
 
   private handleComboboxKeyDown(event: KeyboardEvent) {
+    if (event.key === 'Tab') {
+      return;
+    }
+
     event.stopPropagation();
     this.handleDocumentKeyDown(event);
   }

src/internal/modal.ts

@@ -1,4 +1,4 @@
-import { getDeepestActiveElement } from './active-elements.js';
+import { activeElements, getDeepestActiveElement } from './active-elements.js';
 import { getTabbableElements } from './tabbable.js';
 
 let activeModals: HTMLElement[] = [];
@@ -104,49 +104,44 @@ export default class Modal {
 
     this.previousFocus = this.currentFocus;
 
-    if (currentFocusIndex === -1) {
-      this.currentFocus = tabbableElements[0];
+    const addition = this.tabDirection === 'forward' ? 1 : -1;
 
-      // We don't call event.preventDefault() here because it messes with tabbing to the <iframe> controls.
-      // We just wait until the current focus is no longer an element with possible hidden controls.
-      if (Boolean(this.previousFocus) && this.possiblyHasTabbableChildren(this.previousFocus!)) {
+    // eslint-disable-next-line
+    while (true) {
+      if (currentFocusIndex + addition >= tabbableElements.length) {
+        currentFocusIndex = 0;
+      } else if (currentFocusIndex + addition < 0) {
+        currentFocusIndex = tabbableElements.length - 1;
+      } else {
+        currentFocusIndex += addition;
+      }
+
+      this.previousFocus = this.currentFocus;
+      const nextFocus = /** @type {HTMLElement} */ tabbableElements[currentFocusIndex];
+
+      // This is a special case. We need to make sure we're not calling .focus() if we're already focused on an element
+      // that possibly has "controls"
+      if (this.tabDirection === 'backward') {
+        if (this.previousFocus && this.possiblyHasTabbableChildren(this.previousFocus)) {
+          return;
+        }
+      }
+
+      if (nextFocus && this.possiblyHasTabbableChildren(nextFocus)) {
         return;
       }
 
       event.preventDefault();
+      this.currentFocus = nextFocus;
       this.currentFocus?.focus({ preventScroll: false });
-      return;
-    }
 
-    const addition = this.tabDirection === 'forward' ? 1 : -1;
-
-    if (currentFocusIndex + addition >= tabbableElements.length) {
-      currentFocusIndex = 0;
-    } else if (currentFocusIndex + addition < 0) {
-      currentFocusIndex = tabbableElements.length - 1;
-    } else {
-      currentFocusIndex += addition;
-    }
-
-    this.previousFocus = this.currentFocus;
-    const nextFocus = /** @type {HTMLElement} */ tabbableElements[currentFocusIndex];
-
-    // This is a special case. We need to make sure we're not calling .focus() if we're already focused on an element
-    // that possibly has "controls"
-    if (this.tabDirection === 'backward') {
-      if (this.previousFocus && this.possiblyHasTabbableChildren(this.previousFocus)) {
-        return;
+      // Check to make sure focus actually changed. It may not always be the next focus, we just don't want it to be the previousFocus.
+      const allActiveElements = [...activeElements()];
+      if (allActiveElements.includes(this.currentFocus) || !allActiveElements.includes(this.previousFocus!)) {
+        break;
       }
     }
 
-    if (nextFocus && this.possiblyHasTabbableChildren(nextFocus)) {
-      return;
-    }
-
-    event.preventDefault();
-    this.currentFocus = nextFocus;
-    this.currentFocus?.focus({ preventScroll: true });
-
     setTimeout(() => this.checkFocus());
   };
 

src/internal/tabbable.ts

@@ -2,6 +2,17 @@
 // computedStyle calls are "live" so they only need to be retrieved once for an element.
 const computedStyleMap = new WeakMap<Element, CSSStyleDeclaration>();
 
+function getCachedComputedStyle(el: HTMLElement): CSSStyleDeclaration {
+  let computedStyle: undefined | CSSStyleDeclaration = computedStyleMap.get(el);
+
+  if (!computedStyle) {
+    computedStyle = window.getComputedStyle(el, null);
+    computedStyleMap.set(el, computedStyle);
+  }
+
+  return computedStyle;
+}
+
 function isVisible(el: HTMLElement): boolean {
   // This is the fastest check, but isn't supported in Safari.
   if (typeof el.checkVisibility === 'function') {
@@ -10,16 +21,43 @@ function isVisible(el: HTMLElement): boolean {
   }
 
   // Fallback "polyfill" for "checkVisibility"
-  let computedStyle: undefined | CSSStyleDeclaration = computedStyleMap.get(el);
-
-  if (!computedStyle) {
-    computedStyle = window.getComputedStyle(el, null);
-    computedStyleMap.set(el, computedStyle);
-  }
+  const computedStyle = getCachedComputedStyle(el);
 
   return computedStyle.visibility !== 'hidden' && computedStyle.display !== 'none';
 }
 
+// While this behavior isn't standard in Safari / Chrome yet, I think it's the most reasonable
+// way of handling tabbable overflow areas. Browser sniffing seems gross, and it's the most
+// accessible way of handling overflow areas. [Konnor]
+function isOverflowingAndTabbable(el: HTMLElement): boolean {
+  const computedStyle = getCachedComputedStyle(el);
+
+  const { overflowY, overflowX } = computedStyle;
+
+  if (overflowY === 'scroll' || overflowX === 'scroll') {
+    return true;
+  }
+
+  if (overflowY !== 'auto' || overflowX !== 'auto') {
+    return false;
+  }
+
+  // Always overflow === "auto" by this point
+  const isOverflowingY = el.scrollHeight > el.clientHeight;
+
+  if (isOverflowingY && overflowY === 'auto') {
+    return true;
+  }
+
+  const isOverflowingX = el.scrollWidth > el.clientWidth;
+
+  if (isOverflowingX && overflowX === 'auto') {
+    return true;
+  }
+
+  return false;
+}
+
 /** 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();
@@ -42,11 +80,6 @@ function isTabbable(el: HTMLElement) {
     return false;
   }
 
-  // Elements with a disabled attribute are not tabbable
-  if (el.hasAttribute('disabled')) {
-    return false;
-  }
-
   // Radios without a checked attribute are not tabbable
   if (tag === 'input' && el.getAttribute('type') === 'radio' && !el.hasAttribute('checked')) {
     return false;
@@ -72,7 +105,24 @@ function isTabbable(el: HTMLElement) {
   }
 
   // At this point, the following elements are considered tabbable
-  return ['button', 'input', 'select', 'textarea', 'a', 'audio', 'video', 'summary', 'iframe'].includes(tag);
+  const isNativelyTabbable = [
+    'button',
+    'input',
+    'select',
+    'textarea',
+    'a',
+    'audio',
+    'video',
+    'summary',
+    'iframe'
+  ].includes(tag);
+
+  if (isNativelyTabbable) {
+    return true;
+  }
+
+  // We save the overflow checks for last, because they're the most expensive
+  return isOverflowingAndTabbable(el);
 }
 
 /**