fix(drawer): give each drawer its own panel, and focus it without frames

Two problems, both of which leave a drawer that looks right and cannot
be reached by keyboard.

The panel was found with a document-wide query, which returns the first
drawer in the document rather than the one that just opened. On a page
with two - a cart, and the item being added to it - opening the second
focused the first, which is hidden, so the focus retry spent all ten of
its attempts failing. Each drawer now carries an id and looks up its
own panel, the same idiom Dropdown and Popover already use.

The retry also ran only on `requestAnimationFrame`, which does not fire
at all while a page is not painting - a background tab, or a window the
compositor has parked. A drawer opened in that state never took focus,
and still had none when the reader came back, because the single pending
frame lands long after everything else has moved on. A timer now races
the frame and the first to arrive wins, so the retry survives a sleeping
compositor without firing twice per round.

The second one showed up while testing the first: `requestAnimationFrame`
never fires in the browser pane these were being checked in, so the
focus move could not be observed there at all. That is a quirk of the
test environment and also exactly the condition a parked tab is in.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: chrisbbreuer

packages/components/src/ui/drawer/Drawer.stx

@@ -50,10 +50,22 @@ const closeButtonPosition = {
 export const containerClasses = `pointer-events-none fixed ${positionClasses[position]} flex max-w-full`.trim()
 export const panelClasses = `pointer-events-auto relative ${sizeClasses[position]} ${className}`.trim()
 export const closeButtonClasses = `absolute top-0 ${closeButtonPosition[position]} flex pt-4 pr-2 sm:pr-4`.trim()
+
+/*
+ * This drawer's own id.
+ *
+ * The focus move looked the panel up with a document-wide query, which finds
+ * the first drawer on the page rather than the one that just opened. With two
+ * on a page - a cart and the item being added to it - opening the second
+ * focused the first, which is hidden, so focus went nowhere and the retry
+ * spent its ten frames failing. Same idiom as Dropdown and Popover.
+ */
+export const drawerInstanceId = `stx-drawer-${Math.random().toString(36).slice(2, 11)}`
 </script>
 
 <script client>
 const emit = defineEmits()
+const drawerInstanceId = {{ drawerInstanceId }}
 // useReactiveProp lets the parent drive `open` via signals
 // (`:open="drawerOpen()"`) instead of the prop being captured once.
 // See stacksjs/stx#1704.
@@ -139,24 +151,47 @@ let opener = null
  * meantime, so a drawer opened and shut inside those few frames does not
  * pull focus back afterwards.
  */
+/*
+ * The next chance to try, whichever comes first.
+ *
+ * `requestAnimationFrame` is the right signal when the page is painting, and
+ * it is suspended entirely when the page is not - a background tab, or a
+ * window the compositor has parked. A drawer opened in that state would never
+ * take focus, and would still not have it when the reader came back, because
+ * the one scheduled callback fires long after everything else has moved on.
+ * So a timer races the frame and the first to arrive wins.
+ */
+function soon(callback) {
+  let done = false
+
+  const once = () => {
+    if (done) return
+    done = true
+    callback()
+  }
+
+  requestAnimationFrame(once)
+  setTimeout(once, 32)
+}
+
 function focusFirst(attempts) {
   if (!isOpen()) return
 
-  const panel = document.querySelector('[data-stx-drawer-panel]')
+  const panel = document.querySelector(`[data-stx-drawer-panel="${drawerInstanceId}"]`)
   if (!panel) return
 
   const target = panel.querySelector('[autofocus]') || panel.querySelector('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])') || panel
 
   target.focus?.()
 
   if (document.activeElement === target) return
-  if (attempts > 0) requestAnimationFrame(() => focusFirst(attempts - 1))
+  if (attempts > 0) soon(() => focusFirst(attempts - 1))
 }
 
 function captureFocus() {
   opener = document.activeElement
 
-  requestAnimationFrame(() => focusFirst(10))
+  soon(() => focusFirst(10))
 }
 
 function releaseFocus() {
@@ -206,7 +241,7 @@ defineExpose({
         <div
           class="{{ panelClasses }}"
           style="animation: slideIn 0.5s ease-in-out"
-          data-stx-drawer-panel
+          data-stx-drawer-panel="{{ drawerInstanceId }}"
           role="dialog"
           aria-modal="true"
           {{ title ? 'aria-label="' + title + '"' : '' }}

packages/components/test/drawer.test.ts

@@ -101,11 +101,11 @@ describe('Drawer', () => {
  * thing a source-text check cannot notice.
  */
 function loadFocusFirst(environment: Record<string, any>) {
-  const start = source.indexOf('function focusFirst(')
+  const start = source.indexOf('function soon(')
   const end = source.indexOf('function captureFocus(')
 
   if (start < 0 || end < 0)
-    throw new Error('focusFirst is no longer in Drawer.stx under that name')
+    throw new Error('soon/focusFirst are no longer in Drawer.stx under those names')
 
   const names = Object.keys(environment)
   const make = new Function(...names, `${source.slice(start, end)}; return focusFirst`)
@@ -159,6 +159,7 @@ describe('Drawer focus retry', () => {
 
     const focusFirst = loadFocusFirst({
       isOpen: () => true,
+      drawerInstanceId: 'stx-drawer-test',
       document: dom.document,
       requestAnimationFrame: (callback: () => void) => {
         frames++
@@ -182,6 +183,7 @@ describe('Drawer focus retry', () => {
 
     const focusFirst = loadFocusFirst({
       isOpen: () => true,
+      drawerInstanceId: 'stx-drawer-test',
       document: dom.document,
       requestAnimationFrame: (callback: () => void) => { frames++; callback() },
     })
@@ -199,6 +201,7 @@ describe('Drawer focus retry', () => {
 
     const focusFirst = loadFocusFirst({
       isOpen: () => open,
+      drawerInstanceId: 'stx-drawer-test',
       document: dom.document,
       requestAnimationFrame: (callback: () => void) => { frames++; open = false; callback() },
     })
@@ -249,3 +252,117 @@ describe('Drawer theming', () => {
     expect(source).toContain('export const panelClasses = `pointer-events-auto relative ${sizeClasses[position]} ${className}`')
   })
 })
+
+/**
+ * Two drawers on one page.
+ *
+ * The panel was found with a document-wide query, which returns the first
+ * drawer in the document and not the one that just opened. A page with a cart
+ * drawer and an item drawer therefore focused the wrong panel - a hidden one -
+ * so the retry spent its ten frames failing and focus never moved.
+ */
+describe('Drawer instance scoping', () => {
+  test('each drawer looks up its own panel', () => {
+    expect(source).toContain('drawerInstanceId')
+    expect(source).toContain('[data-stx-drawer-panel="${drawerInstanceId}"]')
+  })
+
+  test('the id it queries for is the id it renders', () => {
+    // The two halves are written in different places; a rename in one is
+    // invisible until a drawer stops taking focus.
+    expect(source).toContain('data-stx-drawer-panel="{{ drawerInstanceId }}"')
+  })
+
+  test('the id differs per instance', () => {
+    expect(source).toContain('Math.random()')
+  })
+
+  test('finds nothing rather than the wrong panel', () => {
+    // The point of the id: a query scoped to another drawer's id must miss,
+    // where the old document-wide one would have returned that drawer.
+    const dom = stubDom({ shown: true })
+    let asked = ''
+
+    const focusFirst = loadFocusFirst({
+      isOpen: () => true,
+      drawerInstanceId: 'drawer-two',
+      document: {
+        querySelector: (selector: string) => {
+          asked = selector
+          // Only drawer-one exists in this document.
+          return selector.includes('drawer-one') ? dom.panel : null
+        },
+        get activeElement() { return 'body' },
+      },
+      requestAnimationFrame: (callback: () => void) => callback(),
+    })
+
+    focusFirst(10)
+
+    expect(asked).toContain('drawer-two')
+    expect(dom.active.current).toBe('body')
+  })
+})
+
+/**
+ * A drawer opened while the page is not painting.
+ *
+ * `requestAnimationFrame` does not fire at all in a background tab or a parked
+ * window, so a retry built only on frames never runs: the drawer opens without
+ * focus and still has none when the reader comes back, because the single
+ * pending callback fires after everything else has moved on. A timer races the
+ * frame so the retry survives a compositor that is asleep.
+ */
+describe('Drawer focus without frames', () => {
+  test('still focuses when requestAnimationFrame never fires', () => {
+    const dom = stubDom({ shown: true })
+    const timers: Array<() => void> = []
+
+    const focusFirst = loadFocusFirst({
+      isOpen: () => true,
+      drawerInstanceId: 'stx-drawer-test',
+      document: dom.document,
+      requestAnimationFrame: () => {},
+      setTimeout: (callback: () => void) => { timers.push(callback); return 1 },
+    })
+
+    // Hidden on the first pass, revealed before the timer runs the retry.
+    dom.shown.value = false
+    focusFirst(10)
+    dom.shown.value = true
+    while (timers.length) timers.shift()!()
+
+    expect(dom.document.activeElement).toBe(dom.target)
+  })
+
+  test('the frame and the timer do not both retry', () => {
+    // Racing two schedulers must not double the work each round; ten attempts
+    // would otherwise become a thousand.
+    const dom = stubDom({ shown: false })
+    const frames: Array<() => void> = []
+    const timers: Array<() => void> = []
+    let attempts = 0
+
+    const focusFirst = loadFocusFirst({
+      isOpen: () => true,
+      drawerInstanceId: 'stx-drawer-test',
+      document: {
+        querySelector: () => { attempts++; return dom.panel },
+        get activeElement() { return 'body' },
+      },
+      requestAnimationFrame: (callback: () => void) => { frames.push(callback); return 1 },
+      setTimeout: (callback: () => void) => { timers.push(callback); return 1 },
+    })
+
+    focusFirst(2)
+
+    // Run both schedulers, frame first, then the timer that raced it.
+    while (frames.length || timers.length) {
+      while (frames.length) frames.shift()!()
+      while (timers.length) timers.shift()!()
+    }
+
+    // Three attempts total: the initial call plus two retries.
+    expect(attempts).toBe(3)
+  })
+})

packages/components/test/visual/__snapshots__/drawer-template.snap.txt

@@ -56,9 +56,21 @@ const closeButtonPosition = {
 export const containerClasses = `pointer-events-none fixed ${positionClasses[position]} flex max-w-full`.trim()
 export const panelClasses = `pointer-events-auto relative ${sizeClasses[position]} ${className}`.trim()
 export const closeButtonClasses = `absolute top-0 ${closeButtonPosition[position]} flex pt-4 pr-2 sm:pr-4`.trim()
+
+/*
+ * This drawer's own id.
+ *
+ * The focus move looked the panel up with a document-wide query, which finds
+ * the first drawer on the page rather than the one that just opened. With two
+ * on a page - a cart and the item being added to it - opening the second
+ * focused the first, which is hidden, so focus went nowhere and the retry
+ * spent its ten frames failing. Same idiom as Dropdown and Popover.
+ */
+export const drawerInstanceId = `stx-drawer-${Math.random().toString(36).slice(2, 11)}`
 </script>
 <script client>
   const emit = defineEmits()
+const drawerInstanceId = {{ drawerInstanceId }}
 // useReactiveProp lets the parent drive `open` via signals
 // (`:open="drawerOpen()"`) instead of the prop being captured once.
 // See stacksjs/stx#1704.
@@ -144,24 +156,47 @@ let opener = null
  * meantime, so a drawer opened and shut inside those few frames does not
  * pull focus back afterwards.
  */
+/*
+ * The next chance to try, whichever comes first.
+ *
+ * `requestAnimationFrame` is the right signal when the page is painting, and
+ * it is suspended entirely when the page is not - a background tab, or a
+ * window the compositor has parked. A drawer opened in that state would never
+ * take focus, and would still not have it when the reader came back, because
+ * the one scheduled callback fires long after everything else has moved on.
+ * So a timer races the frame and the first to arrive wins.
+ */
+function soon(callback) {
+  let done = false
+
+  const once = () => {
+    if (done) return
+    done = true
+    callback()
+  }
+
+  requestAnimationFrame(once)
+  setTimeout(once, 32)
+}
+
 function focusFirst(attempts) {
   if (!isOpen()) return
 
-  const panel = document.querySelector('[data-stx-drawer-panel]')
+  const panel = document.querySelector(`[data-stx-drawer-panel="${drawerInstanceId}"]`)
   if (!panel) return
 
   const target = panel.querySelector('[autofocus]') || panel.querySelector('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])') || panel
 
   target.focus?.()
 
   if (document.activeElement === target) return
-  if (attempts > 0) requestAnimationFrame(() => focusFirst(attempts - 1))
+  if (attempts > 0) soon(() => focusFirst(attempts - 1))
 }
 
 function captureFocus() {
   opener = document.activeElement
 
-  requestAnimationFrame(() => focusFirst(10))
+  soon(() => focusFirst(10))
 }
 
 function releaseFocus() {
@@ -210,7 +245,7 @@ defineExpose({
         <div
           class="{{ panelClasses }}"
           style="animation: slideIn 0.5s ease-in-out"
-          data-stx-drawer-panel
+          data-stx-drawer-panel="{{ drawerInstanceId }}"
           role="dialog"
           aria-modal="true"
           {{ title ? 'aria-label="' + title + '"' : '' }}