Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @joaomorenoMatched files:
@bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces animated show/hide transitions for core workbench parts (sidebar/panel/aux bar) and quick input, backed by a new motion utility module and a new animated visibility API on the grid/splitview layout primitives.
Changes:
- Add
setViewVisibleAnimated(...)toSplitView/GridView/Gridand use it from workbench layout when toggling sidebar/panel/aux bar visibility. - Add quick input entrance/exit animations (fade + translate) with reduced-motion support.
- Introduce a new base motion utility module (durations, easing, cubic-bezier helpers) and add motion-related CSS.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/browser/style.ts | Imports new workbench motion stylesheet. |
| src/vs/workbench/browser/media/motion.css | Adds workbench-scoped motion CSS custom properties for enabled/reduced motion. |
| src/vs/workbench/browser/layout.ts | Switches sidebar/panel/aux bar visibility toggles to animated grid visibility changes with deferred CSS class updates. |
| src/vs/platform/quickinput/browser/quickInputController.ts | Adds quick input open/close animations and reduced-motion checks. |
| src/vs/base/browser/ui/splitview/splitview.ts | Adds requestAnimationFrame-driven setViewVisibleAnimated and animation cancellation behavior. |
| src/vs/base/browser/ui/motion/motion.ts | Adds shared motion constants and cubic-bezier parsing/solving helpers + reduced-motion detection helper. |
| src/vs/base/browser/ui/motion/motion.css | Adds (currently unused) CSS class for animation overflow clipping. |
| src/vs/base/browser/ui/grid/gridview.ts | Adds animated child visibility plumbing from grid view into splitview. |
| src/vs/base/browser/ui/grid/grid.ts | Exposes setViewVisibleAnimated on the public Grid wrapper API. |
Comments suppressed due to low confidence (3)
src/vs/workbench/browser/layout.ts:2045
setPanelHidden(true)defers addingLayoutClasses.PANEL_HIDDEN(and hiding the active panel composite) to theonCompletecallback ofsetViewVisibleAnimated, but the underlyingSplitView.setViewVisiblecan cancel animations and the animated API does not invokeonCompleteon cancellation. If the close animation gets canceled, the.nopanelclass (andpaneCompositeService.hideActivePaneComposite) may never be applied even though the panel ends up hidden. The deferred work should be made robust to animation cancellation to avoid leaving layout/CSS state inconsistent.
// Adjust CSS - for hiding, defer adding the class until animation
// completes because `.nopanel .part.panel { display: none !important }`
// would instantly hide the panel content mid-animation.
if (!hidden) {
this.mainContainer.classList.remove(LayoutClasses.PANEL_HIDDEN);
}
// Propagate layout changes to grid
this.workbenchGrid.setViewVisibleAnimated(
this.panelPartView,
!hidden,
hidden ? PANEL_CLOSE_DURATION : PANEL_OPEN_DURATION,
hidden ? EASE_IN : EASE_OUT,
hidden ? () => {
// Deferred to after close animation
this.mainContainer.classList.add(LayoutClasses.PANEL_HIDDEN);
if (this.paneCompositeService.getActivePaneComposite(ViewContainerLocation.Panel)) {
this.paneCompositeService.hideActivePaneComposite(ViewContainerLocation.Panel);
}
} : undefined
);
src/vs/workbench/browser/layout.ts:2244
- Same pattern in
setAuxiliaryBarHidden(true): class toggling andhideActivePaneCompositeare deferred tosetViewVisibleAnimated’sonComplete, which won’t run if the animation is canceled. That can leave.noauxiliarybarand the active auxiliary bar composite state out of sync with actual visibility. Please handle the cancellation case so the deferred state updates still occur when the final state is hidden.
// Adjust CSS - for hiding, defer adding the class until animation
// completes because `.noauxiliarybar .part.auxiliarybar { display: none !important }`
// would instantly hide the content mid-animation.
if (!hidden) {
this.mainContainer.classList.remove(LayoutClasses.AUXILIARYBAR_HIDDEN);
}
// Propagate to grid
this.workbenchGrid.setViewVisibleAnimated(
this.auxiliaryBarPartView,
!hidden,
hidden ? PANEL_CLOSE_DURATION : PANEL_OPEN_DURATION,
hidden ? EASE_IN : EASE_OUT,
hidden ? () => {
// Deferred to after close animation
this.mainContainer.classList.add(LayoutClasses.AUXILIARYBAR_HIDDEN);
if (this.paneCompositeService.getActivePaneComposite(ViewContainerLocation.AuxiliaryBar)) {
this.paneCompositeService.hideActivePaneComposite(ViewContainerLocation.AuxiliaryBar);
this.focusPanelOrEditor();
}
} : undefined
);
src/vs/platform/quickinput/browser/quickInputController.ts:820
- The exit animation cleanup (
onDone) can run multiple times:transitionendbubbles (same early-fire risk as the entrance handler) and there is also an unconditionalsetTimeout(onDone, ...)that isn’t cleared whentransitionendfires. Multiple invocations are currently possible and can cause repeated DOM/style mutations. Please guardonDone(e.g. with a boolean), filtertransitionendto the container, and clear the timeout when cleanup runs.
// Animate exit: fade out + slide up (faster than open)
if (!isMotionReduced(container)) {
container.style.transition = `opacity ${QUICK_INPUT_CLOSE_DURATION}ms ${EASE_IN}, transform ${QUICK_INPUT_CLOSE_DURATION}ms ${EASE_IN}`;
container.style.opacity = '0';
container.style.transform = 'translateY(-8px)';
const onDone = () => {
container.style.display = 'none';
container.style.transition = '';
container.style.opacity = '';
container.style.transform = '';
container.style.willChange = '';
container.removeEventListener('transitionend', onDone);
};
container.addEventListener('transitionend', onDone);
// Safety timeout in case transitionend doesn't fire
const window = dom.getWindow(container);
window.setTimeout(onDone, QUICK_INPUT_CLOSE_DURATION + 50);
} else {
| ui.container.style.opacity = '1'; | ||
| ui.container.style.transform = 'translateY(0)'; | ||
| const onDone = () => { | ||
| ui.container.style.transition = ''; | ||
| ui.container.style.willChange = ''; | ||
| ui.container.removeEventListener('transitionend', onDone); | ||
| }; | ||
| ui.container.addEventListener('transitionend', onDone); |
There was a problem hiding this comment.
The transitionend listener used for the quick input entrance animation can fire for transitions on descendant elements because transitionend bubbles. Without checking the event target (and optionally property name), onDone may run early, clearing transition while the container’s own animation is still running. Please filter the event (e.g. only act when e.target === ui.container) and consider a safety timeout similar to the exit path so cleanup always runs.
This issue also appears on line 803 of the same file.
| ui.container.style.opacity = '1'; | |
| ui.container.style.transform = 'translateY(0)'; | |
| const onDone = () => { | |
| ui.container.style.transition = ''; | |
| ui.container.style.willChange = ''; | |
| ui.container.removeEventListener('transitionend', onDone); | |
| }; | |
| ui.container.addEventListener('transitionend', onDone); | |
| ui.container.style.willChange = 'opacity, transform'; | |
| ui.container.style.opacity = '1'; | |
| ui.container.style.transform = 'translateY(0)'; | |
| let entranceCleanupTimeout: number | undefined; | |
| let entranceCleanedUp = false; | |
| const cleanupEntranceTransition = () => { | |
| if (entranceCleanedUp) { | |
| return; | |
| } | |
| entranceCleanedUp = true; | |
| ui.container.style.transition = ''; | |
| ui.container.style.willChange = ''; | |
| ui.container.removeEventListener('transitionend', onDone); | |
| if (entranceCleanupTimeout !== undefined) { | |
| clearTimeout(entranceCleanupTimeout); | |
| entranceCleanupTimeout = undefined; | |
| } | |
| }; | |
| const onDone = (e: TransitionEvent) => { | |
| if (e.target !== ui.container) { | |
| return; // Ignore transitions from descendant elements | |
| } | |
| cleanupEntranceTransition(); | |
| }; | |
| ui.container.addEventListener('transitionend', onDone); | |
| // Fallback in case transitionend does not fire | |
| entranceCleanupTimeout = window.setTimeout(() => { | |
| cleanupEntranceTransition(); | |
| }, QUICK_INPUT_OPEN_DURATION + 50); |
|
@copilot review |
|
@eli-w-king I've opened a new pull request, #294261, to work on those changes. Once the pull request is ready, I'll request review from you. |
TylerLeonhardt
left a comment
There was a problem hiding this comment.
The quick pick change is pretty intimidating. nested functions, event listeners, forced reflowing........ just for some animation.
it feels overly complicated, and I wonder why it's so complicated... but I don't know what is should be to be simpler
I'll approve for now to not unblock but I do feel like we may see some bugs
|
@TylerLeonhardt I've opened a new pull request, #294737, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Simplify animations using CSS @Keyframes instead of transitions Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com> * Remove accidentally committed codicon.ttf binary Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com>
| // Animate exit: fade out + slide up (faster than open) | ||
| if (!isMotionReduced(container)) { | ||
| container.classList.add('animating-exit'); | ||
| const onAnimationEnd = () => { |
There was a problem hiding this comment.
@copilot be DRY. Also I'd expect onAnimationEnd to set this._cancelExitAnimation to undefined no?
|
@TylerLeonhardt I've opened a new pull request, #294739, to work on those changes. Once the pull request is ready, I'll request review from you. |
…294739) * Initial plan * Refactor exit animation cleanup to be DRY and properly reset cancel function Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com> * Remove accidentally committed codicon.ttf build artifact Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: TylerLeonhardt <2644648+TylerLeonhardt@users.noreply.github.com> Co-authored-by: Elijah King <elijahwilliamking@icloud.com>
sped animation back up by 25ms :)
Screen.Recording.2026-02-09.at.2.05.33.PM.mov