diff --git a/frontend/src/__tests__/confirmDialog.test.ts b/frontend/src/__tests__/confirmDialog.test.ts index d57d7ab96..2a949a3f7 100644 --- a/frontend/src/__tests__/confirmDialog.test.ts +++ b/frontend/src/__tests__/confirmDialog.test.ts @@ -88,4 +88,52 @@ describe('confirmDialog', () => { expect(document.querySelector('.btn-primary')?.textContent).toBe('Reset all'); expect(document.querySelector('.btn-secondary')?.textContent).toBe('Keep them'); }); + + it('names the dialog by its title and describes it by its body', () => { + void confirmDialog({ title: 'Delete?', body: 'Permanent action.' }); + const backdrop = document.querySelector('.modal-confirm-backdrop')!; + const titleId = backdrop.getAttribute('aria-labelledby')!; + const bodyId = backdrop.getAttribute('aria-describedby')!; + expect(document.getElementById(titleId)?.textContent).toBe('Delete?'); + expect(document.getElementById(bodyId)?.textContent).toBe('Permanent action.'); + }); + + it('focuses Cancel for destructive dialogs and Confirm otherwise', () => { + void confirmDialog({ title: 't', body: 'b', destructive: true }); + expect(document.activeElement?.textContent).toBe('Cancel'); + document.body.replaceChildren(); + void confirmDialog({ title: 't', body: 'b' }); + expect(document.activeElement?.textContent).toBe('Confirm'); + }); + + it('focuses Cancel when initialFocus is cancel even if not destructive', () => { + void confirmDialog({ title: 't', body: 'b', initialFocus: 'cancel' }); + expect(document.activeElement?.textContent).toBe('Cancel'); + }); + + it('focuses the close-X when the cancel button is hidden and focus is cancel', () => { + void confirmDialog({ title: 't', body: 'b', destructive: true, hideCancelButton: true }); + expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); + }); + + it('cycles focus over all focusables in DOM order, including body inputs', () => { + const body = document.createElement('div'); + const ta = document.createElement('textarea'); + body.appendChild(ta); + void confirmDialog({ title: 't', body, destructive: true }); + const tab = (shiftKey = false): void => { + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab', shiftKey })); + }; + // DOM order: close-X, textarea, Cancel, Confirm; focus starts on Cancel. + tab(true); + expect(document.activeElement).toBe(ta); + tab(true); + expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); + tab(); + expect(document.activeElement).toBe(ta); + tab(); tab(); + expect(document.activeElement?.textContent).toBe('Confirm'); + tab(); + expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); + }); }); diff --git a/frontend/src/__tests__/navigation.test.ts b/frontend/src/__tests__/navigation.test.ts index a7a46af23..acc672af4 100644 --- a/frontend/src/__tests__/navigation.test.ts +++ b/frontend/src/__tests__/navigation.test.ts @@ -57,7 +57,7 @@ import { loadDashboard } from '../dashboard'; import { loadRecommendations } from '../recommendations'; import { loadPlans } from '../plans'; import { initHistoryDateRange, loadHistory } from '../history'; -import { loadGlobalSettings } from '../settings'; +import { loadGlobalSettings, isUnsavedChanges } from '../settings'; import { loadAutomationSettings } from '../riexchange'; import { canAccess } from '../permissions'; import { loadInventory } from '../inventory'; @@ -663,4 +663,34 @@ describe('Navigation Module', () => { expect(getInventorySubTabFromPath()).toBe('active-commitments'); }); }); + + describe('unsaved settings guard', () => { + afterEach(() => { + (isUnsavedChanges as jest.Mock).mockReturnValue(false); + }); + + const dialogButton = (label: string): HTMLButtonElement => + Array.from(document.querySelectorAll('.modal-confirm button')).find( + b => b.textContent === label + )!; + + test('asks via the styled dialog, stays put on cancel and leaves on confirm', async () => { + switchTab('admin'); + (isUnsavedChanges as jest.Mock).mockReturnValue(true); + + switchTab('home'); + expect(document.querySelector('.modal-confirm-title')?.textContent).toBe('Leave without saving?'); + expect(document.getElementById('admin-tab')?.classList.contains('active')).toBe(true); + dialogButton('Stay on this tab').click(); + await Promise.resolve(); + await Promise.resolve(); + expect(document.getElementById('admin-tab')?.classList.contains('active')).toBe(true); + + switchTab('home'); + dialogButton('Leave without saving').click(); + await Promise.resolve(); + await Promise.resolve(); + expect(document.getElementById('home-tab')?.classList.contains('active')).toBe(true); + }); + }); }); diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 503d2c2ba..cf77085cb 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -1305,13 +1305,13 @@ describe('users/userActions', () => { await userActions.bulkAddToGroup('admins'); - expect(global.confirm).toHaveBeenCalledWith( - expect.stringContaining('Admins') + expect(mockConfirmDialog).toHaveBeenCalledWith( + expect.objectContaining({ title: expect.stringContaining('Admins') }) ); }); it('should not add when cancelled', async () => { - (global.confirm as jest.Mock).mockReturnValue(false); + mockConfirmDialog.mockResolvedValueOnce(false); userState.addSelectedUserId('1'); await userActions.bulkAddToGroup('admins'); diff --git a/frontend/src/confirmDialog.ts b/frontend/src/confirmDialog.ts index 98ca74986..6b5a7f136 100644 --- a/frontend/src/confirmDialog.ts +++ b/frontend/src/confirmDialog.ts @@ -4,11 +4,21 @@ * on confirm and false on cancel / ESC / backdrop click. * * Rendered as a centred modal with backdrop, ESC cancels, focus-trap - * keeps Tab inside the dialog. The `destructive: true` flag renders the - * confirm button in the destructive variant so users see red before they - * click a deletion. + * keeps Tab and Shift+Tab inside the dialog. The `destructive: true` flag + * renders the confirm button in the destructive variant so users see red + * before they click a deletion. + * + * Initial focus lands on the safe action (Cancel) for destructive dialogs + * and for any dialog that passes `initialFocus: 'cancel'` (money-moving + * confirms), so a reflex Enter never executes the action. The dialog is + * named by its title (aria-labelledby) and described by its body + * (aria-describedby). */ +import { FOCUSABLE_SELECTOR } from './modal'; + +let dialogSeq = 0; + export interface ConfirmDialogOptions { title: string; body: string | HTMLElement; @@ -21,6 +31,11 @@ export interface ConfirmDialogOptions { // both labelled "Cancel"). Users can still dismiss via the close-X, // ESC, or a backdrop click. hideCancelButton?: boolean; + // Which control receives focus on open. Defaults to 'cancel' for + // destructive dialogs and 'confirm' otherwise. Pass 'cancel' for + // dialogs that move money so Enter cannot execute by reflex. With + // hideCancelButton the safe target is the close-X. + initialFocus?: 'cancel' | 'confirm'; } export function confirmDialog(opts: ConfirmDialogOptions): Promise { @@ -29,6 +44,11 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { backdrop.className = 'modal-confirm-backdrop'; backdrop.setAttribute('role', 'dialog'); backdrop.setAttribute('aria-modal', 'true'); + const seq = ++dialogSeq; + const titleId = `modal-confirm-title-${seq}`; + const bodyId = `modal-confirm-body-${seq}`; + backdrop.setAttribute('aria-labelledby', titleId); + backdrop.setAttribute('aria-describedby', bodyId); const dialog = document.createElement('div'); dialog.className = 'modal-confirm'; @@ -45,11 +65,13 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { dialog.appendChild(closeBtn); const titleEl = document.createElement('h3'); + titleEl.id = titleId; titleEl.className = 'modal-confirm-title'; titleEl.textContent = opts.title; dialog.appendChild(titleEl); const bodyEl = document.createElement('div'); + bodyEl.id = bodyId; bodyEl.className = 'modal-confirm-body'; if (typeof opts.body === 'string') { bodyEl.textContent = opts.body; @@ -81,7 +103,8 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { document.body.appendChild(backdrop); const previousActive = document.activeElement as HTMLElement | null; - confirmBtn.focus(); + const focusSafe = (opts.initialFocus ?? (opts.destructive ? 'cancel' : 'confirm')) === 'cancel'; + (focusSafe ? (hideCancel ? closeBtn : cancelBtn) : confirmBtn).focus(); let settled = false; const settle = (value: boolean): void => { @@ -100,15 +123,14 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { e.preventDefault(); settle(false); } else if (e.key === 'Tab') { - // Focus trap: cycle Tab between the focusable elements the dialog - // actually rendered. When hideCancelButton is true, cycle between - // close-X and confirm; otherwise include the dismiss button too. + // Focus trap: cycle over every focusable the dialog rendered (close-X, + // any inputs in a caller-supplied body, then the action buttons), in + // DOM order. e.preventDefault(); - const cycle: HTMLButtonElement[] = hideCancel - ? [confirmBtn, closeBtn] - : [confirmBtn, cancelBtn, closeBtn]; - const idx = cycle.indexOf(document.activeElement as HTMLButtonElement); - const next = cycle[(idx + 1 + cycle.length) % cycle.length] ?? confirmBtn; + const cycle = Array.from(dialog.querySelectorAll(FOCUSABLE_SELECTOR)); + const idx = cycle.indexOf(document.activeElement as HTMLElement); + const step = e.shiftKey ? -1 : 1; + const next = cycle[(idx + step + cycle.length) % cycle.length] ?? confirmBtn; next.focus(); } }; diff --git a/frontend/src/modules/registrations.ts b/frontend/src/modules/registrations.ts index 6e41989c4..81e5272c3 100644 --- a/frontend/src/modules/registrations.ts +++ b/frontend/src/modules/registrations.ts @@ -238,8 +238,23 @@ async function handleDelete(reg: AccountRegistration): Promise { } async function handleReject(reg: AccountRegistration): Promise { - const reason = prompt(`Reject registration for "${reg.account_name}"?\n\nOptional reason:`); - if (reason === null) return; // User cancelled. + const body = document.createElement('div'); + const label = document.createElement('label'); + label.textContent = 'Reason (optional)'; + label.htmlFor = 'registration-reject-reason'; + const reasonInput = document.createElement('textarea'); + reasonInput.id = 'registration-reject-reason'; + reasonInput.rows = 3; + reasonInput.className = 'registration-reject-reason'; + body.append(label, reasonInput); + const ok = await confirmDialog({ + title: `Reject registration for "${reg.account_name}"?`, + body, + confirmLabel: 'Reject registration', + destructive: true, + }); + if (!ok) return; + const reason = reasonInput.value.trim(); try { await api.rejectRegistration(reg.id, reason || undefined); diff --git a/frontend/src/navigation.ts b/frontend/src/navigation.ts index 014f11e2c..abc0408dc 100644 --- a/frontend/src/navigation.ts +++ b/frontend/src/navigation.ts @@ -18,6 +18,7 @@ import { DEFAULT_INVENTORY_SUB_SECTION, } from './inventory'; import { isAdmin } from './auth'; +import { confirmDialog } from './confirmDialog'; import { canAccess } from './permissions'; interface TabMeta { @@ -123,6 +124,16 @@ function renderNoAccess(tabId: string): void { /** * Switch between tabs */ +function confirmLeaveSettings(): Promise { + return confirmDialog({ + title: 'Leave without saving?', + body: 'You have unsaved settings changes. Leaving this tab discards them.', + confirmLabel: 'Leave without saving', + cancelLabel: 'Stay on this tab', + destructive: true, + }); +} + export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { if (!isKnownKey(TABS, tabName)) tabName = 'home'; @@ -134,7 +145,10 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { tabName !== 'admin' && isUnsavedChanges() ) { - if (!confirm('You have unsaved settings changes. Leave without saving?')) return; + void confirmLeaveSettings().then(leave => { + if (leave) switchTab(tabName, { ...opts, skipDirtyGuard: true }); + }); + return; } document.querySelectorAll('.tab-btn').forEach(btn => { @@ -359,10 +373,15 @@ export function initRouter(): void { target !== 'admin' && isUnsavedChanges() ) { - if (!confirm('You have unsaved settings changes. Leave without saving?')) { - if (delta !== 0) window.history.go(-delta); - return; - } + void confirmLeaveSettings().then(leave => { + if (leave) { + historyId = newId; + switchTab(target, { push: false, skipDirtyGuard: true }); + } else if (delta !== 0) { + window.history.go(-delta); + } + }); + return; } historyId = newId; diff --git a/frontend/src/styles/forms.css b/frontend/src/styles/forms.css index 1ea2163d2..ac372bb47 100644 --- a/frontend/src/styles/forms.css +++ b/frontend/src/styles/forms.css @@ -422,3 +422,8 @@ input:checked + .slider:before { tr.selected { background: var(--cudly-info-bg) !important; } + +.registration-reject-reason { + width: 100%; + box-sizing: border-box; +} diff --git a/frontend/src/users/userActions.ts b/frontend/src/users/userActions.ts index ab1a3bc45..8827509d0 100644 --- a/frontend/src/users/userActions.ts +++ b/frontend/src/users/userActions.ts @@ -197,9 +197,12 @@ export async function bulkAddToGroup(groupId: string): Promise { const group = availableGroups.find(g => g.id === groupId); if (!group) return; - if (!confirm(`Add ${count} user(s) to group "${group.name}"?`)) { - return; - } + const addOk = await confirmDialog({ + title: `Add ${count} user${count === 1 ? '' : 's'} to "${group.name}"?`, + body: `The ${count === 1 ? 'selected user' : 'selected users'} will gain the permissions of the "${group.name}" group.`, + confirmLabel: 'Add to group', + }); + if (!addOk) return; // Partition the selected users by whether adding this group would produce a // contradictory combination (view-only group + write-capable group, issue diff --git a/frontend/tests-e2e/history-action-confirmation.spec.ts b/frontend/tests-e2e/history-action-confirmation.spec.ts index 4d27b96d6..dc224748b 100644 --- a/frontend/tests-e2e/history-action-confirmation.spec.ts +++ b/frontend/tests-e2e/history-action-confirmation.spec.ts @@ -79,12 +79,12 @@ for (const action of actions) { fixture.failNextMutation(); await btn.click(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); await expect(btn).toBeEnabled(); await btn.click(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(2); await expect(btn).toHaveCount(0); expect(fixture.posts.every(post => post.url.includes(ID))).toBe(true); @@ -112,7 +112,7 @@ test('actual double-click and second projection cannot duplicate pending approva details.release(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); expect(detailGets).toBe(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); }); @@ -133,7 +133,7 @@ test('approve is not offered when the purchase details cannot be loaded (issue # await expect(btn).toBeEnabled(); await btn.click(); await expect(page.locator('.modal-confirm-body')).toContainText('$1,200'); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); }); @@ -200,7 +200,7 @@ test('success survives failed refresh, cached redraw and old GET; fresh same-ID await page.locator('#history-list .history-approve-btn').click(); failHistory = true; await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect(page.locator('#history-list .error')).toBeVisible(); const oldResponse = page.waitForResponse(response => response.url().includes('/api/history') && response.status() === 200); oldGet.release(); @@ -215,7 +215,7 @@ test('success survives failed refresh, cached redraw and old GET; fresh same-ID await page.getByRole('button', { name: 'Load History', exact: true }).click(); await page.locator('#history-list .history-revoke-btn').click(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(2); expect(fixture.posts[1]!.url).toContain(`/purchases/${ID}/revoke`); }); @@ -241,6 +241,6 @@ test('completed revoke owns its row before refund quote and recovers from quote expect(fixture.posts).toHaveLength(0); await btn.click(); await expect(page.locator('.modal-confirm-body')).toContainText('100.00 USD'); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); }); diff --git a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts index 8560d6b11..060679ba9 100644 --- a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts +++ b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts @@ -109,9 +109,10 @@ for (const multiple of [false, true]) { await expect(dialog(page)).toContainText('us-east-1'); if (multiple) await expect(dialog(page)).toContainText(`3 × ${secondOffering}`); expect(state.requests).toHaveLength(0); - await expect(confirm(page)).toBeFocused(); - await page.keyboard.press('Tab'); + // Destructive dialogs open on the safe action, so a reflex Enter cancels. await expect(dialog(page).getByRole('button', { name: 'Cancel', exact: true })).toBeFocused(); + await page.keyboard.press('Tab'); + await expect(confirm(page)).toBeFocused(); await confirm(page).click(); await expect(page.locator('#modal-exchange-result')).toContainText('Exchange completed. ID: synthetic-exchange'); expect(state.requests).toEqual([{ path: '/api/ri-exchange/execute', body: { @@ -143,7 +144,7 @@ for (const action of ['Execute', 'Approve']) { await expect(trigger).toBeEnabled(); await expect(trigger).toBeFocused(); await trigger.click(); - await page.keyboard.press('Enter'); + await confirm(page).click(); await expect.poll(() => state.requests.length).toBe(1); if (action === 'Approve') { expect(state.requests[0]).toEqual({ path: `/api/ri-exchange/approve/${state.history.id}`, body: null });