Enhance Activitybar layout and item separation

build/lib/stylelint/vscode-known-variables.json

@@ -977,6 +977,7 @@
 	"others": [
 		"--action-widget-close-start-opacity",
 		"--action-widget-close-start-transform",
+		"--activity-bar-action-gap",
 		"--activity-bar-action-height",
 		"--activity-bar-icon-size",
 		"--activity-bar-width",

src/vs/workbench/browser/media/floatingPanels.css

@@ -194,10 +194,11 @@
 	margin-top: var(--vscode-spacing-size20);
 }
 
-/* At the default (non-compact) size, separate the activity bar items with an 8px gap
- * so they read as distinct floating targets. Compact keeps the tighter default stack. */
-.monaco-workbench.floating-panels .part.activitybar:not(.compact) > .content .monaco-action-bar .action-item + .action-item {
-	margin-top: var(--vscode-spacing-size80);
+/* Separate the activity bar items so they read as distinct floating targets. The gap is
+ * published by activitybarPart.ts as `--activity-bar-action-gap` (0px at the compact size)
+ * so that it stays in step with the overflow computation. */
+.monaco-workbench.floating-panels .part.activitybar > .content .monaco-action-bar .action-item + .action-item {
+	margin-top: var(--activity-bar-action-gap, 0px);
 }
 
 /* Inset and vertically center status bar items within the full-width bottom rail. */

src/vs/workbench/browser/parts/activitybar/activitybarPart.ts

@@ -55,6 +55,13 @@ export class ActivitybarPart extends Part {
 	static readonly FLOATING_ACTIVITYBAR_WIDTH = 36;
 	static readonly FLOATING_COMPACT_ACTIVITYBAR_WIDTH = 28;
 
+	/**
+	 * Vertical gap between activity bar items at the default size under the floating
+	 * panels experiment. Published to CSS as `--activity-bar-action-gap` so that the
+	 * stylesheet and the overflow computation cannot drift apart.
+	 */
+	static readonly FLOATING_ACTION_GAP = 8;
+
 	static readonly ICON_SIZE = 24;
 	static readonly COMPACT_ICON_SIZE = 16;
 
@@ -85,14 +92,32 @@ export class ActivitybarPart extends Part {
 		return this._isCompact ? ActivitybarPart.COMPACT_ACTIVITYBAR_WIDTH : ActivitybarPart.ACTIVITYBAR_WIDTH;
 	}
 
-	/** The action (item) height that drives visible item sizing and the composite bar overflow size. */
+	/** The action (item) height that drives visible item sizing. */
 	private get actionHeight(): number {
 		if (this._isCompact) {
 			return ActivitybarPart.COMPACT_ACTION_HEIGHT;
 		}
 		return this.layoutService.isFloatingPanelsEnabled() ? ActivitybarPart.FLOATING_ACTION_HEIGHT : ActivitybarPart.ACTION_HEIGHT;
 	}
 
+	/**
+	 * Vertical gap rendered between two adjacent items. Only the floating panels
+	 * experiment separates items, and only at the default size.
+	 */
+	private get actionGap(): number {
+		return this.layoutService.isFloatingPanelsEnabled() && !this._isCompact ? ActivitybarPart.FLOATING_ACTION_GAP : 0;
+	}
+
+	/**
+	 * The vertical space a single item occupies in the bar (its height plus the gap that
+	 * separates it from the next one). This drives the overflow computation, so it has to
+	 * track the current activity bar size, otherwise items collapse into the overflow menu
+	 * prematurely.
+	 */
+	private get compositeSize(): number {
+		return this.actionHeight + this.actionGap;
+	}
+
 	private get floatingHorizontalGutter(): number {
 		if (!this.layoutService.isFloatingPanelsEnabled()) {
 			return 0;
@@ -164,6 +189,7 @@ export class ActivitybarPart extends Part {
 			this.layoutService.mainContainer.classList.toggle('activitybar-compact', this._isCompact);
 			this.element.style.setProperty('--activity-bar-width', `${this.baseWidth}px`);
 			this.element.style.setProperty('--activity-bar-action-height', `${this.actionHeight}px`);
+			this.element.style.setProperty('--activity-bar-action-gap', `${this.actionGap}px`);
 			this.element.style.setProperty('--activity-bar-icon-size', `${this._isCompact ? ActivitybarPart.COMPACT_ICON_SIZE : ActivitybarPart.ICON_SIZE}px`);
 		}
 	}
@@ -184,7 +210,7 @@ export class ActivitybarPart extends Part {
 	}
 
 	private createCompositeBar(): PaneCompositeBar {
-		const actionHeight = this.actionHeight;
+		const compositeSize = this.compositeSize;
 		const iconSize = this._isCompact ? ActivitybarPart.COMPACT_ICON_SIZE : ActivitybarPart.ICON_SIZE;
 
 		return this.instantiationService.createInstance(ActivityBarCompositeBar, this.location, {
@@ -201,7 +227,7 @@ export class ActivitybarPart extends Part {
 			preventLoopNavigation: true,
 			recomputeSizes: false,
 			fillExtraContextMenuActions: (actions, e?: MouseEvent | GestureEvent) => { },
-			compositeSize: 52,
+			compositeSize,
 			colors: (theme: IColorTheme) => ({
 				activeForegroundColor: theme.getColor(ACTIVITY_BAR_FOREGROUND),
 				inactiveForegroundColor: theme.getColor(ACTIVITY_BAR_INACTIVE_FOREGROUND),
@@ -212,7 +238,7 @@ export class ActivitybarPart extends Part {
 				dragAndDropBorder: theme.getColor(ACTIVITY_BAR_DRAG_AND_DROP_BORDER),
 				activeBackgroundColor: undefined, inactiveBackgroundColor: undefined, activeBorderBottomColor: undefined,
 			}),
-			overflowActionSize: actionHeight,
+			overflowActionSize: compositeSize,
 		}, Parts.ACTIVITYBAR_PART, this.paneCompositePart, true);
 	}
 
@@ -306,8 +332,8 @@ export class ActivitybarPart extends Part {
 		// Layout contents
 		const contentAreaSize = super.layoutContents(contentWidth, contentHeight).contentSize;
 
-		// Layout composite bar
-		this.compositeBar.value?.layout(contentWidth, contentAreaSize.height);
+		// The first item has no preceding gap, so give one gap back to the composite bar.
+		this.compositeBar.value?.layout(contentWidth, contentAreaSize.height + this.actionGap);
 	}
 
 	/**
@@ -502,7 +528,7 @@ export class ActivityBarCompositeBar extends PaneCompositeBar {
 		}
 		if (this.globalCompositeBar) {
 			if (this.options.orientation === ActionsOrientation.VERTICAL) {
-				height -= (this.globalCompositeBar.size() * this.options.overflowActionSize);
+				height -= this.globalCompositeBar.element.clientHeight;
 			} else {
 				width -= this.globalCompositeBar.element.clientWidth;
 			}

src/vs/workbench/browser/parts/globalCompositeBar.ts

@@ -133,10 +133,6 @@ export class GlobalCompositeBar extends Disposable {
 		this.globalActivityActionBar.focus(true);
 	}
 
-	size(): number {
-		return this.globalActivityActionBar.viewItems.length;
-	}
-
 	getContextMenuActions(): IAction[] {
 		return [toAction({ id: 'toggleAccountsVisibility', label: localize('accounts', "Accounts"), checked: this.accountsVisibilityPreference, run: () => this.accountsVisibilityPreference = !this.accountsVisibilityPreference })];
 	}

src/vs/workbench/test/browser/parts/activitybar/activitybarPart.test.ts

@@ -10,18 +10,34 @@ import { TestConfigurationService } from '../../../../../platform/configuration/
 import { TestColorTheme, TestThemeService } from '../../../../../platform/theme/test/common/testThemeService.js';
 import { TestStorageService } from '../../../common/workbenchTestServices.js';
 import { TestHostService, TestLayoutService } from '../../workbenchTestServices.js';
-import { ActivitybarPart } from '../../../../browser/parts/activitybar/activitybarPart.js';
+import { ActivitybarPart, ActivityBarCompositeBar } from '../../../../browser/parts/activitybar/activitybarPart.js';
 import { IViewSize } from '../../../../../base/browser/ui/grid/grid.js';
 import { LayoutSettings, Parts, Position } from '../../../../services/layout/browser/layoutService.js';
 import { mainWindow } from '../../../../../base/browser/window.js';
 import { IConfigurationChangeEvent } from '../../../../../platform/configuration/common/configuration.js';
 import { IPaneCompositePart } from '../../../../browser/parts/paneCompositePart.js';
+import { IPaneCompositeBarOptions } from '../../../../browser/parts/paneCompositeBar.js';
 import { Event, Emitter } from '../../../../../base/common/event.js';
 import { IPaneComposite } from '../../../../common/panecomposite.js';
 import { Extensions, PaneCompositeDescriptor } from '../../../../browser/panecomposite.js';
 import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js';
 import { ViewContainerLocation } from '../../../../common/views.js';
 import { ACTIVITY_BAR_BACKGROUND, MODERN_ACTIVITY_BAR_BACKGROUND, MODERN_ACTIVITY_BAR_INACTIVE_BACKGROUND } from '../../../../common/theme.js';
+import { ActionsOrientation } from '../../../../../base/browser/ui/actionbar/actionbar.js';
+import { Dimension } from '../../../../../base/browser/dom.js';
+
+interface ILayoutTestHarness {
+	menuBarContainer: HTMLElement | undefined;
+	globalCompositeBar: { element: HTMLElement } | undefined;
+	options: { orientation: ActionsOrientation };
+	compositeBar: { layout: (dimension: Dimension) => void };
+}
+
+// `super.layout()` resolves through the prototype chain, so calling the extracted method
+// against a harness still runs the real `PaneCompositeBar.layout` and hands the resulting
+// dimension to `compositeBar`.
+const activityBarCompositeBarLayout = Reflect.get(ActivityBarCompositeBar.prototype, 'layout') as (this: ILayoutTestHarness, width: number, height: number) => void;
+
 
 class StubPaneCompositePart implements IPaneCompositePart {
 	declare readonly _serviceBrand: undefined;
@@ -74,7 +90,7 @@ suite('ActivitybarPart', () => {
 		disposables.clear();
 	});
 
-	function createActivitybarPart(compact: boolean, floatingPanelsEnabled = false, sideBarPosition = Position.LEFT, colors: { [id: string]: string | undefined } = {}): { part: ActivitybarPart; configService: TestConfigurationService; layoutService: TestFloatingPanelsLayoutService; hostService: TestHostService } {
+	function createActivitybarPart(compact: boolean, floatingPanelsEnabled = false, sideBarPosition = Position.LEFT, colors: { [id: string]: string | undefined } = {}, instantiationService?: IInstantiationService): { part: ActivitybarPart; configService: TestConfigurationService; layoutService: TestFloatingPanelsLayoutService; hostService: TestHostService } {
 		const configService = new TestConfigurationService({
 			[LayoutSettings.ACTIVITY_BAR_COMPACT]: compact,
 			[LayoutSettings.MODERN_UI]: floatingPanelsEnabled,
@@ -92,7 +108,7 @@ suite('ActivitybarPart', () => {
 
 		// Stub instantiation service—createCompositeBar is only called in show(),
 		// which we skip in unit tests focused on dimensions / style behaviour.
-		const stubInstantiationService = { createInstance: () => { throw new Error('not expected'); } } as unknown as IInstantiationService;
+		const stubInstantiationService = instantiationService ?? { createInstance: () => { throw new Error('not expected'); } } as unknown as IInstantiationService;
 
 		const part = disposables.add(new ActivitybarPart(
 			ViewContainerLocation.Sidebar,
@@ -295,6 +311,7 @@ suite('ActivitybarPart', () => {
 
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-width'), `${ActivitybarPart.ACTIVITYBAR_WIDTH}px`);
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-action-height'), `${ActivitybarPart.ACTION_HEIGHT}px`);
+		assert.strictEqual(el.style.getPropertyValue('--activity-bar-action-gap'), '0px');
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-icon-size'), `${ActivitybarPart.ICON_SIZE}px`);
 		assert.strictEqual(el.classList.contains('compact'), false);
 	});
@@ -308,6 +325,7 @@ suite('ActivitybarPart', () => {
 
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-width'), `${ActivitybarPart.COMPACT_ACTIVITYBAR_WIDTH}px`);
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-action-height'), `${ActivitybarPart.COMPACT_ACTION_HEIGHT}px`);
+		assert.strictEqual(el.style.getPropertyValue('--activity-bar-action-gap'), '0px');
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-icon-size'), `${ActivitybarPart.COMPACT_ICON_SIZE}px`);
 		assert.strictEqual(el.classList.contains('compact'), true);
 	});
@@ -321,6 +339,7 @@ suite('ActivitybarPart', () => {
 
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-width'), `${ActivitybarPart.FLOATING_ACTIVITYBAR_WIDTH}px`);
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-action-height'), `${ActivitybarPart.FLOATING_ACTION_HEIGHT}px`);
+		assert.strictEqual(el.style.getPropertyValue('--activity-bar-action-gap'), `${ActivitybarPart.FLOATING_ACTION_GAP}px`);
 		assert.strictEqual(el.style.getPropertyValue('--activity-bar-icon-size'), `${ActivitybarPart.ICON_SIZE}px`);
 		assert.strictEqual(el.classList.contains('compact'), false);
 	});
@@ -438,5 +457,147 @@ suite('ActivitybarPart', () => {
 		});
 	});
 
+	// --- composite bar item sizing -------------------------------------------
+
+	// The composite bar decides how many activity icons fit before collapsing the rest into
+	// the overflow ("Additional Views") menu, so the size it is handed has to match the
+	// vertical space an item actually occupies in the current mode.
+	function capturedCompositeBarOptions(compact: boolean, floatingPanelsEnabled: boolean): IPaneCompositeBarOptions {
+		let captured: IPaneCompositeBarOptions | undefined;
+		const stubCompositeBar = { create: () => { }, layout: () => { }, dispose: () => { } };
+		const { part } = createActivitybarPart(compact, floatingPanelsEnabled, Position.LEFT, {}, {
+			createInstance: (_descriptor: unknown, _location: unknown, options: IPaneCompositeBarOptions) => {
+				captured = options;
+				return stubCompositeBar;
+			}
+		} as unknown as IInstantiationService);
+
+		const el = document.createElement('div');
+		fixture.appendChild(el);
+		part.create(el);
+		part.show();
+
+		return captured!;
+	}
+
+	test('composite bar item size tracks the rendered item stride in every mode', () => {
+		const sizesFor = (compact: boolean, floatingPanelsEnabled: boolean) => {
+			const { compositeSize, overflowActionSize } = capturedCompositeBarOptions(compact, floatingPanelsEnabled);
+			return { compositeSize, overflowActionSize };
+		};
+
+		assert.deepStrictEqual(
+			{
+				classicDefault: sizesFor(false, false),
+				classicCompact: sizesFor(true, false),
+				modernDefault: sizesFor(false, true),
+				modernCompact: sizesFor(true, true),
+			},
+			{
+				// Items stack flush against each other, so the stride is just the action height.
+				classicDefault: { compositeSize: 48, overflowActionSize: 48 },
+				classicCompact: { compositeSize: 28, overflowActionSize: 28 },
+				// Modern UI separates items with an 8px gap, but only at the default size.
+				modernDefault: { compositeSize: 44, overflowActionSize: 44 },
+				modernCompact: { compositeSize: 28, overflowActionSize: 28 },
+			}
+		);
+	});
+
+	// The gap is rendered *between* items, so N items occupy `N * height + (N - 1) * gap`.
+	// `compositeSize` bakes a trailing gap into every item, which over-counts by exactly one
+	// gap, so `layout()` hands that gap back to the composite bar. Without it the last item
+	// is pushed into the overflow menu a gap earlier than it needs to be.
+	function compositeBarLayoutHeight(compact: boolean, floatingPanelsEnabled: boolean): number {
+		let layoutHeight = -1;
+		const stubCompositeBar = {
+			create: () => { },
+			layout: (_width: number, height: number) => { layoutHeight = height; },
+			dispose: () => { }
+		};
+		const { part, layoutService } = createActivitybarPart(compact, floatingPanelsEnabled, Position.LEFT, {}, {
+			createInstance: () => stubCompositeBar
+		} as unknown as IInstantiationService);
+
+		const el = document.createElement('div');
+		fixture.appendChild(el);
+		part.create(el);
+		part.show();
+
+		// A visible title and status bar means neither edge is a window edge.
+		const visible = new Set([Parts.TITLEBAR_PART, Parts.STATUSBAR_PART]);
+		layoutService.isVisible = (partId: Parts) => visible.has(partId);
+		part.layout(100, 300);
+
+		return layoutHeight;
+	}
+
+	test('composite bar is given back the leading item gap it does not render', () => {
+		const margin = ActivitybarPart.FLOATING_MARGIN;
+
+		assert.deepStrictEqual(
+			{
+				classicDefault: compositeBarLayoutHeight(false, false),
+				classicCompact: compositeBarLayoutHeight(true, false),
+				modernDefault: compositeBarLayoutHeight(false, true),
+				modernCompact: compositeBarLayoutHeight(true, true),
+			},
+			{
+				// No floating gutters and no gap between items.
+				classicDefault: 300,
+				classicCompact: 300,
+				// Floating reserves a bottom gutter; the 8px gap is then handed back.
+				modernDefault: 300 - margin + ActivitybarPart.FLOATING_ACTION_GAP,
+				modernCompact: 300 - margin,
+			}
+		);
+	});
+
+	// --- global activity icons reservation -----------------------------------
+
+	// The global (Accounts/Manage) icons are a separate action bar stacked beneath the view
+	// containers, so the room they take has to be measured rather than derived from the item
+	// size: the gap sits only *between* items, so N icons occupy N * height + (N - 1) * gap.
+	function heightLeftForCompositeBar(globalActionCount: number, itemHeight: number, gap: number): number {
+		const globalBarElement = document.createElement('div');
+		for (let i = 0; i < globalActionCount; i++) {
+			const item = document.createElement('div');
+			item.style.height = `${itemHeight}px`;
+			if (i > 0) {
+				item.style.marginTop = `${gap}px`;
+			}
+			globalBarElement.appendChild(item);
+		}
+		fixture.appendChild(globalBarElement);
+
+		let laidOut: Dimension | undefined;
+		activityBarCompositeBarLayout.call({
+			menuBarContainer: undefined,
+			globalCompositeBar: { element: globalBarElement },
+			options: { orientation: ActionsOrientation.VERTICAL },
+			compositeBar: { layout: dimension => { laidOut = dimension; } },
+		}, ActivitybarPart.FLOATING_ACTIVITYBAR_WIDTH, 300);
+
+		return laidOut!.height;
+	}
+
+	test('reserves the measured height of the global activity icons', () => {
+		const gap = ActivitybarPart.FLOATING_ACTION_GAP;
+		const itemHeight = ActivitybarPart.FLOATING_ACTION_HEIGHT;
+
+		assert.deepStrictEqual(
+			{
+				oneGlobalAction: heightLeftForCompositeBar(1, itemHeight, gap),
+				twoGlobalActions: heightLeftForCompositeBar(2, itemHeight, gap),
+			},
+			{
+				// A lone icon has no gap at all, so it occupies exactly its own height.
+				oneGlobalAction: 300 - itemHeight,
+				// Two icons share a single gap: 36 + 8 + 36 = 80, not 2 * (36 + 8) = 88.
+				twoGlobalActions: 300 - (itemHeight * 2 + gap),
+			}
+		);
+	});
+
 	ensureNoDisposablesAreLeakedInTestSuite();
 });