constrain and make chat questions scrollable#298786
Conversation
meganrogge
requested review from
justschen and
roblourens
and removed request for
Copilot
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Constrain the chat question carousel so long questions/option lists don’t expand beyond the chat widget, and make the question input area internally scrollable (fixes #298287).
Changes:
- Prevent forwarding mouse-wheel events to the chat list when the wheel event originates inside the chat widget container.
- Add CSS constraints (max-height + flex adjustments) to cap the question carousel height and enable internal scrolling layout.
- Wrap the question input container in a
DomScrollableElementand dynamically size its viewport to the available space.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts | Refines wheel event forwarding to avoid interfering with normal in-widget scrolling. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/media/chatQuestionCarousel.css | Adds max-height constraints and flex rules needed for a scrollable question UI. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts | Implements a scrollable wrapper around question inputs and calculates available scroll height. |
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.ts
Outdated
Show resolved
Hide resolved
| border: 1px solid var(--vscode-input-border, transparent); | ||
| background-color: var(--vscode-editor-background); | ||
| border-radius: 4px; | ||
| max-height: min(420px, 45vh); |
There was a problem hiding this comment.
How does this fix it? Isn't vh relative to the window?
There was a problem hiding this comment.
It is. This is only an upper bound for the carousel size (so it doesn’t grow too tall). The actual scroll viewport calculation no longer uses window.innerHeight; it uses this._questionContainer.clientHeight (the resolved layout size in context), so behavior tracks the real available space in the chat UI.
| // Constrain the content element (DomScrollableElement._element) so that | ||
| // scanDomNode sees clientHeight < scrollHeight and enables scrolling. | ||
| // The wrapper inherits the same constraint via CSS flex. | ||
| scrollableContent.style.height = `${constrainedScrollableHeight}px`; |
There was a problem hiding this comment.
I still don't really understand why this is necessary. Seems like scanDomNode is meant to measure things for you.
There was a problem hiding this comment.
scanDomNode is measure-only; we still need the measured element (inputContainer) to be height-bounded. Otherwise it can auto-grow with content and not report overflow. Setting scrollableContent.style.height is one way to enforce that bound.
fixes #298287
Makes the chat question carousel scrollable — wraps the input area in a
DomScrollableElementwith a max-height so long option lists scroll instead of pushing the chat UI off-screen. A ResizeObserver keeps the scroll layout in sync.question-scroll.mov