diff --git a/.changeset/fix-model-picker-navigation.md b/.changeset/fix-model-picker-navigation.md new file mode 100644 index 00000000000..d026b0382bc --- /dev/null +++ b/.changeset/fix-model-picker-navigation.md @@ -0,0 +1,5 @@ +--- +"kilo-code": patch +--- + +Improve screen reader navigation and provider group controls in the model picker. diff --git a/packages/kilo-vscode/tests/model-selector-accessibility.spec.ts b/packages/kilo-vscode/tests/model-selector-accessibility.spec.ts index fcd2111199e..c818e6b9a34 100644 --- a/packages/kilo-vscode/tests/model-selector-accessibility.spec.ts +++ b/packages/kilo-vscode/tests/model-selector-accessibility.spec.ts @@ -16,13 +16,14 @@ test("model selector exposes combobox relationships and active option movement", await page.getByRole("button", { name: "Review model: Alpha" }).click() const combobox = page.getByRole("combobox", { name: "Review model: Alpha. Search models" }) - const listbox = page.getByRole("listbox", { name: "Review model" }) - const alpha = page.getByRole("option", { name: "Alpha" }) - const bravo = page.getByRole("option", { name: "Bravo" }) + const tree = page.getByRole("tree", { name: "Review model" }) + const alpha = page.getByRole("treeitem", { name: "Alpha" }) + const bravo = page.getByRole("treeitem", { name: "Bravo" }) await expect(combobox).toBeFocused() await expect(combobox).toHaveAttribute("aria-expanded", "true") - await expect(combobox).toHaveAttribute("aria-controls", await listbox.getAttribute("id")) + await expect(combobox).toHaveAttribute("aria-haspopup", "tree") + await expect(combobox).toHaveAttribute("aria-controls", await tree.getAttribute("id")) await expect(combobox).toHaveAttribute("aria-activedescendant", await alpha.getAttribute("id")) await expect(combobox).toHaveAccessibleDescription("Choose the model used for code review tasks.") await expect(alpha.locator("button")).toHaveCount(0) @@ -46,12 +47,81 @@ test("model selector exposes combobox relationships and active option movement", await expect(preview.getByRole("button", { name: "Add to favorites" })).toBeVisible() }) +test("typing a provider initial moves the active descendant to matching results", async ({ page }) => { + await load(page, "shared--model-selector-accessible") + + await page.getByRole("button", { name: "Review model: Alpha" }).click() + const combobox = page.getByRole("combobox", { name: "Review model: Alpha. Search models" }) + await combobox.fill("N") + + const nova = page.getByRole("treeitem", { name: "Nova" }) + await expect(nova).toBeVisible() + await expect(combobox).toHaveAttribute("aria-activedescendant", await nova.getAttribute("id")) + await expect(page.getByRole("treeitem", { name: "NVIDIA" })).toHaveAttribute("aria-expanded", "true") +}) + +test("provider groups collapse, expand, and skip their model rows", async ({ page }) => { + await load(page, "shared--model-selector-accessible") + + await page.getByRole("button", { name: "Review model: Alpha" }).click() + const combobox = page.getByRole("combobox", { name: "Review model: Alpha. Search models" }) + const kilo = page.getByRole("treeitem", { name: "Kilo" }) + const nvidia = page.getByRole("treeitem", { name: "NVIDIA" }) + + await combobox.press("ArrowDown") + await combobox.press("ArrowLeft") + await expect(combobox).toHaveAttribute("aria-activedescendant", await kilo.getAttribute("id")) + await combobox.press("ArrowLeft") + await expect(kilo).toHaveAttribute("aria-expanded", "false") + await expect(page.getByRole("treeitem", { name: "Bravo" })).toBeHidden() + + await combobox.press("ArrowDown") + await expect(combobox).toHaveAttribute("aria-activedescendant", await nvidia.getAttribute("id")) + await combobox.press("ArrowLeft") + await expect(nvidia).toHaveAttribute("aria-expanded", "false") + await combobox.press("ArrowRight") + await expect(nvidia).toHaveAttribute("aria-expanded", "true") + await combobox.press("ArrowRight") + await expect(combobox).toHaveAttribute( + "aria-activedescendant", + await page.getByRole("treeitem", { name: "Nemotron" }).getAttribute("id"), + ) +}) + +test("active descendant always identifies a visible tree item", async ({ page }) => { + await load(page, "shared--model-selector-accessible") + + await page.getByRole("button", { name: "Review model: Alpha" }).click() + const combobox = page.getByRole("combobox", { name: "Review model: Alpha. Search models" }) + const active = async () => { + await expect.poll(() => combobox.getAttribute("aria-activedescendant")).toBeTruthy() + const id = await combobox.getAttribute("aria-activedescendant") + await expect(page.locator(`[id="${id}"]`)).toBeVisible() + } + + await active() + await combobox.press("ArrowDown") + await active() + await combobox.press("ArrowLeft") + await active() + await combobox.press("ArrowRight") + await active() + await combobox.fill("N") + await active() + await combobox.press("ArrowLeft") + await combobox.press("ArrowDown") + await combobox.press("ArrowLeft") + await active() + await combobox.fill("no matching model") + await active() +}) + test("expanded preview waits for explicit pointer selection", async ({ page }) => { await load(page, "shared--model-selector-accessible") await page.getByRole("button", { name: "Review model: Alpha" }).click() await page.getByRole("button", { name: "Expand" }).click() - await page.getByRole("option", { name: "Bravo" }).click() + await page.getByRole("treeitem", { name: "Bravo" }).click() await expect(page.getByTestId("model-selector-value")).toHaveText("alpha") await expect(page.getByRole("combobox", { name: "Review model: Alpha. Search models" })).toBeVisible() @@ -66,15 +136,15 @@ test("selected favorite remains selected when its duplicate group is collapsed", await page.getByRole("button", { name: "Review model: Alpha" }).click() const combobox = page.getByRole("combobox", { name: "Review model: Alpha. Search models" }) - const alpha = page.getByRole("option", { name: "Alpha" }) - const favorites = page.getByRole("button", { name: "Collapse Favorites" }) + const alpha = page.getByRole("treeitem", { name: "Alpha" }) + const favorites = page.getByRole("treeitem", { name: "Favorites" }) await expect(alpha.first()).toHaveAttribute("aria-selected", "true") await expect.poll(() => favorites.evaluate((el) => getComputedStyle(el).borderTopStyle)).toBe("solid") await favorites.click() await expect(alpha).toHaveCount(1) await expect(alpha).toHaveAttribute("aria-selected", "true") - await expect(combobox).toHaveAttribute("aria-activedescendant", await alpha.getAttribute("id")) + await expect(combobox).toHaveAttribute("aria-activedescendant", await favorites.getAttribute("id")) }) test("Enter selects the active option and Escape restores selector focus", async ({ page }) => { @@ -106,7 +176,7 @@ test("no-match search announces the empty result and can choose the default opti await combobox.fill("no matching model") await expect(page.locator(".model-selector-empty")).toHaveText("No model results") - const clear = page.getByRole("option", { name: "Use default model" }) + const clear = page.getByRole("treeitem", { name: "Use default model" }) await expect(combobox).toHaveAttribute("aria-activedescendant", await clear.getAttribute("id")) await combobox.press("Enter") @@ -156,3 +226,17 @@ test("chat picker Escape returns focus to the prompt", async ({ page }) => { await expect(page.locator("textarea.prompt-input")).toBeFocused() }) + +test("slash model picker Escape returns focus to the prompt", async ({ page }) => { + await load(page, "prompt-input--default-420") + + const prompt = page.locator("textarea.prompt-input") + await prompt.evaluate((el) => el.setAttribute("aria-disabled", "false")) + await prompt.fill("/model") + await prompt.press("Enter") + const combobox = page.getByRole("combobox", { name: /^Select model:.*Search models$/ }) + await expect(combobox).toBeFocused() + await combobox.press("Escape") + + await expect(prompt).toBeFocused() +}) diff --git a/packages/kilo-vscode/webview-ui/src/components/shared/ModelSelector.tsx b/packages/kilo-vscode/webview-ui/src/components/shared/ModelSelector.tsx index 2c53c04948f..df7ff77f087 100644 --- a/packages/kilo-vscode/webview-ui/src/components/shared/ModelSelector.tsx +++ b/packages/kilo-vscode/webview-ui/src/components/shared/ModelSelector.tsx @@ -61,6 +61,10 @@ function rowKey(kind: "model" | "favorite", providerID: string, modelID: string) return `${kind}:${providerID}/${modelID}` } +function groupKey(key: string) { + return `group:${key}` +} + // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- @@ -77,6 +81,13 @@ interface ModelGroup { rows: ModelRow[] } +interface ModelNode { + key: string + kind: "group" | "row" + group?: ModelGroup + row?: ModelRow +} + interface ScrollAnchor { key: string top: number | undefined @@ -129,7 +140,6 @@ export const ModelSelectorBase: Component = (props) => { const previewID = `${uid}-preview` const descriptionID = `${uid}-description` const optionID = (key: string) => `${uid}-option-${encodeURIComponent(key)}` - const groupID = (key: string) => `${uid}-group-${encodeURIComponent(key)}` const activeModel = () => { const items = props.models if (items) return items.find((m) => m.providerID === props.value?.providerID && m.id === props.value?.modelID) @@ -141,6 +151,8 @@ export const ModelSelectorBase: Component = (props) => { const [search, setSearch] = createSignal("") const [debouncedSearch, setDebouncedSearch] = createSignal("") const [selectedKey, setSelectedKey] = createSignal(CLEAR_KEY) + const [browsing, setBrowsing] = createSignal(false) + const [navigating, setNavigating] = createSignal(false) const [preActiveKey, setPreActiveKey] = createSignal(null) const [previewKey, setPreviewKey] = createSignal(null) const [previewHeight, setPreviewHeight] = createSignal(500) @@ -162,7 +174,7 @@ export const ModelSelectorBase: Component = (props) => { const [pointer, setPointer] = createSignal(true) // Ref map: row key → DOM element. Populated by each row's ref callback, // avoids DOM queries for scroll anchoring and scrollIntoView. - const refs = new Map() + const refs = new Map() function onSplitterMouseDown(e: MouseEvent) { e.preventDefault() @@ -313,12 +325,17 @@ export const ModelSelectorBase: Component = (props) => { const isGroupOpen = (key: string) => !collapsed().has(key) function toggleGroup(key: string) { + const target = groupKey(key) + setSelectedKey(target) + setBrowsing(true) + setNavigating(true) setCollapsed((prev) => { const next = new Set(prev) if (next.has(key)) next.delete(key) else next.add(key) return next }) + scrollSelectedIntoView() } const rows = createMemo(() => { @@ -328,11 +345,22 @@ export const ModelSelectorBase: Component = (props) => { return [{ key: CLEAR_KEY, kind: "clear" }, ...list] }) + const nodes = createMemo(() => { + const result: ModelNode[] = [] + if (props.allowClear) result.push({ key: CLEAR_KEY, kind: "row", row: { key: CLEAR_KEY, kind: "clear" } }) + for (const group of groups()) { + result.push({ key: groupKey(group.key), kind: "group", group }) + if (!isGroupOpen(group.key)) continue + result.push(...group.rows.map((row) => ({ key: row.key, kind: "row" as const, row, group }))) + } + return result + }) + const nodeMap = createMemo(() => new Map(nodes().map((node) => [node.key, node] as const))) + const nodeIndex = createMemo(() => new Map(nodes().map((node, i) => [node.key, i] as const))) const rowMap = createMemo(() => new Map(rows().map((row) => [row.key, row] as const))) - const rowIndex = createMemo(() => new Map(rows().map((row, i) => [row.key, i] as const))) const canonicalKey = (m: EnrichedModel) => rowKey("model", m.providerID, m.id) const favoriteKey = (m: EnrichedModel) => rowKey("favorite", m.providerID, m.id) - const defaultKey = () => rows()[0]?.key ?? CLEAR_KEY + const defaultKey = () => nodes()[0]?.key ?? CLEAR_KEY const activeKey = (m?: EnrichedModel | null) => { if (!m) return props.allowClear ? CLEAR_KEY : defaultKey() const key = modelKey(m.providerID, m.id) @@ -345,7 +373,7 @@ export const ModelSelectorBase: Component = (props) => { if (!row.model || !isActive(row.model)) return false return activeKey(row.model) === row.key } - const activeOptionID = () => (rowMap().has(selectedKey()) ? optionID(selectedKey()) : undefined) + const activeOptionID = () => (browsing() && nodeMap().has(selectedKey()) ? optionID(selectedKey()) : undefined) const [anchor, setAnchor] = createSignal(null) const previewModel = createMemo(() => rowMap().get(previewKey() ?? "")?.model ?? null) @@ -353,17 +381,15 @@ export const ModelSelectorBase: Component = (props) => { const isSelected = createSelector(selectedKey) const isPreActive = createSelector(preActiveKey) - // When the row list changes (filter, favorite toggle, provider connect), - // preserve the current selection if it still exists; otherwise fall back - // to the first row. This must NOT read activeModel() — doing so would - // cause a reactive loop where picking a model triggers a rows rebuild - // which resets selection. + // When the visible tree changes, preserve virtual focus only while its + // active descendant remains rendered. Collapsing a group moves focus to + // its heading before removing the child nodes. createEffect(() => { - rows() // track + nodes() // track setSelectedKey((prev) => { - if (rowMap().has(prev)) return prev + if (nodeMap().has(prev)) return prev const next = untrack(() => activeKey(activeModel())) - return rowMap().has(next) ? next : defaultKey() + return nodeMap().has(next) ? next : defaultKey() }) setPreActiveKey((prev) => (prev && rowMap().has(prev) ? prev : null)) setPreviewKey((prev) => (prev && rowMap().has(prev) ? prev : null)) @@ -393,13 +419,26 @@ export const ModelSelectorBase: Component = (props) => { // which would cause star/unstar to reset selection mid-interaction. // Falls back to defaultKey when the active model is filtered out. createEffect(() => { - filtered() // track - const active = activeModel() - const canon = active ? canonicalKey(active) : null - const next = canon && rowMap().has(canon) ? canon : props.allowClear ? CLEAR_KEY : defaultKey() - setSelectedKey(next) - setPreActiveKey(next) - setPreviewKey(next) + const list = filtered() + untrack(() => { + const active = activeModel() + const canon = active ? canonicalKey(active) : null + const match = list[0] + const first = match ? canonicalKey(match) : null + const next = + canon && rowMap().has(canon) + ? canon + : first && rowMap().has(first) + ? first + : props.allowClear + ? CLEAR_KEY + : defaultKey() + setSelectedKey(next) + setBrowsing(!!debouncedSearch() && nodeMap().has(next)) + setNavigating(false) + setPreActiveKey(next) + setPreviewKey(next) + }) }) createEffect(() => { @@ -411,7 +450,9 @@ export const ModelSelectorBase: Component = (props) => { // recompute with the snapshot before we try to resolve the key. queueMicrotask(() => { const next = activeKey(activeModel()) - setSelectedKey(next ?? CLEAR_KEY) + setSelectedKey(next ?? defaultKey()) + setBrowsing(true) + setNavigating(false) setPreActiveKey(next) setPreviewKey(next) requestAnimationFrame(() => { @@ -422,28 +463,28 @@ export const ModelSelectorBase: Component = (props) => { return } setOpenSnapshot(null) + setBrowsing(false) + setNavigating(false) setSearch("") setDebouncedSearch("") clearTimeout(previewTimer) }) - // Listen for slash command trigger + // Register before the popover mounts so programmatic slash-command opens + // always restore the prompt before the popover's own Escape handler runs. const onTrigger = () => setOpen(true) - window.addEventListener("openModelPicker", onTrigger) - onCleanup(() => { - window.removeEventListener("openModelPicker", onTrigger) - clearTimeout(previewTimer) - }) - const onEscape = (e: KeyboardEvent) => { if (!open() || e.key !== "Escape") return e.preventDefault() + e.stopImmediatePropagation() cancel() } - createEffect(() => { - if (!open()) return - window.addEventListener("keydown", onEscape, true) - onCleanup(() => window.removeEventListener("keydown", onEscape, true)) + window.addEventListener("openModelPicker", onTrigger) + window.addEventListener("keydown", onEscape, true) + onCleanup(() => { + window.removeEventListener("openModelPicker", onTrigger) + window.removeEventListener("keydown", onEscape, true) + clearTimeout(previewTimer) }) function pick(model: EnrichedModel) { @@ -481,15 +522,49 @@ export const ModelSelectorBase: Component = (props) => { if (key) refs.get(key)?.scrollIntoView({ block }) } + function activate(key: string) { + setSelectedKey(key) + setBrowsing(true) + setNavigating(true) + const row = nodeMap().get(key)?.row + setPreActiveKey(row?.model ? key : null) + schedulePreview(row?.model ? key : null) + scrollSelectedIntoView() + } + function move(step: number) { - const list = rows() + const list = nodes() if (list.length === 0) return - const idx = rowIndex().get(selectedKey()) ?? 0 + const idx = nodeIndex().get(selectedKey()) ?? (step > 0 ? -1 : list.length) const next = Math.max(0, Math.min(idx + step, list.length - 1)) - const key = list[next]?.key ?? CLEAR_KEY - setRow(key) - schedulePreview(key) - scrollSelectedIntoView() + const key = list[next]?.key + if (key) activate(key) + } + + function edge(index: number) { + const key = nodes()[index]?.key + if (key) activate(key) + } + + function horizontal(step: -1 | 1) { + const node = nodeMap().get(selectedKey()) + if (!node) return + if (node.kind === "group" && node.group) { + if (step === -1 && isGroupOpen(node.group.key)) { + toggleGroup(node.group.key) + return + } + if (step === 1 && !isGroupOpen(node.group.key)) { + toggleGroup(node.group.key) + return + } + if (step === 1) { + const key = node.group.rows[0]?.key + if (key) activate(key) + } + return + } + if (step === -1 && node.group) activate(groupKey(node.group.key)) } function selectRow(row: ModelRow) { @@ -524,7 +599,7 @@ export const ModelSelectorBase: Component = (props) => { } function handleKeyDown(e: KeyboardEvent) { - const list = rows() + const list = nodes() if (e.key === "Escape") { e.preventDefault() @@ -532,28 +607,38 @@ export const ModelSelectorBase: Component = (props) => { return } - if (list.length === 0) { + if (list.length === 0) return + + if (e.key === "ArrowDown" || e.key === "ArrowUp") { + e.preventDefault() + setPointer(false) + move(e.key === "ArrowDown" ? 1 : -1) return } - if (e.key === "ArrowDown") { + if (navigating() && (e.key === "ArrowLeft" || e.key === "ArrowRight")) { e.preventDefault() setPointer(false) - move(1) + horizontal(e.key === "ArrowLeft" ? -1 : 1) return } - if (e.key === "ArrowUp") { + if (navigating() && (e.key === "Home" || e.key === "End")) { e.preventDefault() setPointer(false) - move(-1) + edge(e.key === "Home" ? 0 : list.length - 1) return } if (isEnterKeyCommitNotIme(e)) { + const node = nodeMap().get(selectedKey()) + if (!node) return e.preventDefault() - const row = rowMap().get(selectedKey()) - if (row) selectRow(row) + if (node.kind === "group" && node.group) { + toggleGroup(node.group.key) + return + } + if (node.row) selectRow(node.row) } } @@ -673,13 +758,24 @@ export const ModelSelectorBase: Component = (props) => { aria-label={searchLabel()} aria-describedby={describedBy()} aria-autocomplete="list" - aria-haspopup="listbox" + aria-haspopup="tree" aria-expanded={open()} aria-controls={listID} aria-activedescendant={activeOptionID()} placeholder={language.t("dialog.model.search.placeholder")} value={search()} - onInput={(e) => setSearch(e.currentTarget.value)} + onInput={(e) => { + setBrowsing(false) + setNavigating(false) + setSearch(e.currentTarget.value) + }} + onMouseDown={(e) => { + const input = e.currentTarget + if (input.selectionStart !== input.selectionEnd || input.selectionStart !== input.value.length) { + setBrowsing(false) + setNavigating(false) + } + }} /> = (props) => { -
+
{language.t("dialog.model.empty")} @@ -719,8 +815,12 @@ export const ModelSelectorBase: Component = (props) => {
{ + refs.set(CLEAR_KEY, el) + onCleanup(() => refs.delete(CLEAR_KEY)) + }} class={`model-selector-item${isSelected(CLEAR_KEY) && !pointer() ? " keyboard-focused" : ""}${isSelected(CLEAR_KEY) ? " selected" : ""}${!props.value?.providerID ? " active" : ""}`} - role="option" + role="treeitem" aria-selected={!props.value?.providerID} onClick={() => pickClear()} onMouseMove={() => { @@ -740,17 +840,22 @@ export const ModelSelectorBase: Component = (props) => { {(group) => { const shown = () => isGroupOpen(group.key) return ( -
- +
- - {(row) => { - if (!row.model) return null - const model = row.model - const hovered = () => isSelected(row.key) - const preActive = () => isPreActive(row.key) - const starred = () => favoriteKeys().has(modelKey(model.providerID, model.id)) - const showProvider = () => row.kind === "favorite" - const showSelect = () => expanded() && preActive() && !isActive(model) - const starLabel = () => - `${starred() ? language.t("model.favorite.remove") : language.t("model.favorite.add")}: ${sanitizeName(model.name)}` - return ( - + + + + + + +
+ ) + }} + +
) diff --git a/packages/kilo-vscode/webview-ui/src/stories/shared.stories.tsx b/packages/kilo-vscode/webview-ui/src/stories/shared.stories.tsx index 7474b0809a4..2efefc401b2 100644 --- a/packages/kilo-vscode/webview-ui/src/stories/shared.stories.tsx +++ b/packages/kilo-vscode/webview-ui/src/stories/shared.stories.tsx @@ -41,6 +41,10 @@ const ACCESSIBLE_MODELS: EnrichedModel[] = [ { id: "alpha", name: "Alpha", providerID: "kilo", providerName: "Kilo" }, { id: "bravo", name: "Bravo", providerID: "kilo", providerName: "Kilo" }, { id: "charlie", name: "Charlie", providerID: "kilo", providerName: "Kilo" }, + { id: "delta", name: "Delta", providerID: "kilo", providerName: "Kilo" }, + { id: "echo", name: "Echo", providerID: "kilo", providerName: "Kilo" }, + { id: "nova", name: "Nova", providerID: "nvidia", providerName: "NVIDIA" }, + { id: "nemotron", name: "Nemotron", providerID: "nvidia", providerName: "NVIDIA" }, ] const AccessibleModelSelector = () => { diff --git a/packages/kilo-vscode/webview-ui/src/styles/model-selector.css b/packages/kilo-vscode/webview-ui/src/styles/model-selector.css index daaab91a4a7..7140c77dd83 100644 --- a/packages/kilo-vscode/webview-ui/src/styles/model-selector.css +++ b/packages/kilo-vscode/webview-ui/src/styles/model-selector.css @@ -325,11 +325,13 @@ font-family: inherit; } -.model-selector-group-label:hover { +.model-selector-group-label:hover, +.model-selector-group-label.selected { color: var(--text-base, var(--vscode-foreground)); + background: var(--surface-interactive-hover, var(--vscode-list-hoverBackground)); } -.model-selector-group-label:focus-visible { +.model-selector-group-label.keyboard-focused { outline: 1px solid var(--vscode-focusBorder); outline-offset: -1px; }