From ed9c635607e796b6e11fea8ba68610624cf74ade Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 23 Jun 2023 15:12:41 -0400
Subject: [PATCH 1/9] add modal tab tracking

---
 src/components/dialog/dialog.test.ts | 41 +++++++++++++++++++++
 src/components/dialog/dialog.ts      |  6 ++-
 src/internal/modal.ts                | 55 ++++++++++++++++++++++++----
 src/internal/tabbable.ts             | 18 ++++++---
 4 files changed, 105 insertions(+), 15 deletions(-)

diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts
index a8d9ff82c2..8e78764687 100644
--- a/src/components/dialog/dialog.test.ts
+++ b/src/components/dialog/dialog.test.ts
@@ -1,5 +1,6 @@
 import '../../../dist/shoelace.js';
 // cspell:dictionaries lorem-ipsum
+import { LitElement } from "lit"
 import { expect, fixture, html, waitUntil } from '@open-wc/testing';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
@@ -146,4 +147,44 @@ describe('<sl-dialog>', () => {
 
     expect(el.open).to.be.false;
   });
+
+  // https://github.com/shoelace-style/shoelace/issues/1382
+  it('should properly cycle through tabbable elements when sl-dialog is used in a shadowRoot', async () => {
+    class AContainer extends LitElement {
+      get dialog () {
+        return this.shadowRoot?.querySelector("sl-dialog")
+      }
+
+      openDialog() {
+        this.dialog?.show();
+      }
+
+      render() {
+        return html`
+          <h1>Dialog Example</h1>
+          <sl-dialog label="Dialog" class="dialog-overview">
+            Lorem ipsum dolor sit amet, consectetur adipiscing elit.
+            <br/>
+            <input type="checkbox">A</input>
+            <input type="checkbox">B</input>
+          </sl-dialog>
+
+          <sl-button @click=${this.openDialog}>Open Dialog</sl-button>
+        `;
+      }
+    }
+
+    window.customElements.define("a-container", AContainer);
+
+    const testCase = await fixture(html`
+      <a-container></a-container>
+
+      <p>
+      Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches things outside the dialog.
+      </p>
+    `)
+
+
+    testCase.querySelector<AContainer>("a-container")?.openDialog()
+  })
 });
