ReviewOS

stacks/stx

stxConfirm/stxAlert after #1875: no scroll lock, destructive action gets initial focus, and one Escape answers every open dialog

#1998
Closed glennmichael123 opened this 9 days ago ·
9 days ago

_createDialog (packages/stx/src/signals.ts:6557) got a real focus trap, focus restore and listener cleanup in #1875, and that fix holds up — I re-verified the trap, the restore and the closed guard on current main. What follows is what #1875 did not cover, found while preparing to migrate an app's seven native confirm() calls onto stxConfirm.

All six verified in a browser against the runtime generated from main (a198835, 0.2.343) via generateSignalsRuntime(), on a 3000px-tall page.

1. No scroll lock: the page scrolls behind an aria-modal dialog

window.scrollTo(0, 0)
const p = window.stxConfirm('Delete server web-01?', { type: 'error', title: 'Delete server' })
await new Promise(r => setTimeout(r, 250))
getComputedStyle(document.body).overflow          // "visible"
getComputedStyle(document.documentElement).overflow // "visible"
window.scrollTo(0, 800)
window.scrollY                                     // 800  <-- moved, dialog still open

The backdrop is position:fixed;inset:0, so it stays put while the content behind it scrolls away underneath. Nothing in _createDialog touches overflow on body or documentElement.

2. aria-modal="true" is asserted but the background is never hidden from AT

document.getElementById('behind').closest('[inert]')  // null
[...document.body.children]
  .filter(c => !c.id?.startsWith('stx-dialog-'))
  .map(c => c.getAttribute('aria-hidden'))            // [null, null, null]

The keyboard trap means Tab cannot leave, which is the important half. But aria-modal support for the virtual cursor is uneven, and the usual belt-and-braces (inert, or aria-hidden on the siblings) is absent, so a screen-reader user can still browse the page behind the dialog.

3. Initial focus always lands on the confirm button, including when it is the destructive one

signals.ts:6703 is an unconditional okBtn.focus().

window.stxConfirm('Delete server web-01? This cannot be undone.',
  { type: 'error', title: 'Delete server', confirmText: 'Delete', cancelText: 'Cancel' })
// document.activeElement.textContent                     -> "Delete"
// getComputedStyle(document.activeElement).backgroundColor -> "rgb(220, 38, 38)"

type: 'error' already paints that button red, so the API knows the action is destructive, and still hands it the keyboard. Any stray Enter or Space confirms the delete. The options _createDialog reads are exactly type, title, confirmText, cancelText, dark - there is no way to ask for Cancel instead.

A defaultFocus: 'cancel' | 'confirm' option would cover it, or simply defaulting to Cancel whenever isConfirm && type === 'error'.

4. One Escape resolves every open dialog, not just the topmost

let a = null, b = null
const A = stxConfirm('Cancel your subscription?', { title: 'Billing'  }).then(v => a = v)
await new Promise(r => setTimeout(r, 150))
const B = stxConfirm('Also delete all invoices?', { title: 'Invoices' }).then(v => b = v)
await new Promise(r => setTimeout(r, 250))

document.querySelectorAll('div[role=alertdialog]').length  // 2
document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }))
await new Promise(r => setTimeout(r, 300))

a  // false   <-- answered
b  // false   <-- also answered, by the same keypress
document.querySelectorAll('div[role=alertdialog]').length  // 0

Every dialog adds its own capture-phase keydown listener on document (signals.ts:6700) and none of them check whether they are the topmost, so one Escape runs all of them. The two traps also compete for Tab while both are open. A module-level stack that only lets the last-opened dialog handle the key would fix both.

5. No prefers-reduced-motion guard

The open and close transitions are inline (transition: opacity 0.2s ease, transform 0.2s ease), so a stylesheet media query cannot reach them. Worth noting the primitive already exists in this very file - usePreferredReducedMotion() at signals.ts:5835 - it is just not consulted here.

6. The backdrop outlives the promise by 200ms and still swallows clicks

cleanup() calls resolve(result) immediately but defers backdrop.remove() by 200ms (signals.ts:6626).

