-
Notifications
You must be signed in to change notification settings - Fork 37.7k
fix: switch to list-style selection for single and multi-select questions #291351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates the chat question carousel UI to use a list-style selection experience (QuickPick-like) for both single- and multi-select questions, instead of native radio buttons/checkboxes.
Changes:
- Reworked single-select rendering to a
role="listbox"list with selectable rows and a checkmark indicator. - Reworked multi-select rendering to list rows with VS Code’s
Checkboxcomponent and listbox semantics. - Updated CSS and adjusted unit tests to validate the new DOM structure and selection state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts | Implements list-style single/multi-select rendering, keyboard handling, and answer extraction. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/media/chatQuestionCarousel.css | Adds styling for list-style selection rows (QuickPick-like look/feel). |
| src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatQuestionCarouselPart.test.ts | Updates tests to assert list-based DOM structure and selection state classes. |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts:681
- Pressing Space in multi-select toggles
checkboxes[focusedIndex].checkedprogrammatically, but this won’t emitcheckbox.onChange, so the corresponding list item’scheckedclass andaria-selectedcan get out of sync. Use the same toggle path as mouse/keyboard interaction (e.g., invoke the checkbox click/toggle method or update list item state alongside the assignment).
} else if (event.keyCode === KeyCode.Space) {
e.preventDefault();
// Toggle the currently focused checkbox
if (focusedIndex >= 0 && focusedIndex < checkboxes.length) {
checkboxes[focusedIndex].checked = !checkboxes[focusedIndex].checked;
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatQuestionCarouselPart.test.ts:140
- The updated UI adds row-level click handling and keyboard navigation for list-style selection, but the test suite only asserts initial DOM structure/classes. Adding interaction tests (clicking on the list item outside the checkbox, arrow navigation + Space/Enter behavior) would help prevent regressions and would have caught state-sync issues.
test('renders list items with checkboxes for multiSelect type questions', () => {
const carousel = createMockCarousel([
{
id: 'q1',
type: 'multiSelect',
title: 'Choose multiple',
options: [
{ id: 'a', label: 'Option A', value: 'a' },
{ id: 'b', label: 'Option B', value: 'b' },
{ id: 'c', label: 'Option C', value: 'c' }
]
}
]);
createWidget(carousel);
const listItems = widget.domNode.querySelectorAll('.chat-question-list-item.multi-select');
assert.strictEqual(listItems.length, 3, 'Should have 3 list items for multiSelect');
const checkboxes = widget.domNode.querySelectorAll('.chat-question-list-checkbox');
assert.strictEqual(checkboxes.length, 3, 'Should have 3 checkboxes');
});
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts:683
- The keyboard handler toggles
checkboxes[focusedIndex].checkedvia the setter, which won’t fireonChangeeither, so visual state/ARIA can get out of sync when using Space. Consider toggling through the Checkbox’s UI event path (e.g. click its domNode) or manually mirroring the state update here as well.
freeformTextarea.placeholder = localize('chat.questionCarousel.enterCustomAnswer', 'Enter custom answer');
freeformTextarea.rows = 1;
freeformTextarea.setAttribute('aria-labelledby', freeformLabelId);
if (previousFreeform !== undefined) {
freeformTextarea.value = previousFreeform;
}
...workbench/contrib/chat/test/browser/widget/chatContentParts/chatQuestionCarouselPart.test.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Show resolved
Hide resolved
b88182c to
9bc1596
Compare
|
Latest UI: Screen.Recording.2026-01-29.at.4.10.11.PM.mov |
Screen.Recording.2026-01-30.at.1.39.07.PM.movUpdated UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /* Secondary buttons (prev, next) use gray secondary background */ | ||
| .chat-question-carousel-nav .monaco-button.chat-question-nav-arrow.chat-question-nav-prev, | ||
| .chat-question-carousel-nav .monaco-button.chat-question-nav-arrow.chat-question-nav-next { | ||
| background: var(--vscode-button-secondaryBackground) !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some changes from Lee that got merged as a candidate regarding the secondaryBackground not working as intended - just an extra UI nit to keep an eye out for when testing.
| margin-left: 4px; | ||
| .chat-question-carousel-nav .monaco-button.chat-question-nav-arrow.chat-question-nav-prev:hover:not(.disabled), | ||
| .chat-question-carousel-nav .monaco-button.chat-question-nav-arrow.chat-question-nav-next:hover:not(.disabled) { | ||
| background: var(--vscode-button-secondaryHoverBackground) !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also nit or maybe cleanup - i'm not a super big fan of doing !important, and ik with the monaco buttons it's kinda annoying, but with enough specficity (ie, wrapping in .interactive-session .interactive-response etc, we can maybe avoid.
not blocking tho.
| label.htmlFor = `option-${question.id}-${index}`; | ||
| label.id = `label-${question.id}-${index}`; | ||
| label.textContent = option.label; | ||
| if (isChecked) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe can look into using toggle for things like this
also not blocking

Fixes #291328
Fixes #291708