Give WorkbenchButtonBar ownership of icon-label spacing

[Cherry Wang (chryw)]

Predictable UI composition - 3 of 3: shared-control pilot

This is the third PR in a three-part draft stack:

1. Add UI composition size tokens #335345 - candidate token foundation: register the proposed spacing and icon-size vocabulary.
2. Document predictable UI composition #335346 - candidate authoring guidance: derive a property-agnostic ownership rule.
3. Give WorkbenchButtonBar ownership of icon-label spacing #335350 - shared-control pilot (this PR): test one relationship in WorkbenchButtonBar and the Changes view.

This PR is an experiment supporting review of the broader proposal. It validates a concrete composition, not every token or every ownership boundary in #335354.

Problem

Before this draft, the effective icon-label separation in the Changes view had two contributors:

• WorkbenchButtonBar supplied 4px through its internal leading icon/spinner margin.
• The Changes view supplied another 2px through consumer CSS on the button.

The resulting 6px distance was not described by either rule alone. A future change to either layer could silently alter or double the relationship.

The old margin was already part of the shared control implementation; a child-targeted margin is not inherently child-owned. The problem is the additional consumer contribution, not the use of margin itself.

Pilot implementation

The draft consolidates the relationship in WorkbenchButtonBar:

• The leading icon/spinner slot uses --vscode-iconSize-small (16px).
• A control-owned selector applies pair-specific spacing to the leading slot, so multi-segment labels keep their own internal relationships.
• Omitting iconLabelSpacing resolves to the explicit compact 4px variant for current consumers.
• iconLabelSpacing: 'default' selects a 6px relationship for the Changes view.
• The icon and busy spinner continue to share one leading slot, preserving width when the representation swaps.
• The Changes view removes its additional 2px contribution.

Review findings addressed

The review identified and the latest commits address these gaps:

1. Multi-segment labels: broad column-gap was replaced with a targeted leading-slot margin. A unit test and shared button fixture cover a label containing an inline icon.
2. Production context: a Sessions-owned fixture renders the actual .changes-view-body .chat-editing-session-actions.outside-card hierarchy with its production single-button geometry. It consumes the same named outside-card spacing constant as the production host, so the mapping cannot drift independently.
3. Synthetic diff statistics: the fixture no longer invents a working-set-diff-stats button-bar consumer. Production diff statistics use a separate ActionViewItem and are outside this pilot.
4. Shared-widget spacing leak: iconLabelSpacing was hardcoded to 'default' inside the widget classes shared by both the outside-card composition and the single-pane editor header, so the header picked up the wider 6px relationship it never had before. The widgets retain an explicit host-supplied constructor parameter; production and fixtures consume shared named constants for compact header and default outside-card spacing.

The explicit 'compact' value and omission intentionally resolve to the same compatibility variant; both apply an explicit compact selector rather than relying on an implicit base rule.

Shared-control visual validation

The updated platform/buttonBar/ButtonBars fixture exercises primary/trailing actions, split buttons, busy spinner replacement, and secondary overflow.

Dark	Light

Component Explorer validation:

• All eight platform/buttonBar/ fixtures and both production-derived Changes fixtures render and dispose without errors.
• Dark and light Buttons and ButtonBars fixtures are stable across three captures.
• The dark and light production-derived Changes fixtures are stable across three captures and visually approved.
• The shared inline-label fixture visually distinguishes the 6px leading relationship from the tighter spacing around the inline icon.
• The production fixture measures a 26px-tall, full-width primary control with 4px padding, matching the single button the Changes view actually renders in this composition.
• The labelled variant keeps a 16 x 16px leading slot.
• Accessible names are preserved in the tested variants.

Automated validation

• ./scripts/test.sh --run src/vs/platform/actions/test/browser/buttonbar.test.ts - 8 focused button tests pass
• npm run stylelint -- src/vs/platform/actions/browser/buttonbar.css src/vs/sessions/contrib/changes/browser/media/changesView.css - clean
• Editor diagnostics on all changed files - clean
• git diff --check - clean