diff --git a/src/components/dialog/dialog.ts b/src/components/dialog/dialog.ts
index b483922bff..7af34c3cd9 100644
--- a/src/components/dialog/dialog.ts
+++ b/src/components/dialog/dialog.ts
@@ -104,6 +104,7 @@ export default class SlDialog extends ShoelaceElement {
 
   disconnectedCallback() {
     super.disconnectedCallback();
+    this.modal.deactivate()
     unlockBodyScrolling(this);
   }
 
@@ -269,7 +270,7 @@ export default class SlDialog extends ShoelaceElement {
           aria-hidden=${this.open ? 'false' : 'true'}
           aria-label=${ifDefined(this.noHeader ? this.label : undefined)}
           aria-labelledby=${ifDefined(!this.noHeader ? 'title' : undefined)}
-          tabindex="0"
+          tabindex="-1"
         >
           ${!this.noHeader
             ? html`
@@ -293,7 +294,8 @@ export default class SlDialog extends ShoelaceElement {
               `
             : ''}
 
-          <slot part="body" class="dialog__body"></slot>
+          ${'' /* 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. */}
+          <slot part="body" class="dialog__body" tabindex="-1"></slot>
 
           <footer part="footer" class="dialog__footer">
             <slot name="footer"></slot>
diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index fc8715b9be..2ab849ad62 100644
--- a/src/internal/modal.ts
+++ b/src/internal/modal.ts
@@ -1,10 +1,13 @@
-import { getTabbableBoundary } from './tabbable.js';
+import {
+  getTabbableElements
+} from './tabbable.js';
 
 let activeModals: HTMLElement[] = [];
 
 export default class Modal {
   element: HTMLElement;
   tabDirection: 'forward' | 'backward' = 'forward';
+  currentFocus: HTMLElement | null
 
   constructor(element: HTMLElement) {
     this.element = element;
@@ -22,6 +25,7 @@ export default class Modal {
 
   deactivate() {
     activeModals = activeModals.filter(modal => modal !== this.element);
+    this.currentFocus = null
     document.removeEventListener('focusin', this.handleFocusIn);
     document.removeEventListener('keydown', this.handleKeyDown);
     document.removeEventListener('keyup', this.handleKeyUp);
@@ -34,11 +38,14 @@ export default class Modal {
 
   checkFocus() {
     if (this.isActive()) {
-      if (!this.element.matches(':focus-within')) {
-        const { start, end } = getTabbableBoundary(this.element);
-        const target = this.tabDirection === 'forward' ? start : end;
+      const tabbableElements = getTabbableElements(this.element)
+      if (!this.element.matches(":focus-within")) {
+        const start = tabbableElements[0]
+        const end = tabbableElements[tabbableElements.length - 1]
+        const target = this.tabDirection === "forward" ? start : end
 
         if (typeof target?.focus === 'function') {
+          this.currentFocus = target
           target.focus({ preventScroll: true });
         }
       }
@@ -49,13 +56,47 @@ export default class Modal {
     this.checkFocus();
   }
 
+  get currentFocusIndex () {
+    return getTabbableElements(this.element).findIndex((el) => el === this.currentFocus)
+  }
+
   handleKeyDown(event: KeyboardEvent) {
-    if (event.key === 'Tab' && event.shiftKey) {
+    if (event.key !== "Tab") return
+
+    if (event.shiftKey) {
       this.tabDirection = 'backward';
+    } else {
+      this.tabDirection = 'forward';
+    }
+
+    event.preventDefault()
 
-      // Ensure focus remains trapped after the key is pressed
-      requestAnimationFrame(() => this.checkFocus());
+    const tabbableElements = getTabbableElements(this.element)
+    const start = tabbableElements[0];
+    // const end = tabbableElements[tabbableElements.length - 1]
+    let focusIndex = this.currentFocusIndex
+
+    if (focusIndex === -1) {
+      this.currentFocus = start
+      this.currentFocus.focus({ preventScroll: true })
+      return
     }
+
+    // This looks interesting right?
+    // Well, in Chrome / Edge if you have a dialog inside of a shadow root it will
+    // jump to the "address bar" on Shift+Tab. This prevents that.
+    let addition = this.tabDirection === 'forward' ? 1 : -1;
+
+    if (focusIndex + addition >= tabbableElements.length) {
+      focusIndex = 0
+    } else if (this.currentFocusIndex + addition < 0) {
+      focusIndex = tabbableElements.length - 1
+    } else {
+      focusIndex += addition
+    }
+
+    this.currentFocus = tabbableElements[focusIndex]
+    this.currentFocus?.focus({ preventScroll: true })
   }
 
   handleKeyUp() {
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index 163477d722..2e7be9eaef 100644
--- a/src/internal/tabbable.ts
+++ b/src/internal/tabbable.ts
@@ -23,7 +23,7 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  if (el.offsetParent === null) {
+  if (el.offsetParent == null) {
     return false;
   }
 
@@ -56,6 +56,16 @@ function isTabbable(el: HTMLElement) {
  * element because it short-circuits after finding the first and last ones.
  */
 export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
+  const tabbableElements = getTabbableElements(root)
+
+  // Find the first and last tabbable elements
+  const start = tabbableElements[0] ?? null;
+  const end = tabbableElements[tabbableElements.length - 1] ?? null;
+
+  return { start, end };
+}
+
+export function getTabbableElements (root: HTMLElement | ShadowRoot) {
   const allElements: HTMLElement[] = [];
 
   function walk(el: HTMLElement | ShadowRoot) {
@@ -73,9 +83,5 @@ export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
   // Collect all elements including the root
   walk(root);
 
-  // Find the first and last tabbable elements
-  const start = allElements.find(el => isTabbable(el)) ?? null;
-  const end = allElements.reverse().find(el => isTabbable(el)) ?? null;
-
-  return { start, end };
+  return [...new Set(allElements.filter(isTabbable))]
 }

From f54775053436929efd48cf0fbef0afb62939a066 Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 23 Jun 2023 15:21:33 -0400
Subject: [PATCH 2/9] prettier

---
 src/components/dialog/dialog.test.ts | 18 +++++-----
 src/components/dialog/dialog.ts      |  7 ++--
 src/internal/modal.ts                | 50 +++++++++++++---------------
 src/internal/tabbable.ts             |  8 ++---
 4 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts
index 8e78764687..4a5309d8ac 100644
--- a/src/components/dialog/dialog.test.ts
+++ b/src/components/dialog/dialog.test.ts
@@ -1,7 +1,7 @@
 import '../../../dist/shoelace.js';
 // cspell:dictionaries lorem-ipsum
-import { LitElement } from "lit"
 import { expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { LitElement } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
 import type SlDialog from './dialog';
@@ -151,8 +151,8 @@ describe('<sl-dialog>', () => {
   // https://github.com/shoelace-style/shoelace/issues/1382
   it('should properly cycle through tabbable elements when sl-dialog is used in a shadowRoot', async () => {
     class AContainer extends LitElement {
-      get dialog () {
-        return this.shadowRoot?.querySelector("sl-dialog")
+      get dialog() {
+        return this.shadowRoot?.querySelector('sl-dialog');
       }
 
       openDialog() {
@@ -174,17 +174,17 @@ describe('<sl-dialog>', () => {
       }
     }
 
-    window.customElements.define("a-container", AContainer);
+    window.customElements.define('a-container', AContainer);
 
     const testCase = await fixture(html`
       <a-container></a-container>
 
       <p>
-      Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches things outside the dialog.
+        Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches
+        things outside the dialog.
       </p>
-    `)
-
+    `);
 
-    testCase.querySelector<AContainer>("a-container")?.openDialog()
-  })
+    testCase.querySelector<AContainer>('a-container')?.openDialog();
+  });
 });
diff --git a/src/components/dialog/dialog.ts b/src/components/dialog/dialog.ts
index 7af34c3cd9..efa5672d2c 100644
--- a/src/components/dialog/dialog.ts
+++ b/src/components/dialog/dialog.ts
@@ -104,7 +104,7 @@ export default class SlDialog extends ShoelaceElement {
 
   disconnectedCallback() {
     super.disconnectedCallback();
-    this.modal.deactivate()
+    this.modal.deactivate();
     unlockBodyScrolling(this);
   }
 
@@ -293,8 +293,9 @@ export default class SlDialog extends ShoelaceElement {
                 </header>
               `
             : ''}
-
-          ${'' /* 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. */
+          }
           <slot part="body" class="dialog__body" tabindex="-1"></slot>
 
           <footer part="footer" class="dialog__footer">
diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index 2ab849ad62..4851da2e0e 100644
--- a/src/internal/modal.ts
+++ b/src/internal/modal.ts
@@ -1,13 +1,11 @@
-import {
-  getTabbableElements
-} from './tabbable.js';
+import { getTabbableElements } from './tabbable.js';
 
 let activeModals: HTMLElement[] = [];
 
 export default class Modal {
   element: HTMLElement;
   tabDirection: 'forward' | 'backward' = 'forward';
-  currentFocus: HTMLElement | null
+  currentFocus: HTMLElement | null;
 
   constructor(element: HTMLElement) {
     this.element = element;
@@ -25,7 +23,7 @@ export default class Modal {
 
   deactivate() {
     activeModals = activeModals.filter(modal => modal !== this.element);
-    this.currentFocus = null
+    this.currentFocus = null;
     document.removeEventListener('focusin', this.handleFocusIn);
     document.removeEventListener('keydown', this.handleKeyDown);
     document.removeEventListener('keyup', this.handleKeyUp);
@@ -38,14 +36,14 @@ export default class Modal {
 
   checkFocus() {
     if (this.isActive()) {
-      const tabbableElements = getTabbableElements(this.element)
-      if (!this.element.matches(":focus-within")) {
-        const start = tabbableElements[0]
-        const end = tabbableElements[tabbableElements.length - 1]
-        const target = this.tabDirection === "forward" ? start : end
+      const tabbableElements = getTabbableElements(this.element);
+      if (!this.element.matches(':focus-within')) {
+        const start = tabbableElements[0];
+        const end = tabbableElements[tabbableElements.length - 1];
+        const target = this.tabDirection === 'forward' ? start : end;
 
         if (typeof target?.focus === 'function') {
-          this.currentFocus = target
+          this.currentFocus = target;
           target.focus({ preventScroll: true });
         }
       }
@@ -56,12 +54,12 @@ export default class Modal {
     this.checkFocus();
   }
 
-  get currentFocusIndex () {
-    return getTabbableElements(this.element).findIndex((el) => el === this.currentFocus)
+  get currentFocusIndex() {
+    return getTabbableElements(this.element).findIndex(el => el === this.currentFocus);
   }
 
   handleKeyDown(event: KeyboardEvent) {
-    if (event.key !== "Tab") return
+    if (event.key !== 'Tab') return;
 
     if (event.shiftKey) {
       this.tabDirection = 'backward';
@@ -69,34 +67,34 @@ export default class Modal {
       this.tabDirection = 'forward';
     }
 
-    event.preventDefault()
+    event.preventDefault();
 
-    const tabbableElements = getTabbableElements(this.element)
+    const tabbableElements = getTabbableElements(this.element);
     const start = tabbableElements[0];
     // const end = tabbableElements[tabbableElements.length - 1]
-    let focusIndex = this.currentFocusIndex
+    let focusIndex = this.currentFocusIndex;
 
     if (focusIndex === -1) {
-      this.currentFocus = start
-      this.currentFocus.focus({ preventScroll: true })
-      return
+      this.currentFocus = start;
+      this.currentFocus.focus({ preventScroll: true });
+      return;
     }
 
     // This looks interesting right?
     // Well, in Chrome / Edge if you have a dialog inside of a shadow root it will
     // jump to the "address bar" on Shift+Tab. This prevents that.
-    let addition = this.tabDirection === 'forward' ? 1 : -1;
+    const addition = this.tabDirection === 'forward' ? 1 : -1;
 
     if (focusIndex + addition >= tabbableElements.length) {
-      focusIndex = 0
+      focusIndex = 0;
     } else if (this.currentFocusIndex + addition < 0) {
-      focusIndex = tabbableElements.length - 1
+      focusIndex = tabbableElements.length - 1;
     } else {
-      focusIndex += addition
+      focusIndex += addition;
     }
 
-    this.currentFocus = tabbableElements[focusIndex]
-    this.currentFocus?.focus({ preventScroll: true })
+    this.currentFocus = tabbableElements[focusIndex];
+    this.currentFocus?.focus({ preventScroll: true });
   }
 
   handleKeyUp() {
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index 2e7be9eaef..5dc83ad0fe 100644
--- a/src/internal/tabbable.ts
+++ b/src/internal/tabbable.ts
@@ -23,7 +23,7 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  if (el.offsetParent == null) {
+  if (el.offsetParent === null) {
     return false;
   }
 
@@ -56,7 +56,7 @@ function isTabbable(el: HTMLElement) {
  * element because it short-circuits after finding the first and last ones.
  */
 export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
-  const tabbableElements = getTabbableElements(root)
+  const tabbableElements = getTabbableElements(root);
 
   // Find the first and last tabbable elements
   const start = tabbableElements[0] ?? null;
@@ -65,7 +65,7 @@ export function getTabbableBoundary(root: HTMLElement | ShadowRoot) {
   return { start, end };
 }
 
-export function getTabbableElements (root: HTMLElement | ShadowRoot) {
+export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   const allElements: HTMLElement[] = [];
 
   function walk(el: HTMLElement | ShadowRoot) {
@@ -83,5 +83,5 @@ export function getTabbableElements (root: HTMLElement | ShadowRoot) {
   // Collect all elements including the root
   walk(root);
 
-  return [...new Set(allElements.filter(isTabbable))]
+  return [...new Set(allElements.filter(isTabbable))];
 }

From 2244b7b4bbd0e98a2c34bae4e2e953231d1278f1 Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 23 Jun 2023 16:44:21 -0400
Subject: [PATCH 3/9] sort by tabindex

---
 src/components/dialog/dialog.test.ts | 42 ++++++++++++++++++++++------
 src/internal/modal.ts                |  6 +---
 src/internal/tabbable.ts             | 13 ++++++---
 3 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts
index 4a5309d8ac..3145f6418c 100644
--- a/src/components/dialog/dialog.test.ts
+++ b/src/components/dialog/dialog.test.ts
@@ -1,6 +1,6 @@
 import '../../../dist/shoelace.js';
 // cspell:dictionaries lorem-ipsum
-import { expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { aTimeout, elementUpdated, expect, fixture, html, waitUntil } from '@open-wc/testing';
 import { LitElement } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
@@ -174,17 +174,41 @@ describe('<sl-dialog>', () => {
       }
     }
 
-    window.customElements.define('a-container', AContainer);
+    if (!window.customElements.get("a-container")) {
+      window.customElements.define('a-container', AContainer);
+    }
 
     const testCase = await fixture(html`
-      <a-container></a-container>
-
-      <p>
-        Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches
-        things outside the dialog.
-      </p>
+      <div>
+        <a-container></a-container>
+
+        <p>
+          Open the dialog, then use <kbd>Tab</kbd> to cycle through the inputs. Focus should be trapped, but it reaches
+          things outside the dialog.
+        </p>
+      </div>
     `);
 
-    testCase.querySelector<AContainer>('a-container')?.openDialog();
+
+    const container = testCase.querySelector('a-container') as AContainer
+
+    await elementUpdated(container)
+
+    const dialog = container.shadowRoot?.querySelector("sl-dialog") as SlDialog
+
+    container.openDialog();
+
+    await aTimeout(10)
+    await sendKeys({down: "Tab"})
+    await sendKeys({up: "Tab"})
+    await aTimeout(10)
+
+    expect(container.shadowRoot?.activeElement).to.equal(dialog)
+
+    expect(dialog.shadowRoot?.activeElement).to.equal(dialog.shadowRoot?.querySelector("sl-icon-button"))
+    //
+    // await sendKeys({ press: "Tab" })
+    //
+    // expect(container.shadowRoot?.activeElement).to.equal(container.shadowRoot?.querySelector("sl-dialog"))
   });
 });
diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index 4851da2e0e..a972886bef 100644
--- a/src/internal/modal.ts
+++ b/src/internal/modal.ts
@@ -37,7 +37,7 @@ export default class Modal {
   checkFocus() {
     if (this.isActive()) {
       const tabbableElements = getTabbableElements(this.element);
-      if (!this.element.matches(':focus-within')) {
+      if (!(this.element.matches(":focus-within"))) {
         const start = tabbableElements[0];
         const end = tabbableElements[tabbableElements.length - 1];
         const target = this.tabDirection === 'forward' ? start : end;
@@ -71,7 +71,6 @@ export default class Modal {
 
     const tabbableElements = getTabbableElements(this.element);
     const start = tabbableElements[0];
-    // const end = tabbableElements[tabbableElements.length - 1]
     let focusIndex = this.currentFocusIndex;
 
     if (focusIndex === -1) {
@@ -80,9 +79,6 @@ export default class Modal {
       return;
     }
 
-    // This looks interesting right?
-    // Well, in Chrome / Edge if you have a dialog inside of a shadow root it will
-    // jump to the "address bar" on Shift+Tab. This prevents that.
     const addition = this.tabDirection === 'forward' ? 1 : -1;
 
     if (focusIndex + addition >= tabbableElements.length) {
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index 5dc83ad0fe..43330825eb 100644
--- a/src/internal/tabbable.ts
+++ b/src/internal/tabbable.ts
@@ -23,7 +23,7 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  if (el.offsetParent === null) {
+  if (el.offsetParent == null) {
     return false;
   }
 
@@ -69,7 +69,7 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
   const allElements: HTMLElement[] = [];
 
   function walk(el: HTMLElement | ShadowRoot) {
-    if (el instanceof HTMLElement) {
+    if (el instanceof Element) {
       allElements.push(el);
 
       if (el.shadowRoot !== null && el.shadowRoot.mode === 'open') {
@@ -77,11 +77,16 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
       }
     }
 
-    [...el.children].forEach((e: HTMLElement) => walk(e));
+    ;[...el.children].forEach((e: HTMLElement) => walk(e));
   }
 
   // Collect all elements including the root
   walk(root);
 
-  return [...new Set(allElements.filter(isTabbable))];
+  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)
+  });
 }

From 4a0dac61f8841a3a7cf89594b55e1746f9bbdd53 Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 23 Jun 2023 16:54:58 -0400
Subject: [PATCH 4/9] sort by tabindex

---
 src/internal/modal.ts    |  2 ++
 src/internal/tabbable.ts | 19 ++++++++++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index a972886bef..0145d1bc09 100644
--- a/src/internal/modal.ts
+++ b/src/internal/modal.ts
@@ -91,6 +91,8 @@ export default class Modal {
 
     this.currentFocus = tabbableElements[focusIndex];
     this.currentFocus?.focus({ preventScroll: true });
+
+    window.setTimeout(() => this.checkFocus())
   }
 
   handleKeyUp() {
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index 43330825eb..a8fa22887e 100644
--- a/src/internal/tabbable.ts
+++ b/src/internal/tabbable.ts
@@ -1,3 +1,5 @@
+import { offsetParent } from "composed-offset-position";
+
 /** 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();
@@ -23,7 +25,8 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  if (el.offsetParent == null) {
+  // offsetParent polyfill is added because otherwise it misses elements in Safari.
+  if (el.offsetParent === null && offsetParent(el) === null) {
     return false;
   }
 
@@ -83,10 +86,12 @@ 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 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)
+    });
 }

From 70b1020e3719ef123479a7e6fa63312c06f8e06a Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 7 Jul 2023 14:12:10 -0400
Subject: [PATCH 5/9] add a dialog test case for shadow roots

---
 src/components/dialog/dialog.test.ts | 60 ++++++++++++++++++++++------
 src/internal/modal.ts                |  2 +-
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts
index 3145f6418c..adf9d7a469 100644
--- a/src/components/dialog/dialog.test.ts
+++ b/src/components/dialog/dialog.test.ts
@@ -5,6 +5,7 @@ import { LitElement } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
 import type SlDialog from './dialog';
+import SlButton from '../button/button.js';
 
 describe('<sl-dialog>', () => {
   it('should be visible with the open attribute', async () => {
@@ -165,8 +166,9 @@ describe('<sl-dialog>', () => {
           <sl-dialog label="Dialog" class="dialog-overview">
             Lorem ipsum dolor sit amet, consectetur adipiscing elit.
             <br/>
-            <input type="checkbox">A</input>
-            <input type="checkbox">B</input>
+            <label><input type="checkbox">A</label>
+            <label><input type="checkbox">B</label>
+            <button>Button</button>
           </sl-dialog>
 
           <sl-button @click=${this.openDialog}>Open Dialog</sl-button>
@@ -195,20 +197,54 @@ describe('<sl-dialog>', () => {
     await elementUpdated(container)
 
     const dialog = container.shadowRoot?.querySelector("sl-dialog") as SlDialog
+    const closeButton = dialog.shadowRoot?.querySelector("sl-icon-button")
+    const checkbox1 = dialog.querySelector("input[type='checkbox']")
+    const checkbox2 = dialog.querySelectorAll("input[type='checkbox']")[1]
+    const button = dialog.querySelector("button")
 
-    container.openDialog();
 
-    await aTimeout(10)
-    await sendKeys({down: "Tab"})
-    await sendKeys({up: "Tab"})
-    await aTimeout(10)
+    ;(container.shadowRoot?.querySelector("sl-button") as SlButton).click();
+
+    // Test tab cycling
+    await sendKeys({press: "Tab"})
 
     expect(container.shadowRoot?.activeElement).to.equal(dialog)
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(button)
+
+    await sendKeys({ press: "Tab" })
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1)
+
+    // Test Shift+Tab cycling
+    await sendKeys({ down: "Shift" })
+
+    await sendKeys({ press: "Tab" })
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(button)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2)
+
+    await sendKeys({ press: "Tab" })
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1)
+
+    await sendKeys({ press: "Tab" })
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
 
-    expect(dialog.shadowRoot?.activeElement).to.equal(dialog.shadowRoot?.querySelector("sl-icon-button"))
-    //
-    // await sendKeys({ press: "Tab" })
-    //
-    // expect(container.shadowRoot?.activeElement).to.equal(container.shadowRoot?.querySelector("sl-dialog"))
+    // End shift+tab cycling
+    await sendKeys({ up: "Shift" })
   });
 });
diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index 0145d1bc09..3d8c905715 100644
--- a/src/internal/modal.ts
+++ b/src/internal/modal.ts
@@ -92,7 +92,7 @@ export default class Modal {
     this.currentFocus = tabbableElements[focusIndex];
     this.currentFocus?.focus({ preventScroll: true });
 
-    window.setTimeout(() => this.checkFocus())
+    setTimeout(() => this.checkFocus())
   }
 
   handleKeyUp() {

From 8dfda3133c4597559d945d9a944b157f2011ad5a Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 7 Jul 2023 14:14:06 -0400
Subject: [PATCH 6/9] add a changelog note

---
 docs/pages/resources/changelog.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
index 6c415285da..cfb6031de9 100644
--- a/docs/pages/resources/changelog.md
+++ b/docs/pages/resources/changelog.md
@@ -16,6 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 - Added tests for `<sl-qr-code>` [#1416]
 - Added support for pressing [[Space]] to select/toggle selected `<sl-menu-item>` elements [#1429]
+- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `composedParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot. [#1399]
 - Fixed a bug in `<sl-qr-code>` where the `background` attribute was never passed to the QR code [#1416]
 - Fixed a bug in `<sl-dropdown>` where aria attributes were incorrectly applied to the default `<slot>` causing Lighthouse errors [#1417]
 - Fixed a bug in `<sl-carousel>` that caused navigation to work incorrectly in some case [#1420]
@@ -1588,4 +1589,4 @@ The following pages demonstrate why this change was necessary.
 
 ## 2.0.0-beta.1
 
-- Initial release
+- Initial release
\ No newline at end of file

From b86226a5073225f0b80753a836c6f62b3fc7c27a Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 7 Jul 2023 14:15:38 -0400
Subject: [PATCH 7/9] add a changelog note

---
 docs/pages/resources/changelog.md | 2 +-
 src/internal/tabbable.ts          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
index cfb6031de9..3b13aad912 100644
--- a/docs/pages/resources/changelog.md
+++ b/docs/pages/resources/changelog.md
@@ -16,7 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 - Added tests for `<sl-qr-code>` [#1416]
 - Added support for pressing [[Space]] to select/toggle selected `<sl-menu-item>` elements [#1429]
-- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `composedParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot. [#1399]
+- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `offsetParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot [#1399]
 - Fixed a bug in `<sl-qr-code>` where the `background` attribute was never passed to the QR code [#1416]
 - Fixed a bug in `<sl-dropdown>` where aria attributes were incorrectly applied to the default `<slot>` causing Lighthouse errors [#1417]
 - Fixed a bug in `<sl-carousel>` that caused navigation to work incorrectly in some case [#1420]
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index a8fa22887e..b1a4bc5855 100644
--- a/src/internal/tabbable.ts
+++ b/src/internal/tabbable.ts
@@ -25,7 +25,7 @@ function isTabbable(el: HTMLElement) {
   }
 
   // Elements that are hidden have no offsetParent and are not tabbable
-  // offsetParent polyfill is added because otherwise it misses elements in Safari.
+  // offsetParent() is added because otherwise it misses elements in Safari
   if (el.offsetParent === null && offsetParent(el) === null) {
     return false;
   }

From 4bb225f1167ce8a87686bad0ec3a970fb4d049a8 Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 7 Jul 2023 14:29:16 -0400
Subject: [PATCH 8/9] prettier + test fixes

---
 docs/pages/resources/changelog.md    |  2 +-
 src/components/dialog/dialog.test.ts | 96 +++++++++++++++++-----------
 src/internal/modal.ts                |  4 +-
 src/internal/tabbable.ts             | 18 +++---
 4 files changed, 69 insertions(+), 51 deletions(-)

diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
index 3b13aad912..e8ab6f4cc5 100644
--- a/docs/pages/resources/changelog.md
+++ b/docs/pages/resources/changelog.md
@@ -1589,4 +1589,4 @@ The following pages demonstrate why this change was necessary.
 
 ## 2.0.0-beta.1
 
-- Initial release
\ No newline at end of file
+- Initial release
diff --git a/src/components/dialog/dialog.test.ts b/src/components/dialog/dialog.test.ts
index adf9d7a469..0cabd06302 100644
--- a/src/components/dialog/dialog.test.ts
+++ b/src/components/dialog/dialog.test.ts
@@ -5,7 +5,6 @@ import { LitElement } from 'lit';
 import { sendKeys } from '@web/test-runner-commands';
 import sinon from 'sinon';
 import type SlDialog from './dialog';
-import SlButton from '../button/button.js';
 
 describe('<sl-dialog>', () => {
   it('should be visible with the open attribute', async () => {
@@ -165,9 +164,9 @@ describe('<sl-dialog>', () => {
           <h1>Dialog Example</h1>
           <sl-dialog label="Dialog" class="dialog-overview">
             Lorem ipsum dolor sit amet, consectetur adipiscing elit.
-            <br/>
-            <label><input type="checkbox">A</label>
-            <label><input type="checkbox">B</label>
+            <br />
+            <label><input type="checkbox" />A</label>
+            <label><input type="checkbox" />B</label>
             <button>Button</button>
           </sl-dialog>
 
@@ -176,7 +175,7 @@ describe('<sl-dialog>', () => {
       }
     }
 
-    if (!window.customElements.get("a-container")) {
+    if (!window.customElements.get('a-container')) {
       window.customElements.define('a-container', AContainer);
     }
 
@@ -191,60 +190,81 @@ describe('<sl-dialog>', () => {
       </div>
     `);
 
+    const container = testCase.querySelector('a-container');
 
-    const container = testCase.querySelector('a-container') as AContainer
+    if (!container) {
+      throw Error('Could not find <a-container> element.');
+    }
+
+    await elementUpdated(container);
+    const dialog = container.shadowRoot?.querySelector('sl-dialog');
 
-    await elementUpdated(container)
+    if (!dialog) {
+      throw Error('Could not find <sl-dialog> element.');
+    }
 
-    const dialog = container.shadowRoot?.querySelector("sl-dialog") as SlDialog
-    const closeButton = dialog.shadowRoot?.querySelector("sl-icon-button")
-    const checkbox1 = dialog.querySelector("input[type='checkbox']")
-    const checkbox2 = dialog.querySelectorAll("input[type='checkbox']")[1]
-    const button = dialog.querySelector("button")
+    const closeButton = dialog.shadowRoot?.querySelector('sl-icon-button');
+    const checkbox1 = dialog.querySelector("input[type='checkbox']");
+    const checkbox2 = dialog.querySelectorAll("input[type='checkbox']")[1];
+    const button = dialog.querySelector('button');
 
+    // Opens modal.
+    const openModalButton = container.shadowRoot?.querySelector('sl-button');
 
-    ;(container.shadowRoot?.querySelector("sl-button") as SlButton).click();
+    if (openModalButton) openModalButton.click();
 
     // Test tab cycling
-    await sendKeys({press: "Tab"})
+    await pressTab();
 
-    expect(container.shadowRoot?.activeElement).to.equal(dialog)
-    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+    expect(container.shadowRoot?.activeElement).to.equal(dialog);
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(checkbox1)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(checkbox2)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(button)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(button);
 
-    await sendKeys({ press: "Tab" })
-    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(checkbox1)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
 
     // Test Shift+Tab cycling
-    await sendKeys({ down: "Shift" })
 
-    await sendKeys({ press: "Tab" })
-    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+    // I found these timeouts were needed for WebKit locally.
+    await aTimeout(10);
+    await sendKeys({ down: 'Shift' });
+    await aTimeout(10);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(button)
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(checkbox2)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(button);
 
-    await sendKeys({ press: "Tab" })
-    expect(container.shadowRoot?.activeElement).to.equal(checkbox1)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox2);
 
-    await sendKeys({ press: "Tab" })
-    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton)
+    await pressTab();
+    expect(container.shadowRoot?.activeElement).to.equal(checkbox1);
+
+    await pressTab();
+    expect(dialog.shadowRoot?.activeElement).to.equal(closeButton);
 
     // End shift+tab cycling
-    await sendKeys({ up: "Shift" })
+    await sendKeys({ up: 'Shift' });
   });
 });
+
+// We wait 50ms just to give the browser some time to figure out the current focus.
+// 50 was the magic number I found locally :shrug:
+async function pressTab() {
+  await aTimeout(50);
+  await sendKeys({ press: 'Tab' });
+  await aTimeout(50);
+}
diff --git a/src/internal/modal.ts b/src/internal/modal.ts
index 3d8c905715..bde892f132 100644
--- a/src/internal/modal.ts
+++ b/src/internal/modal.ts
@@ -37,7 +37,7 @@ export default class Modal {
   checkFocus() {
     if (this.isActive()) {
       const tabbableElements = getTabbableElements(this.element);
-      if (!(this.element.matches(":focus-within"))) {
+      if (!this.element.matches(':focus-within')) {
         const start = tabbableElements[0];
         const end = tabbableElements[tabbableElements.length - 1];
         const target = this.tabDirection === 'forward' ? start : end;
@@ -92,7 +92,7 @@ export default class Modal {
     this.currentFocus = tabbableElements[focusIndex];
     this.currentFocus?.focus({ preventScroll: true });
 
-    setTimeout(() => this.checkFocus())
+    setTimeout(() => this.checkFocus());
   }
 
   handleKeyUp() {
diff --git a/src/internal/tabbable.ts b/src/internal/tabbable.ts
index b1a4bc5855..4925e2ab7f 100644
--- a/src/internal/tabbable.ts
+++ b/src/internal/tabbable.ts
@@ -1,4 +1,4 @@
-import { offsetParent } from "composed-offset-position";
+import { offsetParent } from 'composed-offset-position';
 
 /** Determines if the specified element is tabbable using heuristics inspired by https://github.com/focus-trap/tabbable */
 function isTabbable(el: HTMLElement) {
@@ -80,18 +80,16 @@ export function getTabbableElements(root: HTMLElement | ShadowRoot) {
       }
     }
 
-    ;[...el.children].forEach((e: HTMLElement) => walk(e));
+    [...el.children].forEach((e: HTMLElement) => walk(e));
   }
 
   // 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 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;
+  });
 }

From 603d20ec32c05cd7e309ec52c8b905c6e10335d9 Mon Sep 17 00:00:00 2001
From: konnorrogers <konnor5456@gmail.com>
Date: Fri, 7 Jul 2023 14:30:12 -0400
Subject: [PATCH 9/9] prettier + test fixes

---
 docs/pages/resources/changelog.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
index e8ab6f4cc5..e59428afb3 100644
--- a/docs/pages/resources/changelog.md
+++ b/docs/pages/resources/changelog.md
@@ -16,7 +16,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 
 - Added tests for `<sl-qr-code>` [#1416]
 - Added support for pressing [[Space]] to select/toggle selected `<sl-menu-item>` elements [#1429]
-- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `offsetParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot [#1399]
+- Fixed a bug in focus trapping of modal elements like `<sl-dialog>`. We now manually handle focus ordering as well as added `offsetParent()` check for tabbable boundaries in Safari. Test cases added for `<sl-dialog>` inside a shadowRoot [#1403]
 - Fixed a bug in `<sl-qr-code>` where the `background` attribute was never passed to the QR code [#1416]
 - Fixed a bug in `<sl-dropdown>` where aria attributes were incorrectly applied to the default `<slot>` causing Lighthouse errors [#1417]
 - Fixed a bug in `<sl-carousel>` that caused navigation to work incorrectly in some case [#1420]