ReviewOS

stacks/stx

fix(signals): stxConfirm/stxAlert ignore the app theme, don't trap or restore focus, and leak a keydown listener on every close-by-click

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

Summary

The built-in stxConfirm / stxAlert dialog hard-codes its palette and picks light-vs-dark from prefers-color-scheme alone, so it renders off-theme in any app that drives theming from a data-theme attribute. Its accessibility is half-done: it sets role="alertdialog" and focuses the primary button, but there is no focus trap, no focus restore, no scroll lock, and no aria-labelledby/aria-describedby linking the title and message it just created. The Escape handler is only removed on the Escape path, so closing by button leaks a document-level keydown listener every time.

Impact (real app)

analyticshq uses it for both of its destructive confirmations and documents a property it does not have:

// resources/views/dashboard.stx:877-880
// stxConfirm rather than window.confirm: a promise-returning in-page dialog that
// matches the app's theme, instead of a browser chrome prompt that blocks the whole
// thread. Resolves true on OK, false on Cancel or Escape.
if (!id || !(await stxConfirm('Delete this goal? Its conversions will be removed too.')))

and again at dashboard.stx:922 for "Revoke the share link?". It does not match the app's theme. The app sets its own palette via data-theme (dashboard.stx:1083-1085, account.stx:98-101), so a user whose OS is in light mode and whose app is on data-theme="dark" gets a white panel with an indigo #5672cd button floating over a dark app. A keyboard user can Tab straight out of the modal into the page behind it, and after closing by click the page keeps a dead keydown listener per dialog opened.

Evidence in stx

All in packages/stx/src/signals.ts, inside _createDialog (:5368), which backs stxAlert (:5457) and stxConfirm (:5461), published as window.stxConfirm at :6204:

  • :5375 — var isDark = window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches; — the only theme input. data-theme is never read.
  • :5376-5378 — bg = isDark ? '#1f2937' : '#ffffff', textColor, subColor: hard-coded.
  • :5433 — btnColor = type === 'error' ? '#dc2626' : type === 'warning' ? '#d97706' : '#5672cd'.
  • :5370-5373 — the options bag is type, title, confirmText, cancelText. No colour or theme override exists.
  • :5385-5386 — role="alertdialog" + aria-modal="true" are set, but :5396 (titleEl) and :5404 (msgEl) get no id, and no aria-labelledby / aria-describedby is ever set on the backdrop, so the accessible name and description are empty.
  • :5453 — okBtn.focus() is the only focus call in the function. No document.activeElement capture before opening, no restore in cleanup (:5410-5416), no focus trap, and no document.body.style.overflow lock.
  • :5448-5450 —
    var escHandler = function(e) {
      if (e.key === 'Escape') { cleanup(…); document.removeEventListener('keydown', escHandler); }
    };
    document.addEventListener('keydown', escHandler);
    removeEventListener is inside the Escape branch only. cleanup() called from cancelBtn.onclick (:5427) or okBtn.onclick (:5437) never removes it.
  • packages/components/src/composables/utilities/useFocusTrap.ts exists in this repo and is not imported by signals.ts — the trap is already written, just not wired to the builtin.

Prior art

  • Radix AlertDialog and Headless UI Dialog — focus trap, focus restore to the trigger, background scroll lock and full ARIA wiring, all default, no configuration.
  • Angular Material MatDialog — autoFocus, restoreFocus, cdkTrapFocus, and it inherits the app theme rather than sniffing the OS.
  • Vuetify v-dialog — retain-focus, scrim, theme-aware.

Ask

  1. Read the app's resolved theme, not just the OS: honour document.documentElement.dataset.theme (and the useColorMode attribute/class the framework already supports) before falling back to prefers-color-scheme.
  2. Drive the panel, text and button colours from CSS custom properties with the current hex values as fallbacks (var(--stx-dialog-bg, #1f2937) etc.), and accept overrides on the options bag, so an app can theme it without forking.
  3. Move cleanup()'s listener teardown out of the Escape branch so document.removeEventListener('keydown', escHandler) runs on every close path.
  4. Capture document.activeElement before opening and restore focus to it in cleanup().
  5. Trap Tab inside the panel — reuse packages/components/src/composables/utilities/useFocusTrap.ts rather than writing a second one — and lock background scroll while open.
  6. Give titleEl and msgEl generated ids and set aria-labelledby / aria-describedby on the backdrop.

Related: #1671

9 days ago

Claiming this one — working on it now.

Sign in to comment on this issue.