Current unit coverage includes normal and dropdown buttons, icon/spinner replacement, menu and non-menu actions, icon-only variants, an inline-icon label, compact/default margins, and an assertion that the button container contributes no broad column gap. Component Explorer covers the computed production geometry.

Review questions

• Is the explicit compact/default compatibility model clear enough for future consumers?

Context

• Umbrella proposal: Predictable UI composition: spacing ownership and icon sizing #335354
• Token foundation: Add UI composition size tokens #335345
• Authoring guidance: Document predictable UI composition #335346
• Originating report: Agents window single pane: Missing right padding on tabs? #331234

[Copilot]

🟡 Changes recommended

Container-wide gaps mis-space multipart labels, and production selector coverage remains absent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Moves leading icon-label spacing ownership into WorkbenchButtonBar while preserving the Changes view’s intended geometry.

Changes:

• Adds configurable compact/default icon-label spacing.
• Migrates Changes view spacing to the shared control.
• Extends button-bar tests and fixtures.

File summaries

File	Description
buttonBar.fixture.ts	Exercises default spacing visually.
changesView.css	Removes consumer spacing and preserves diff-stat gaps.
changesView.ts	Selects default spacing for primary actions.
buttonbar.test.ts	Verifies spacing classes and leading slots.
buttonbar.ts	Adds spacing configuration and state classes.
buttonbar.css	Defines icon sizing and spacing variants.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The compact CSS variant remains implicit, and the new fixture does not accurately reproduce two claimed production compositions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/vs/sessions/contrib/changes/test/browser/changesButtonBar.fixture.ts:60

• This fixture is the only code in the repository that emits working-set-diff-stats; neither production button configuration adds that class, and production diff stats render through ChangesDiffStatsActionItem with changes-diff-stats-action. As a result, the fixture validates a synthetic control rather than the claimed production diff-stat composition. Render the production action item, or wire the marker into production if this button is intended to exist there.

					case lineCounts.id:
						return {
							showLabel: true,
							customLabel: '$(add) 2 $(remove) 1',
							customClass: 'working-set-diff-stats',
							isSecondary: true,

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Cherry Wang (chryw)]

Addressed the review summary's synthetic diff-stat fixture finding in 78bd91f. The fixture now contains only the production button-bar composition: a primary labelled action and icon-only secondary action. Diff statistics use a separate ActionViewItem in production and are no longer represented as a fabricated working-set-diff-stats button.

[Copilot]

🟡 Changes recommended

The optional spacing API does not document that omission selects compact rather than its explicitly named default variant.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The spacing contract is consistently implemented, migrated, and covered without unresolved correctness issues.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The spacing contract is consistently implemented with focused unit and production-derived visual coverage.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The central pair-specific spacing behavior lacks an automated regression assertion or blocking visual fixture.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/vs/sessions/contrib/changes/test/browser/changesButtonBar.fixture.ts:41

• This four-line comment restates the option and the current implementation detail in WorkbenchButtonBar.update, so it can become stale when the cap changes. Keep only the fixture invariant in a single line.

	// Both production call sites of this outside-card composition pass
	// `renderSecondaryActions: false`, which caps the bar at a single button
	// (see `WorkbenchButtonBar.update`). Match that here so the fixture cannot
	// claim geometry the real Changes view never renders.

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[Cherry Wang (chryw)]

Pushed 00a636eabb0:

• Trimmed the explanatory comment in changesButtonBar.fixture.ts to one line (durable invariant only, no implementation-detail restatement).
• buttonbar.test.ts now imports buttonbar.css, mounts the bar in the live document with --vscode-spacing-size40/size60 defined, and asserts the actual computed margin-inline-end for both the default (6px) and compact (4px, new test) pair-specific spacing — plus confirms the unrelated inline codicon keeps its ordinary symmetric margin. This closes the gap where only DOM class/text shape was asserted.

