From dcbb970c660a9b49d7f63bdaff9ce88204bae2de Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:13:34 -0700 Subject: [PATCH] fix(terminal): close the display:none fail-open in the inert-focus visibility guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The checkVisibility-less fallback read computed `visibility` off the container alone. `display` does not cascade to descendants in either engine, so a pane inside a Tailwind `hidden` wrapper — the real Terminal.tsx shape for a hidden worktree — was reported as rendered. That failed OPEN, toward claiming the chord, which is the destructive arm. Walk ancestors for display:none, and treat a container with no view as not rendered rather than as rendered. --- ...erminal-inert-focus-paste-fallback.test.ts | 138 ++++++++++++++++++ .../terminal-inert-focus-paste-fallback.ts | 21 ++- 2 files changed, 156 insertions(+), 3 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts index 6908447a7d4..06fed6e7d7c 100644 --- a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.test.ts @@ -252,6 +252,59 @@ describe('inert-focus paste fallback', () => { expect(onPasteKey).toHaveBeenCalledTimes(1) }) + // Coordinator finding, verified here at the fallback level: with several panes + // mounted, a pane that lost focus only because it was HIDDEN must not claim the + // chord. Real Tailwind `hidden` is display:none on a WRAPPER, and computed + // `display` does not cascade to descendants in either engine, so the check has + // to walk ancestors — reading `visibility` off the container alone fails OPEN. + it('does not claim inert focus when an ancestor wrapper is display:none', () => { + const wrapper = document.createElement('div') + document.body.append(wrapper) + wrapper.append(container) + const fallback = install() + inside.focus() + inside.blur() + expect(fallback.ownsInertFocus()).toBe(true) + + // Exactly what Terminal.tsx does to a hidden worktree: `absolute inset-0 hidden`. + wrapper.style.display = 'none' + + expect(fallback.ownsInertFocus()).toBe(false) + pressPasteChordOn(document.body) + expect(onPasteKey).not.toHaveBeenCalled() + }) + + // The defensive path has to fail CLOSED. A missed recovery is a dropped chord — + // the pre-PR behaviour, and the user can click and retry. A false claim pastes + // into an invisible terminal and consumes the chord app-wide, which they cannot + // undo. So an engine that can tell us nothing must not be read as "rendered". + it('refuses to claim when the container has no view to ask about visibility', () => { + const foreign = document.implementation.createHTMLDocument('detached') + const foreignContainer = foreign.createElement('div') + const foreignInside = foreign.createElement('textarea') + foreignContainer.append(foreignInside) + foreign.body.append(foreignContainer) + expect(foreignContainer.isConnected).toBe(true) + expect(foreignContainer.ownerDocument.defaultView).toBe(null) + + // Claim ownership at install time, the path the pane itself takes on remount. + foreignInside.focus() + expect(foreign.activeElement).toBe(foreignInside) + const recovered: KeyboardEvent[] = [] + const fallback = installInertFocusPasteFallback({ + container: foreignContainer, + documentTarget: foreign, + onPasteKey: (event) => recovered.push(event), + onPasteEvent: () => {} + }) + foreignInside.blur() + expect(foreign.activeElement).toBe(foreign.body) + + expect(fallback.ownsInertFocus()).toBe(false) + expect(recovered).toEqual([]) + fallback.dispose() + }) + it('stops recovering pastes after dispose', () => { const fallback = install() inside.focus() @@ -267,6 +320,91 @@ describe('inert-focus paste fallback', () => { }) }) +// Coordinator finding on #17020: `TerminalPane` is not a singleton, so several +// fallbacks are installed at the document at once. Two questions follow — can a +// hidden instance capture a chord aimed elsewhere, and can two instances both act +// on the same event? Real DOM and real dispatched events throughout. +describe('inert-focus paste fallback with several panes mounted', () => { + type Scene = { + panes: { container: HTMLElement; inside: HTMLTextAreaElement; keys: KeyboardEvent[] }[] + dispose: () => void + } + + const buildScene = (count: number): Scene => { + document.body.innerHTML = '' + const disposers: (() => void)[] = [] + const panes = Array.from({ length: count }, () => { + const wrapper = document.createElement('div') + const paneContainer = document.createElement('div') + const helper = document.createElement('textarea') + helper.className = 'xterm-helper-textarea' + paneContainer.append(helper) + wrapper.append(paneContainer) + document.body.append(wrapper) + const keys: KeyboardEvent[] = [] + const fallback = installInertFocusPasteFallback({ + container: paneContainer, + onPasteKey: (event) => keys.push(event), + onPasteEvent: () => {} + }) + disposers.push(fallback.dispose) + return { container: paneContainer, inside: helper, keys } + }) + return { panes, dispose: () => disposers.forEach((d) => d()) } + } + + const chord = (): void => { + document.body.dispatchEvent( + new KeyboardEvent('keydown', { key: 'v', ctrlKey: true, bubbles: true, cancelable: true }) + ) + } + + it('lets a hidden background pane capture nothing after a focus-less switch away', () => { + // The reported chain: pane A holds focus, the user switches by KEYBOARD (no + // pointerdown), A's wrapper is hidden so the browser blurs it to with + // NO focusin, and the destination surface takes no focus of its own. + const scene = buildScene(3) + scene.panes[0]!.inside.focus() + ;(scene.panes[0]!.container.parentElement as HTMLElement).style.display = 'none' + expect(document.activeElement).toBe(scene.panes[0]!.inside) + // happy-dom does not blur on hide, so blur explicitly: the browser's behaviour + // is the premise, not the thing under test. + scene.panes[0]!.inside.blur() + expect(document.activeElement).toBe(document.body) + + chord() + + expect(scene.panes.map((pane) => pane.keys.length)).toEqual([0, 0, 0]) + scene.dispose() + }) + + it('never lets two panes act on the same chord', () => { + const scene = buildScene(3) + scene.panes[0]!.inside.focus() + // A real focus move to another pane, which every instance sees at the document. + scene.panes[1]!.inside.focus() + scene.panes[1]!.inside.blur() + + chord() + + expect(scene.panes.map((pane) => pane.keys.length)).toEqual([0, 1, 0]) + scene.dispose() + }) + + it('leaves the chord to the app when no pane ever held focus', () => { + const scene = buildScene(3) + const elsewhere = document.createElement('input') + document.body.append(elsewhere) + elsewhere.focus() + elsewhere.blur() + + chord() + + expect(scene.panes.map((pane) => pane.keys.length)).toEqual([0, 0, 0]) + scene.dispose() + }) +}) + describe('restoreInertFocusPasteTarget', () => { const buildPane = (): { container: HTMLElement; terminal: { focus: Mock<() => void> } } => { document.body.innerHTML = '' diff --git a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts index 2b230b3ec91..447d46e7975 100644 --- a/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts +++ b/src/renderer/src/components/terminal-pane/terminal-inert-focus-paste-fallback.ts @@ -95,9 +95,24 @@ function isContainerRendered(container: HTMLElement): boolean { if (typeof checkVisibility === 'function') { return checkVisibility.call(container, { visibilityProperty: true }) } - // Fallback for engines without checkVisibility; `visibility` is inherited, so - // reading it off the container also catches a hidden ancestor. - return container.ownerDocument.defaultView?.getComputedStyle(container).visibility !== 'hidden' + // Why: only for engines without checkVisibility. `display` does not cascade to + // descendants, so reading it off the container alone would report a pane inside + // a `hidden` wrapper as rendered — failing open, toward claiming the chord. Walk + // the ancestors instead. `visibility` is inherited, so the container answers for + // itself. Bounded by tree depth and only reached on this defensive path. + const view = container.ownerDocument.defaultView + if (!view) { + return false + } + if (view.getComputedStyle(container).visibility === 'hidden') { + return false + } + for (let node: HTMLElement | null = container; node; node = node.parentElement) { + if (view.getComputedStyle(node).display === 'none') { + return false + } + } + return true } /** Focus is on but the pane still owns it logically; putting it back before