const p = stxConfirm('timing', { title: 'T' })
await new Promise(r => setTimeout(r, 250))
document.querySelector('div[role=alertdialog] button:last-of-type').click()
await p                                   // caller is now running
const bd = document.querySelector('div[role=alertdialog]')
!!bd                                       // true
getComputedStyle(bd).pointerEvents         // "auto"
getComputedStyle(bd).zIndex                // "999999"
document.elementFromPoint(innerWidth/2, innerHeight/2).id  // "stx-dialog-4-message"

So for 200ms after await stxConfirm(...) returns, a full-viewport element at z-index:999999 is still hit-testing at the centre of the screen. Fading out is right; it should just stop taking pointer events the moment it starts (backdrop.style.pointerEvents = 'none' in cleanup).

Why this matters here

We are moving seven destructive confirmations - delete a server, rotate a token, disable 2FA, delete a passkey, cancel a subscription, delete a monitor, delete a status report - from native confirm() onto stxConfirm. Native confirm() gives us a real scroll lock and a genuinely modal surface for free; stxConfirm gives us theming, a focus trap and async. #3 is the one that would actually change our answer, because every one of those seven is an error-type confirm where the default keyboard action should be Cancel.

Happy to send a PR for any of these - #1, #3 and #6 are each a couple of lines, and #4 wants a small dialog stack.

Environment: main @ a198835, @stacksjs/stx 0.2.343.

9 days ago

All six, in 5e39b13c9d. Thank you for the measured repros — each one of those snippets became a test.

Five of the six were the same underlying thing: each dialog behaved as if it were the only one on the page. They share a fix — a module-level stack and a _dialogSyncModality() that runs on every open and close.

1. Scroll lock. Overflow locked on body and documentElement, restored to whatever the app had rather than to empty (an app that keeps the page unscrollable for its own reasons must still be unscrollable afterwards). The scrollbar's width is added back as padding, or the whole page jumps sideways the moment a dialog opens.

2. Background hidden from AT. Everything except the topmost dialog goes inert. Chosen over aria-hidden on the siblings because aria-hidden on an element containing focus is itself a violation, and inert removes the subtree from the tab order and the accessibility tree together. Only what the dialog set is cleared, so an app's own inert survives — pinned by a test.

3. Initial focus. Your read was right that this is the one that changes the answer, so: an error-type confirm defaults to Cancel, and defaultFocus: 'cancel' | 'confirm' overrides either way. The API already knew the action was destructive — type: 'error' is what paints the button red — and handed it the keyboard anyway. An alert keeps its single button: nothing destructive to land on, and focus has to go somewhere inside the trap.

4. One Escape, one dialog. The handler returns early unless its backdrop is the top of the stack. Your two-dialog repro is now a test asserting the second resolves false while the first is still unanswered. The competing Tab traps are the same fix from the other side — a backgrounded dialog is inert, so its buttons stop being focusable at all.

5. Reduced motion. Read once at open time with matchMedia rather than through usePreferredReducedMotion(). That returns a signal with a change listener, and an imperative dialog that reads the value once would leak one listener per call — so the primitive is the right thing in a component and the wrong thing here. With the preference set, the dialog opens and closes at its final opacity and scale and removes itself immediately rather than on a 200ms timer.

6. Backdrop pointer events. pointerEvents = 'none' in cleanup, as you suggested. It also splices itself off the stack before resolving, so a caller that opens another dialog the moment this one answers does not land underneath a backdrop that is still fading.

Two things worth recording

I put backticks in a comment inside the runtime template literal and terminated it — Expected ; but found inert. That is the documented trap in CLAUDE.md note 41, and I walked into it anyway. The parse guard named the line immediately, which is exactly what it is for.

And the new test first used el.click(); four cases timed out at 5s rather than failing, which is a much worse signal than a wrong assertion. very-happy-dom's click() does not run an onclick assigned as a property, which is how the dialog wires its buttons. dialog-a11y.test.ts calls .onclick() for the same reason, so it is now a press() helper with the reason written down.

12 new tests; the signals suite is at 1,115 passing.

For your seven migrations: all of them being error-type confirms means they get Cancel-by-default with no option passed. If any one of them should default to the action instead, defaultFocus: 'confirm' is there.

Sign in to comment on this issue.