From be283c7ac2a4063dca6765def2be6eee84610ca3 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sun, 7 Jun 2026 01:57:45 +0100 Subject: [PATCH] Tighten model picker keyboard ownership Ensure the shared model picker owns search-to-list keyboard navigation and Escape focus restoration without letting the App-level Assistant drawer close consume the same keypress. --- .../v6/internal/subsystems/ai-runtime.md | 5 +- .../subsystems/frontend-primitives.md | 12 +-- frontend-modern/src/App.tsx | 5 +- .../src/__tests__/App.architecture.test.ts | 3 +- .../src/components/shared/AIModelPicker.tsx | 87 +++++++++++++++---- .../shared/__tests__/AIModelPicker.test.tsx | 78 +++++++++++++++-- .../shared/__tests__/SearchField.test.tsx | 32 +++++++ .../components/shared/useSearchFieldState.ts | 6 +- 8 files changed, 194 insertions(+), 34 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 39bb89aae..6e15f94eb 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -554,7 +554,10 @@ runtime cost control, and shared AI transport surfaces. focused composer arm the visible Stop control first and letting the next Escape confirm the same governed `chat.stop()` path as the Stop button, including aborting the active stream, clearing queued follow-ups, preserving - partial text, and returning focus to the composer. + partial text, and returning focus to the composer. The App-level Assistant + drawer Escape guard must not close the drawer when Escape originates inside + the shared model picker; model-picker Escape is a local search/listbox close + path that returns focus to the picker trigger while the drawer remains open. The referenced OpenCode source at fetched `origin/dev` commit `fa2b63f850fc0a23bec2bdff9e660450d3fe7913` keeps prompt/footer status visible only while the session is non-idle in diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 84793b08e..f19cc164e 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -1039,7 +1039,9 @@ not a replacement status card, CTA band, or page-local nested card. must expose its owned listbox while expanded, and keyboard movement from search through the option rows must support current-row focus, filtered-result focus, up/down, page, home/end, and Escape return to the - trigger so model choice does not depend on mouse interaction. + trigger so model choice does not depend on mouse interaction. Picker-owned + navigation keys, including Escape, must be consumed by the picker so parent + shells do not also treat the same keypress as drawer or page-level Escape. Gateway-routed model choices must not look like direct-provider choices: the shared picker, System AI settings status strip, and per-surface inherited-default descriptions must render OpenRouter-hosted provider @@ -2341,10 +2343,10 @@ Escape clear/blur behavior and input-ref lifecycle, and visibility rules plus trailing-control padding policy. Future search-field work should extend those owners instead of pushing event behavior or layout policy back into the shared shell. Forwarded keyboard and blur events must preserve -native browser event getters while normalizing `currentTarget` and `target`; -shared search-field wrappers must not proxy native event properties through a -receiver that can break `KeyboardEvent`/`FocusEvent` getters in live browser -surfaces. +native browser event getters and methods while normalizing `currentTarget` and +`target`; shared search-field wrappers must not proxy native event properties or +methods through a receiver that can break `KeyboardEvent`/`FocusEvent` getters, +`preventDefault()`, or `stopPropagation()` in live browser surfaces. The shared search input now follows that same owner split. `frontend-modern/src/components/shared/SearchInput.tsx` stays the render shell, `frontend-modern/src/components/shared/useSearchInputState.ts` owns input-ref diff --git a/frontend-modern/src/App.tsx b/frontend-modern/src/App.tsx index d825e52bd..a65699303 100644 --- a/frontend-modern/src/App.tsx +++ b/frontend-modern/src/App.tsx @@ -397,8 +397,11 @@ function App() { // Escape closes the drawer only after mounted drawer controls have had // a chance to claim the key for local flows such as interrupt confirm. if (e.key === 'Escape' && aiChatStore.isOpen) { + const escapeTarget = e.target instanceof Element ? e.target : null; + const isModelPickerEscape = Boolean(escapeTarget?.closest('[data-ai-model-picker]')); + window.setTimeout(() => { - if (!e.defaultPrevented && aiChatStore.isOpen) { + if (!e.defaultPrevented && !isModelPickerEscape && aiChatStore.isOpen) { aiChatStore.close(); } }, 0); diff --git a/frontend-modern/src/__tests__/App.architecture.test.ts b/frontend-modern/src/__tests__/App.architecture.test.ts index 4d60d98ff..a92a9bf0d 100644 --- a/frontend-modern/src/__tests__/App.architecture.test.ts +++ b/frontend-modern/src/__tests__/App.architecture.test.ts @@ -174,7 +174,8 @@ describe('App architecture', () => { ); expect(appSource).toContain("if (e.key === 'Escape' && aiChatStore.isOpen) {"); expect(appSource).toContain('window.setTimeout(() => {'); - expect(appSource).toContain('if (!e.defaultPrevented && aiChatStore.isOpen) {'); + expect(appSource).toContain("closest('[data-ai-model-picker]')"); + expect(appSource).toContain('if (!e.defaultPrevented && !isModelPickerEscape && aiChatStore.isOpen) {'); expect(appSource).toContain(' aiChatStore.close()} />'); expect(appSource).toContain('showOrgSwitcher={runtime.showOrgSwitcher}'); expect(appSource).not.toContain('TrialBanner'); diff --git a/frontend-modern/src/components/shared/AIModelPicker.tsx b/frontend-modern/src/components/shared/AIModelPicker.tsx index f14f47689..92303aac8 100644 --- a/frontend-modern/src/components/shared/AIModelPicker.tsx +++ b/frontend-modern/src/components/shared/AIModelPicker.tsx @@ -369,6 +369,17 @@ export const AIModelPicker: Component = (props) => { setSearchQuery(''); }; + const focusTriggerAfterClose = () => { + const trigger = buttonRef; + if (!trigger) return; + window.setTimeout(() => trigger.focus(), 0); + }; + + const closePickerAndFocusTrigger = () => { + closePicker(); + focusTriggerAfterClose(); + }; + const focusSearchInput = () => { queueMicrotask(() => searchInputRef?.focus()); }; @@ -420,12 +431,32 @@ export const AIModelPicker: Component = (props) => { return true; }; - const focusInitialOption = () => { + const initialOptionIndex = () => { const keys = displayedOptionKeys(); - if (keys.length === 0) return false; + if (keys.length === 0) return -1; const currentKey = currentOptionKey(); const currentIndex = !searchQuery().trim() && currentKey ? keys.indexOf(currentKey) : -1; - return focusOptionAtIndex(currentIndex >= 0 ? currentIndex : 0); + return currentIndex >= 0 ? currentIndex : 0; + }; + + const focusInitialOption = () => { + const nextIndex = initialOptionIndex(); + return nextIndex >= 0 && focusOptionAtIndex(nextIndex); + }; + + const focusOptionFromSearchByOffset = (offset: number) => { + const keys = displayedOptionKeys(); + const startIndex = initialOptionIndex(); + if (keys.length === 0 || startIndex < 0) return false; + let nextIndex = startIndex + offset; + if (nextIndex < 0) nextIndex = keys.length - 1; + if (nextIndex >= keys.length) nextIndex = 0; + return focusOptionAtIndex(nextIndex); + }; + + const consumePickerKey = (event: KeyboardEvent) => { + event.preventDefault(); + event.stopPropagation(); }; const focusOptionRelativeTo = (optionKey: string, offset: number) => { @@ -441,15 +472,36 @@ export const AIModelPicker: Component = (props) => { const handleSearchKeyDown = (event: KeyboardEvent) => { if (event.altKey || event.ctrlKey || event.metaKey) return; if (event.key === 'ArrowDown' && focusInitialOption()) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'ArrowUp' && focusOptionAtIndex(displayedOptionKeys().length - 1)) { - event.preventDefault(); + consumePickerKey(event); + return; + } + if (event.key === 'PageDown' && focusOptionFromSearchByOffset(10)) { + consumePickerKey(event); + return; + } + if (event.key === 'PageUp' && focusOptionFromSearchByOffset(-10)) { + consumePickerKey(event); + return; + } + if (event.key === 'Home' && focusOptionAtIndex(0)) { + consumePickerKey(event); + return; + } + if (event.key === 'End' && focusOptionAtIndex(displayedOptionKeys().length - 1)) { + consumePickerKey(event); + return; + } + if (event.key === 'Escape') { + consumePickerKey(event); + closePickerAndFocusTrigger(); return; } if (event.key === 'Enter') { - event.preventDefault(); + consumePickerKey(event); const candidate = customModelCandidate(); if (candidate && (exactCandidateModel() || showCustomModelOption())) { handleSelect(candidate); @@ -464,33 +516,32 @@ export const AIModelPicker: Component = (props) => { if (event.altKey || event.ctrlKey || event.metaKey) return; if (event.key === 'ArrowDown' && focusOptionRelativeTo(optionKey, 1)) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'ArrowUp' && focusOptionRelativeTo(optionKey, -1)) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'PageDown' && focusOptionRelativeTo(optionKey, 10)) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'PageUp' && focusOptionRelativeTo(optionKey, -10)) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'Home' && focusOptionAtIndex(0)) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'End' && focusOptionAtIndex(displayedOptionKeys().length - 1)) { - event.preventDefault(); + consumePickerKey(event); return; } if (event.key === 'Escape') { - event.preventDefault(); - closePicker(); - buttonRef?.focus(); + consumePickerKey(event); + closePickerAndFocusTrigger(); } }; @@ -498,9 +549,8 @@ export const AIModelPicker: Component = (props) => { event: KeyboardEvent & { currentTarget: HTMLButtonElement }, ) => { if (event.key !== 'Escape' || event.altKey || event.ctrlKey || event.metaKey) return; - event.preventDefault(); - closePicker(); - buttonRef?.focus(); + consumePickerKey(event); + closePickerAndFocusTrigger(); }; createEffect(() => { @@ -584,6 +634,7 @@ export const AIModelPicker: Component = (props) => { value={searchQuery()} onChange={setSearchQuery} onKeyDown={handleSearchKeyDown} + clearOnFocusedEscape={false} placeholder={props.searchPlaceholder || 'Search or enter model ID'} class="flex-1" inputClass="py-1.5 text-xs focus:ring-blue-400" diff --git a/frontend-modern/src/components/shared/__tests__/AIModelPicker.test.tsx b/frontend-modern/src/components/shared/__tests__/AIModelPicker.test.tsx index 67fa5aa2f..e1c581448 100644 --- a/frontend-modern/src/components/shared/__tests__/AIModelPicker.test.tsx +++ b/frontend-modern/src/components/shared/__tests__/AIModelPicker.test.tsx @@ -238,6 +238,63 @@ describe('AIModelPicker', () => { expect(document.activeElement).toBe(currentOption); }); + it('supports page, home, end, and Escape from the search field', async () => { + const pageModels: ModelInfo[] = Array.from({ length: 12 }, (_, index) => ({ + id: `openrouter:page/model-${index}`, + name: `Page Model ${index}`, + notable: true, + provider: 'openrouter', + })); + const onParentKeyDown = vi.fn(); + + render(() => ( +
+ +
+ )); + + const button = screen.getByTitle('Select shared default model'); + fireEvent.click(button); + + const searchInput = screen.getByPlaceholderText('Search or enter model ID'); + const optionFor = (index: number) => + screen.getByRole('option', { + name: new RegExp(`^Page Model ${index} via OpenRouter(?:, Current)?\\. `), + }); + + await waitFor(() => { + expect(document.activeElement).toBe(searchInput); + }); + + fireEvent.keyDown(searchInput, { key: 'PageDown' }); + expect(document.activeElement).toBe(optionFor(10)); + + searchInput.focus(); + fireEvent.keyDown(searchInput, { key: 'PageUp' }); + expect(document.activeElement).toBe(optionFor(11)); + + searchInput.focus(); + fireEvent.keyDown(searchInput, { key: 'End' }); + expect(document.activeElement).toBe(optionFor(11)); + + searchInput.focus(); + fireEvent.keyDown(searchInput, { key: 'Home' }); + expect(document.activeElement).toBe(optionFor(0)); + + searchInput.focus(); + fireEvent.keyDown(searchInput, { key: 'Escape' }); + expect(screen.queryByRole('listbox', { name: 'Select shared default model' })).not.toBeInTheDocument(); + await waitFor(() => { + expect(document.activeElement).toBe(button); + }); + expect(onParentKeyDown).not.toHaveBeenCalled(); + }); + it('moves keyboard focus to the first filtered result while searching', async () => { render(() => ( { }); it('closes the picker and returns focus to the trigger from option Escape', async () => { + const onParentKeyDown = vi.fn(); + render(() => ( - +
+ +
)); const button = screen.getByTitle('Select shared default model'); @@ -290,7 +351,10 @@ describe('AIModelPicker', () => { fireEvent.keyDown(currentOption, { key: 'Escape' }); expect(screen.queryByPlaceholderText('Search or enter model ID')).not.toBeInTheDocument(); - expect(document.activeElement).toBe(button); + await waitFor(() => { + expect(document.activeElement).toBe(button); + }); + expect(onParentKeyDown).not.toHaveBeenCalled(); }); it('constrains the dropdown to the available mobile viewport height', () => { diff --git a/frontend-modern/src/components/shared/__tests__/SearchField.test.tsx b/frontend-modern/src/components/shared/__tests__/SearchField.test.tsx index 9adc8f3d5..4992f6de9 100644 --- a/frontend-modern/src/components/shared/__tests__/SearchField.test.tsx +++ b/frontend-modern/src/components/shared/__tests__/SearchField.test.tsx @@ -117,4 +117,36 @@ describe('SearchField', () => { expect(onBlur).toHaveBeenCalledTimes(1); expect(onBlur.mock.calls[0][0].currentTarget).toBe(input); }); + + it('lets explicit keyboard handlers claim the native event', () => { + const onKeyDown = vi.fn((event: KeyboardEvent) => { + event.preventDefault(); + event.stopPropagation(); + }); + const onParentKeyDown = vi.fn(); + + render(() => ( +
+ +
+ )); + + const input = screen.getByPlaceholderText('Claimed field'); + const event = new KeyboardEvent('keydown', { + key: 'Escape', + bubbles: true, + cancelable: true, + }); + input.dispatchEvent(event); + + expect(onKeyDown).toHaveBeenCalledTimes(1); + expect(event.defaultPrevented).toBe(true); + expect(onParentKeyDown).not.toHaveBeenCalled(); + }); }); diff --git a/frontend-modern/src/components/shared/useSearchFieldState.ts b/frontend-modern/src/components/shared/useSearchFieldState.ts index 374afb2ae..99b2da281 100644 --- a/frontend-modern/src/components/shared/useSearchFieldState.ts +++ b/frontend-modern/src/components/shared/useSearchFieldState.ts @@ -36,7 +36,11 @@ export function useSearchFieldState(options: SearchFieldStateOptions) { get(eventTarget, prop) { if (prop === 'currentTarget') return currentTarget; if (prop === 'target') return normalizedTarget; - return Reflect.get(eventTarget, prop); + const value = Reflect.get(eventTarget, prop); + if (typeof value === 'function') { + return value.bind(eventTarget); + } + return value; }, }); };