diff --git a/e2e/full-chat.spec.ts b/e2e/full-chat.spec.ts index c99de18c..4d2ff164 100644 --- a/e2e/full-chat.spec.ts +++ b/e2e/full-chat.spec.ts @@ -257,3 +257,25 @@ test("a reply in the thread tab adds to the count under its parent", async ({ ).toBeVisible(); await expect(log.getByRole("button", { name: /4 replies/ })).toBeVisible(); }); + +for (const [item, list, draft, typed] of [ + ["Mention an agent", "Mentions", "", "@"], + ["Mention an agent", "Mentions", "Check this", "Check this @"], + ["Use a command", "Commands", "", "/"], + // A command starts a line, so it goes on a new one. + ["Use a command", "Commands", "Check this", "Check this\n/"], +] as const) { + test(`"${item}" in the Add menu opens the ${list.toLowerCase()} at once, after "${draft}"`, async ({ + page, + }) => { + await open(page, "settle"); + const main = page.getByRole("main"); + const message = main.getByRole("textbox", { name: "Message" }); + await message.fill(draft); + await main.getByRole("button", { name: "Add", exact: true }).click(); + await page.getByRole("menuitem", { name: item }).click(); + await expect(message).toBeFocused(); + await expect(message).toHaveValue(typed); + await expect(main.getByRole("listbox", { name: list })).toBeVisible(); + }); +} diff --git a/e2e/prompt-input-toolbar.spec.ts b/e2e/prompt-input-toolbar.spec.ts index 6c5aa241..647a490f 100644 --- a/e2e/prompt-input-toolbar.spec.ts +++ b/e2e/prompt-input-toolbar.spec.ts @@ -2,9 +2,9 @@ import { expect, test, type Page } from "@playwright/test"; const events = (page: Page) => page.getByRole("status", { name: "Events" }); const message = (page: Page) => page.getByRole("textbox", { name: "Message" }); -const model = (page: Page) => page.getByRole("combobox", { name: "Model" }); -const mode = (page: Page) => - page.getByRole("combobox", { name: "Mode", exact: true }); +// A closed picker is named for its field and its choice: "Model, Deep". +const model = (page: Page) => page.getByRole("combobox", { name: /^Model, / }); +const mode = (page: Page) => page.getByRole("combobox", { name: /^Mode, / }); const add = (page: Page) => page.getByRole("button", { name: "Add", exact: true }); @@ -22,6 +22,7 @@ test.beforeEach(async ({ page }) => { test("the model picker opens, moves and chooses by keyboard, keeping focus on its trigger", async ({ page, }) => { + await expect(model(page)).toHaveAccessibleName("Model, Deep"); await model(page).focus(); await page.keyboard.press("Enter"); const list = page.getByRole("listbox", { name: "Model" }); @@ -41,7 +42,7 @@ test("the model picker opens, moves and chooses by keyboard, keeping focus on it await page.keyboard.press("Enter"); await expect(list).toBeHidden(); await expect(events(page)).toHaveText("model:lite"); - await expect(model(page)).toHaveText("Lite"); + await expect(model(page)).toHaveAccessibleName("Model, Lite"); await expect(model(page)).toBeFocused(); }); @@ -59,7 +60,7 @@ test("Escape closes a picker without choosing and leaves focus on its trigger", page.getByRole("listbox", { name: "Mode", exact: true }), ).toBeHidden(); await expect(mode(page)).toBeFocused(); - await expect(mode(page)).toHaveText("Ask"); + await expect(mode(page)).toHaveAccessibleName("Mode, Ask"); await expect(events(page)).toHaveText(""); // Space opens and chooses too. @@ -67,6 +68,7 @@ test("Escape closes a picker without choosing and leaves focus on its trigger", await page.keyboard.press("ArrowDown"); await page.keyboard.press(" "); await expect(events(page)).toHaveText("mode:plan"); + await expect(mode(page)).toHaveAccessibleName("Mode, Plan"); await expect( page.getByRole("listbox", { name: "Mode", exact: true }), ).toBeHidden(); @@ -79,7 +81,7 @@ test("a picker chooses by pointer and closes on a press outside", async ({ await model(page).click(); await page.getByRole("option", { name: "Swift 2" }).click(); await expect(events(page)).toHaveText("model:swift"); - await expect(model(page)).toHaveText("Swift"); + await expect(model(page)).toHaveAccessibleName("Model, Swift"); await model(page).click(); await expect(page.getByRole("listbox", { name: "Model" })).toBeVisible(); diff --git a/src/lib/prompt-tokens.test.ts b/src/lib/prompt-tokens.test.ts index caef150b..1873cd5b 100644 --- a/src/lib/prompt-tokens.test.ts +++ b/src/lib/prompt-tokens.test.ts @@ -129,4 +129,18 @@ describe("splitPromptSegments", () => { expect(splitPromptSegments("hello", TOKENS)).toEqual([{ text: "hello" }]); expect(splitPromptSegments("", TOKENS)).toEqual([]); }); + + test("an empty token is skipped rather than matched forever", () => { + const empty: PromptToken[] = [ + { id: "empty-mention", kind: "mention", token: "" }, + { id: "empty-command", kind: "command", token: "" }, + ]; + expect( + splitPromptSegments(" @Ada and\n\n", [...empty, ...TOKENS]), + ).toEqual([ + { text: " " }, + { text: "@Ada", token: ADA }, + { text: " and\n\n" }, + ]); + }); }); diff --git a/src/lib/prompt-tokens.ts b/src/lib/prompt-tokens.ts index e9b1b50e..bbd7b8fc 100644 --- a/src/lib/prompt-tokens.ts +++ b/src/lib/prompt-tokens.ts @@ -112,15 +112,15 @@ export function insertPromptToken( * Splits prompt text into plain runs and known tokens. A token counts only as * a whole word where its kind may start (see `findActivePromptToken`): "@Ada" * in "@Ada, can you", not in "@Adam" or "x@Ada". Longer tokens win, so - * "@Ada Lovelace" is found before "@Ada". + * "@Ada Lovelace" is found before "@Ada". An empty token matches nothing. */ export function splitPromptSegments( value: string, tokens: readonly PromptToken[], ): readonly PromptSegment[] { - const longestFirst = [...tokens].sort( - (a, b) => b.token.length - a.token.length, - ); + const longestFirst = tokens + .filter((token) => token.token !== "") + .sort((a, b) => b.token.length - a.token.length); const segments: PromptSegment[] = []; let runStart = 0; let index = 0; diff --git a/src/ui/prompt-input-toolbar.test.tsx b/src/ui/prompt-input-toolbar.test.tsx index a1059202..75c3affe 100644 --- a/src/ui/prompt-input-toolbar.test.tsx +++ b/src/ui/prompt-input-toolbar.test.tsx @@ -63,12 +63,21 @@ function mount(node: ReactNode) { }; } -function press(target: Element, key: string): KeyboardEvent { +/** `keyCode` is set on the event itself: `KeyboardEventInit` drops it. */ +function press( + target: Element, + key: string, + { keyCode, ...init }: KeyboardEventInit & { keyCode?: number } = {}, +): KeyboardEvent { const event = new KeyboardEvent("keydown", { + ...init, key, bubbles: true, cancelable: true, }); + if (keyCode !== undefined) { + Object.defineProperty(event, "keyCode", { value: keyCode }); + } act(() => { target.dispatchEvent(event); }); @@ -115,7 +124,8 @@ describe("PromptInputModelSelect", () => { test("shows the first model's short name by default and lists models by group", () => { const view = mount(); const trigger = view.query("[role='combobox']"); - expect(trigger.getAttribute("aria-label")).toBe("Model"); + // Named for the field and the choice, as a closed picker is heard. + expect(trigger.getAttribute("aria-label")).toBe("Model, Swift"); expect(trigger.textContent).toBe("Swift"); expect(trigger.getAttribute("aria-expanded")).toBe("false"); expect(trigger.hasAttribute("aria-controls")).toBe(false); @@ -255,7 +265,9 @@ describe("PromptInputModelSelect", () => { onValueChange={() => undefined} />, ); - expect(view.query("[role='combobox']").textContent).toBe("Model"); + const trigger = view.query("[role='combobox']"); + expect(trigger.textContent).toBe("Model"); + expect(trigger.getAttribute("aria-label")).toBe("Model"); view.unmount(); }); @@ -369,7 +381,7 @@ describe("PromptInputModeSelect", () => { const events: string[] = []; const view = mount(); const trigger = view.query("[role='combobox']"); - expect(trigger.getAttribute("aria-label")).toBe("Mode"); + expect(trigger.getAttribute("aria-label")).toBe("Mode, Agent"); expect(trigger.textContent).toBe("Agent"); act(() => trigger.focus()); @@ -381,6 +393,7 @@ describe("PromptInputModeSelect", () => { press(trigger, " "); expect(events).toEqual(["ask"]); expect(trigger.textContent).toBe("Ask"); + expect(trigger.getAttribute("aria-label")).toBe("Mode, Ask"); view.unmount(); }); @@ -590,6 +603,66 @@ describe("PromptInputAddMenu", () => { }); }); +describe("a popup whose options run out", () => { + /** Each popup with or without its options, open state owned by the host + * when `controlled`. */ + const POPUP_PARTS = { + add: { + popup: "[role='menu']", + render: (empty: boolean, state: PopupState) => ( + + ), + }, + model: { + popup: "[role='listbox']", + render: (empty: boolean, state: PopupState) => ( + + ), + }, + mode: { + popup: "[role='listbox']", + render: (empty: boolean, state: PopupState) => ( + + ), + }, + } as const; + type PopupState = { + readonly open?: boolean; + readonly defaultOpen?: boolean; + readonly onOpenChange: (open: boolean) => void; + }; + + for (const which of ["add", "model", "mode"] as const) { + for (const controlled of [false, true]) { + test(`the ${controlled ? "host-owned" : "own"} ${which} popup closes once and stays closed when they return`, () => { + const { popup, render } = POPUP_PARTS[which]; + const changes: boolean[] = []; + function Host({ empty }: { readonly empty: boolean }) { + const [open, setOpen] = useState(true); + const onOpenChange = (next: boolean) => { + changes.push(next); + setOpen(next); + }; + return render( + empty, + controlled + ? { open, onOpenChange } + : { defaultOpen: true, onOpenChange }, + ); + } + const view = mount(); + expect(shown(view, popup)).toBe(true); + view.rerender(); + expect(changes).toEqual([false]); + view.rerender(); + expect(shown(view, popup)).toBe(false); + expect(changes).toEqual([false]); + view.unmount(); + }); + } + } +}); + describe("PromptInputButton", () => { test("never submits the form it sits in", () => { const submits: string[] = []; @@ -862,6 +935,27 @@ describe("toolbar parts inside PromptInput", () => { }); } + // Safari sends the key that ends a composition with `isComposing` already + // false and `keyCode` 229. + for (const [how, init] of [ + ["isComposing", { isComposing: true }], + ["keyCode 229", { isComposing: false, keyCode: 229 }], + ] as const) { + for (const which of ["add", "model"] as const) { + test(`an Escape that ends an IME composition (${how}) leaves the host-opened ${which} popup open`, () => { + const events: string[] = []; + const view = mount(); + const textarea = view.query("textarea"); + act(() => textarea.focus()); + const event = press(textarea, "Escape", init); + expect(event.defaultPrevented).toBe(false); + expect(events).toEqual([]); + expect(shown(view, POPUPS[which].popup)).toBe(true); + view.unmount(); + }); + } + } + test("with the suggestion list open too, the first Escape closes only the list", () => { const view = mount(); const textarea = view.query("textarea"); diff --git a/src/ui/prompt-input-toolbar.tsx b/src/ui/prompt-input-toolbar.tsx index 316cbe64..0d27c32a 100644 --- a/src/ui/prompt-input-toolbar.tsx +++ b/src/ui/prompt-input-toolbar.tsx @@ -67,8 +67,9 @@ const OPEN_POPUP = [ * say, beside a popup the host opened) closes the popup here and leaves * focus where it is. Left alone are an Escape from outside the * `PromptInput`, which belongs to whatever layer it was pressed in, one - * inside another open toolbar popup, which that popup closes, and any while - * the `PromptInput`'s suggestion list is showing, which the list takes. + * inside another open toolbar popup, which that popup closes, any while + * the `PromptInput`'s suggestion list is showing, which the list takes, and + * any that ends an IME composition, which is the IME's. */ function useClaimEscape( open: boolean, @@ -82,6 +83,10 @@ function useClaimEscape( const { target } = event; if ( event.key !== "Escape" || + // 229 is what Safari reports for the key that ends a composition, + // after `isComposing` has already turned false. + event.isComposing || + event.keyCode === 229 || event.defaultPrevented || root === null || !(target instanceof Element) @@ -111,14 +116,14 @@ function useClaimEscape( } /** - * What a popup shows of its `open` state: closed while `disabled`, whatever - * its owner holds, so a disabled part offers nothing to choose. Turning - * disabled while open closes it for the owner too, so it stays closed once - * the part is enabled again. + * What a popup shows of its `open` state: closed while `unavailable` + * (disabled, or out of options), whatever its owner holds, so such a part + * offers nothing to choose. Turning unavailable while open closes it for the + * owner too, so it stays closed once the part is available again. */ -function useClosedWhileDisabled( +function useClosedWhileUnavailable( open: boolean, - disabled: boolean, + unavailable: boolean, setOpen: (open: boolean) => void, ): boolean { // The latest `setOpen`, which is new on every render of a popup the owner @@ -128,9 +133,9 @@ function useClosedWhileDisabled( setOpenRef.current = setOpen; }); useEffect(() => { - if (disabled && open) setOpenRef.current(false); - }, [disabled, open]); - return open && !disabled; + if (unavailable && open) setOpenRef.current(false); + }, [unavailable, open]); + return open && !unavailable; } /** Open state a parent may own, as on every popup here. */ @@ -267,7 +272,11 @@ export const PromptInputAddMenu = forwardRef< const { setOpen, rootRef, triggerRef, close } = popover; const boxDisabled = usePromptInputDisabled(); const disabled = disabledProp ?? boxDisabled; - const open = useClosedWhileDisabled(popover.open, disabled, setOpen); + const open = useClosedWhileUnavailable( + popover.open, + disabled || items.length === 0, + setOpen, + ); useClaimEscape(open, rootRef, setOpen); const menuRef = useRef(null); const menuId = useId(); @@ -470,8 +479,8 @@ type SelectStateProps = Omit< type OptionSelectProps = SelectStateProps & { readonly slot: string; - /** The trigger's and the list's accessible name; also what the trigger - * shows while the value matches no option. */ + /** The list's accessible name, and the trigger's ahead of the chosen + * option; also what the trigger shows while the value matches no option. */ readonly label: string; readonly options: readonly SelectOption[]; }; @@ -526,7 +535,11 @@ const OptionSelect = forwardRef(function OptionSelect( const { setOpen, rootRef, triggerRef, close } = popover; const boxDisabled = usePromptInputDisabled(); const disabled = disabledProp ?? boxDisabled; - const open = useClosedWhileDisabled(popover.open, disabled, setOpen); + const open = useClosedWhileUnavailable( + popover.open, + disabled || options.length === 0, + setOpen, + ); useClaimEscape(open, rootRef, setOpen); const listId = useId(); const [activeId, setActiveId] = useState(undefined); @@ -620,7 +633,9 @@ const OptionSelect = forwardRef(function OptionSelect( ref={triggerRef} type="button" role="combobox" - aria-label={label} + // The field and the choice, "Model, Swift", since the label alone + // would hide what the trigger shows. + aria-label={chosen === undefined ? label : `${label}, ${chosen.short}`} aria-haspopup="listbox" aria-expanded={open} aria-controls={open ? listId : undefined} @@ -732,7 +747,8 @@ export type PromptInputModelOption = { export type PromptInputModelSelectProps = SelectStateProps & { readonly models: readonly PromptInputModelOption[]; - /** The accessible name; also shown when `value` matches no model. */ + /** The field's accessible name, which the trigger follows with the chosen + * model; also shown when `value` matches no model. */ readonly label?: string; }; @@ -776,7 +792,8 @@ export type PromptInputModeOption = { export type PromptInputModeSelectProps = SelectStateProps & { readonly modes: readonly PromptInputModeOption[]; - /** The accessible name; also shown when `value` matches no mode. */ + /** The field's accessible name, which the trigger follows with the chosen + * mode; also shown when `value` matches no mode. */ readonly label?: string; }; diff --git a/stories/chat/full-chat.stories.tsx b/stories/chat/full-chat.stories.tsx index c5b80eea..8d339132 100644 --- a/stories/chat/full-chat.stories.tsx +++ b/stories/chat/full-chat.stories.tsx @@ -574,11 +574,13 @@ function Composer({ setAttachments((current) => [...current, ...accepted]); setNotice(rejected.map(refusal).join("; ")); }; - const startToken = (prefix: string) => { + const startToken = (prefix: "@" | "/") => { + // A mention starts a word and a command a line, or the prompt would not + // take it for one and offer its suggestions. + const [fresh, gap] = prefix === "/" ? [/(^|\n)$/, "\n"] : [/(^|\s)$/, " "]; setDraft( (current) => - (current === "" || current.endsWith(" ") ? current : `${current} `) + - prefix, + (fresh.test(current) ? current : `${current}${gap}`) + prefix, ); textarea.current?.focus(); };