diff --git a/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-action.svelte b/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-action.svelte index fbb3f768a..fdf84e33a 100644 --- a/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-action.svelte +++ b/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-action.svelte @@ -22,6 +22,7 @@ diff --git a/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.component.test.ts b/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.component.test.ts new file mode 100644 index 000000000..1904260c1 --- /dev/null +++ b/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.component.test.ts @@ -0,0 +1,294 @@ +import { flushSync, mount, tick, unmount } from 'svelte'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import AlertDialogWithButtons from './testStubs/AlertDialogWithButtons.svelte'; +import { reactiveProps } from '$lib/features/sessions/testStubs/reactiveProps.svelte'; + +// An alert dialog opens with focus on its own container (bits-ui puts it +// there, not on a button), so nothing native answers Return. The content +// presses the default button itself: the Action, unless it is destructive. +// The real chrome is mounted here; the feature tests stub it. + +let component: ReturnType | null = null; +let target: HTMLElement; +let windowKeydown: ReturnType void>>; + +/** bits-ui focuses the container on a 0 ms timer after mount; let it, then render. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 30)); + await tick(); + flushSync(); +} + +type FixtureProps = NonNullable[1]>; + +async function render(overrides: Partial = {}) { + const props = reactiveProps({ + open: true, + onConfirm: vi.fn(), + onCancel: vi.fn(), + onOpenChange: vi.fn(), + ...overrides, + }); + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(AlertDialogWithButtons, { target, props }); + flushSync(); + await settle(); + return props; +} + +function content(): HTMLElement { + const el = document.querySelector('[data-slot="alert-dialog-content"]'); + if (!el) throw new Error('alert dialog content not rendered'); + return el; +} + +function button(slot: 'action' | 'cancel'): HTMLButtonElement { + const el = content().querySelector(`[data-slot="alert-dialog-${slot}"]`); + if (!el) throw new Error(`alert dialog ${slot} not rendered`); + return el; +} + +/** A Return keydown on `el`; returns the event so a test can see who took it. */ +function pressReturn(el: Element, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +beforeEach(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + disconnect() {} + unobserve() {} + } + ); + windowKeydown = vi.fn(); + window.addEventListener('keydown', windowKeydown); +}); + +afterEach(() => { + window.removeEventListener('keydown', windowKeydown); + if (component) unmount(component); + component = null; + target.remove(); + vi.unstubAllGlobals(); +}); + +describe('AlertDialog.Content Return key', () => { + it('opens with focus on the dialog itself, not on a button', async () => { + await render(); + + expect(document.activeElement).toBe(content()); + expect(content().getAttribute('role')).toBe('alertdialog'); + }); + + it('confirms on Return while focus is on the dialog', async () => { + const props = await render(); + + const event = pressReturn(document.activeElement!); + + expect(props.onConfirm).toHaveBeenCalledTimes(1); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(true); + }); + + it('confirms the same way when focus started on a button behind the dialog', async () => { + target = document.createElement('div'); + document.body.appendChild(target); + const props = reactiveProps({ open: false, onConfirm: vi.fn(), onCancel: vi.fn() }); + component = mount(AlertDialogWithButtons, { target, props }); + flushSync(); + document.querySelector('[data-testid="page-button"]')!.focus(); + + props.open = true; + flushSync(); + await settle(); + expect(document.activeElement).toBe(content()); + pressReturn(document.activeElement!); + + expect(props.onConfirm).toHaveBeenCalledTimes(1); + }); + + it('confirms on Return aimed at text inside the dialog', async () => { + const props = await render(); + + pressReturn(content().querySelector('[data-testid="description"]')!); + + expect(props.onConfirm).toHaveBeenCalledTimes(1); + }); + + it('keeps a handled Return from listeners on the window', async () => { + await render(); + + pressReturn(document.activeElement!); + + expect(windowKeydown).not.toHaveBeenCalled(); + }); + + it('marks the Action with its variant', async () => { + await render({ variant: 'destructive' }); + + expect(button('action').dataset.variant).toBe('destructive'); + }); + + it('leaves Return alone when the Action is destructive', async () => { + const props = await render({ variant: 'destructive' }); + + const event = pressReturn(document.activeElement!); + + expect(props.onConfirm).not.toHaveBeenCalled(); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(props.onOpenChange).not.toHaveBeenCalled(); + expect(props.open).toBe(true); + // Untouched, as before there was any Return handling. + expect(event.defaultPrevented).toBe(false); + expect(windowKeydown).toHaveBeenCalledTimes(1); + }); + + it('leaves Return alone when the caller says none, whatever the variant', async () => { + const props = await render({ defaultAction: 'none' }); + + const event = pressReturn(document.activeElement!); + + expect(props.onConfirm).not.toHaveBeenCalled(); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(props.onOpenChange).not.toHaveBeenCalled(); + expect(props.open).toBe(true); + expect(event.defaultPrevented).toBe(false); + expect(windowKeydown).toHaveBeenCalledTimes(1); + }); + + it('does not confirm while the Action is disabled', async () => { + const props = await render({ actionDisabled: true }); + + const event = pressReturn(document.activeElement!); + + expect(props.onConfirm).not.toHaveBeenCalled(); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it('does not confirm on a Return that commits an IME composition', async () => { + const props = await render(); + + pressReturn(document.activeElement!, { isComposing: true }); + pressReturn(document.activeElement!, { keyCode: 229 }); + + expect(props.onConfirm).not.toHaveBeenCalled(); + }); + + it('does not confirm on a held Return repeating', async () => { + const props = await render(); + + pressReturn(document.activeElement!, { repeat: true }); + + expect(props.onConfirm).not.toHaveBeenCalled(); + }); + + it.each(['metaKey', 'ctrlKey', 'altKey', 'shiftKey'] as const)( + 'does not confirm on %s+Return', + async (modifier) => { + const props = await render(); + + pressReturn(document.activeElement!, { [modifier]: true }); + + expect(props.onConfirm).not.toHaveBeenCalled(); + } + ); + + it('only cancels on Return on a focused Cancel', async () => { + const props = await render(); + button('cancel').focus(); + + pressReturn(document.activeElement!); + await settle(); + + expect(props.onConfirm).not.toHaveBeenCalled(); + expect(props.onOpenChange).toHaveBeenCalledTimes(1); + expect(props.onOpenChange).toHaveBeenCalledWith(false); + expect(props.open).toBe(false); + }); + + it('does not press the Action a second time on Return on the focused Action', async () => { + const props = await render(); + button('action').focus(); + + // The browser turns this keydown into the button's own click; the + // dialog must not add one. + const event = pressReturn(document.activeElement!); + + expect(props.onConfirm).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it('cancels on Return when the caller makes Cancel the default', async () => { + const props = await render({ defaultAction: 'cancel', withAction: false }); + + pressReturn(document.activeElement!); + await settle(); + + expect(props.onCancel).toHaveBeenCalledTimes(1); + expect(props.open).toBe(false); + }); + + it('does not cancel while a default Cancel is disabled', async () => { + const props = await render({ + defaultAction: 'cancel', + withAction: false, + cancelDisabled: true, + }); + + pressReturn(document.activeElement!); + await settle(); + + expect(props.onCancel).not.toHaveBeenCalled(); + expect(props.open).toBe(true); + }); + + it('does nothing on Return when there is no Action to default to', async () => { + const props = await render({ withAction: false }); + + const event = pressReturn(document.activeElement!); + await settle(); + + expect(props.onCancel).not.toHaveBeenCalled(); + expect(props.open).toBe(true); + expect(event.defaultPrevented).toBe(false); + }); + + it("runs the caller's keydown handler first, which can take the Return itself", async () => { + const seen: string[] = []; + const props = await render({ + onkeydown: (event) => { + seen.push(event.key); + event.preventDefault(); + }, + }); + + pressReturn(document.activeElement!); + + expect(seen).toEqual(['Enter']); + expect(props.onConfirm).not.toHaveBeenCalled(); + }); + + it('still cancels on Escape', async () => { + const props = await render(); + + document.activeElement!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }) + ); + await settle(); + + expect(props.onConfirm).not.toHaveBeenCalled(); + expect(props.open).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.svelte b/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.svelte index a66da5fd3..b0750829f 100644 --- a/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.svelte +++ b/apps/staged/src/lib/components/ui/alert-dialog/alert-dialog-content.svelte @@ -7,6 +7,10 @@ import { onMount } from 'svelte'; import { viewport, watchViewport } from '$lib/shared/viewport.svelte'; import { restoreFocusWithoutScroll } from '$lib/shared/restoreFocusWithoutScroll'; + import { findEnabledButton, isDefaultActionKeydown } from '$lib/shared/dialogDefaultAction'; + + /** Which button a plain Return presses; see `defaultAction`. */ + type DefaultAction = 'action' | 'cancel' | 'none'; let { ref = $bindable(null), @@ -15,9 +19,18 @@ portalProps, onOpenAutoFocus, onCloseAutoFocus, + defaultAction, + onkeydown, ...restProps }: WithoutChild & { size?: 'default' | 'sm'; + /** + * The button a plain Return presses while focus is on the dialog rather + * than on one of its controls: the Action, the Cancel, or neither. Left + * unset, it is the Action, unless that is destructive: Return never + * confirms a destructive action by default, so there it does nothing. + */ + defaultAction?: DefaultAction; portalProps?: WithoutChildrenOrChild>; } = $props(); @@ -36,6 +49,26 @@ onCloseAutoFocus?.(event); focusRestore.restore(event); } + + // The dialog opens with focus on its own container, where no button hears + // Return. The default button is clicked, not called, so its disabled state + // and click handler apply as they do for the mouse. + function handleKeydown(event: KeyboardEvent & { currentTarget: EventTarget & HTMLDivElement }) { + onkeydown?.(event); + if (!isDefaultActionKeydown(event)) return; + const content = event.currentTarget; + const action = content.querySelector('[data-slot="alert-dialog-action"]'); + const which: DefaultAction = + defaultAction ?? (action?.dataset.variant === 'destructive' ? 'none' : 'action'); + if (which === 'none') return; + const button = findEnabledButton(content, `[data-slot="alert-dialog-${which}"]`); + if (!button) return; + // Stopped as well as prevented: views under the dialog listen on the + // window (the splash form creates a project on Return). + event.preventDefault(); + event.stopPropagation(); + button.click(); + } @@ -58,6 +91,7 @@ )} onOpenAutoFocus={handleOpenAutoFocus} onCloseAutoFocus={handleCloseAutoFocus} + onkeydown={handleKeydown} preventOverflowTextSelection={false} preventScroll={false} {...restProps} diff --git a/apps/staged/src/lib/components/ui/alert-dialog/testStubs/AlertDialogWithButtons.svelte b/apps/staged/src/lib/components/ui/alert-dialog/testStubs/AlertDialogWithButtons.svelte new file mode 100644 index 000000000..552b5d3a4 --- /dev/null +++ b/apps/staged/src/lib/components/ui/alert-dialog/testStubs/AlertDialogWithButtons.svelte @@ -0,0 +1,54 @@ + + + +Elsewhere on the page + + + + + Scan still running + + Files not listed yet may be lost. + + + + Cancel + {#if withAction} + + Confirm + + {/if} + + + diff --git a/apps/staged/src/lib/components/ui/dialog/dialog-content.component.test.ts b/apps/staged/src/lib/components/ui/dialog/dialog-content.component.test.ts index 98aa506f1..07beb16cf 100644 --- a/apps/staged/src/lib/components/ui/dialog/dialog-content.component.test.ts +++ b/apps/staged/src/lib/components/ui/dialog/dialog-content.component.test.ts @@ -1,6 +1,7 @@ import { flushSync, mount, tick, unmount } from 'svelte'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import DialogBesideToaster from './testStubs/DialogBesideToaster.svelte'; +import DialogWithForm from './testStubs/DialogWithForm.svelte'; import { reactiveProps } from '$lib/features/sessions/testStubs/reactiveProps.svelte'; // bits-ui closes a dialog on a pointer press outside its content. The app's @@ -93,3 +94,345 @@ describe('Dialog.Content outside interactions', () => { expect(props.open).toBe(true); }); }); + +// A dialog with a form submits it on a plain Return from anywhere inside +// that has no use of its own for the key. To hear the key it takes the focus +// itself as it opens, unless something inside already has it. Dialogs with +// nothing for Return to submit are left exactly as they were: a viewer, a +// form around a multiline editor (Cmd+Return submits those), and any dialog +// whose caller handles the open focus. + +type FormFixtureProps = NonNullable[1]>; + +/** Open the form fixture from a focused button on the page behind it. */ +async function openFromPage(overrides: Partial = {}) { + const props = reactiveProps({ + open: false, + onSubmit: vi.fn(), + onCancel: vi.fn(), + ...overrides, + }); + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(DialogWithForm, { target, props }); + flushSync(); + pageButton().focus(); + props.open = true; + flushSync(); + await settle(); + return props; +} + +function pageButton(): HTMLElement { + return document.querySelector('[data-testid="page-button"]')!; +} + +function dialogContent(): HTMLElement { + const el = document.querySelector('[data-slot="dialog-content"]'); + if (!el) throw new Error('dialog content not rendered'); + return el; +} + +function part(testid: string): HTMLElement { + const el = dialogContent().querySelector(`[data-testid="${testid}"]`); + if (!el) throw new Error(`no ${testid} in the dialog`); + return el; +} + +/** A Return keydown on `el`; returns the event so a test can see who took it. */ +function pressReturn(el: Element, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +describe('Dialog.Content open focus', () => { + it('takes the focus itself when it has a form to submit', async () => { + await openFromPage(); + + expect(document.activeElement).toBe(dialogContent()); + }); + + it('takes it while the submit button is still disabled', async () => { + await openFromPage({ submitDisabled: true }); + + expect(document.activeElement).toBe(dialogContent()); + }); + + it('leaves the focus on a field the dialog focuses itself', async () => { + await openFromPage({ focusField: true }); + + expect(document.activeElement).toBe(part('field')); + }); + + it('leaves the focus where it was for a dialog with nothing to submit', async () => { + await openFromPage({ body: 'viewer' }); + + expect(document.activeElement).toBe(pageButton()); + }); + + it.each(['textarea', 'prompt'] as const)( + 'leaves the focus where it was for a form with a multiline editor (%s)', + async (body) => { + await openFromPage({ body }); + + expect(document.activeElement).toBe(pageButton()); + } + ); + + it('leaves the focus where it was when the caller says none', async () => { + await openFromPage({ defaultAction: 'none' }); + + expect(document.activeElement).toBe(pageButton()); + }); + + it("leaves the focus to the caller's handler, which can keep it unset", async () => { + const onOpenAutoFocus = vi.fn((event: Event) => event.preventDefault()); + await openFromPage({ onOpenAutoFocus }); + + expect(onOpenAutoFocus).toHaveBeenCalledTimes(1); + expect(document.activeElement).toBe(pageButton()); + }); + + it("leaves the focus where the caller's handler puts it", async () => { + await openFromPage({ + onOpenAutoFocus: (event) => { + event.preventDefault(); + part('cancel').focus(); + }, + }); + + expect(document.activeElement).toBe(part('cancel')); + }); + + it('takes no focus once the dialog has closed again', async () => { + const props = reactiveProps({ open: false }); + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(DialogWithForm, { target, props }); + flushSync(); + pageButton().focus(); + + props.open = true; + flushSync(); + props.open = false; + flushSync(); + await settle(); + + expect(document.activeElement).toBe(pageButton()); + }); +}); + +describe('Dialog.Content Return key', () => { + let windowKeydown: ReturnType void>>; + + beforeEach(() => { + windowKeydown = vi.fn(); + window.addEventListener('keydown', windowKeydown); + }); + + afterEach(() => { + window.removeEventListener('keydown', windowKeydown); + }); + + it('submits the form on Return as it opens, with focus on the dialog', async () => { + const props = await openFromPage(); + + const event = pressReturn(document.activeElement!); + + expect(props.onSubmit).toHaveBeenCalledTimes(1); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(true); + }); + + it('submits on Return aimed at text inside the dialog', async () => { + const props = await openFromPage(); + + pressReturn(part('text')); + + expect(props.onSubmit).toHaveBeenCalledTimes(1); + }); + + it('submits on Return on the dialog after its own field had the focus', async () => { + const props = await openFromPage({ focusField: true }); + // Where WebKit puts the focus after a click on a picker row or a label. + dialogContent().focus(); + + pressReturn(document.activeElement!); + + expect(props.onSubmit).toHaveBeenCalledTimes(1); + }); + + it('keeps a handled Return from listeners on the window', async () => { + await openFromPage(); + + pressReturn(document.activeElement!); + + expect(windowKeydown).not.toHaveBeenCalled(); + }); + + it('does not submit while the submit button is disabled, and does once it is not', async () => { + const props = await openFromPage({ submitDisabled: true }); + + const event = pressReturn(document.activeElement!); + expect(props.onSubmit).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + + props.submitDisabled = false; + flushSync(); + pressReturn(document.activeElement!); + expect(props.onSubmit).toHaveBeenCalledTimes(1); + }); + + it('does not add a submit to a Return in a field, which submits on its own', async () => { + const props = await openFromPage({ focusField: true }); + + // The browser turns this keydown into the form's submit; the dialog + // must not add one. + const event = pressReturn(document.activeElement!); + + expect(props.onSubmit).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it.each(['cancel', 'row', 'submit'] as const)( + 'leaves a Return on the focused %s button to that button', + async (testid) => { + const props = await openFromPage(); + part(testid).focus(); + + const event = pressReturn(document.activeElement!); + + expect(props.onSubmit).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + } + ); + + it('does not submit on a Return that commits an IME composition', async () => { + const props = await openFromPage(); + + pressReturn(document.activeElement!, { isComposing: true }); + pressReturn(document.activeElement!, { keyCode: 229 }); + + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it('does not submit on a held Return repeating', async () => { + const props = await openFromPage(); + + pressReturn(document.activeElement!, { repeat: true }); + + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it.each(['metaKey', 'ctrlKey', 'altKey', 'shiftKey'] as const)( + 'does not submit on %s+Return', + async (modifier) => { + const props = await openFromPage(); + + pressReturn(document.activeElement!, { [modifier]: true }); + + expect(props.onSubmit).not.toHaveBeenCalled(); + } + ); + + it.each(['textarea', 'prompt'] as const)( + 'never submits a form with a multiline editor on a plain Return (%s)', + async (body) => { + const props = await openFromPage({ body }); + + // In the editor, on the dialog (after a click on its header) and on + // its text: untouched, as before there was any Return handling. + const events = [part('editor'), dialogContent(), part('text')].map((el) => pressReturn(el)); + + expect(props.onSubmit).not.toHaveBeenCalled(); + expect(events.map((event) => event.defaultPrevented)).toEqual([false, false, false]); + expect(windowKeydown).toHaveBeenCalledTimes(3); + } + ); + + it('leaves Return alone on a list holding the focus, and in a panel docked in the dialog', async () => { + const props = await openFromPage(); + + part('listbox').focus(); + expect(document.activeElement).toBe(part('listbox')); + const onList = pressReturn(document.activeElement!); + + part('docked-text').focus(); + expect(document.activeElement).toBe(part('docked-text')); + const inPanel = pressReturn(document.activeElement!); + + // A pick in the list, a send in the panel: not this dialog's submit. + expect(props.onSubmit).not.toHaveBeenCalled(); + expect([onList.defaultPrevented, inPanel.defaultPrevented]).toEqual([false, false]); + expect(windowKeydown).toHaveBeenCalledTimes(2); + + // The dialog carries role="dialog" itself, and still submits. + pressReturn(dialogContent()); + expect(props.onSubmit).toHaveBeenCalledTimes(1); + }); + + it('leaves Return alone when the caller says none', async () => { + const props = await openFromPage({ defaultAction: 'none' }); + + const event = pressReturn(dialogContent()); + + expect(props.onSubmit).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + expect(windowKeydown).toHaveBeenCalledTimes(1); + }); + + it('submits a multiline form from the dialog when the caller says submit, never from its editor', async () => { + const props = await openFromPage({ body: 'textarea', defaultAction: 'submit' }); + expect(document.activeElement).toBe(dialogContent()); + + pressReturn(part('editor')); + expect(props.onSubmit).not.toHaveBeenCalled(); + + pressReturn(dialogContent()); + expect(props.onSubmit).toHaveBeenCalledTimes(1); + }); + + it('leaves Return alone in a dialog with nothing to submit', async () => { + const props = await openFromPage({ body: 'viewer' }); + + const event = pressReturn(part('text')); + + expect(props.open).toBe(true); + expect(event.defaultPrevented).toBe(false); + expect(windowKeydown).toHaveBeenCalledTimes(1); + }); + + it("runs the caller's keydown handler first, which can take the Return itself", async () => { + const seen: string[] = []; + const props = await openFromPage({ + onkeydown: (event) => { + seen.push(event.key); + event.preventDefault(); + }, + }); + + pressReturn(document.activeElement!); + + expect(seen).toEqual(['Enter']); + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it('still closes on Escape', async () => { + const props = await openFromPage(); + + document.activeElement!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }) + ); + await settle(); + + expect(props.onSubmit).not.toHaveBeenCalled(); + expect(props.open).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/components/ui/dialog/dialog-content.svelte b/apps/staged/src/lib/components/ui/dialog/dialog-content.svelte index 368dd79e0..7c68504d3 100644 --- a/apps/staged/src/lib/components/ui/dialog/dialog-content.svelte +++ b/apps/staged/src/lib/components/ui/dialog/dialog-content.svelte @@ -12,6 +12,11 @@ import { visibleViewport, watchVisualViewport } from '$lib/shared/visualViewport.svelte'; import { restoreFocusWithoutScroll } from '$lib/shared/restoreFocusWithoutScroll'; import { getWindowSync } from '$lib/transport'; + import { + findEnabledSubmitButton, + findReturnSubmitForm, + isDefaultActionKeydown, + } from '$lib/shared/dialogDefaultAction'; let { ref = $bindable(null), @@ -21,7 +26,9 @@ children, showCloseButton = true, fullScreenOnMobile = true, + defaultAction, onpointerdown: onPointerDown, + onkeydown: onKeydown, onOpenAutoFocus, onCloseAutoFocus, onInteractOutside, @@ -31,6 +38,15 @@ children: Snippet; showCloseButton?: boolean; fullScreenOnMobile?: boolean; + /** + * What a plain Return does while focus is on the dialog rather than on + * one of its controls: `submit` presses the submit button of the + * dialog's form, `none` does nothing. Left unset, it submits, unless the + * form holds a multiline editor: there Return breaks a line and + * Cmd+Return submits, so plain Return does nothing. A dialog with no + * form has nothing to submit either way. + */ + defaultAction?: 'submit' | 'none'; } = $props(); // Keep `viewport.isMobile` live even when the host screen never subscribed. @@ -139,11 +155,58 @@ // keyboard, bits-ui's restore (which shows the trigger) is kept. const focusRestore = restoreFocusWithoutScroll(() => viewport.hasCoarsePointer); + /** The form a plain Return submits here, if any; see `defaultAction`. */ + function returnSubmitForm(content: HTMLElement): HTMLFormElement | null { + if (defaultAction === 'none') return null; + return findReturnSubmitForm(content, defaultAction === 'submit'); + } + function handleOpenAutoFocus(event: Event) { focusRestore.capture(); - // Nothing is focused on open unless the caller says otherwise. - if (onOpenAutoFocus) onOpenAutoFocus(event); - else event.preventDefault(); + if (onOpenAutoFocus) { + onOpenAutoFocus(event); + return; + } + // No control is focused on open unless the caller says otherwise: + // bits-ui would pick the first one, usually Close or Cancel. + event.preventDefault(); + focusContentForReturn(); + } + + // A dialog that submits on Return has to hold the focus to hear the key: + // left on the page behind, Return goes to whatever opened the dialog. The + // dialog itself takes it, as an alert dialog does, a frame after opening + // so that a field the dialog focuses on its own keeps it. A dialog with + // nothing for Return to submit (a viewer, a multiline editor) is left as + // it was, with focus where it started. + function focusContentForReturn() { + requestAnimationFrame(() => { + const content = ref; + if (!content?.isConnected || content.dataset.state !== 'open') return; + if (content.contains(content.ownerDocument.activeElement)) return; + if (!returnSubmitForm(content)) return; + content.focus({ preventScroll: true }); + }); + } + + type DialogKeyboardEvent = KeyboardEvent & { currentTarget: EventTarget & HTMLDivElement }; + + // A plain Return on the dialog, or on text in it, submits its form. A + // field submits on its own and a focused button presses itself, so those + // are left alone (see isDefaultActionKeydown) and nothing fires twice. The + // submit button is clicked, not the handler called, so its disabled state + // and the form's validation apply as they do for the mouse. + function handleKeydown(event: DialogKeyboardEvent) { + onKeydown?.(event); + if (!isDefaultActionKeydown(event)) return; + const form = returnSubmitForm(event.currentTarget); + const button = form && findEnabledSubmitButton(form); + if (!button) return; + // Stopped as well as prevented: views under the dialog listen on the + // window. + event.preventDefault(); + event.stopPropagation(); + button.click(); } function handleCloseAutoFocus(event: Event) { @@ -254,6 +317,7 @@ onCloseAutoFocus={handleCloseAutoFocus} onInteractOutside={handleInteractOutside} onpointerdown={handlePointerDown} + onkeydown={handleKeydown} preventOverflowTextSelection={false} preventScroll={false} {...restProps} diff --git a/apps/staged/src/lib/components/ui/dialog/testStubs/DialogWithForm.svelte b/apps/staged/src/lib/components/ui/dialog/testStubs/DialogWithForm.svelte new file mode 100644 index 000000000..29ff83336 --- /dev/null +++ b/apps/staged/src/lib/components/ui/dialog/testStubs/DialogWithForm.svelte @@ -0,0 +1,74 @@ + + + +Elsewhere on the page + + + + Move to Project + {#if body === 'viewer'} + Something to read. + {:else} + + Pick a destination. + + {#if body === 'textarea'} + + {:else if body === 'prompt'} + + {/if} + Alpha + + Cancel + Move + + + Reply to the selection. + + {/if} + + diff --git a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte index 380288f75..f48178a95 100644 --- a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte @@ -49,6 +49,7 @@ } from './branchCardHelpers'; import { buildPrButtonTitle } from './prButtonTooltip'; import { derivePrPushAction } from './prButtonGitState'; + import BranchPushErrorDialog from './BranchPushErrorDialog.svelte'; import { buildPrMenu, deferredPushLabel, @@ -1246,7 +1247,8 @@ open={showPrDetectionDialog} onOpenChange={(open) => !open && linkingPrNumber === null && handlePrDetectionDialogClose()} > - + + {#if prDetectionFailure.kind === 'notFound'} No pull request found @@ -1347,22 +1349,13 @@ {/if} {#if showPushErrorDialog} - - - - {pushForce ? 'Force Push Failed' : 'Push Failed'} - - {pushError ?? 'An unknown error occurred while pushing.'} - - - - Cancel - - Retry - - - - + {/if} {#if showForcePushDialog} diff --git a/apps/staged/src/lib/features/branches/BranchPushErrorDialog.component.test.ts b/apps/staged/src/lib/features/branches/BranchPushErrorDialog.component.test.ts new file mode 100644 index 000000000..a37db3380 --- /dev/null +++ b/apps/staged/src/lib/features/branches/BranchPushErrorDialog.component.test.ts @@ -0,0 +1,193 @@ +import { flushSync, mount, tick, unmount } from 'svelte'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import BranchPushErrorDialog from './BranchPushErrorDialog.svelte'; +import { reactiveProps } from '../sessions/testStubs/reactiveProps.svelte'; + +// The push failure alert, with the real alert dialog chrome, which decides +// where focus lands and which keys reach the buttons. Retry repeats the +// failed push as it was asked for, so on a failed force push it force pushes +// again: Return must not do that. + +let component: ReturnType | null = null; +let target: HTMLElement; + +/** bits-ui focuses the dialog on a 0 ms timer after mount; let it, then render. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 30)); + await tick(); + flushSync(); +} + +async function render(force: boolean) { + const props = reactiveProps({ + open: true, + force, + error: 'remote: rejected' as string | null, + onRetry: vi.fn(), + onCancel: vi.fn(), + }); + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(BranchPushErrorDialog, { target, props }); + flushSync(); + await settle(); + return props; +} + +function keydown(el: Element, init: KeyboardEventInit = {}) { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +function buttonNamed(name: string): HTMLButtonElement { + const found = [...document.querySelectorAll('button')].find( + (b) => b.textContent?.trim() === name + ); + if (!found) throw new Error(`no "${name}" button`); + return found; +} + +function dialog(): HTMLElement { + const found = document.querySelector('[role="alertdialog"]'); + if (!found) throw new Error('no alert dialog'); + return found; +} + +beforeEach(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + disconnect() {} + unobserve() {} + } + ); +}); + +afterEach(() => { + if (component) unmount(component); + component = null; + target.remove(); + vi.unstubAllGlobals(); +}); + +describe('BranchPushErrorDialog', () => { + describe('after a plain push failed', () => { + it('shows the error under a Push Failed title', async () => { + await render(false); + + expect(dialog().textContent).toContain('Push Failed'); + expect(dialog().textContent).not.toContain('Force Push Failed'); + expect(dialog().textContent).toContain('remote: rejected'); + }); + + it('retries on Return as it opens, with focus on the dialog', async () => { + const props = await render(false); + expect(document.activeElement).toBe(dialog()); + + const event = keydown(document.activeElement!); + + expect(props.onRetry).toHaveBeenCalledTimes(1); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(true); + }); + + it('only cancels on Return on a focused Cancel', async () => { + const props = await render(false); + buttonNamed('Cancel').focus(); + + keydown(document.activeElement!); + + expect(props.onRetry).not.toHaveBeenCalled(); + }); + }); + + describe('after a force push failed', () => { + it('says so in the title', async () => { + await render(true); + + expect(dialog().textContent).toContain('Force Push Failed'); + }); + + it('does not retry on Return as it opens, with focus on the dialog', async () => { + const props = await render(true); + expect(document.activeElement).toBe(dialog()); + + const event = keydown(document.activeElement!); + + expect(props.onRetry).not.toHaveBeenCalled(); + expect(props.onCancel).not.toHaveBeenCalled(); + // Left alone, not swallowed, and the dialog stays up. + expect(event.defaultPrevented).toBe(false); + expect(props.open).toBe(true); + expect(document.querySelector('[role="alertdialog"]')).not.toBeNull(); + }); + + it('does not retry on Return on the error text', async () => { + const props = await render(true); + + keydown(dialog().querySelector('[data-slot="alert-dialog-description"]')!); + + expect(props.onRetry).not.toHaveBeenCalled(); + }); + + it('still retries on a click on Retry', async () => { + const props = await render(true); + + buttonNamed('Retry').click(); + flushSync(); + + expect(props.onRetry).toHaveBeenCalledTimes(1); + expect(props.onCancel).not.toHaveBeenCalled(); + }); + + it('leaves a Return on a focused Retry to the button, which presses itself', async () => { + const props = await render(true); + buttonNamed('Retry').focus(); + + // The dialog leaves a focused button's Return to the button. + const event = keydown(document.activeElement!); + + expect(event.defaultPrevented).toBe(false); + expect(props.onRetry).not.toHaveBeenCalled(); + }); + + it('goes back to retrying on Return once the failed push is a plain one', async () => { + const props = await render(true); + props.force = false; + flushSync(); + + keydown(dialog()); + + expect(props.onRetry).toHaveBeenCalledTimes(1); + }); + }); + + it('clears the failed push on Cancel', async () => { + const props = await render(true); + + buttonNamed('Cancel').click(); + await settle(); + + expect(props.onCancel).toHaveBeenCalledTimes(1); + expect(props.onRetry).not.toHaveBeenCalled(); + }); + + it('only hides on Escape, keeping the failed push to reopen', async () => { + const props = await render(true); + + keydown(dialog(), { key: 'Escape' }); + await settle(); + + expect(props.open).toBe(false); + expect(props.onCancel).not.toHaveBeenCalled(); + expect(props.onRetry).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/staged/src/lib/features/branches/BranchPushErrorDialog.svelte b/apps/staged/src/lib/features/branches/BranchPushErrorDialog.svelte new file mode 100644 index 000000000..1a8baf583 --- /dev/null +++ b/apps/staged/src/lib/features/branches/BranchPushErrorDialog.svelte @@ -0,0 +1,41 @@ + + + + + + + {force ? 'Force Push Failed' : 'Push Failed'} + + {error ?? 'An unknown error occurred while pushing.'} + + + + Cancel + Retry + + + diff --git a/apps/staged/src/lib/features/projects/AddRepoModal.component.test.ts b/apps/staged/src/lib/features/projects/AddRepoModal.component.test.ts new file mode 100644 index 000000000..6933b0f8e --- /dev/null +++ b/apps/staged/src/lib/features/projects/AddRepoModal.component.test.ts @@ -0,0 +1,223 @@ +import { flushSync, mount, tick, unmount } from 'svelte'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { RecentRepo } from '../../types'; + +// The Add Repository dialog with its real form, repo fields and dialog +// chrome. The recent-repo chips sit inside the form, ahead of Add. While +// they had no `type` each was a submit button: a click on one picked its +// repo and submitted the form in the same press, and a Return, which +// presses a form's first submit button, did that to the first chip. So one +// Return added a repo nobody picked, with Add itself still disabled. +const { commands } = vi.hoisted(() => ({ + commands: { + listRecentRepos: vi.fn(), + listUserRepos: vi.fn(), + searchGithubRepos: vi.fn(), + getGithubRepo: vi.fn(), + checkMonorepoModules: vi.fn(), + detectDefaultBranch: vi.fn(), + listRepoDirectories: vi.fn(), + validateSubpath: vi.fn(), + getPrForRepo: vi.fn(), + getParentRepo: vi.fn(), + listPullRequests: vi.fn(), + listGitBranches: vi.fn(), + }, +})); + +vi.mock('../../api/commands', () => commands); +vi.mock('../../stores/repoBadges.svelte', () => ({ + repoBadgeStore: { lookup: () => undefined, ensureForRepos: vi.fn() }, +})); + +import AddRepoModal from './AddRepoModal.svelte'; + +const recents: RecentRepo[] = [ + { id: 'r1', githubRepo: 'acme/widgets', subpath: null, lastUsedAt: 2 }, + { id: 'r2', githubRepo: 'acme/gadgets', subpath: 'packages/app', lastUsedAt: 1 }, +]; + +let component: ReturnType | null = null; + +/** Let the dialog open, bits-ui settle its focus and the recents load. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 30)); + await tick(); + flushSync(); +} + +async function open() { + const props = { open: true, onAdded: vi.fn(), onClose: vi.fn() }; + const target = document.createElement('div'); + document.body.appendChild(target); + component = mount(AddRepoModal, { target, props }); + flushSync(); + await settle(); + await vi.waitFor(() => expect(chips()).toHaveLength(recents.length)); + return props; +} + +function dialog(): HTMLElement { + const found = document.querySelector('[data-slot="dialog-content"]'); + if (!found) throw new Error('no dialog'); + return found; +} + +function form(): HTMLFormElement { + return dialog().querySelector('form')!; +} + +function chips(): HTMLButtonElement[] { + return [...document.querySelectorAll('.recent-chip')]; +} + +function addButton(): HTMLButtonElement { + const found = [...form().querySelectorAll('button')].find((b) => b.textContent?.trim() === 'Add'); + if (!found) throw new Error('no Add button'); + return found; +} + +function pressReturn(el: Element) { + const event = new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +/** + * What a browser does for a Return typed in one of the form's fields + * (HTML's implicit submission), which jsdom leaves out: it presses the + * form's default button, the first one in tree order that submits, and does + * nothing while that button is disabled. + */ +function browserReturnInField(target: HTMLFormElement) { + const defaultButton = [...target.elements].find( + (el): el is HTMLButtonElement => el instanceof HTMLButtonElement && el.type === 'submit' + ); + if (defaultButton && !defaultButton.disabled) defaultButton.click(); + flushSync(); + return defaultButton; +} + +function expectNothingAdded(props: { onAdded: unknown; onClose: unknown }) { + expect(props.onAdded).not.toHaveBeenCalled(); + expect(props.onClose).not.toHaveBeenCalled(); + // No repo was picked either: the chips are still offered, Add is still off + // and nothing was looked up for a repo. + expect(chips()).toHaveLength(recents.length); + expect(addButton().disabled).toBe(true); + expect(commands.checkMonorepoModules).not.toHaveBeenCalled(); +} + +describe('AddRepoModal', () => { + beforeAll(() => { + // jsdom has no Web Animations, which the chips' slide-out starts. + Element.prototype.animate = () => ({ cancel: () => {} }) as unknown as Animation; + }); + + afterAll(() => { + delete (Element.prototype as Partial).animate; + }); + + beforeEach(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + disconnect() {} + unobserve() {} + } + ); + for (const command of Object.values(commands)) command.mockReset().mockResolvedValue([]); + commands.listRecentRepos.mockResolvedValue(recents); + commands.checkMonorepoModules.mockResolvedValue(0); + commands.detectDefaultBranch.mockResolvedValue('main'); + commands.getPrForRepo.mockResolvedValue(null); + commands.getParentRepo.mockResolvedValue(null); + commands.getGithubRepo.mockResolvedValue(null); + }); + + afterEach(() => { + if (component) unmount(component); + component = null; + document.body.innerHTML = ''; + vi.unstubAllGlobals(); + }); + + describe('with recent-repo chips shown and no repo selected', () => { + it('has Add, which is disabled, as the only button that submits the form', async () => { + await open(); + + for (const chip of chips()) { + expect(chip.form).toBe(form()); + expect(chip.getAttribute('type')).toBe('button'); + } + const submitters = [...form().elements].filter( + (el) => (el as HTMLButtonElement).type === 'submit' + ); + expect(submitters).toEqual([addButton()]); + expect(addButton().disabled).toBe(true); + }); + + it('adds nothing on Return on the dialog itself', async () => { + const props = await open(); + dialog().focus(); + expect(document.activeElement).toBe(dialog()); + + pressReturn(document.activeElement!); + await settle(); + + expectNothingAdded(props); + }); + + it('adds nothing on Return on the title', async () => { + const props = await open(); + + pressReturn(dialog().querySelector('[data-slot="dialog-title"]')!); + await settle(); + + expectNothingAdded(props); + }); + + it('adds nothing on the Return a browser handles in the empty search field', async () => { + const props = await open(); + + const pressed = browserReturnInField(form()); + await settle(); + + expect(pressed).toBe(addButton()); + expectNothingAdded(props); + }); + }); + + it('picks the repo on a click on its chip, and waits for Add', async () => { + const props = await open(); + + chips()[0].click(); + flushSync(); + await settle(); + + expect(props.onAdded).not.toHaveBeenCalled(); + expect(props.onClose).not.toHaveBeenCalled(); + expect(dialog().textContent).toContain('acme/widgets'); + expect(addButton().disabled).toBe(false); + }); + + it('adds the picked repo on Return on the dialog', async () => { + const props = await open(); + chips()[1].click(); + flushSync(); + await settle(); + dialog().focus(); + + const event = pressReturn(document.activeElement!); + + await vi.waitFor(() => expect(props.onAdded).toHaveBeenCalledTimes(1)); + expect(props.onAdded.mock.calls[0][0]).toMatchObject({ + nameWithOwner: 'acme/gadgets', + subpath: 'packages/app', + }); + expect(props.onClose).toHaveBeenCalledTimes(1); + expect(event.defaultPrevented).toBe(true); + }); +}); diff --git a/apps/staged/src/lib/features/projects/BranchPicker.svelte b/apps/staged/src/lib/features/projects/BranchPicker.svelte index e87d46c41..039dd9899 100644 --- a/apps/staged/src/lib/features/projects/BranchPicker.svelte +++ b/apps/staged/src/lib/features/projects/BranchPicker.svelte @@ -407,6 +407,7 @@ {#each filteredItems as item, i} { diff --git a/apps/staged/src/lib/features/projects/NewProjectForm.component.test.ts b/apps/staged/src/lib/features/projects/NewProjectForm.component.test.ts index b9671e4fb..a10d6ad56 100644 --- a/apps/staged/src/lib/features/projects/NewProjectForm.component.test.ts +++ b/apps/staged/src/lib/features/projects/NewProjectForm.component.test.ts @@ -29,13 +29,13 @@ const created = { id: 'created', name: 'Fresh' } as Project; let component: ReturnType | null = null; /** Mount the form with a name typed and, unless `selectedRepo` is `null`, a repo picked. */ -function open(selectedRepo: string | null = 'acme/widgets') { +function open(selectedRepo: string | null = 'acme/widgets', onCancel?: () => void) { const target = document.createElement('div'); document.body.appendChild(target); const onCreated = vi.fn(); component = mount(NewProjectForm, { target, - props: { onCreated, name: 'Fresh', selectedRepo }, + props: { onCreated, onCancel, name: 'Fresh', selectedRepo }, }); flushSync(); return { target, onCreated }; @@ -150,3 +150,158 @@ describe('NewProjectForm location', () => { expect(setNewProjectLocation).not.toHaveBeenCalled(); }); }); + +// Return creates the project through a form that owns only the Name field +// and Create: from the Name field the browser submits it, and a Return on +// the fields' container (where WebKit puts the focus after a click on one of +// its buttons) presses Create. There is no listener on the window, which +// used to create the project for a Return on a focused Cancel, and under any +// dialog opened over the splash screen. +describe('NewProjectForm Return key', () => { + beforeEach(() => { + createProject.mockReset().mockResolvedValue(created); + }); + + afterEach(() => { + if (component) unmount(component); + component = null; + document.body.innerHTML = ''; + }); + + function pressReturn(el: Element, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; + } + + const buttonNamed = (target: HTMLElement, name: string) => + [...target.querySelectorAll('button')].find((b) => b.textContent?.includes(name))!; + const container = (target: HTMLElement) => + target.querySelector('.new-project-form')!; + + /** Put `el` where RepoConfigForm (stubbed out here) renders: after the Name field, before Create. */ + function amongTheRepoFields(target: HTMLElement, el: T): T { + target.querySelector('form')!.after(el); + return el; + } + + it('has a form that owns the Name field and Create, and nothing else', () => { + const { target } = open('acme/widgets', vi.fn()); + const form = target.querySelector('form')!; + + expect([...form.elements]).toEqual([ + target.querySelector('#project-name'), + buttonNamed(target, 'Create'), + ]); + expect(buttonNamed(target, 'Create').type).toBe('submit'); + expect(buttonNamed(target, 'Cancel').type).toBe('button'); + }); + + it('creates the project when the form is submitted, as Return in the Name field does', async () => { + const { target, onCreated } = open(); + + target.querySelector('form')!.requestSubmit(); + + await vi.waitFor(() => expect(onCreated).toHaveBeenCalledWith(created)); + expect(createProject).toHaveBeenCalledTimes(1); + }); + + it('leaves a Return in the Name field to the browser, which submits the form', () => { + const { target } = open(); + + const event = pressReturn(target.querySelector('#project-name')!); + + // Not pressed here as well: the browser's submit is the only one. + expect(event.defaultPrevented).toBe(false); + expect(createProject).not.toHaveBeenCalled(); + }); + + it("creates the project on Return on the fields' container, which can take the focus", async () => { + const { target, onCreated } = open(); + container(target).focus(); + expect(document.activeElement).toBe(container(target)); + + const event = pressReturn(document.activeElement!); + + await vi.waitFor(() => expect(onCreated).toHaveBeenCalledWith(created)); + expect(createProject).toHaveBeenCalledTimes(1); + expect(event.defaultPrevented).toBe(true); + }); + + it('presses Create, not a repo chip, which is a button with no type', async () => { + const { target, onCreated } = open(); + const chip = amongTheRepoFields(target, document.createElement('button')); + const chipClicked = vi.fn(); + chip.addEventListener('click', chipClicked); + + pressReturn(container(target)); + + await vi.waitFor(() => expect(onCreated).toHaveBeenCalledWith(created)); + expect(chipClicked).not.toHaveBeenCalled(); + }); + + it('does not create on a Return in a repo field, which is outside the form', () => { + const { target } = open(); + const field = amongTheRepoFields(target, document.createElement('input')); + + const event = pressReturn(field); + + expect(field.form).toBeNull(); + expect(event.defaultPrevented).toBe(false); + expect(createProject).not.toHaveBeenCalled(); + }); + + it('does not create on a Return on the container that commits an IME composition, repeats or has a modifier', () => { + const { target } = open(); + + pressReturn(container(target), { keyCode: 229 }); + pressReturn(container(target), { repeat: true }); + pressReturn(container(target), { metaKey: true }); + + expect(createProject).not.toHaveBeenCalled(); + }); + + it('only cancels on Return on a focused Cancel', () => { + const onCancel = vi.fn(); + const { target } = open('acme/widgets', onCancel); + const cancel = buttonNamed(target, 'Cancel'); + cancel.focus(); + + // The browser turns this keydown into Cancel's own click. + const event = pressReturn(cancel); + expect(event.defaultPrevented).toBe(false); + expect(createProject).not.toHaveBeenCalled(); + + cancel.click(); + expect(onCancel).toHaveBeenCalledTimes(1); + expect(createProject).not.toHaveBeenCalled(); + }); + + it('does not create on a Return outside the form', () => { + open(); + + pressReturn(document.body); + + expect(createProject).not.toHaveBeenCalled(); + }); + + it('does not create while Create is disabled', () => { + const target = document.createElement('div'); + document.body.appendChild(target); + component = mount(NewProjectForm, { target, props: { onCreated: vi.fn(), name: ' ' } }); + flushSync(); + + expect(buttonNamed(target, 'Create').disabled).toBe(true); + buttonNamed(target, 'Create').click(); + const event = pressReturn(container(target)); + + expect(createProject).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/features/projects/NewProjectForm.svelte b/apps/staged/src/lib/features/projects/NewProjectForm.svelte index 84160ede5..01ce339ee 100644 --- a/apps/staged/src/lib/features/projects/NewProjectForm.svelte +++ b/apps/staged/src/lib/features/projects/NewProjectForm.svelte @@ -22,6 +22,10 @@ import { parseGitHubUrl } from '../../shared/githubUrl'; import type { RepoSelection } from '../../shared/githubUrl'; import { autofocusOnOpen } from '../../shared/focusAfterDialogEnter'; + import { + findEnabledSubmitButton, + isDefaultActionKeydown, + } from '../../shared/dialogDefaultAction'; import type { PullRequest } from '../../types'; import { bloxAvailable, sqState } from '../settings/sq.svelte'; import { preferences, setNewProjectLocation } from '../settings/preferences.svelte'; @@ -150,17 +154,33 @@ } } + // Return creates the project through a form that owns just the Name field + // and Create (tied to it by its `form` attribute). In the Name field the + // browser submits it. The repo, subpath and branch fields are outside it: + // Return there picks from their dropdowns and never creates the project + // half filled in. The repo chips are outside it too. + const formId = $props.id(); + let formEl = $state(null); + + function handleSubmit(e: SubmitEvent) { + e.preventDefault(); + void handleCreate(); + } + + // A Return on the fields' container presses Create, as one on a dialog + // submits its form (see dialog-content). WebKit gives a clicked button no + // focus; it goes to the nearest focusable ancestor, which `tabindex="-1"` + // makes this container. So a Return after a click on a repo chip or a + // location tile still creates the project, on the splash screen too, where + // there is no dialog around the form to hear it. A focused Cancel or + // location tile takes its own Return. function handleKeydown(e: KeyboardEvent) { - if (e.key === 'Enter') { - const target = e.target as HTMLElement; - if (target.closest('.repo-search-wrapper')) return; - // Don't submit if a suggestion is highlighted in the subpath dropdown - if (target.closest('.subpath-input-wrapper')) return; - // Don't submit if a suggestion is highlighted in the branch picker dropdown - if (target.closest('.branch-picker-wrapper')) return; - e.preventDefault(); - handleCreate(); - } + if (!formEl || !isDefaultActionKeydown(e)) return; + const create = findEnabledSubmitButton(formEl); + if (!create) return; + e.preventDefault(); + e.stopPropagation(); + create.click(); } /** Derive a human-friendly project name from a branch name like "feat/dark-mode". */ @@ -196,10 +216,9 @@ } - - - - + + + Name - + {#if remoteProjectsAvailable} @@ -245,9 +264,10 @@ Cancel {/if} {#if saving} @@ -267,6 +287,8 @@ display: flex; flex-direction: column; gap: 14px; + /* Focusable only to hear Return (see handleKeydown); it shows no ring. */ + outline: none; } /* In a full-screen dialog (NewProjectModal on a phone) the form is the diff --git a/apps/staged/src/lib/features/projects/ProjectTransferDialogReturn.component.test.ts b/apps/staged/src/lib/features/projects/ProjectTransferDialogReturn.component.test.ts new file mode 100644 index 000000000..7ba36300a --- /dev/null +++ b/apps/staged/src/lib/features/projects/ProjectTransferDialogReturn.component.test.ts @@ -0,0 +1,275 @@ +import { flushSync, mount, tick, unmount } from 'svelte'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Project, ProjectTransferPreflight, ProjectTransferRepoPreflight } from '../../types'; +import type { PendingTransfer } from './projectActions.svelte'; + +// The transfer dialog's Return key, with the real dialog and alert dialog +// chrome: ProjectTransferDialog.component.test.ts stubs both, and the chrome +// is what decides where focus lands and which keys reach the form. The +// dialog has no text field, so nothing native answers Return in it. +const { actions, prefs } = vi.hoisted(() => ({ + actions: { + pendingTransfer: null as unknown, + confirmPendingTransfer: vi.fn(async () => {}), + cancelPendingTransfer: vi.fn(), + }, + prefs: { transferUncommittedChanges: true, transferIgnoredFiles: false }, +})); + +vi.mock('./projectActions.svelte', async () => { + const { reactiveProps } = await import('../sessions/testStubs/reactiveProps.svelte'); + const state = reactiveProps({ pending: null as unknown }); + Object.defineProperty(actions, 'pendingTransfer', { + get: () => state.pending, + set: (value: unknown) => { + state.pending = value; + }, + }); + return { projectActions: actions }; +}); +vi.mock('../settings/preferences.svelte', () => ({ preferences: prefs })); + +import ProjectTransferDialog from './ProjectTransferDialog.svelte'; + +const project: Project = { + id: 'p1', + name: 'Alpha', + githubRepo: 'org/alpha', + location: 'local', + subpath: null, + createdAt: 0, + updatedAt: 0, +}; + +function repo(overrides: Partial = {}): ProjectTransferRepoPreflight { + return { + branchId: 'b1', + projectRepoId: 'r1', + githubRepo: 'org/alpha', + subpath: null, + branchName: 'feature', + destBranchName: 'feature', + state: 'carry', + head: 'abc', + upstream: 'origin/feature', + unpushedCommits: 2, + staged: 0, + unstaged: 0, + untracked: 0, + conflicted: 0, + untrackedBytes: 0, + ignoredFiles: 0, + ignoredBytes: 0, + ignoredDirs: 0, + lfsPresent: 0, + lfsPresentBytes: 0, + lfsMissing: 0, + lfsMissingBytes: 0, + hasUncommittedChanges: false, + scanning: false, + blockers: [], + warnings: [], + ...overrides, + }; +} + +function preflight(overrides: Partial = {}): ProjectTransferPreflight { + return { + projectId: 'p1', + from: 'local', + to: 'remote', + direction: 'toRemote', + workspaceName: null, + repos: [repo()], + blockers: [], + warnings: [], + canTransfer: true, + hasUncommittedChanges: false, + scanning: false, + ...overrides, + }; +} + +function ready(pf: ProjectTransferPreflight): PendingTransfer { + return { project, direction: pf.direction, status: 'ready', preflight: pf }; +} + +let component: ReturnType | null = null; +let target: HTMLElement; +let pageButton: HTMLButtonElement; + +/** The dialogs take focus a frame or a 0 ms timer after opening; let them, then render. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 30)); + await tick(); + flushSync(); +} + +/** Open the dialog from a focused button on the page behind it, as a menu item does. */ +async function openDialog(pending: PendingTransfer) { + actions.pendingTransfer = null; + target = document.createElement('div'); + document.body.appendChild(target); + pageButton = document.createElement('button'); + target.appendChild(pageButton); + component = mount(ProjectTransferDialog, { target }); + flushSync(); + pageButton.focus(); + actions.pendingTransfer = pending; + flushSync(); + await settle(); +} + +function dialogContent(): HTMLElement { + const el = document.querySelector('[data-slot="dialog-content"]'); + if (!el) throw new Error('transfer dialog not rendered'); + return el; +} + +function buttonNamed(name: string): HTMLButtonElement { + const found = [...document.querySelectorAll('button')].find( + (b) => b.textContent?.trim() === name + ); + if (!found) throw new Error(`no "${name}" button`); + return found; +} + +function pressReturn(el: Element, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +beforeEach(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + disconnect() {} + unobserve() {} + } + ); + actions.confirmPendingTransfer.mockClear(); + actions.cancelPendingTransfer.mockClear(); + prefs.transferUncommittedChanges = true; + prefs.transferIgnoredFiles = false; +}); + +afterEach(() => { + if (component) unmount(component); + component = null; + target.remove(); + vi.unstubAllGlobals(); +}); + +describe('ProjectTransferDialog Return key', () => { + it('opens with focus on the dialog, not on the page behind or a button', async () => { + await openDialog(ready(preflight())); + + expect(document.activeElement).toBe(dialogContent()); + }); + + it('transfers on Return as it opens', async () => { + await openDialog(ready(preflight())); + + const event = pressReturn(document.activeElement!); + + expect(actions.confirmPendingTransfer).toHaveBeenCalledExactlyOnceWith({ + includeUncommitted: true, + includeIgnored: false, + }); + expect(actions.cancelPendingTransfer).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(true); + }); + + it('transfers with the answer a click on the checkbox just gave', async () => { + await openDialog( + ready( + preflight({ + repos: [repo({ unstaged: 1, hasUncommittedChanges: true })], + hasUncommittedChanges: true, + }) + ) + ); + document + .querySelector('[role="checkbox"][id^="project-transfer-uncommitted"]')! + .click(); + flushSync(); + // WebKit leaves a clicked checkbox unfocused and focuses the dialog. + dialogContent().focus(); + + pressReturn(document.activeElement!); + + expect(actions.confirmPendingTransfer).toHaveBeenCalledExactlyOnceWith({ + includeUncommitted: false, + includeIgnored: false, + }); + }); + + it('does not transfer on Return while the preflight is loading', async () => { + await openDialog({ project, direction: 'toRemote', status: 'loading' }); + expect(document.activeElement).toBe(dialogContent()); + + const event = pressReturn(document.activeElement!); + + expect(actions.confirmPendingTransfer).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it('does not transfer on Return while something blocks the transfer', async () => { + await openDialog( + ready( + preflight({ + canTransfer: false, + blockers: [{ code: 'session_running', message: 'A session is still running.' }], + }) + ) + ); + + pressReturn(document.activeElement!); + + expect(actions.confirmPendingTransfer).not.toHaveBeenCalled(); + }); + + it('does not transfer on Return on a focused Cancel', async () => { + await openDialog(ready(preflight())); + buttonNamed('Cancel').focus(); + + // The browser turns this keydown into Cancel's own click. + const event = pressReturn(document.activeElement!); + + expect(actions.confirmPendingTransfer).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it('does not transfer on a held Return repeating, such as the press that opened it', async () => { + await openDialog(ready(preflight())); + + pressReturn(document.activeElement!, { repeat: true }); + + expect(actions.confirmPendingTransfer).not.toHaveBeenCalled(); + }); + + it('asks first on Return mid-scan, and a second Return transfers anyway', async () => { + await openDialog(ready(preflight({ repos: [repo({ scanning: true })], scanning: true }))); + + pressReturn(document.activeElement!); + await settle(); + + expect(actions.confirmPendingTransfer).not.toHaveBeenCalled(); + expect(document.activeElement?.getAttribute('role')).toBe('alertdialog'); + expect(document.activeElement?.textContent).toContain('Transfer anyway'); + + pressReturn(document.activeElement!); + await settle(); + + expect(actions.confirmPendingTransfer).toHaveBeenCalledTimes(1); + expect(document.querySelector('[role="alertdialog"]')).toBeNull(); + }); +}); diff --git a/apps/staged/src/lib/features/projects/ProjectTransferScanConfirm.component.test.ts b/apps/staged/src/lib/features/projects/ProjectTransferScanConfirm.component.test.ts new file mode 100644 index 000000000..b6f3ba193 --- /dev/null +++ b/apps/staged/src/lib/features/projects/ProjectTransferScanConfirm.component.test.ts @@ -0,0 +1,113 @@ +import { flushSync, mount, tick, unmount } from 'svelte'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import ProjectTransferScanConfirm from './ProjectTransferScanConfirm.svelte'; +import { reactiveProps } from '../sessions/testStubs/reactiveProps.svelte'; + +// The "Scan still running" confirmation, with the real alert dialog chrome: +// ProjectTransferDialog's own test stubs it, and the chrome is what decides +// where focus lands and which keys reach the buttons. + +let component: ReturnType | null = null; +let target: HTMLElement; + +/** bits-ui focuses the dialog on a 0 ms timer after mount; let it, then render. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 30)); + await tick(); + flushSync(); +} + +async function render() { + const props = reactiveProps({ open: true, onConfirm: vi.fn(), onCancel: vi.fn() }); + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(ProjectTransferScanConfirm, { target, props }); + flushSync(); + await settle(); + return props; +} + +function keydown(el: Element, key: string) { + el.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true })); + flushSync(); +} + +function buttonNamed(name: string): HTMLButtonElement { + const found = [...document.querySelectorAll('button')].find( + (b) => b.textContent?.trim() === name + ); + if (!found) throw new Error(`no "${name}" button`); + return found; +} + +beforeEach(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + disconnect() {} + unobserve() {} + } + ); +}); + +afterEach(() => { + if (component) unmount(component); + component = null; + target.remove(); + vi.unstubAllGlobals(); +}); + +describe('ProjectTransferScanConfirm', () => { + it('starts the transfer on Return as it opens, with focus on the dialog', async () => { + const props = await render(); + expect(document.activeElement?.getAttribute('role')).toBe('alertdialog'); + + keydown(document.activeElement!, 'Enter'); + + expect(props.onConfirm).toHaveBeenCalledTimes(1); + expect(props.onCancel).not.toHaveBeenCalled(); + }); + + it('starts the transfer on a click on Transfer anyway', async () => { + const props = await render(); + + buttonNamed('Transfer anyway').click(); + flushSync(); + + expect(props.onConfirm).toHaveBeenCalledTimes(1); + expect(props.onCancel).not.toHaveBeenCalled(); + }); + + it('keeps checking on Escape', async () => { + const props = await render(); + + keydown(document.activeElement!, 'Escape'); + await settle(); + + expect(props.onCancel).toHaveBeenCalledTimes(1); + expect(props.onConfirm).not.toHaveBeenCalled(); + }); + + it('keeps checking, and does not transfer, on Return on a focused Keep checking', async () => { + const props = await render(); + buttonNamed('Keep checking').focus(); + + keydown(document.activeElement!, 'Enter'); + await settle(); + + expect(props.onCancel).toHaveBeenCalledTimes(1); + expect(props.onConfirm).not.toHaveBeenCalled(); + }); + + it('does not transfer on a held Return repeating', async () => { + const props = await render(); + + document.activeElement!.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Enter', repeat: true, bubbles: true, cancelable: true }) + ); + flushSync(); + + expect(props.onConfirm).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/staged/src/lib/features/projects/RepoConfigForm.svelte b/apps/staged/src/lib/features/projects/RepoConfigForm.svelte index 0f42ee8ee..2beb25565 100644 --- a/apps/staged/src/lib/features/projects/RepoConfigForm.svelte +++ b/apps/staged/src/lib/features/projects/RepoConfigForm.svelte @@ -325,6 +325,7 @@ {#each filteredRecentRepos.slice(0, 5) as recent, i} {@const hue = repoHue(recent)} {#each filteredSuggestions as dir, i} { diff --git a/apps/staged/src/lib/features/sessions/NewSessionModal.svelte b/apps/staged/src/lib/features/sessions/NewSessionModal.svelte index ab8ece7f2..ff53c5835 100644 --- a/apps/staged/src/lib/features/sessions/NewSessionModal.svelte +++ b/apps/staged/src/lib/features/sessions/NewSessionModal.svelte @@ -700,8 +700,12 @@ !v && handleClose()}> + import('./testStubs/PromptInputStub.svelte')); +vi.mock('./ImageAttachment.svelte', () => import('./testStubs/EmptyStub.svelte')); +vi.mock('../agents/AcpConfigPicker.svelte', () => import('./testStubs/EmptyStub.svelte')); +vi.mock('../branches/dragDrop', () => ({ subscribeDragDrop: () => () => {} })); + +import NewSessionModal from './NewSessionModal.svelte'; + +const branch = { + id: 'branch-1', + projectId: 'project-1', + branchName: 'feature-branch', + worktreePath: '/tmp/worktree', +} as unknown as Branch; + +let component: ReturnType | null = null; +let target: HTMLElement; + +/** Let the draft load, the prompt's focus and the dialog's open frame run. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 30)); + for (let i = 0; i < 8; i++) await tick(); + flushSync(); +} + +async function openModal() { + const onSubmit = vi.fn(); + target = document.createElement('div'); + document.body.appendChild(target); + component = mount(NewSessionModal, { + target, + props: { + open: true, + branch, + mode: 'commit', + initialPrompt: 'Tidy the imports', + hashtagItems: [], + onClose: () => {}, + onSubmit, + }, + }); + flushSync(); + await settle(); + return onSubmit; +} + +function dialogContent(): HTMLElement { + const el = document.querySelector('[data-slot="dialog-content"]'); + if (!el) throw new Error('new session dialog not rendered'); + return el; +} + +function prompt(): HTMLElement { + return dialogContent().querySelector('[data-testid="prompt-input"]')!; +} + +function pressReturn(el: Element, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +beforeEach(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + disconnect() {} + unobserve() {} + } + ); +}); + +afterEach(() => { + if (component) unmount(component); + component = null; + target.remove(); + vi.unstubAllGlobals(); +}); + +describe('NewSessionModal Return key', () => { + it('opens with focus on the prompt, not on the dialog', async () => { + await openModal(); + + expect(document.activeElement).toBe(prompt()); + }); + + it('can start the session, so a Return has something it could wrongly press', async () => { + await openModal(); + + const submit = dialogContent().querySelector('button[type="submit"]')!; + expect(submit.disabled).toBe(false); + }); + + it('does not start the session on a plain Return in the prompt', async () => { + const onSubmit = await openModal(); + + const event = pressReturn(prompt()); + + expect(onSubmit).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it('does not start the session on a plain Return on the dialog itself', async () => { + const onSubmit = await openModal(); + // Where WebKit puts the focus after a click on the header or a button. + dialogContent().focus(); + + const event = pressReturn(document.activeElement!); + + expect(onSubmit).not.toHaveBeenCalled(); + expect(event.defaultPrevented).toBe(false); + }); + + it('still starts the session on Ctrl+Return', async () => { + const onSubmit = await openModal(); + + pressReturn(prompt(), { ctrlKey: true }); + + expect(onSubmit).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/staged/src/lib/features/settings/ConnectedAppsPanel.svelte b/apps/staged/src/lib/features/settings/ConnectedAppsPanel.svelte index 02b8a0184..b11009046 100644 --- a/apps/staged/src/lib/features/settings/ConnectedAppsPanel.svelte +++ b/apps/staged/src/lib/features/settings/ConnectedAppsPanel.svelte @@ -160,7 +160,9 @@ if (!open && !revoking) confirming = null; }} > - + + {#if confirming} Revoke access for “{confirming.client}”? diff --git a/apps/staged/src/lib/features/settings/ControlAuthorizationDialog.svelte b/apps/staged/src/lib/features/settings/ControlAuthorizationDialog.svelte index 062814bde..5f4cb89f6 100644 --- a/apps/staged/src/lib/features/settings/ControlAuthorizationDialog.svelte +++ b/apps/staged/src/lib/features/settings/ControlAuthorizationDialog.svelte @@ -29,7 +29,9 @@ } }} > - + + {#if request} Allow “{request.client}” to control Staged? diff --git a/apps/staged/src/lib/features/tags/TagPicker.component.test.ts b/apps/staged/src/lib/features/tags/TagPicker.component.test.ts new file mode 100644 index 000000000..4e24304c1 --- /dev/null +++ b/apps/staged/src/lib/features/tags/TagPicker.component.test.ts @@ -0,0 +1,119 @@ +// The tag picker's keys over the real tagsStore and tagActions, with the +// dialog chrome stubbed (the mobile layout, so the list renders at once) and +// the commands mocked. +import { flushSync, mount, tick, unmount } from 'svelte'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Project, TagState } from '../../types'; +import { tagState } from './testStubs/tagFixtures'; + +const { backend } = vi.hoisted(() => ({ backend: { state: null as unknown as TagState } })); + +const commandsMock = vi.hoisted(() => ({ + getTagState: vi.fn(async () => ({ data: structuredClone(backend.state), revalidating: null })), + setProjectTagAssigned: vi.fn( + async (_projectId: string, _tagId: string, _assigned: boolean) => {} + ), +})); + +vi.mock('../../commands', () => commandsMock); +vi.mock('../../api/commands', () => commandsMock); +vi.mock('../../transport', async (importOriginal) => ({ + ...(await importOriginal()), + listenToEvent: () => () => {}, +})); +vi.mock('../../stores/projectsData.svelte', () => ({ + projectsDataStore: { liveProjects: [{ id: 'p1' }], isProjectDeleting: () => false }, +})); +vi.mock('../../shared/viewport.svelte', () => ({ viewport: { isMobile: true } })); +vi.mock('../layout/navigation.svelte', () => ({ openSettings: vi.fn() })); +vi.mock('$lib/components/ui/dialog', () => + import('../sessions/testStubs/dialogMock').then((m) => m.dialogModuleMock()) +); +vi.mock('svelte-sonner', () => ({ toast: { error: vi.fn() } })); + +import { tagsStore } from '../../stores/tags.svelte'; +import TagPicker from './TagPicker.svelte'; + +const project: Project = { + id: 'p1', + name: 'Project', + githubRepo: null, + location: 'local', + subpath: null, + createdAt: 1, + updatedAt: 1, +}; + +let component: ReturnType | null = null; + +async function settle() { + for (let i = 0; i < 3; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + await tick(); + flushSync(); + } +} + +/** Open the picker and type `query` into its search box. */ +async function search(query: string): Promise { + backend.state = tagState(); + await tagsStore.load({ force: true }); + const target = document.createElement('div'); + document.body.appendChild(target); + component = mount(TagPicker, { target, props: { project, open: true } }); + await settle(); + + const input = target.querySelector('input[role="combobox"]')!; + input.value = query; + input.dispatchEvent(new Event('input', { bubbles: true })); + flushSync(); + return input; +} + +function pressReturn(el: Element, init: KeyboardEventInit = {}): KeyboardEvent { + const event = new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + cancelable: true, + ...init, + }); + el.dispatchEvent(event); + flushSync(); + return event; +} + +beforeEach(() => { + commandsMock.setProjectTagAssigned.mockClear(); +}); + +afterEach(() => { + if (component) unmount(component); + component = null; + document.body.innerHTML = ''; +}); + +describe('TagPicker Return key', () => { + it('toggles the highlighted tag on Return', async () => { + const input = await search('q4'); + + const event = pressReturn(input); + await settle(); + + expect(commandsMock.setProjectTagAssigned).toHaveBeenCalledExactlyOnceWith('p1', 'q4', true); + expect(event.defaultPrevented).toBe(true); + }); + + it('leaves a Return that commits an IME composition to the IME', async () => { + const input = await search('q4'); + + // Mid-composition, then Safari's committing Return: compositionend has + // already fired, so isComposing is false and only keyCode 229 tells. + const composing = pressReturn(input, { isComposing: true }); + const committing = pressReturn(input, { keyCode: 229 }); + await settle(); + + expect(commandsMock.setProjectTagAssigned).not.toHaveBeenCalled(); + expect(composing.defaultPrevented).toBe(false); + expect(committing.defaultPrevented).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/features/tags/TagPicker.svelte b/apps/staged/src/lib/features/tags/TagPicker.svelte index 33bc4d8e1..40c87ea6f 100644 --- a/apps/staged/src/lib/features/tags/TagPicker.svelte +++ b/apps/staged/src/lib/features/tags/TagPicker.svelte @@ -29,6 +29,7 @@ import type { Project } from '../../types'; import { tagsStore } from '../../stores/tags.svelte'; import { autofocusOnOpen } from '../../shared/focusAfterDialogEnter'; + import { isImeKeydown } from '../../shared/imeKeydown'; import { viewport } from '../../shared/viewport.svelte'; import { openSettings } from '../layout/navigation.svelte'; import TagIcon from './TagIcon.svelte'; @@ -88,7 +89,9 @@ } function handleKeydown(event: KeyboardEvent) { - if (event.isComposing) return; + // A key the IME is handling is the IME's: Safari's Return committing a + // composition has isComposing false, and toggled the highlighted tag. + if (isImeKeydown(event)) return; if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { event.preventDefault(); active = moveActiveIndex(activeIndex, event.key === 'ArrowDown' ? 1 : -1, model.rows.length); diff --git a/apps/staged/src/lib/shared/dialogDefaultAction.test.ts b/apps/staged/src/lib/shared/dialogDefaultAction.test.ts new file mode 100644 index 000000000..30d82a358 --- /dev/null +++ b/apps/staged/src/lib/shared/dialogDefaultAction.test.ts @@ -0,0 +1,293 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + findEnabledButton, + findEnabledSubmitButton, + findReturnSubmitForm, + isDefaultActionKeydown, +} from './dialogDefaultAction'; + +let dialog: HTMLElement; + +beforeEach(() => { + dialog = document.createElement('div'); + dialog.setAttribute('role', 'alertdialog'); + dialog.tabIndex = -1; + document.body.appendChild(dialog); +}); + +afterEach(() => { + dialog.remove(); +}); + +/** Dispatch a keydown on `target` and return what the dialog's handler decided. */ +function decide(target: Element, init: KeyboardEventInit = {}, prevent = false): boolean { + let decided: boolean | null = null; + const listener = (e: KeyboardEvent) => { + if (prevent) e.preventDefault(); + decided = isDefaultActionKeydown(e); + }; + dialog.addEventListener('keydown', listener); + target.dispatchEvent( + new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true, ...init }) + ); + dialog.removeEventListener('keydown', listener); + if (decided === null) throw new Error('keydown did not reach the dialog'); + return decided; +} + +function child(html: string): Element { + dialog.innerHTML = html; + return dialog.firstElementChild!; +} + +describe('isDefaultActionKeydown', () => { + it('accepts a plain Return on the dialog itself', () => { + expect(decide(dialog)).toBe(true); + }); + + it('accepts a plain Return on text inside the dialog', () => { + expect(decide(child('Are you sure?'))).toBe(true); + }); + + it('rejects other keys', () => { + expect(decide(dialog, { key: ' ' })).toBe(false); + expect(decide(dialog, { key: 'Escape' })).toBe(false); + }); + + it.each(['metaKey', 'ctrlKey', 'altKey', 'shiftKey'] as const)('rejects %s+Return', (mod) => { + expect(decide(dialog, { [mod]: true })).toBe(false); + }); + + it('rejects a held key repeating', () => { + expect(decide(dialog, { repeat: true })).toBe(false); + }); + + it('rejects a Return that commits an IME composition', () => { + expect(decide(dialog, { isComposing: true })).toBe(false); + // Safari's committing Return, after compositionend. + expect(decide(dialog, { keyCode: 229 })).toBe(false); + }); + + it('rejects a Return another handler already took', () => { + expect(decide(dialog, {}, true)).toBe(false); + }); + + it.each([ + ['a button', 'Cancel'], + ['a link', 'Docs'], + ['a text field', ''], + ['a radio', ''], + ['a select', 'One'], + ['a textarea', ''], + ['editable content', ''], + ['a widget role', 'One'], + ])('rejects a Return on %s', (_name, html) => { + expect(decide(child(html))).toBe(false); + }); + + it('rejects a Return on an element inside a button', () => { + const button = child('Cancel'); + expect(decide(button.firstElementChild!)).toBe(false); + }); + + it('accepts a Return on content marked not editable', () => { + expect(decide(child('Read only'))).toBe(true); + }); + + // Containers that hold the focus for their items, and panels with their + // own Return, docked inside the dialog. + it.each([ + 'listbox', + 'menu', + 'menubar', + 'grid', + 'treegrid', + 'tree', + 'radiogroup', + 'tablist', + 'dialog', + 'alertdialog', + ])('rejects a Return on a focused %s container', (role) => { + expect(decide(child(``))).toBe(false); + }); + + it('rejects a Return on a plain element inside such a container', () => { + const listbox = child('Loading…'); + expect(decide(listbox.firstElementChild!)).toBe(false); + + const panel = child('Reply'); + expect(decide(panel.querySelector('p')!)).toBe(false); + }); + + it('accepts a Return beside such a container, not inside it', () => { + dialog.innerHTML = 'Pick one'; + expect(decide(dialog.querySelector('p')!)).toBe(true); + }); + + it('does not count the listening dialog itself, or what it sits in', () => { + // `dialog` has role="alertdialog"; put it inside a menu and a dialog too. + const outer = document.createElement('div'); + outer.setAttribute('role', 'menu'); + const outerDialog = document.createElement('div'); + outerDialog.setAttribute('role', 'dialog'); + outer.appendChild(outerDialog); + document.body.appendChild(outer); + outerDialog.appendChild(dialog); + try { + expect(decide(dialog)).toBe(true); + expect(decide(child('Are you sure?'))).toBe(true); + expect(decide(child('Cancel'))).toBe(false); + } finally { + document.body.appendChild(dialog); + outer.remove(); + } + }); +}); + +describe('findEnabledButton', () => { + it('finds the matching button', () => { + dialog.innerHTML = + 'NoYes'; + expect(findEnabledButton(dialog, '[data-slot="action"]')?.textContent).toBe('Yes'); + }); + + it('is null when there is no such button', () => { + dialog.innerHTML = 'No'; + expect(findEnabledButton(dialog, '[data-slot="action"]')).toBeNull(); + }); + + it('is null when the button is disabled', () => { + dialog.innerHTML = 'Yes'; + expect(findEnabledButton(dialog, '[data-slot="action"]')).toBeNull(); + }); + + it('is null when the button is aria-disabled', () => { + dialog.innerHTML = 'Yes'; + expect(findEnabledButton(dialog, '[data-slot="action"]')).toBeNull(); + }); +}); + +describe('findReturnSubmitForm', () => { + it('finds a form with a submit button', () => { + dialog.innerHTML = 'Move'; + expect(findReturnSubmitForm(dialog)).toBe(dialog.querySelector('form')); + }); + + it('finds it while the submit button is disabled', () => { + dialog.innerHTML = 'Move'; + expect(findReturnSubmitForm(dialog)).toBe(dialog.querySelector('form')); + }); + + it('finds a form whose submit button is an input, or spells its type in capitals', () => { + dialog.innerHTML = ''; + expect(findReturnSubmitForm(dialog)).not.toBeNull(); + dialog.innerHTML = 'Move'; + expect(findReturnSubmitForm(dialog)).not.toBeNull(); + }); + + it('is null when the only button has no type: it submits, but was not written to', () => { + dialog.innerHTML = 'Chip'; + expect(findReturnSubmitForm(dialog)).toBeNull(); + }); + + it('is null when the type is misspelt, which the browser also takes for submit', () => { + dialog.innerHTML = 'Move'; + expect((dialog.querySelector('button') as HTMLButtonElement).type).toBe('submit'); + expect(findReturnSubmitForm(dialog)).toBeNull(); + }); + + it('is null when the dialog has no form', () => { + dialog.innerHTML = 'Something to read.Close'; + expect(findReturnSubmitForm(dialog)).toBeNull(); + }); + + it('is null when the form has nothing that submits it', () => { + dialog.innerHTML = 'Cancel'; + expect(findReturnSubmitForm(dialog)).toBeNull(); + }); + + it.each([ + ['a textarea', ''], + ['editable content', ''], + ['editable content turned off while busy', ''], + ])('is null when the form holds %s', (_name, editor) => { + dialog.innerHTML = `${editor}Start`; + expect(findReturnSubmitForm(dialog)).toBeNull(); + }); + + it('finds a multiline form when asked to allow one', () => { + dialog.innerHTML = 'Start'; + expect(findReturnSubmitForm(dialog, true)).toBe(dialog.querySelector('form')); + }); + + it('ignores a multiline editor outside the form', () => { + dialog.innerHTML = + 'Move'; + expect(findReturnSubmitForm(dialog)).not.toBeNull(); + }); +}); + +describe('findEnabledSubmitButton', () => { + function form(html: string): HTMLFormElement { + dialog.innerHTML = `${html}`; + return dialog.querySelector('form')!; + } + + it('finds the submit button, past the other buttons', () => { + const found = findEnabledSubmitButton( + form('CancelMove') + ); + expect(found?.textContent).toBe('Move'); + }); + + it('is null when the submit button is disabled', () => { + expect( + findEnabledSubmitButton(form('Move')) + ).toBeNull(); + }); + + it('finds a submit button outside the form that is tied to it by a form attribute', () => { + dialog.innerHTML = + 'ChipCreate'; + const form = dialog.querySelector('form')!; + expect(findReturnSubmitForm(dialog)).toBe(form); + expect(findEnabledSubmitButton(form)?.textContent).toBe('Create'); + }); + + it('passes over a button with no type, which the browser would press first', () => { + const chips = form('ChipAdd'); + // What the browser picks for a Return in a field: the first button that submits. + const browserDefault = [...chips.elements].find( + (el) => (el as HTMLButtonElement).type === 'submit' + ); + expect(browserDefault?.textContent).toBe('Chip'); + + expect(findEnabledSubmitButton(chips)?.textContent).toBe('Add'); + }); + + it('is null, not an untyped button, while the explicit submit button is disabled', () => { + const found = findEnabledSubmitButton( + form('ChipAdd') + ); + expect(found).toBeNull(); + }); + + it('is null when no button in the form says type="submit"', () => { + expect( + findEnabledSubmitButton(form('ChipCancel')) + ).toBeNull(); + }); + + it('finds an input written as a submit button', () => { + const found = findEnabledSubmitButton(form('')); + expect((found as HTMLInputElement | null)?.value).toBe('Move'); + }); + + it('is null when the first submit button is disabled, as for the browser', () => { + const found = findEnabledSubmitButton( + form('MoveCopy') + ); + expect(found).toBeNull(); + }); +}); diff --git a/apps/staged/src/lib/shared/dialogDefaultAction.ts b/apps/staged/src/lib/shared/dialogDefaultAction.ts new file mode 100644 index 000000000..3167cecfd --- /dev/null +++ b/apps/staged/src/lib/shared/dialogDefaultAction.ts @@ -0,0 +1,148 @@ +// dialogDefaultAction.ts — plain Return on a dialog activates its default button. + +import { isImeKeydown } from './imeKeydown'; + +/** + * Controls that take Return themselves: a focused button or link activates + * on it, a field submits or breaks a line, a widget picks its item. The + * dialog leaves their keys alone, or a focused Cancel would both cancel and + * confirm. + * + * The composite roles at the end are containers that can hold the focus for + * their items (a listbox tracking its option through `aria-activedescendant`, + * a menu, a grid) and panels with their own Return (a dialog docked inside + * this one). A Return on such a container, or on anything in it, is a pick + * or a confirmation there, never this dialog's default button. + */ +const TAKES_RETURN_SELECTOR = [ + 'button', + 'a[href]', + 'input', + 'select', + 'textarea', + 'summary', + '[contenteditable]:not([contenteditable="false"])', + ...[ + 'button', + 'link', + 'checkbox', + 'radio', + 'switch', + 'tab', + 'option', + 'menuitem', + 'menuitemcheckbox', + 'menuitemradio', + 'treeitem', + 'combobox', + 'textbox', + 'searchbox', + 'slider', + 'spinbutton', + 'listbox', + 'menu', + 'menubar', + 'grid', + 'treegrid', + 'tree', + 'radiogroup', + 'tablist', + 'dialog', + 'alertdialog', + ].map((role) => `[role="${role}"]`), +].join(', '); + +/** + * Whether `e` is a plain Return for the dialog's default button: no + * modifier, not a held key repeating (the press that opened the dialog must + * not also confirm it), not an IME commit, not already handled, and not + * aimed at a control with its own use for Return. + * + * Call it from the dialog's own keydown listener: the controls looked for + * are the target and what lies between it and the listening element + * (`e.currentTarget`). The listening element is left out, since the dialog + * carries `role="dialog"` itself. + */ +export function isDefaultActionKeydown(e: KeyboardEvent): boolean { + if (e.key !== 'Enter') return false; + if (e.metaKey || e.ctrlKey || e.altKey || e.shiftKey) return false; + if (e.repeat || e.defaultPrevented || isImeKeydown(e)) return false; + return !(e.target instanceof Element && takesReturn(e.target, e.currentTarget)); +} + +/** Whether `target`, or an element between it and `listener`, takes Return itself. */ +function takesReturn(target: Element, listener: EventTarget | null): boolean { + for (let el: Element | null = target; el && el !== listener; el = el.parentElement) { + if (el.matches(TAKES_RETURN_SELECTOR)) return true; + } + return false; +} + +/** + * The button matching `selector` inside `container`, if it can be pressed: + * null when there is none or it is disabled, so a busy dialog stays busy. + */ +export function findEnabledButton(container: ParentNode, selector: string): HTMLElement | null { + const button = container.querySelector(selector); + return button && isPressable(button) ? button : null; +} + +function isPressable(button: HTMLElement): boolean { + return !(button as HTMLButtonElement).disabled && button.getAttribute('aria-disabled') !== 'true'; +} + +/** A button written as a submit button: `type="submit"` spelled out. */ +const EXPLICIT_SUBMIT_SELECTOR = 'button[type="submit" i], input[type="submit" i]'; + +/** + * The form's default button: the first button that belongs to the form, + * inside it or tied to it by a `form` attribute, and says `type="submit"`. + * + * The browser, for a Return typed in a field, also counts a button with no + * `type`, which submits by default. This does not. An untyped button in a + * form is nearly always a chip or an option row whose author never meant it + * to submit, and pressing one from the keyboard runs its click handler + * first: Return on the Add Repository dialog used to pick the first recent + * repo and add it. So every button in a form states its type, and one that + * does not is never pressed from here. + */ +function defaultSubmitButton(form: HTMLFormElement): HTMLElement | null { + for (const control of Array.from(form.elements)) { + if (control.matches(EXPLICIT_SUBMIT_SELECTOR)) return control as HTMLElement; + } + return null; +} + +/** + * Editors where Return breaks a line. A disabled one (`contenteditable` + * turned "false" while the form is busy) still marks its form as multiline. + */ +const MULTILINE_EDITOR_SELECTOR = 'textarea, [contenteditable]'; + +/** + * The form a plain Return submits in a dialog: its first form with an + * explicit submit button (see `defaultSubmitButton`), whether or not that + * button can be pressed right now. Null when there is none (a viewer has + * nothing to submit), and, unless `allowMultiline`, when the form holds a + * multiline editor: there Return breaks a line and Cmd+Return submits, so + * plain Return submits from nowhere in the dialog. + */ +export function findReturnSubmitForm( + container: ParentNode, + allowMultiline = false +): HTMLFormElement | null { + const form = container.querySelector('form'); + if (!form || !defaultSubmitButton(form)) return null; + if (!allowMultiline && form.querySelector(MULTILINE_EDITOR_SELECTOR)) return null; + return form; +} + +/** + * The button a Return submits `form` with, its default button (see + * `defaultSubmitButton`), or null when that is disabled: the browser + * submits nothing then either. + */ +export function findEnabledSubmitButton(form: HTMLFormElement): HTMLElement | null { + const button = defaultSubmitButton(form); + return button && isPressable(button) ? button : null; +}
Something to read.
Pick a destination.
Reply to the selection.
Are you sure?
Reply
Pick one