From 7e201f6af6ac6c1fe8c068f30d2f0604aa729c60 Mon Sep 17 00:00:00 2001
From: Mats Bosson <mats.bosson@gmail.com>
Date: Sun, 29 Mar 2026 04:37:45 +0700
Subject: [PATCH] Harden focus trap and add tests

---
 .../utilities/focus-trap/create-focus-trap.ts |  8 +-
 .../core/tests/utilities/focus-trap.test.ts   | 89 +++++++++++++++++++
 2 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/packages/core/src/utilities/focus-trap/create-focus-trap.ts b/packages/core/src/utilities/focus-trap/create-focus-trap.ts
index bab93be..c3dcc8d 100644
--- a/packages/core/src/utilities/focus-trap/create-focus-trap.ts
+++ b/packages/core/src/utilities/focus-trap/create-focus-trap.ts
@@ -3,7 +3,7 @@ import type { Accessor } from "solid-js";
 const FOCUSABLE_SELECTORS = [
   "a[href]",
   "button:not([disabled])",
-  "input:not([disabled])",
+  "input:not([disabled]):not([type='hidden'])",
   "select:not([disabled])",
   "textarea:not([disabled])",
   "[tabindex]:not([tabindex='-1'])",
@@ -52,10 +52,14 @@ export function createFocusTrap(getContainer: Accessor<HTMLElement | null>): Foc
     }
   };
 
+  let isActive = false;
+
   return {
     activate() {
+      if (isActive) return;
       const container = getContainer();
       if (!container) return;
+      isActive = true;
 
       previouslyFocused = document.activeElement as HTMLElement | null;
       const focusable = getFocusableElements(container);
@@ -63,6 +67,8 @@ export function createFocusTrap(getContainer: Accessor<HTMLElement | null>): Foc
       document.addEventListener("keydown", handleKeyDown);
     },
     deactivate() {
+      if (!isActive) return;
+      isActive = false;
       document.removeEventListener("keydown", handleKeyDown);
       previouslyFocused?.focus();
       previouslyFocused = null;
diff --git a/packages/core/tests/utilities/focus-trap.test.ts b/packages/core/tests/utilities/focus-trap.test.ts
index fe50713..81dfb2f 100644
--- a/packages/core/tests/utilities/focus-trap.test.ts
+++ b/packages/core/tests/utilities/focus-trap.test.ts
@@ -57,4 +57,93 @@ describe("createFocusTrap", () => {
     document.body.removeChild(outside);
     document.body.removeChild(container);
   });
+
+  it("wraps Tab forward from last focusable to first", () => {
+    const container = document.createElement("div");
+    const button1 = document.createElement("button");
+    const button2 = document.createElement("button");
+    button1.textContent = "First";
+    button2.textContent = "Second";
+    container.appendChild(button1);
+    container.appendChild(button2);
+    document.body.appendChild(container);
+
+    createRoot((dispose) => {
+      const trap = createFocusTrap(() => container);
+      trap.activate();
+      button2.focus();
+      const event = new KeyboardEvent("keydown", { key: "Tab", bubbles: true, cancelable: true });
+      document.dispatchEvent(event);
+      expect(document.activeElement).toBe(button1);
+      trap.deactivate();
+      dispose();
+    });
+
+    document.body.removeChild(container);
+  });
+
+  it("wraps Shift+Tab backward from first focusable to last", () => {
+    const container = document.createElement("div");
+    const button1 = document.createElement("button");
+    const button2 = document.createElement("button");
+    button1.textContent = "First";
+    button2.textContent = "Second";
+    container.appendChild(button1);
+    container.appendChild(button2);
+    document.body.appendChild(container);
+
+    createRoot((dispose) => {
+      const trap = createFocusTrap(() => container);
+      trap.activate();
+      // button1 is already focused after activate
+      const event = new KeyboardEvent("keydown", { key: "Tab", shiftKey: true, bubbles: true, cancelable: true });
+      document.dispatchEvent(event);
+      expect(document.activeElement).toBe(button2);
+      trap.deactivate();
+      dispose();
+    });
+
+    document.body.removeChild(container);
+  });
+
+  it("does not throw when container has no focusable elements", () => {
+    const container = document.createElement("div");
+    container.textContent = "No focusable children";
+    document.body.appendChild(container);
+
+    createRoot((dispose) => {
+      const trap = createFocusTrap(() => container);
+      trap.activate();
+      // Tab should be prevented but not throw
+      const event = new KeyboardEvent("keydown", { key: "Tab", bubbles: true, cancelable: true });
+      expect(() => document.dispatchEvent(event)).not.toThrow();
+      trap.deactivate();
+      dispose();
+    });
+
+    document.body.removeChild(container);
+  });
+
+  it("second activate() is a no-op — does not attach duplicate listeners", () => {
+    const container = document.createElement("div");
+    const button = document.createElement("button");
+    container.appendChild(button);
+    document.body.appendChild(container);
+
+    createRoot((dispose) => {
+      const trap = createFocusTrap(() => container);
+      trap.activate();
+      trap.activate(); // second call should be ignored
+      button.focus();
+      // Tab from the only element — Tab wraps back to itself (first === last)
+      // What matters is it doesn't fire twice causing erratic behaviour
+      expect(() => {
+        document.dispatchEvent(new KeyboardEvent("keydown", { key: "Tab", bubbles: true, cancelable: true }));
+      }).not.toThrow();
+      trap.deactivate();
+      dispose();
+    });
+
+    document.body.removeChild(container);
+  });
 });