fix: internal logic for tabbable checks slotted elements

src/internal/active-elements.ts

@@ -0,0 +1,22 @@
+/**
+ * Use a generator so we can iterate and possibly break early.
+ * @example
+ *   // to operate like a regular array. This gets kinda nullify generator benefits.
+ *   const allActiveElements = [...activeElements()]
+ *
+ *   // Earlier return
+ *   for (const activeElement of activeElements()) {
+ *     if (<cond>) {
+ *       break; // Break the loop, dont need to iterate over the whole array or store an array in memory!
+ *     }
+ *   }
+ */
+export function* activeElements (activeElement: Element | null = document.activeElement): Generator<Element> {
+  if (activeElement == null) return
+
+  yield activeElement
+
+  if ("shadowRoot" in activeElement && activeElement.shadowRoot && activeElement.shadowRoot.mode !== "closed") {
+    yield* activeElements(activeElement.shadowRoot.activeElement)
+  }
+}

src/internal/modal.ts

@@ -1,4 +1,5 @@
 import { getTabbableElements } from './tabbable.js';
+import { activeElements } from './active-elements.js'
 
 let activeModals: HTMLElement[] = [];
 
@@ -55,6 +56,21 @@ export default class Modal {
     return getTabbableElements(this.element).findIndex(el => el === this.currentFocus);
   }
 
+  /**
+   * Checks if the `startElement` is already focused. This is important if the modal already
+   *   has an existing focused prior to the first tab key.
+   */
+  startElementAlreadyFocused (startElement: HTMLElement) {
+    for (const activeElement of activeElements()) {
+      if (startElement === activeElement) {
+      //
+        return true
+      }
+    }
+
+    return false
+  }
+
   handleKeyDown = (event: KeyboardEvent) => {
     if (event.key !== 'Tab') return;
 
@@ -68,7 +84,10 @@ export default class Modal {
 
     const tabbableElements = getTabbableElements(this.element);
     const start = tabbableElements[0];
-    let focusIndex = this.currentFocusIndex;
+
+    // Sometimes we programmatically focus the first element in a modal.
+    // Lets make sure the start element isn't already focused.
+    let focusIndex = this.startElementAlreadyFocused(start) ? 0 : this.currentFocusIndex;
 
     if (focusIndex === -1) {
       this.currentFocus = start;

src/internal/tabbable.test.ts

@@ -0,0 +1,148 @@
+import { elementUpdated, expect, fixture, aTimeout } from '@open-wc/testing';
+
+import "../../dist/shoelace.js"
+import { html } from 'lit';
+import type { SlDrawer } from '../../dist/shoelace.js';
+import { sendKeys } from '@web/test-runner-commands';
+
+function getActiveElements (el: null | Element = document.activeElement) {
+  const elements: Element[] = []
+
+  function walk (el: null | Element) {
+    if (el == null) {
+      return
+    }
+
+    elements.push(el)
+
+    if ("shadowRoot" in el && el.shadowRoot && el.shadowRoot.mode !== "closed") {
+      walk(el.shadowRoot.activeElement)
+    }
+  }
+
+  walk(el)
+
+  return elements
+}
+
+function getDeepestActiveElement (el: null | Element = document.activeElement) {
+  const activeElements = getActiveElements(el)
+
+  return activeElements[activeElements.length - 1]
+}
+
+async function holdShiftKey (callback: () => Promise<void>) {
+  await sendKeys({ down: "Shift" })
+  await callback()
+  await sendKeys({ up: "Shift" })
+}
+
+window.customElements.define("tab-test-1", class extends HTMLElement {
+  connectedCallback () {
+    this.attachShadow({ mode: "open" })
+    this.shadowRoot!.innerHTML = `
+      <sl-drawer>
+        <slot name="label" slot="label"></slot>
+
+        <slot></slot>
+
+        <slot name="footer" slot="footer"></slot>
+      </sl-drawer>
+    `
+  }
+})
+
+
+it("Should allow tabbing to slotted elements", async () => {
+  const el = await fixture(html`
+    <tab-test-1>
+      <div slot="label">
+        <sl-button id="focus-1">Focus 1</sl-button>
+      </div>
+
+      <div>
+        <!-- Focus 2 lives as the close-button from <sl-drawer> -->
+        <sl-button id="focus-3">Focus 3</sl-button>
+        <button id="focus-4">Focus 4</sl-button>
+        <input id="focus-5" value="Focus 5">
+      </div>
+
+      <div slot="footer">
+        <div id="focus-6" tabindex="0">Focus 6</div>
+        <button tabindex="-1">No Focus</button>
+      </div>
+    </tab-test-1>
+  `)
+
+  const drawer = el.shadowRoot!.querySelector("sl-drawer") as unknown as SlDrawer
+
+  await drawer.show()
+
+  await elementUpdated(drawer)
+
+  const focusZero = drawer.shadowRoot!.querySelector("[role='dialog']")
+  const focusOne = el.querySelector("#focus-1")
+  const focusTwo = drawer.shadowRoot!.querySelector("[part~='close-button']")
+  const focusThree = el.querySelector("#focus-3")
+  const focusFour = el.querySelector("#focus-4")
+  const focusFive = el.querySelector("#focus-5")
+  const focusSix = el.querySelector("#focus-6")
+
+  // When we open drawer, we should be focused on the panel to start.
+  expect(getDeepestActiveElement()).to.equal(focusZero)
+
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusOne)
+
+  // When we hit the <Tab> key we should go to the "close button" on the drawer
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusTwo)
+
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusThree)
+
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusFour)
+
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusFive)
+
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusSix)
+
+  // Now we should loop back to #panel
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusZero)
+
+  // Now we should loop back to #panel
+  await sendKeys({ press: "Tab" })
+  expect(getActiveElements()).to.include(focusOne)
+
+  // Let's reset and try from starting point 0 and go backwards.
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusZero)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusSix)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusFive)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusFour)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusThree)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusTwo)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusOne)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusZero)
+
+  await holdShiftKey(async () => await sendKeys({ press: "Tab" }))
+  expect(getActiveElements()).to.include(focusSix)
+})

