From 773255881b925ca260ca5eefce817e8d8de17bf2 Mon Sep 17 00:00:00 2001
From: Konnor Rogers <konnor5456@gmail.com>
Date: Tue, 23 Jan 2024 10:45:20 -0500
Subject: [PATCH] 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         |  1 +
 src/components/dialog/dialog.test.ts      |  2 +-
 src/components/select/select.component.ts |  4 ++
 src/internal/modal.ts                     | 63 +++++++++-----------
 src/internal/tabbable.ts                  | 72 +++++++++++++++++++----
 5 files changed, 96 insertions(+), 46 deletions(-)

diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
index 0b88b2a08f..cb075a4c34 100644
--- a/docs/pages/resources/changelog.md
+++ b/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]
diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts
index 2ed4837292..277ac830a8 100644
--- a/src/components/dialog/dialog.test.ts
+++ b/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();
diff --git a/src/components/select/select.component.ts b/src/components/select/select.component.ts
index 14c830d384..979f0ca8e3 100644
--- a/src/components/select/select.component.ts
+++ b/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);
   }
diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index cefd2bf060..c3463d954d 100644
--- a/src/internal/modal.ts
+++ b/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,48 +104,43 @@ 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!)) {
-        return;
+    // 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;
       }
 
-      event.preventDefault();
-      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.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;
+        }
+      }
 
-    // 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)) {
+      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 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;
+      }
+    }
 
     setTimeout(() => this.checkFocus());
   };
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index fd57ed3484..d96ad70d6b 100644
--- a/src/internal/tabbable.ts
+++ b/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,14 +21,41 @@ function isVisible(el: HTMLElement): boolean {
   }
 
   // Fallback "polyfill" for "checkVisibility"
-  let computedStyle: undefined | CSSStyleDeclaration = computedStyleMap.get(el);
+  const computedStyle = getCachedComputedStyle(el);
 
-  if (!computedStyle) {
-    computedStyle = window.getComputedStyle(el, null);
-    computedStyleMap.set(el, computedStyle);
+  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;
   }
 
-  return computedStyle.visibility !== 'hidden' && computedStyle.display !== 'none';
+  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 */
@@ -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);
 }
 
 /**