Account for elements with tabbable controls (#1755)

* account for elements with tabbable controls

* prettier

* add changelog entry

* prettier

docs/pages/resources/changelog.md

@@ -14,6 +14,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 ## Next
 
+- Fixed `<sl-dialog>` not accounting for elements with hidden dialog controls like `<video>` [#1755]
 - Added the `loading` attribute and the `spinner` and `spinner__base` part to `<sl-menu-item>` [#1700]
 - Fixed focus trapping not scrolling elements into view. [#1750]
 - Fixed more performance issues with focus trapping performance. [#1750]

src/components/dialog/dialog.component.ts

@@ -300,9 +300,9 @@ 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. */
+            '' /* 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 */
           }
-          <slot part="body" class="dialog__body" tabindex="-1"></slot>
+          <div part="body" class="dialog__body" tabindex="-1"><slot></slot></div>
 
           <footer part="footer" class="dialog__footer">
             <slot name="footer"></slot>

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,29 +71,50 @@ 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;
 
+    // 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;
+
+    if (this.previousFocus && this.possiblyHasTabbableChildren(this.previousFocus)) {
+      return;
+    }
+
     if (event.shiftKey) {
       this.tabDirection = 'backward';
     } else {
       this.tabDirection = 'forward';
     }
 
-    event.preventDefault();
-
     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];
-      this.currentFocus?.focus({ preventScroll: true });
+
+      // 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;
+      }
+
+      event.preventDefault();
+      this.currentFocus?.focus({ preventScroll: false });
       return;
     }
 
@@ -103,7 +128,23 @@ export default class Modal {
       currentFocusIndex += addition;
     }
 
-    this.currentFocus = 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;
+      }
+    }
+
+    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;
   }
 
@@ -58,7 +72,7 @@ function isTabbable(el: HTMLElement) {
   }
 
   // At this point, the following elements are considered tabbable
-  return ['button', 'input', 'select', 'textarea', 'a', 'audio', 'video', 'summary'].includes(tag);
+  return ['button', 'input', 'select', 'textarea', 'a', 'audio', 'video', 'summary', 'iframe'].includes(tag);
 }
 
 /**
@@ -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;
       }