prettier

src/components/dialog/dialog.component.ts

@@ -300,7 +300,7 @@ export default class SlDialog extends ShoelaceElement {
               `
             : ''}
           ${
-          '' /* 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. Previously this was just a <slot>, but tabindex="-1" on the slot causes children to not be focusable. https://github.com/shoelace-style/shoelace/issues/1753#issuecomment-1836803277 */
+            '' /* 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. Previously this was just a <slot>, but tabindex="-1" on the slot causes children to not be focusable. https://github.com/shoelace-style/shoelace/issues/1753#issuecomment-1836803277 */
           }
           <div part="body" class="dialog__body" tabindex="-1"><slot></slot></div>
 

src/internal/modal.ts

@@ -8,9 +8,13 @@ export default class Modal {
   isExternalActivated: boolean;
   tabDirection: 'forward' | 'backward' = 'forward';
   currentFocus: HTMLElement | null;
+  previousFocus: HTMLElement | null;
+  elementsWithTabbableControls: string[];
 
   constructor(element: HTMLElement) {
     this.element = element;
+
+    this.elementsWithTabbableControls = ['iframe'];
   }
 
   /** Activates focus trapping. */
@@ -56,7 +60,7 @@ export default class Modal {
 
         if (typeof target?.focus === 'function') {
           this.currentFocus = target;
-          target.focus({ preventScroll: true });
+          target.focus({ preventScroll: false });
         }
       }
     }
@@ -67,22 +71,25 @@ export default class Modal {
     this.checkFocus();
   };
 
+  private possiblyHasTabbableChildren(element: HTMLElement) {
+    return (
+      this.elementsWithTabbableControls.includes(element.tagName.toLowerCase()) || element.hasAttribute('controls')
+      // Should we add a data-attribute for people to set just in case they have an element where we don't know if it has possibly tabbable elements?
+    );
+  }
+
   private handleKeyDown = (event: KeyboardEvent) => {
     if (event.key !== 'Tab' || this.isExternalActivated) return;
     if (!this.isActive()) return;
 
-    const elementsWithTabbableControls = [
-      "audio",
-      "video",
-      "iframe"
-    ]
+    // 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();
+    this.previousFocus = currentActiveElement as HTMLElement | null;
 
-    const possiblyHasTabbableChildren = (element: HTMLElement) => {
-      return (
-        elementsWithTabbableControls.includes(element.tagName.toLowerCase())
-        || element.hasAttribute("controls")
-        // Should we add a data-attribute for people to set just in case they have an element where we don't know if it has possibly tabbable elements?
-      )
+    if (this.previousFocus && this.possiblyHasTabbableChildren(this.previousFocus)) {
+      return;
     }
 
     if (event.shiftKey) {
@@ -93,23 +100,21 @@ export default class Modal {
 
     const tabbableElements = getTabbableElements(this.element);
 
-    // 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);
 
+    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 (possiblyHasTabbableChildren(this.currentFocus)) {
-        return
+      if (Boolean(this.previousFocus) && this.possiblyHasTabbableChildren(this.previousFocus!)) {
+        return;
       }
 
       event.preventDefault();
-      this.currentFocus?.focus({ preventScroll: true });
+      this.currentFocus?.focus({ preventScroll: false });
       return;
     }
 
@@ -123,15 +128,23 @@ export default class Modal {
       currentFocusIndex += addition;
     }
 
-    this.currentFocus = tabbableElements[currentFocusIndex];
+    this.previousFocus = this.currentFocus;
+    const nextFocus = /** @type {HTMLElement} */ tabbableElements[currentFocusIndex];
 
-    // 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 (possiblyHasTabbableChildren(this.currentFocus)) {
-      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)) {
+        return;
+      }
     }
 
-    event.preventDefault()
+    if (nextFocus && this.possiblyHasTabbableChildren(nextFocus)) {
+      return;
+    }
+
+    event.preventDefault();
+    this.currentFocus = nextFocus;
     this.currentFocus?.focus({ preventScroll: true });
 
     setTimeout(() => this.checkFocus());

src/internal/tabbable.ts

@@ -5,7 +5,8 @@ const computedStyleMap = new WeakMap<Element, CSSStyleDeclaration>();
 function isVisible(el: HTMLElement): boolean {
   // This is the fastest check, but isn't supported in Safari.
   if (typeof el.checkVisibility === 'function') {
-    return el.checkVisibility({ checkOpacity: false });
+    // Opacity is focusable, visibility is not.
+    return el.checkVisibility({ checkOpacity: false, checkVisibilityCSS: true });
   }
 
   // Fallback "polyfill" for "checkVisibility"
@@ -23,8 +24,21 @@ function isVisible(el: HTMLElement): boolean {
 function isTabbable(el: HTMLElement) {
   const tag = el.tagName.toLowerCase();
 
-  // Elements with a -1 tab index are not tabbable
-  if (el.getAttribute('tabindex') === '-1') {
+  const tabindex = Number(el.getAttribute('tabindex'));
+  const hasTabindex = el.hasAttribute('tabindex');
+
+  // elements with a tabindex attribute that is either NaN or <= -1 are not tabbable
+  if (hasTabindex && (isNaN(tabindex) || tabindex <= -1)) {
+    return false;
+  }
+
+  // Elements with a disabled attribute are not tabbable
+  if (el.hasAttribute('disabled')) {
+    return false;
+  }
+
+  // If any parents have "inert", we aren't "tabbable"
+  if (el.closest('[inert]')) {
     return false;
   }
 
@@ -91,7 +105,7 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   function walk(el: HTMLElement | ShadowRoot) {
     if (el instanceof Element) {
       // if the element has "inert" we can just no-op it.
-      if (el.hasAttribute('inert')) {
+      if (el.hasAttribute('inert') || el.closest('[inert]')) {
         return;
       }