mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-10-04 05:07:30 +00:00
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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('<AIChat onClose={() => aiChatStore.close()} />');
|
||||
expect(appSource).toContain('showOrgSwitcher={runtime.showOrgSwitcher}');
|
||||
expect(appSource).not.toContain('TrialBanner');
|
||||
|
||||
@@ -369,6 +369,17 @@ export const AIModelPicker: Component<AIModelPickerProps> = (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<AIModelPickerProps> = (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<AIModelPickerProps> = (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<AIModelPickerProps> = (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<AIModelPickerProps> = (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<AIModelPickerProps> = (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"
|
||||
|
||||
@@ -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(() => (
|
||||
<div onKeyDown={onParentKeyDown}>
|
||||
<AIModelPicker
|
||||
models={pageModels}
|
||||
selectedModel="openrouter:page/model-0"
|
||||
onModelSelect={vi.fn()}
|
||||
title="Select shared default model"
|
||||
/>
|
||||
</div>
|
||||
));
|
||||
|
||||
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(() => (
|
||||
<AIModelPicker
|
||||
@@ -265,13 +322,17 @@ describe('AIModelPicker', () => {
|
||||
});
|
||||
|
||||
it('closes the picker and returns focus to the trigger from option Escape', async () => {
|
||||
const onParentKeyDown = vi.fn();
|
||||
|
||||
render(() => (
|
||||
<AIModelPicker
|
||||
models={models}
|
||||
selectedModel="openrouter:minimax/minimax-m2.5"
|
||||
onModelSelect={vi.fn()}
|
||||
title="Select shared default model"
|
||||
/>
|
||||
<div onKeyDown={onParentKeyDown}>
|
||||
<AIModelPicker
|
||||
models={models}
|
||||
selectedModel="openrouter:minimax/minimax-m2.5"
|
||||
onModelSelect={vi.fn()}
|
||||
title="Select shared default model"
|
||||
/>
|
||||
</div>
|
||||
));
|
||||
|
||||
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', () => {
|
||||
|
||||
@@ -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(() => (
|
||||
<div onKeyDown={onParentKeyDown}>
|
||||
<SearchField
|
||||
value="alpha"
|
||||
onChange={vi.fn()}
|
||||
placeholder="Claimed field"
|
||||
onKeyDown={onKeyDown}
|
||||
clearOnFocusedEscape={false}
|
||||
/>
|
||||
</div>
|
||||
));
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
},
|
||||
});
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user