Skip to content

[cherry-pick] Fix tab close button stuck showing "Close Others" after focus changes - #334829

Open
vs-code-engineering[bot] wants to merge 1 commit into
release/1.136from
cherry-pick/334515
Open

[cherry-pick] Fix tab close button stuck showing "Close Others" after focus changes#334829
vs-code-engineering[bot] wants to merge 1 commit into
release/1.136from
cherry-pick/334515

Conversation

@vs-code-engineering

Copy link
Copy Markdown
Contributor

Cherry-pick of #334515 from main.

Description

Alt+hover swaps a tab's Close action for Close Others. Both inputs to that swap could go stale across a focus change: redrawTabAction() read modifierKeyEmitter.keyStatus.altKey at paint time (the Alt keyup is delivered to whichever app you Alt+Tab'd into), and hoveredTabIndex was only cleared by a real mouseleave. Coming back to the window without moving the mouse could therefore leave the x armed as "Close Others", so a plain click silently closed every other editor in the group.

Changes — MultiEditorTabsControl

  • Local Alt state. A isAltPressed field, fed by the ModifierKeyEmitter event, replaces the two on-demand reads of modifierKeyEmitter.keyStatus.altKey. It is the single source of truth for the swap and redraws the hovered tab when it flips.
  • Give up the swap on focus loss. IHostService.onDidChangeFocus(false) clears both the Alt state and the hovered tab, so a hover left over from before the focus change cannot be re-armed by Alt state re-established on return. Re-hovering a tab arms it again as usual.
  • Re-validate Alt from mouse events. mousemove/mousedown on the tabs container carry the real modifier state even when a keyup was missed entirely — this covers macOS Mission Control, where the window may never see a blur.

The mouse listeners are registered in the capture phase because BaseActionViewItem calls EventHelper.stop(e, true) on mousedown; a bubble-phase listener never sees a press on the close button itself, which is precisely the reported "click without moving the mouse" case.

for (const eventType of [EventType.MOUSE_MOVE, EventType.MOUSE_DOWN]) {
	this._register(addDisposableListener(tabsContainer, eventType, (e: MouseEvent) => {
		this.setAltPressed(e.altKey);
	}, true));
}

In the stale-Alt case the capture handler swaps the action item mid-mousedown, so that first click is swallowed rather than executing "Close Others" — the icon is then correct and the next click closes the single tab.

EditorTabsControl.hostService becomes protected so the subclass can use it.

Tests

src/vs/workbench/test/browser/parts/editor/multiEditorTabsControl.test.ts covers the baseline swap plus the focus-loss, mouse-move and action-mousedown paths; each regression test was checked to fail with its corresponding part of the change reverted.

Notes for reviewers

  • After returning from another application the swap requires a fresh hover (move to another tab, or leave and re-enter). This is the behaviour suggested in the issue and is what makes a stale hover unexploitable.
  • menuEntryActionViewItem.ts has the same latent staleness pattern; left untouched to keep this scoped to tabs.

Copilot AI balanced review requested due to automatic review settings September 6, 2026 21:15
@vs-code-engineering vs-code-engineering Bot added the cherry-pick-artifact Auto-generated cherry-pick PR label Sep 6, 2026
@vs-code-engineering

Copy link
Copy Markdown
Contributor Author

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

Benjamin Christopher Simmonds (@benibenj)

Matched files:

  • src/vs/workbench/browser/parts/editor/editorTabsControl.ts
  • src/vs/workbench/browser/parts/editor/multiEditorTabsControl.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Test setup must reset existing modifier-key state to prevent order-dependent failures.

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

Pull request overview

Fixes stale Close Others tab actions after focus changes.

Changes:

  • Tracks and revalidates local Alt/hover state.
  • Clears stale state on focus loss.
  • Adds regression tests and singleton lifecycle handling.
File summaries
File Description
src/vs/workbench/test/browser/parts/editor/multiEditorTabsControl.test.ts Adds regression coverage, but setup does not reset pre-existing singleton Alt state, making tests order-dependent.
src/vs/workbench/browser/parts/editor/multiEditorTabsControl.ts Prevents stale Alt-hover tab actions.
src/vs/workbench/browser/parts/editor/editorTabsControl.ts Exposes the host service to subclasses.
src/vs/base/browser/dom.ts Exempts singleton modifier subscriptions from leak detection.
Review details

Suppressed comments (2)

src/vs/workbench/test/browser/parts/editor/multiEditorTabsControl.test.ts:151

  • Resetting the shared emitter immediately after setFocus(false) masks whether the new focus handler actually clears its local Alt state. This test still passes if setAltPressed(false) is removed: clearing the hovered index redraws the icon, then this reset updates the local state. Re-hover before resetting and assert that the action remains Close, then clear that synthetic hover before testing focus return.
		hostService.setFocus(false);
		ModifierKeyEmitter.getInstance().resetKeyStatus();

src/vs/workbench/test/browser/parts/editor/multiEditorTabsControl.test.ts:195

  • This regression test stops at mousedown, but BaseActionViewItem executes the action from its click handler (src/vs/base/browser/ui/actionbar/actionViewItems.ts:152-158). It therefore verifies the icon swap but not the critical guarantee that the stale Close Others action cannot run. Exercise the complete click path and assert that no other editor was closed (or that only the target closes on the next click).
		mouseDownOnTabAction(0, false);
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// The tabs control resolves the shared modifier key emitter on creation,
// so dispose it again to keep each test independent of the Alt state that
// other suites may have left behind
disposables.add(toDisposable(() => ModifierKeyEmitter.disposeInstance()));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick-artifact Auto-generated cherry-pick PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants