From 41f38a1c1a19853167c7bb8370410b2bb266ef23 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Thu, 3 Sep 2026 12:38:30 -0400 Subject: [PATCH 1/3] Fix duplicate host picker tab stop Delegate toolbar focus to the nested sidebar button so the inert action-item wrapper is not exposed as a separate tab stop. Add regression coverage for focus and activation.\n\nFixes #333576\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browser/hostFilterActionViewItem.ts | 30 ++++++ .../browser/hostFilterActionViewItem.test.ts | 94 +++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts index 6b0138d6321ab..6a58958b06fcd 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts @@ -85,6 +85,36 @@ export class HostFilterActionViewItem extends BaseActionViewItem { this._update(); } + override focus(): void { + if (this._sidebarButton) { + this._sidebarButton.element.tabIndex = 0; + this._sidebarButton.focus(); + } else { + super.focus(); + } + } + + override isFocused(): boolean { + return this._sidebarButton?.hasFocus() ?? super.isFocused(); + } + + override blur(): void { + if (this._sidebarButton) { + this._sidebarButton.element.tabIndex = -1; + this._sidebarButton.element.blur(); + } else { + super.blur(); + } + } + + override setFocusable(focusable: boolean): void { + if (this._sidebarButton) { + this._sidebarButton.element.tabIndex = focusable ? 0 : -1; + } else { + super.setFocusable(focusable); + } + } + /** * Original compact pill rendered in the desktop titlebar's left toolbar. * Custom DOM driven directly by click handlers + context menu service. diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts new file mode 100644 index 0000000000000..9cf71807be4f9 --- /dev/null +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts @@ -0,0 +1,94 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { IContextMenuDelegate } from '../../../../../../base/browser/contextmenu.js'; +import { Action } from '../../../../../../base/common/actions.js'; +import { Codicon } from '../../../../../../base/common/codicons.js'; +import { Event } from '../../../../../../base/common/event.js'; +import { DisposableStore, toDisposable } from '../../../../../../base/common/lifecycle.js'; +import { mock } from '../../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; +import { IContextMenuMenuDelegate, IContextMenuService } from '../../../../../../platform/contextview/browser/contextView.js'; +import { NullHoverService } from '../../../../../../platform/hover/test/browser/nullHoverService.js'; +import { AgentHostFilterConnectionStatus, IAgentHostFilterEntry, IAgentHostFilterService } from '../../../../../services/agentHostFilter/common/agentHostFilter.js'; +import { HostFilterActionViewItem } from '../../browser/hostFilterActionViewItem.js'; + +suite('HostFilterActionViewItem', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + + test('sidebar picker delegates the toolbar tab stop to its button', () => { + const testDisposables = disposables.add(new DisposableStore()); + const container = document.createElement('li'); + document.body.appendChild(container); + testDisposables.add(toDisposable(() => container.remove())); + + const interactiveHost: IAgentHostFilterEntry = { + id: 'interactive', + providerIds: ['interactive'], + label: 'Interactive', + grouped: false, + address: undefined, + icon: Codicon.remote, + status: AgentHostFilterConnectionStatus.Connected, + connectable: false, + }; + const otherHost: IAgentHostFilterEntry = { + ...interactiveHost, + id: 'other', + providerIds: ['other'], + label: 'Other', + }; + const filterService = new class extends mock() { + override readonly onDidChange = Event.None; + override readonly onDidChangeDiscovering = Event.None; + override readonly selectedHostId = interactiveHost.id; + override readonly selectedHost = interactiveHost; + override readonly hosts = [interactiveHost, otherHost]; + override readonly isDiscovering = false; + }(); + let menuShowCount = 0; + const contextMenuService = new class extends mock() { + override showContextMenu(delegate: IContextMenuDelegate | IContextMenuMenuDelegate): void { + menuShowCount++; + for (const action of delegate.getActions?.() ?? []) { + if (action instanceof Action) { + testDisposables.add(action); + } + } + } + }(); + const viewItem = testDisposables.add(new HostFilterActionViewItem( + testDisposables.add(new Action('pickHost', 'Select Agent Host')), + 'sidebar', + filterService, + contextMenuService, + NullHoverService, + )); + + viewItem.render(container); + viewItem.setFocusable(true); + viewItem.focus(); + + const button = container.querySelector('.agent-host-filter-button'); + button?.click(); + + assert.deepStrictEqual({ + wrapperTabIndex: container.tabIndex, + buttonTabIndex: button?.tabIndex, + pickerTabbableDescendants: container.querySelectorAll('.customization-link-button-container [tabindex="0"]').length, + buttonFocused: document.activeElement === button, + viewItemFocused: viewItem.isFocused(), + menuShowCount, + }, { + wrapperTabIndex: -1, + buttonTabIndex: 0, + pickerTabbableDescendants: 1, + buttonFocused: true, + viewItemFocused: true, + menuShowCount: 1, + }); + }); +}); From 9ab5bac6afedb04edd22b8fc2fad656812fe12a0 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Thu, 3 Sep 2026 12:54:48 -0400 Subject: [PATCH 2/3] Fix duplicate agent mode picker tab stop Keep focus on the Interactive picker trigger instead of exposing its inert toolbar wrapper as a second tab stop. Add regression coverage for the wrapper and trigger focus state.\n\nFixes #333576\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browser/agentHostSessionConfigPicker.ts | 18 ++++ .../agentHostSessionConfigPicker.test.ts | 23 ++++- .../browser/hostFilterActionViewItem.ts | 30 ------ .../browser/hostFilterActionViewItem.test.ts | 94 ------------------- 4 files changed, 39 insertions(+), 126 deletions(-) delete mode 100644 src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts diff --git a/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts b/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts index 194dd3ab455e3..82f7259d7310a 100644 --- a/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts +++ b/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts @@ -1233,6 +1233,7 @@ interface IConfigPickerWidget extends IDisposable { export class PickerActionViewItem extends BaseActionViewItem implements IChatInputPickerResponsiveState { private _compact = false; + private _focusableElement: HTMLElement | undefined; constructor(private readonly _picker: IConfigPickerWidget, disposable?: IDisposable) { super(undefined, { id: '', label: '', enabled: true, class: undefined, tooltip: '', run: () => { } }); @@ -1244,9 +1245,26 @@ export class PickerActionViewItem extends BaseActionViewItem implements IChatInp override render(container: HTMLElement): void { this.element = container; this._picker.render(container); + this._focusableElement = container.querySelector('.action-label') ?? undefined; container.classList.toggle('compact-picker', this._compact); } + override focus(): void { + this._focusableElement?.focus(); + } + + override isFocused(): boolean { + return this._focusableElement === dom.getActiveElement(); + } + + override blur(): void { + this._focusableElement?.blur(); + } + + override setFocusable(_focusable: boolean): void { + this.element?.removeAttribute('tabindex'); + } + isCompact(): boolean { return this._compact; } diff --git a/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts b/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts index 0384d32808dfc..c1419f2a7b5d9 100644 --- a/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts +++ b/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts @@ -354,7 +354,12 @@ suite('Agent Host Session Config Picker', () => { test('picker action view items expose responsive compact state', () => { let pickerAnchor: HTMLElement | undefined; const item = store.add(new PickerActionViewItem({ - render: () => { }, + render: container => { + const trigger = document.createElement('a'); + trigger.classList.add('action-label'); + trigger.tabIndex = 0; + container.appendChild(trigger); + }, showPicker: anchor => { pickerAnchor = anchor; return true; @@ -362,6 +367,8 @@ suite('Agent Host Session Config Picker', () => { dispose: () => { }, })); const container = document.createElement('div'); + document.body.appendChild(container); + store.add(toDisposable(() => container.remove())); const overflowAnchor = document.createElement('button'); item.render(container); const expanded = { @@ -370,16 +377,28 @@ suite('Agent Host Session Config Picker', () => { }; item.setCompact(true); + item.setFocusable(true); + item.focus(); item.show(overflowAnchor); const compact = { compact: item.isCompact(), className: container.classList.contains('compact-picker'), usesOverflowAnchor: pickerAnchor === overflowAnchor, + wrapperTabIndex: container.tabIndex, + tabbableDescendants: container.querySelectorAll('[tabindex="0"]').length, + triggerFocused: item.isFocused(), }; assert.deepStrictEqual({ expanded, compact }, { expanded: { compact: false, className: false }, - compact: { compact: true, className: true, usesOverflowAnchor: true }, + compact: { + compact: true, + className: true, + usesOverflowAnchor: true, + wrapperTabIndex: -1, + tabbableDescendants: 1, + triggerFocused: true, + }, }); }); diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts index 6a58958b06fcd..6b0138d6321ab 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/hostFilterActionViewItem.ts @@ -85,36 +85,6 @@ export class HostFilterActionViewItem extends BaseActionViewItem { this._update(); } - override focus(): void { - if (this._sidebarButton) { - this._sidebarButton.element.tabIndex = 0; - this._sidebarButton.focus(); - } else { - super.focus(); - } - } - - override isFocused(): boolean { - return this._sidebarButton?.hasFocus() ?? super.isFocused(); - } - - override blur(): void { - if (this._sidebarButton) { - this._sidebarButton.element.tabIndex = -1; - this._sidebarButton.element.blur(); - } else { - super.blur(); - } - } - - override setFocusable(focusable: boolean): void { - if (this._sidebarButton) { - this._sidebarButton.element.tabIndex = focusable ? 0 : -1; - } else { - super.setFocusable(focusable); - } - } - /** * Original compact pill rendered in the desktop titlebar's left toolbar. * Custom DOM driven directly by click handlers + context menu service. diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts deleted file mode 100644 index 9cf71807be4f9..0000000000000 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/hostFilterActionViewItem.test.ts +++ /dev/null @@ -1,94 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import assert from 'assert'; -import { IContextMenuDelegate } from '../../../../../../base/browser/contextmenu.js'; -import { Action } from '../../../../../../base/common/actions.js'; -import { Codicon } from '../../../../../../base/common/codicons.js'; -import { Event } from '../../../../../../base/common/event.js'; -import { DisposableStore, toDisposable } from '../../../../../../base/common/lifecycle.js'; -import { mock } from '../../../../../../base/test/common/mock.js'; -import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; -import { IContextMenuMenuDelegate, IContextMenuService } from '../../../../../../platform/contextview/browser/contextView.js'; -import { NullHoverService } from '../../../../../../platform/hover/test/browser/nullHoverService.js'; -import { AgentHostFilterConnectionStatus, IAgentHostFilterEntry, IAgentHostFilterService } from '../../../../../services/agentHostFilter/common/agentHostFilter.js'; -import { HostFilterActionViewItem } from '../../browser/hostFilterActionViewItem.js'; - -suite('HostFilterActionViewItem', () => { - const disposables = ensureNoDisposablesAreLeakedInTestSuite(); - - test('sidebar picker delegates the toolbar tab stop to its button', () => { - const testDisposables = disposables.add(new DisposableStore()); - const container = document.createElement('li'); - document.body.appendChild(container); - testDisposables.add(toDisposable(() => container.remove())); - - const interactiveHost: IAgentHostFilterEntry = { - id: 'interactive', - providerIds: ['interactive'], - label: 'Interactive', - grouped: false, - address: undefined, - icon: Codicon.remote, - status: AgentHostFilterConnectionStatus.Connected, - connectable: false, - }; - const otherHost: IAgentHostFilterEntry = { - ...interactiveHost, - id: 'other', - providerIds: ['other'], - label: 'Other', - }; - const filterService = new class extends mock() { - override readonly onDidChange = Event.None; - override readonly onDidChangeDiscovering = Event.None; - override readonly selectedHostId = interactiveHost.id; - override readonly selectedHost = interactiveHost; - override readonly hosts = [interactiveHost, otherHost]; - override readonly isDiscovering = false; - }(); - let menuShowCount = 0; - const contextMenuService = new class extends mock() { - override showContextMenu(delegate: IContextMenuDelegate | IContextMenuMenuDelegate): void { - menuShowCount++; - for (const action of delegate.getActions?.() ?? []) { - if (action instanceof Action) { - testDisposables.add(action); - } - } - } - }(); - const viewItem = testDisposables.add(new HostFilterActionViewItem( - testDisposables.add(new Action('pickHost', 'Select Agent Host')), - 'sidebar', - filterService, - contextMenuService, - NullHoverService, - )); - - viewItem.render(container); - viewItem.setFocusable(true); - viewItem.focus(); - - const button = container.querySelector('.agent-host-filter-button'); - button?.click(); - - assert.deepStrictEqual({ - wrapperTabIndex: container.tabIndex, - buttonTabIndex: button?.tabIndex, - pickerTabbableDescendants: container.querySelectorAll('.customization-link-button-container [tabindex="0"]').length, - buttonFocused: document.activeElement === button, - viewItemFocused: viewItem.isFocused(), - menuShowCount, - }, { - wrapperTabIndex: -1, - buttonTabIndex: 0, - pickerTabbableDescendants: 1, - buttonFocused: true, - viewItemFocused: true, - menuShowCount: 1, - }); - }); -}); From 1428be834a2e345797339f8e2088ced31276a8e6 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Thu, 3 Sep 2026 13:32:11 -0400 Subject: [PATCH 3/3] Avoid querying for the mode picker trigger Have enum pickers return their trigger directly so the action view item can manage focus without a fragile selector. Preserve the existing wrapper fallback for composite pickers.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../agentHost/browser/agentHostModePicker.ts | 3 +- .../browser/agentHostSessionConfigPicker.ts | 29 ++++++++++++++----- .../agentHostSessionConfigPicker.test.ts | 1 + 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/vs/sessions/contrib/providers/agentHost/browser/agentHostModePicker.ts b/src/vs/sessions/contrib/providers/agentHost/browser/agentHostModePicker.ts index 8a2b09fd85f3a..139d4f861d2eb 100644 --- a/src/vs/sessions/contrib/providers/agentHost/browser/agentHostModePicker.ts +++ b/src/vs/sessions/contrib/providers/agentHost/browser/agentHostModePicker.ts @@ -74,7 +74,7 @@ export abstract class AgentHostSessionEnumPicker extends Disposable { this._watchProviders(this._sessionsProvidersService.getProviders()); } - render(container: HTMLElement): void { + render(container: HTMLElement): HTMLElement { this._renderDisposables.clear(); this._containerElement = container; @@ -104,6 +104,7 @@ export abstract class AgentHostSessionEnumPicker extends Disposable { })); this._updateTrigger(); + return trigger; } private _watchProviders(providers: readonly ISessionsProvider[]): void { diff --git a/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts b/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts index 82f7259d7310a..bee227e2999fe 100644 --- a/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts +++ b/src/vs/sessions/contrib/providers/agentHost/browser/agentHostSessionConfigPicker.ts @@ -1227,7 +1227,7 @@ class MobileAgentHostSessionConfigPicker extends AgentHostSessionConfigPicker { } interface IConfigPickerWidget extends IDisposable { - render(container: HTMLElement): void; + render(container: HTMLElement): HTMLElement | void; showPicker?(anchor: HTMLElement, onHide?: () => void): boolean | void; } @@ -1244,25 +1244,38 @@ export class PickerActionViewItem extends BaseActionViewItem implements IChatInp override render(container: HTMLElement): void { this.element = container; - this._picker.render(container); - this._focusableElement = container.querySelector('.action-label') ?? undefined; + this._focusableElement = this._picker.render(container) ?? undefined; container.classList.toggle('compact-picker', this._compact); } override focus(): void { - this._focusableElement?.focus(); + if (this._focusableElement) { + this._focusableElement.focus(); + } else { + super.focus(); + } } override isFocused(): boolean { - return this._focusableElement === dom.getActiveElement(); + return this._focusableElement + ? this._focusableElement === dom.getActiveElement() + : super.isFocused(); } override blur(): void { - this._focusableElement?.blur(); + if (this._focusableElement) { + this._focusableElement.blur(); + } else { + super.blur(); + } } - override setFocusable(_focusable: boolean): void { - this.element?.removeAttribute('tabindex'); + override setFocusable(focusable: boolean): void { + if (this._focusableElement) { + this.element?.removeAttribute('tabindex'); + } else { + super.setFocusable(focusable); + } } isCompact(): boolean { diff --git a/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts b/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts index c1419f2a7b5d9..5037fadd3b1b2 100644 --- a/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts +++ b/src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostSessionConfigPicker.test.ts @@ -359,6 +359,7 @@ suite('Agent Host Session Config Picker', () => { trigger.classList.add('action-label'); trigger.tabIndex = 0; container.appendChild(trigger); + return trigger; }, showPicker: anchor => { pickerAnchor = anchor;