Revert "sessions: hide disabled chat input pickers (#307494)"#308173
Revert "sessions: hide disabled chat input pickers (#307494)"#308173
Conversation
There was a problem hiding this comment.
Pull request overview
Reverts prior sessions UI behavior that hid certain chat input “picker” controls when they were disabled, restoring visibility (with disabled styling/behavior) and removing the CSS that suppressed shared workbench pickers in the sessions app.
Changes:
- Switch multiple picker widgets from
dom.setVisibility(...)/aria-hiddenbehavior to adisabledstate via CSS class toggling. - Remove sessions CSS that hid
.chat-sessionPicker-containerin the sessions workbench. - Remove/adjust menu
whengating and delete picker-specific browser tests that no longer match the updated behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/copilotChatSessions/test/browser/branchPicker.test.ts | Removes BranchPicker tests (previous expectations no longer apply). |
| src/vs/sessions/contrib/copilotChatSessions/browser/modePicker.ts | Stops hiding mode picker; uses disabled styling instead. |
| src/vs/sessions/contrib/copilotChatSessions/browser/modelPicker.ts | Stops hiding model picker; uses disabled styling instead. |
| src/vs/sessions/contrib/copilotChatSessions/browser/isolationPicker.ts | Stops hiding isolation picker; uses disabled styling instead. |
| src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsActions.ts | Removes IsNewChatSessionContext gating for some new-session menus. |
| src/vs/sessions/contrib/copilotChatSessions/browser/branchPicker.ts | Disables branch picker in more cases instead of hiding it. |
| src/vs/sessions/contrib/chat/test/browser/sessionTypePicker.test.ts | Removes SessionTypePicker tests (previous hide/show expectations no longer apply). |
| src/vs/sessions/contrib/chat/browser/sessionTypePicker.ts | Stops hiding session type picker; uses disabled styling instead. |
| src/vs/sessions/browser/media/style.css | Removes CSS rule that hid shared chat session pickers in sessions workbench. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/sessions/contrib/chat/browser/sessionTypePicker.ts:134
- The PR changes picker behavior (e.g. SessionTypePicker no longer uses
dom.setVisibilityand relies on adisabledstate instead), but the corresponding browser tests for these pickers were removed rather than updated. Please add/adjust unit tests to cover the new disabled/visible behavior and the associated accessibility attributes (aria-disabled,tabIndex) to prevent regressions.
private _updateTriggerLabel(): void {
if (!this._triggerElement) {
return;
}
dom.clearNode(this._triggerElement);
const currentType = this._sessionTypes.find(t => t.id === this._sessionType);
const modeIcon = currentType?.icon ?? Codicon.terminal;
const modeLabel = currentType?.label ?? this._sessionType ?? '';
dom.append(this._triggerElement, renderIcon(modeIcon));
const labelSpan = dom.append(this._triggerElement, dom.$('span.sessions-chat-dropdown-label'));
labelSpan.textContent = modeLabel;
const hasMultipleTypes = this._sessionTypes.length > 1;
this._slotElement?.classList.toggle('disabled', !hasMultipleTypes);
dom.append(this._triggerElement, renderIcon(Codicon.chevronDown));
}
- Files reviewed: 9/9 changed files
- Comments generated: 4
| this._slotElement.classList.toggle('disabled', false); | ||
| this._triggerElement.setAttribute('aria-hidden', String(!visible)); | ||
| this._triggerElement.tabIndex = visible ? 0 : -1; | ||
| this._slotElement?.classList.toggle('disabled', modes.length <= 1); |
There was a problem hiding this comment.
_updateTriggerLabel marks the picker as disabled when modes.length <= 1, but the trigger remains focusable (tabIndex stays 0) and _showPicker() doesn’t check this disabled condition. Because .disabled only sets pointer-events: none, keyboard users can still open the picker (and it will open) despite the UI indicating it’s disabled. Consider setting aria-disabled + tabIndex=-1 when disabled and/or early-returning in _showPicker() when there’s no selectable mode choice (to keep behavior consistent across mouse/keyboard and for accessibility).
| this._slotElement?.classList.toggle('disabled', modes.length <= 1); | |
| const isDisabled = modes.length <= 1; | |
| this._slotElement?.classList.toggle('disabled', isDisabled); | |
| this._triggerElement.setAttribute('aria-disabled', isDisabled ? 'true' : 'false'); | |
| this._triggerElement.tabIndex = isDisabled ? -1 : 0; |
| this._slotElement?.classList.toggle('disabled', this._models.length === 0); | ||
| this._triggerElement.setAttribute('aria-disabled', String(this._models.length === 0)); |
There was a problem hiding this comment.
When there are no models, the slot gets the disabled class and aria-disabled is set, but the trigger remains in the tab order (tabIndex is never updated). Since the disabled styling uses pointer-events: none, the element can still receive focus via keyboard, which is inconsistent with a disabled control. Consider setting tabIndex=-1 when this._models.length === 0 (and restoring to 0 when models exist).
| this._slotElement?.classList.toggle('disabled', this._models.length === 0); | |
| this._triggerElement.setAttribute('aria-disabled', String(this._models.length === 0)); | |
| const isDisabled = this._models.length === 0; | |
| this._slotElement?.classList.toggle('disabled', isDisabled); | |
| this._triggerElement.setAttribute('aria-disabled', String(isDisabled)); | |
| this._triggerElement.tabIndex = isDisabled ? -1 : 0; |
| this._slotElement.classList.toggle('disabled', false); | ||
| this._triggerElement.setAttribute('aria-hidden', String(!hasMultipleTypes)); | ||
| this._triggerElement.tabIndex = hasMultipleTypes ? 0 : -1; | ||
| this._slotElement?.classList.toggle('disabled', !hasMultipleTypes); |
There was a problem hiding this comment.
_updateTriggerLabel now only toggles the disabled class when there’s a single session type, but it no longer sets aria-disabled or removes the trigger from the tab order. This makes the control appear disabled while still being keyboard-focusable. Consider updating aria-disabled and tabIndex alongside the disabled class to match the behavior in other pickers (e.g. BranchPicker/IsolationPicker).
This issue also appears on line 116 of the same file.
| this._slotElement?.classList.toggle('disabled', !hasMultipleTypes); | |
| this._slotElement?.classList.toggle('disabled', !hasMultipleTypes); | |
| this._triggerElement.setAttribute('aria-disabled', String(!hasMultipleTypes)); | |
| this._triggerElement.tabIndex = hasMultipleTypes ? 0 : -1; |
| @@ -187,11 +187,9 @@ export class IsolationPicker extends Disposable { | |||
| labelSpan.textContent = modeLabel; | |||
| dom.append(this._triggerElement, renderIcon(Codicon.chevronDown)); | |||
|
|
|||
| const visible = this._isolationOptionEnabled && this._hasGitRepo; | |||
| dom.setVisibility(visible, this._slotElement); | |||
| this._slotElement.classList.toggle('disabled', false); | |||
| this._triggerElement.setAttribute('aria-hidden', String(!visible)); | |||
| this._triggerElement.setAttribute('aria-disabled', String(!visible)); | |||
| this._triggerElement.tabIndex = visible ? 0 : -1; | |||
| const isDisabled = !this._hasGitRepo; | |||
| this._slotElement?.classList.toggle('disabled', isDisabled); | |||
| this._triggerElement.setAttribute('aria-disabled', String(isDisabled)); | |||
| this._triggerElement.tabIndex = isDisabled ? -1 : 0; | |||
| } | |||
There was a problem hiding this comment.
The picker’s interaction is gated on both _hasGitRepo and _isolationOptionEnabled in _showPicker(), but _updateTriggerLabel() only disables the UI when _hasGitRepo is false. If the isolation option gets disabled via configuration while the widget is rendered, the control can appear enabled yet do nothing. Consider including !this._isolationOptionEnabled in the disabled state (class, aria-disabled, and tabIndex) to keep the UI consistent with behavior.
This reverts commit c2f1a6e.