From 080e3498a744d3dc0e17e8b22905a3d1cb2f826c Mon Sep 17 00:00:00 2001 From: lucaronin Date: Sat, 25 Apr 2026 18:20:32 +0200 Subject: [PATCH] fix: submit delete confirmation on enter --- package.json | 2 +- src/components/ConfirmDeleteDialog.test.tsx | 113 ++++++------ src/components/ConfirmDeleteDialog.tsx | 162 +++++++++++++++++- tests/smoke/delete-note-nonblocking.spec.ts | 2 +- tests/smoke/multi-selection-shortcuts.spec.ts | 1 - 5 files changed, 219 insertions(+), 61 deletions(-) diff --git a/package.json b/package.json index 732d2bf3..596da2a5 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "test": "vitest run", "test:watch": "vitest", "test:e2e": "playwright test", - "playwright:smoke": "playwright test --config playwright.smoke.config.ts tests/smoke/example.spec.ts tests/smoke/fix-ai-chat-empty-body-v3.spec.ts tests/smoke/fix-crash-create-note.spec.ts tests/smoke/fresh-start-telemetry-onboarding.spec.ts tests/smoke/getting-started-template.spec.ts tests/smoke/h1-title-decoupled.spec.ts tests/smoke/h1-untitled-auto-rename.spec.ts tests/smoke/keyboard-command-routing.spec.ts tests/smoke/linkify-init-warnings.spec.ts tests/smoke/multi-selection-shortcuts.spec.ts tests/smoke/wikilink-path-fix.spec.ts", + "playwright:smoke": "playwright test --config playwright.smoke.config.ts tests/smoke/delete-note-nonblocking.spec.ts tests/smoke/example.spec.ts tests/smoke/fix-ai-chat-empty-body-v3.spec.ts tests/smoke/fix-crash-create-note.spec.ts tests/smoke/fresh-start-telemetry-onboarding.spec.ts tests/smoke/getting-started-template.spec.ts tests/smoke/h1-title-decoupled.spec.ts tests/smoke/h1-untitled-auto-rename.spec.ts tests/smoke/keyboard-command-routing.spec.ts tests/smoke/linkify-init-warnings.spec.ts tests/smoke/multi-selection-shortcuts.spec.ts tests/smoke/wikilink-path-fix.spec.ts", "playwright:regression": "playwright test tests/smoke/", "playwright:integration": "playwright test --config playwright.integration.config.ts", "test:coverage": "node scripts/run-vitest-coverage.mjs", diff --git a/src/components/ConfirmDeleteDialog.test.tsx b/src/components/ConfirmDeleteDialog.test.tsx index 19446a68..bee47b6a 100644 --- a/src/components/ConfirmDeleteDialog.test.tsx +++ b/src/components/ConfirmDeleteDialog.test.tsx @@ -1,81 +1,94 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' -import { render, screen, fireEvent } from '@testing-library/react' +import { render, screen, fireEvent, waitFor } from '@testing-library/react' import { ConfirmDeleteDialog } from './ConfirmDeleteDialog' -describe('ConfirmDeleteDialog', () => { - const onConfirm = vi.fn() - const onCancel = vi.fn() +const defaultProps = { + open: true, + title: 'Delete permanently?', + message: 'This cannot be undone.', +} +const onConfirm = vi.fn() +const onCancel = vi.fn() + +function renderDialog(overrides: Partial> = {}) { + return render( + , + ) +} + +describe('ConfirmDeleteDialog', () => { beforeEach(() => { vi.clearAllMocks() }) it('renders with title and message', () => { - render( - , - ) + renderDialog() expect(screen.getByText('Delete permanently?')).toBeInTheDocument() expect(screen.getByText('This cannot be undone.')).toBeInTheDocument() }) it('calls onConfirm when delete button clicked', () => { - render( - , - ) + renderDialog() fireEvent.click(screen.getByTestId('confirm-delete-btn')) expect(onConfirm).toHaveBeenCalledTimes(1) }) + it('focuses the destructive action when the dialog opens', async () => { + renderDialog() + + await waitFor(() => { + expect(screen.getByTestId('confirm-delete-btn')).toHaveFocus() + }) + }) + + it('submits the destructive action when Enter is pressed', () => { + renderDialog() + + fireEvent.keyDown(screen.getByTestId('confirm-delete-dialog'), { key: 'Enter' }) + + expect(onConfirm).toHaveBeenCalledTimes(1) + }) + + it('does not submit twice when Enter repeats', () => { + renderDialog() + + fireEvent.keyDown(screen.getByTestId('confirm-delete-dialog'), { key: 'Enter' }) + fireEvent.keyDown(screen.getByTestId('confirm-delete-dialog'), { key: 'Enter', repeat: true }) + + expect(onConfirm).toHaveBeenCalledTimes(1) + }) + + it('treats Enter as the primary confirm shortcut even from the cancel button', () => { + renderDialog() + + fireEvent.keyDown(screen.getByText('Cancel'), { key: 'Enter' }) + + expect(onConfirm).toHaveBeenCalledTimes(1) + }) + it('calls onCancel when cancel button clicked', () => { - render( - , - ) + renderDialog() fireEvent.click(screen.getByText('Cancel')) expect(onCancel).toHaveBeenCalledTimes(1) }) it('does not render when open is false', () => { - render( - , - ) + renderDialog({ open: false }) expect(screen.queryByText('Delete permanently?')).not.toBeInTheDocument() }) it('uses custom confirm label when provided', () => { - render( - , - ) + renderDialog({ + title: 'Empty Trash?', + message: 'Delete all notes?', + confirmLabel: 'Empty Trash', + }) expect(screen.getByText('Empty Trash')).toBeInTheDocument() }) }) diff --git a/src/components/ConfirmDeleteDialog.tsx b/src/components/ConfirmDeleteDialog.tsx index d942723e..f4c6ba78 100644 --- a/src/components/ConfirmDeleteDialog.tsx +++ b/src/components/ConfirmDeleteDialog.tsx @@ -1,4 +1,5 @@ -import { memo } from 'react' +import { memo, useCallback, useEffect, useRef } from 'react' +import type { FormEvent, KeyboardEvent as ReactKeyboardEvent } from 'react' import { Trash } from '@phosphor-icons/react' import { Dialog, @@ -19,6 +20,47 @@ interface ConfirmDeleteDialogProps { onCancel: () => void } +function focusConfirmButton(confirmButton: HTMLButtonElement | null) { + confirmButton?.focus() +} + +function focusPrimaryDeleteButton(root: ParentNode | null) { + focusConfirmButton(root?.querySelector('[data-confirm-delete-primary="true"]') ?? null) +} + +function scheduleConfirmButtonFocus(root: ParentNode | null) { + const focusDelays = [0, 50, 150] + return focusDelays.map((delay) => ( + window.setTimeout(() => { + focusPrimaryDeleteButton(root) + }, delay) + )) +} + +type ConfirmShortcutEvent = Pick< + KeyboardEvent, + 'key' | 'defaultPrevented' | 'repeat' | 'metaKey' | 'ctrlKey' | 'altKey' | 'shiftKey' | 'isComposing' +> & { + nativeEvent?: Pick +} + +function isComposingEvent(event: ConfirmShortcutEvent) { + return event.isComposing || event.nativeEvent?.isComposing === true +} + +function isConfirmShortcut(event: ConfirmShortcutEvent) { + return ( + event.key === 'Enter' + && !event.defaultPrevented + && !event.repeat + && !event.metaKey + && !event.ctrlKey + && !event.altKey + && !event.shiftKey + && !isComposingEvent(event) + ) +} + export const ConfirmDeleteDialog = memo(function ConfirmDeleteDialog({ open, title, @@ -27,9 +69,87 @@ export const ConfirmDeleteDialog = memo(function ConfirmDeleteDialog({ onConfirm, onCancel, }: ConfirmDeleteDialogProps) { + const confirmingRef = useRef(false) + const cancelFocusIsIntentionalRef = useRef(false) + + useEffect(() => { + confirmingRef.current = false + cancelFocusIsIntentionalRef.current = false + if (!open) { + return + } + + focusPrimaryDeleteButton(document) + const timeoutIds = scheduleConfirmButtonFocus(document) + + return () => { + timeoutIds.forEach((timeoutId) => { + window.clearTimeout(timeoutId) + }) + } + }, [open]) + + const handleConfirm = useCallback(() => { + if (confirmingRef.current) { + return + } + + confirmingRef.current = true + onConfirm() + }, [onConfirm]) + + const handleSubmit = useCallback((event: FormEvent) => { + event.preventDefault() + handleConfirm() + }, [handleConfirm]) + + useEffect(() => { + if (!open) { + return + } + + const handleWindowKeyDown = (event: KeyboardEvent) => { + if (!isConfirmShortcut(event)) { + return + } + + event.preventDefault() + handleConfirm() + } + + window.addEventListener('keydown', handleWindowKeyDown, true) + + return () => { + window.removeEventListener('keydown', handleWindowKeyDown, true) + } + }, [handleConfirm, open]) + + const handleKeyDown = useCallback((event: ReactKeyboardEvent) => { + if (!isConfirmShortcut(event)) { + return + } + + event.preventDefault() + handleConfirm() + }, [handleConfirm]) + return ( { if (!isOpen) onCancel() }}> - + { + if (event.key === 'Tab') { + cancelFocusIsIntentionalRef.current = true + } + }} + onOpenAutoFocus={(event) => { + event.preventDefault() + focusPrimaryDeleteButton(event.currentTarget) + scheduleConfirmButtonFocus(event.currentTarget) + }} + > @@ -37,12 +157,38 @@ export const ConfirmDeleteDialog = memo(function ConfirmDeleteDialog({ {message} - - - - +
+ + + + +
) diff --git a/tests/smoke/delete-note-nonblocking.spec.ts b/tests/smoke/delete-note-nonblocking.spec.ts index 88ada93b..d71d58a7 100644 --- a/tests/smoke/delete-note-nonblocking.spec.ts +++ b/tests/smoke/delete-note-nonblocking.spec.ts @@ -35,7 +35,7 @@ test.describe('responsive note deletion', () => { await triggerShortcutCommand(page, APP_COMMAND_IDS.noteDelete) await expect(page.getByTestId('confirm-delete-dialog')).toBeVisible({ timeout: 5_000 }) - await page.getByTestId('confirm-delete-btn').focus() + await expect(page.getByTestId('confirm-delete-btn')).toBeFocused() await page.keyboard.press('Enter') const progressNotice = page.getByTestId('delete-progress-notice') diff --git a/tests/smoke/multi-selection-shortcuts.spec.ts b/tests/smoke/multi-selection-shortcuts.spec.ts index 0e97d6bd..98d4d411 100644 --- a/tests/smoke/multi-selection-shortcuts.spec.ts +++ b/tests/smoke/multi-selection-shortcuts.spec.ts @@ -65,7 +65,6 @@ test.describe('multi-selection shortcuts', () => { await expect(dialog).toBeVisible({ timeout: 5_000 }) await expect(dialog).toContainText(/Delete \d+ notes permanently\?/) - await page.keyboard.press('Tab') await expect(confirmButton).toBeFocused() await page.keyboard.press('Enter')