fix(desktop): confirm dialogs take focus so Enter confirms
The delete-session dialog opted out of Radix's autofocus, which left focus on the sidebar row that opened it — Enter re-activated the row instead of confirming, and ConfirmDialog's Enter handler never saw the key. ConfirmDialog now focuses its own Confirm button on open. The existing Enter test fired the key at the dialog node, so it passed over the bug; it now fires at whatever actually holds focus.
This commit is contained in:
committed by
brooklyn!
parent
bfcfdb30d1
commit
bb0e9ee95a
@@ -247,12 +247,19 @@ describe('SessionActionsMenu', () => {
|
||||
expect(await screen.queryByRole('dialog')).toBeNull()
|
||||
expect(onDelete).not.toHaveBeenCalled()
|
||||
|
||||
// Re-open and confirm with Enter: the delete call fires.
|
||||
// Re-open and confirm with Enter at wherever focus actually is. Firing on
|
||||
// the dialog node would pass even when the menu leaves focus on the row
|
||||
// trigger — where Enter re-activates the row instead of confirming.
|
||||
fireEvent.pointerDown(trigger, { button: 0, pointerType: 'mouse' })
|
||||
fireEvent.pointerUp(trigger, { button: 0, pointerType: 'mouse' })
|
||||
fireEvent.click(trigger)
|
||||
fireEvent.click(await screen.findByRole('menuitem', { name: /delete/i }))
|
||||
fireEvent.keyDown(await screen.findByRole('dialog'), { key: 'Enter' })
|
||||
|
||||
const reopened = await screen.findByRole('dialog')
|
||||
// eslint-disable-next-line no-restricted-globals -- asserting real focus requires the live document
|
||||
await waitFor(() => expect(reopened.contains(document.activeElement)).toBe(true))
|
||||
// eslint-disable-next-line no-restricted-globals -- asserting real focus requires the live document
|
||||
fireEvent.keyDown(document.activeElement!, { key: 'Enter' })
|
||||
|
||||
expect(await screen.findByText('Session deleted')).toBeTruthy()
|
||||
expect(onDelete).toHaveBeenCalledTimes(1)
|
||||
|
||||
@@ -22,14 +22,7 @@ import { Codicon } from '@/components/ui/codicon'
|
||||
import { ColorSwatches } from '@/components/ui/color-swatches'
|
||||
import { ConfirmDialog } from '@/components/ui/confirm-dialog'
|
||||
import { CopyButton } from '@/components/ui/copy-button'
|
||||
import {
|
||||
Dialog,
|
||||
DialogContent,
|
||||
DialogFooter,
|
||||
DialogHeader,
|
||||
DialogTitle,
|
||||
preventCloseButtonAutoFocus
|
||||
} from '@/components/ui/dialog'
|
||||
import { Dialog, DialogContent, DialogFooter, DialogHeader, DialogTitle } from '@/components/ui/dialog'
|
||||
import { Input } from '@/components/ui/input'
|
||||
import { renameSession } from '@/hermes'
|
||||
import { useI18n } from '@/i18n'
|
||||
@@ -580,7 +573,6 @@ function DeleteSessionDialog({ open, onOpenChange, onConfirm, sessionTitle }: De
|
||||
doneLabel={r.deleted}
|
||||
onClose={() => onOpenChange(false)}
|
||||
onConfirm={onConfirm}
|
||||
onOpenAutoFocus={preventCloseButtonAutoFocus}
|
||||
open={open}
|
||||
title={r.deleteTitle}
|
||||
/>
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import type { ReactNode } from 'react'
|
||||
import { useEffect, useState } from 'react'
|
||||
import { useEffect, useRef, useState } from 'react'
|
||||
|
||||
import { ActionStatus } from '@/components/ui/action-status'
|
||||
import { Button } from '@/components/ui/button'
|
||||
@@ -28,15 +28,12 @@ interface ConfirmDialogProps {
|
||||
destructive?: boolean
|
||||
/** Close as soon as onConfirm resolves — for optimistic actions that finish in the background. */
|
||||
dismissOnConfirm?: boolean
|
||||
/** Focus control for dialogs with no input. Pass `preventCloseButtonAutoFocus`
|
||||
* so opening doesn't land focus on the close/cancel button (which would pop
|
||||
* its tooltip with no pointer near it). */
|
||||
onOpenAutoFocus?: (event: Event) => void
|
||||
}
|
||||
|
||||
// Shared confirmation dialog: Enter confirms (from anywhere in the dialog),
|
||||
// Esc/Cancel/backdrop dismiss. Owns the pending → done → close beat and inline
|
||||
// error, so callers pass only an async onConfirm that does the work.
|
||||
// Shared confirmation dialog: opens focused on Confirm, Enter confirms (from
|
||||
// anywhere in the dialog), Esc/Cancel/backdrop dismiss. Owns the pending → done
|
||||
// → close beat and inline error, so callers pass only an async onConfirm that
|
||||
// does the work.
|
||||
export function ConfirmDialog({
|
||||
open,
|
||||
onClose,
|
||||
@@ -48,10 +45,10 @@ export function ConfirmDialog({
|
||||
doneLabel,
|
||||
cancelLabel,
|
||||
destructive = false,
|
||||
dismissOnConfirm = false,
|
||||
onOpenAutoFocus
|
||||
dismissOnConfirm = false
|
||||
}: ConfirmDialogProps) {
|
||||
const { t } = useI18n()
|
||||
const confirmRef = useRef<HTMLButtonElement>(null)
|
||||
const [status, setStatus] = useState<'done' | 'idle' | 'saving'>('idle')
|
||||
const [error, setError] = useState<null | string>(null)
|
||||
const busy = status === 'saving' || status === 'done'
|
||||
@@ -109,7 +106,14 @@ export function ConfirmDialog({
|
||||
void run()
|
||||
}
|
||||
}}
|
||||
onOpenAutoFocus={onOpenAutoFocus}
|
||||
onOpenAutoFocus={event => {
|
||||
// Focus must land inside the dialog or the handler above never sees
|
||||
// the key: it stays on whatever opened the dialog (a menu item, a
|
||||
// sidebar row) and Enter re-triggers that instead. Radix's default
|
||||
// would take the X — confirm is the button Enter maps to.
|
||||
event.preventDefault()
|
||||
confirmRef.current?.focus()
|
||||
}}
|
||||
>
|
||||
<DialogHeader>
|
||||
<DialogTitle>{title}</DialogTitle>
|
||||
@@ -127,7 +131,12 @@ export function ConfirmDialog({
|
||||
<Button disabled={busy} onClick={onClose} type="button" variant="ghost">
|
||||
{resolvedCancelLabel}
|
||||
</Button>
|
||||
<Button disabled={busy} onClick={() => void run()} variant={destructive ? 'destructive' : 'default'}>
|
||||
<Button
|
||||
disabled={busy}
|
||||
onClick={() => void run()}
|
||||
ref={confirmRef}
|
||||
variant={destructive ? 'destructive' : 'default'}
|
||||
>
|
||||
<ActionStatus
|
||||
busy={resolvedBusyLabel}
|
||||
done={resolvedDoneLabel}
|
||||
|
||||
@@ -55,7 +55,9 @@ const DIALOG_BANNER_TONES: Record<DialogBannerTone, string> = {
|
||||
// element ends up being the close button, and since Tip shows on focus as well
|
||||
// as hover, that autofocus makes the "Close" tip appear immediately with no
|
||||
// pointer ever near the button. Dialogs like that should pass this in
|
||||
// explicitly as `onOpenAutoFocus={preventCloseButtonAutoFocus}`.
|
||||
// explicitly as `onOpenAutoFocus={preventCloseButtonAutoFocus}`. Note it leaves
|
||||
// focus wherever it was — outside the dialog — so a dialog that answers keys
|
||||
// (Enter to confirm) must focus something of its own instead.
|
||||
export function preventCloseButtonAutoFocus(event: Event) {
|
||||
event.preventDefault()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user