src/internal/tabbable.ts

@@ -70,10 +70,35 @@ export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
 
 export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   const allElements: HTMLElement[] = [];
+  const tabbableElements: HTMLElement[] = [];
 
   function walk(el: HTMLElement | ShadowRoot) {
     if (el instanceof Element) {
-      allElements.push(el);
+      // if the element has "inert" we can just no-op it.
+      if (el.hasAttribute("inert")) {
+        return
+      }
+
+      if (!allElements.includes(el)) {
+        allElements.push(el);
+      }
+
+      if (!tabbableElements.includes(el) && isTabbable(el)) {
+        tabbableElements.push(el)
+      }
+
+      /**
+       * This looks funky. Basically a slots children will always be picked up *if* they're within the `root` element.
+       * However, there is an edge case if the `root` is wrapped by another shadowDOM, it won't grab the children.
+       * This fixes that fun edge case.
+       */
+      const slotElementsOutsideRootElement = (el: HTMLSlotElement) => (el.getRootNode({ composed: true }) as ShadowRoot | null)?.host !== root
+
+      if (el instanceof HTMLSlotElement && slotElementsOutsideRootElement(el)) {
+        el.assignedElements({ flatten: true }).forEach((el: HTMLElement) => {
+          walk(el)
+        })
+      }
 
       if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') {
         walk(el.shadowRoot);
@@ -86,10 +111,14 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   // Collect all elements including the root
   walk(root);
 
-  return allElements.filter(isTabbable).sort((a, b) => {
-    // Make sure we sort by tabindex.
-    const aTabindex = Number(a.getAttribute('tabindex')) || 0;
-    const bTabindex = Number(b.getAttribute('tabindex')) || 0;
-    return bTabindex - aTabindex;
-  });
+  return tabbableElements
+
+  // Is this worth having? Most sorts will always add increased overhead. And positive tabindexes shouldn't really be used.
+  // So is it worth being right? Or fast?
+  // return allElements.filter(isTabbable).sort((a, b) => {
+  //   // Make sure we sort by tabindex.
+  //   const aTabindex = Number(a.getAttribute('tabindex')) || 0;
+  //   const bTabindex = Number(b.getAttribute('tabindex')) || 0;
+  //   return bTabindex - aTabindex;
+  // });
 }