fix dialog focus trapping behavior

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 [#]
 - Improved the accessibility of `<sl-tooltip>` so they persist when hovering over the tooltip and dismiss when pressing [[Esc]] [#1734]
 
 ## 2.12.0
@@ -1694,4 +1695,4 @@ The following pages demonstrate why this change was necessary.
 
 ## 2.0.0-beta.1
 
-- Initial release
+- Initial release

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[] = [];
@@ -105,8 +105,6 @@ export default class Modal {
     this.previousFocus = this.currentFocus;
 
     if (currentFocusIndex === -1) {
-      this.currentFocus = tabbableElements[0];
-
       // 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!)) {
@@ -114,38 +112,60 @@ export default class Modal {
       }
 
       event.preventDefault();
-      this.currentFocus?.focus({ preventScroll: false });
+      currentFocusIndex = 0
+
+      // Check to make sure we actually focused.
+      while (true) {
+        this.currentFocus = tabbableElements[currentFocusIndex];
+        this.currentFocus?.focus({ preventScroll: false });
+
+        if (currentFocusIndex >= tabbableElements.length) {
+          break
+        }
+
+        // Focusing can fail silently. This prevents us from getting "stuck".
+        if ([...activeElements()].includes(this.currentFocus!) === false) {
+          break
+        }
+
+        currentFocusIndex += 1
+      }
       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;
-    }
+    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.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)) {
+      // 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;
       }
-    }
 
-    if (nextFocus && this.possiblyHasTabbableChildren(nextFocus)) {
-      return;
-    }
+      event.preventDefault();
+      this.currentFocus = nextFocus;
+      this.currentFocus?.focus({ preventScroll: false });
 
-    event.preventDefault();
-    this.currentFocus = nextFocus;
-    this.currentFocus?.focus({ preventScroll: true });
+      // Check to make sure the element we tried to focus actually focused. `.focus()` can fail silently.
+      if ([...activeElements()].includes(this.currentFocus)) { break }
+    }
 
     setTimeout(() => this.checkFocus());
   };
@@ -154,3 +174,4 @@ export default class Modal {
     this.tabDirection = 'forward';
   };
 }
+

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();
@@ -72,7 +110,12 @@ 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)
 }
 
 /**
@@ -146,3 +189,4 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
     return bTabindex - aTabindex;
   });
 }
+