Verified: 8/8 targeted WorkbenchButtonBar tests pass, stylelint clean, no diagnostics.

[Copilot]

🟡 Changes recommended

Shared widget configuration unintentionally widens icon-label spacing in the single-pane Changes header.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/vs/sessions/contrib/changes/browser/changesView.ts:337

• The Agent Host variant is reused by ChangesActionsBar in the single-pane editor header (changesView.ts:581-583) as well as by the outside-card view (changesView.ts:1645-1647). Only the latter previously received the consumer's extra 2px, so selecting default inside this shared provider unintentionally widens the compact header too. Make spacing a host-supplied option and use default only for the outside-card composition.

						? { showIcon: true, showLabel: true, customLabel: primaryCustomLabel ?? stripIcons(action.label), showSpinner: primaryIsBusy, iconLabelSpacing: 'default' }

src/vs/platform/actions/test/browser/buttonbar.test.ts:192

• This long method-body comment obscures the assertion it explains; reduce it to the pair-specific regression invariant.

		// The pair-specific spacing rule in buttonbar.css only targets the
		// leading icon; the inline icon inside the label keeps its ordinary,
		// symmetric codicon margin. A regression that widens column-gap, or
		// that makes the pair-specific rule apply too broadly, shows up here
		// even though the class list above wouldn't catch it.

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Balanced

[Cherry Wang (chryw)]

Pushed 303e67719b6:

• Fixed the real cross-consumer regression: iconLabelSpacing was hardcoded to 'default' inside the two widget classes shared by the outside-card composition and the single-pane editor header. It's now a per-host constructor parameter — outside-card keeps 'default' (6px), the single-pane header now explicitly gets 'compact' (4px), matching its pre-existing spacing.
• Trimmed both new explanatory comments in buttonbar.test.ts to one line each.
• Updated the PR description to match the current single-button fixture (removed stale icon-only/secondary-padding claims) and documented the spacing-threading fix.

Verified: 8/8 targeted WorkbenchButtonBar tests pass, fixture renders/disposes without errors, no diagnostics.

[Copilot]

🟡 Changes recommended

The tests do not yet guard container-gap regressions or exercise the actual host-specific production wiring.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/vs/platform/actions/test/browser/buttonbar.test.ts:192

• This still does not guard against the regression named by the test. A flex column-gap contributes to rendered separation without changing either child's computed margins, so reintroducing a broad gap can leave all three assertions passing. Assert the button's computed columnGap is zero/normal, or compare the rendered child-to-child distances against the corresponding margins, so spacing added by the container is detected.

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[Cherry Wang (chryw)]

Addressed the review summary's container-gap test gap in 0849550. The inline-icon regression now asserts the button's computed columnGap is normal in addition to checking the pair-specific 6px leading margin and symmetric inline-icon margins, so reintroducing a broad flex gap fails the test.

[Copilot]

🟢 Approval recommended

The scoped migration preserves compatibility and includes focused behavioral and visual regression coverage.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Cherry Wang (chryw)]

Pushed ffb18100209 to complete the host-wiring coverage without DOM-class sniffing:

• Both production hosts keep an explicit iconLabelSpacing constructor contract.
• The compact header and default outside-card mappings are named constants in changesButtonBarSpacing.ts.
• The production call sites and the outside-card fixture consume those same constants, so the fixture cannot silently diverge from the host mapping.
• The computed-style regression also asserts the button container's columnGap remains normal.

Validation: 8 focused WorkbenchButtonBar tests pass; both Changes fixtures render without errors and remain stable across three dark/light captures.

[Cherry Wang (chryw)]

Since the last review, pushed:

• 084955069e6 / f5c66f6c7e8: made iconLabelSpacing an explicit host-supplied constructor parameter on both ChangesWorkbenchButtonBarWidget and ChangesMenuWorkbenchButtonBarWidget (rather than inferring it from the container's CSS class list), plus the missing columnGap regression assertion in buttonbar.test.ts.
• ffb1810020: extracted the two spacing values into changesButtonBarSpacing.ts (a small side-effect-free module) so production's two call sites and the button-bar fixture all reference the same named constants instead of independently duplicating the 'compact'/'default' literals — closing the drift risk the fixture-sharing review comment raised, without importing changesView.ts's heavier module (which registers actions at import time) into the fixture.

Verified: 8/8 targeted WorkbenchButtonBar tests pass, fixture renders/disposes without errors, no diagnostics.

[Copilot]

🔵 Needs a closer look

The production host-specific spacing selection remains uncovered because the fixture constructs the shared button bar directly.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/vs/sessions/contrib/changes/test/browser/changesButtonBar.fixture.ts:51

• This fixture still constructs WorkbenchButtonBar directly and selects the outside-card constant independently of the production wiring at changesView.ts:1648-1650. If that call site is accidentally switched to the compact header constant, this fixture continues rendering the default spacing and stays green. Render through the owning Changes widget/host, or expose a host-to-spacing helper that both production and the fixture invoke so the host-specific selection is actually covered.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Cherry Wang (chryw)]

Re: the "closer look" note that the fixture doesn't exercise the actual per-host constructor call — noted and left as accepted scope for this pilot.

ChangesWorkbenchButtonBarWidget/ChangesMenuWorkbenchButtonBarWidget are driven by IMenuService-contributed menu items and several Changes-specific services (IChangesViewService, git-operation-in-progress state, etc.). Rendering through the real widgets in the fixture would mean standing up that whole menu-contribution surface just to cover a single-line constructor argument at each of the two call sites — disproportionate for a fixture whose purpose is visual regression of the button geometry, not menu wiring.

The pieces that are covered:

• buttonbar.test.ts unit-tests the actual CSS/computed-style mechanics behind 'compact'/'default' (this is the part a fixture screenshot can't verify).
• The two changesView.ts call sites are one line each and now reference shared named constants (changesButtonBarSpacing.ts) instead of literals, so a future PR reviewer sees a diff on the constant itself rather than a silently-duplicated string.

Wiring an automated guard that fails if a call site is switched to the wrong constant would need either full DI-driven fixture rendering or a source-level lint rule; both feel like broader test-infrastructure investment better suited to #335370 than this pilot.

[Cherry Wang (chryw)]

Pushed bbe2b19e4f7, addressing the "closer look" note more concretely than the prior accepted-scope reply:

• Replaced the two independently-imported CHANGES_HEADER_ICON_LABEL_SPACING / CHANGES_OUTSIDE_CARD_ICON_LABEL_SPACING constants with a single getChangesButtonBarIconLabelSpacing(host: 'header' | 'outside-card') helper.
• Both production call sites in changesView.ts and the fixture in changesButtonBar.fixture.ts now invoke the same function, passing only their own host identifier, instead of each independently selecting a spacing value.

This directly implements the suggested "host-to-spacing helper that both production and the fixture invoke." To be transparent about its limits: it does not add a structural guard against a production call site passing the wrong host string (that would still require rendering through the real ChangesWorkbenchButtonBarWidget/ChangesMenuWorkbenchButtonBarWidget instantiation, which needs IMenuService-contributed menu wiring plus several Changes-specific services — disproportionate for this visual-geometry fixture). What it does provide: the two call-site "header"/"outside-card" arguments are visible in the same small diff as any future host wiring change, and both consumers route through one mapping function rather than duplicating the compact/default choice.

Verified: 8/8 targeted WorkbenchButtonBar tests pass, both changesButtonBar fixtures (Dark/Light) render without errors, no diagnostics.

[Copilot]

🟡 Changes recommended

The Changes fixture still bypasses the production host-selection wiring it is intended to protect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced