From 2842c31c9f095ecebae16a1ac1266a8f4c5ff804 Mon Sep 17 00:00:00 2001 From: safi Date: Thu, 20 Aug 2026 09:23:15 +0300 Subject: [PATCH 1/2] fix(core): render hovers in the document of their target element Hovers were always appended to the main window's document and positioned with viewport metrics of the main window, so tooltips for widgets moved to a secondary window appeared detached in the main window. Render and position the hover in the target's ownerDocument instead, listen for dismissing mousedown there, and guard hidePopover() against documents that are no longer fully active (secondary window closed while a hover is open). Also fall back to the perpendicular direction when a hover fits on neither side of its target (e.g. full-width items in a narrow secondary window), and clamp left/right hovers into the viewport as a last resort. --- .../core/src/browser/hover-service.spec.ts | 179 ++++++++++++++++++ packages/core/src/browser/hover-service.ts | 73 ++++++- 2 files changed, 243 insertions(+), 9 deletions(-) create mode 100644 packages/core/src/browser/hover-service.spec.ts diff --git a/packages/core/src/browser/hover-service.spec.ts b/packages/core/src/browser/hover-service.spec.ts new file mode 100644 index 0000000000000..dff3718ccb862 --- /dev/null +++ b/packages/core/src/browser/hover-service.spec.ts @@ -0,0 +1,179 @@ +// ***************************************************************************** +// Copyright (C) 2026 Safi Seid-Ahmad, K2view and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// http://www.eclipse.org/legal/epl-2.0. +// +// This Source Code may also be made available under the following Secondary +// Licenses when the conditions for such availability set forth in the Eclipse +// Public License v. 2.0 are satisfied: GNU General Public License, version 2 +// with the GNU Classpath Exception which is available at +// https://www.gnu.org/software/classpath/license.html. +// +// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 +// ***************************************************************************** + +import { enableJSDOM } from './test/jsdom'; +let disableJSDOM = enableJSDOM(); + +import { Container } from 'inversify'; +import { expect } from 'chai'; +import { HoverService } from './hover-service'; +import { PreferenceService } from '../common'; +import { CoreMarkdownRenderer } from './markdown-rendering/markdown-renderer'; +import { OpenerService } from './opener-service'; + +disableJSDOM(); + +describe('HoverService', () => { + let container: Container; + let hoverService: HoverService; + + before(() => { + disableJSDOM = enableJSDOM(); + // The hover service positions its host after waiting for an animation frame. + // JSDOM (without pretendToBeVisual) does not provide requestAnimationFrame. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (global as any).requestAnimationFrame = (cb: FrameRequestCallback) => setTimeout(cb, 0); + }); + + after(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (global as any).requestAnimationFrame; + disableJSDOM(); + }); + + beforeEach(() => { + container = new Container(); + container.bind(HoverService).toSelf().inSingletonScope(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + container.bind(PreferenceService).toConstantValue({ get: () => 0 } as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + container.bind(CoreMarkdownRenderer).toConstantValue({ render: () => ({ element: document.createElement('div'), dispose: () => { } }) } as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + container.bind(OpenerService).toConstantValue({} as any); + hoverService = container.get(HoverService); + stubPopoverApi(hoverService); + }); + + afterEach(() => { + hoverService.cancelHover(); + }); + + /** + * JSDOM implements neither the Popover API (showPopover/hidePopover) nor the + * `:popover-open` pseudo-class, so stub them on the service's host element. + */ + function stubPopoverApi(service: HoverService): void { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const host: HTMLElement = (service as any).hoverHost; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (host as any).showPopover = () => { }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (host as any).hidePopover = () => { }; + const originalMatches = host.matches.bind(host); + host.matches = (selectors: string) => selectors === ':popover-open' ? false : originalMatches(selectors); + } + + function waitForHover(): Promise { + // hover delay (0ms timeout) + animation frame (0ms timeout stub) + return new Promise(resolve => setTimeout(resolve, 20)); + } + + it('renders the hover in the document of the target element', async () => { + const target = document.createElement('div'); + document.body.appendChild(target); + hoverService.requestHover({ content: 'main window hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(document.querySelector('.theia-hover'), 'hover should be in the main document').to.exist; + target.remove(); + }); + + it('renders the hover in a secondary window document if the target lives there', async () => { + const secondaryDocument = document.implementation.createHTMLDocument('secondary window'); + const target = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(target); + hoverService.requestHover({ content: 'secondary window hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(document.querySelector('.theia-hover'), 'hover should not be in the main document').to.not.exist; + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be in the secondary document').to.exist; + target.remove(); + }); + + describe('position fallback', () => { + // simulated window: 400px wide, 600px high + const windowWidth = 400; + const windowHeight = 600; + let target: HTMLElement; + let originalBodyRect: () => DOMRect; + + function rect(left: number, top: number, width: number, height: number): DOMRect { + return { left, top, width, height, right: left + width, bottom: top + height, x: left, y: top, toJSON: () => '' }; + } + + beforeEach(() => { + target = document.createElement('div'); + document.body.appendChild(target); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const host: HTMLElement = (hoverService as any).hoverHost; + host.getBoundingClientRect = () => rect(0, 0, 300, 50); + originalBodyRect = document.body.getBoundingClientRect.bind(document.body); + document.body.getBoundingClientRect = () => rect(0, 0, windowWidth, windowHeight); + Object.defineProperty(document.documentElement, 'scrollHeight', { value: windowHeight, configurable: true }); + }); + + afterEach(() => { + document.body.getBoundingClientRect = originalBodyRect; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (document.documentElement as any).scrollHeight; + target.remove(); + }); + + function setHostPosition(position: 'left' | 'right' | 'top' | 'bottom'): string { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const service = hoverService as any; + return service.setHostPosition(target, service.hoverHost, position); + } + + it('keeps the requested position when the hover fits', () => { + target.getBoundingClientRect = () => rect(320, 100, 60, 20); // plenty of room on the left + expect(setHostPosition('left')).to.equal('left'); + }); + + it('falls back to bottom when a left hover fits on neither side of a full-width target', () => { + target.getBoundingClientRect = () => rect(0, 100, windowWidth, 20); + expect(setHostPosition('left')).to.equal('bottom'); + }); + + it('falls back to top when the full-width target is near the bottom of the window', () => { + target.getBoundingClientRect = () => rect(0, windowHeight - 30, windowWidth, 20); + expect(setHostPosition('right')).to.equal('top'); + }); + + it('keeps the requested direction when the perpendicular direction does not fit either', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const host: HTMLElement = (hoverService as any).hoverHost; + host.getBoundingClientRect = () => rect(0, 0, 300, windowHeight); // hover as tall as the window + target.getBoundingClientRect = () => rect(0, 100, windowWidth, 20); + expect(setHostPosition('left')).to.equal('right'); + }); + }); + + it('recovers if the document hosting an open hover is no longer active', async () => { + // simulate a hover host left popover-open in a closed secondary window's document: + // hidePopover then throws 'InvalidStateError' and must not break subsequent hovers + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const host: HTMLElement = (hoverService as any).hoverHost; + host.matches = (selectors: string) => selectors === ':popover-open'; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (host as any).hidePopover = () => { throw new Error('InvalidStateError: not fully active'); }; + const target = document.createElement('div'); + document.body.appendChild(target); + hoverService.requestHover({ content: 'after window close', target, position: 'right', skipHoverDelay: true }); + stubPopoverApi(hoverService); // restore working popover stubs for the new hover + await waitForHover(); + expect(document.querySelector('.theia-hover'), 'hover should be rendered again in the main document').to.exist; + target.remove(); + }); +}); diff --git a/packages/core/src/browser/hover-service.ts b/packages/core/src/browser/hover-service.ts index 35fac189590ed..6fa68171cb5c0 100644 --- a/packages/core/src/browser/hover-service.ts +++ b/packages/core/src/browser/hover-service.ts @@ -51,6 +51,22 @@ export namespace HoverPosition { } return position; } + + /** + * Tests whether a hover of the given dimensions fits next to its target in the given + * position without extending beyond the window bounds. + */ + export function fits(position: HoverPosition, target: DOMRect, host: DOMRect, totalWidth: number, totalHeight: number): boolean { + if (position === 'left') { + return target.left - host.width - 5 >= 0; + } else if (position === 'right') { + return target.right + host.width + 5 <= totalWidth; + } else if (position === 'top') { + return target.top - host.height - 5 >= 0; + } else { + return target.bottom + host.height + 5 <= totalHeight; + } + } } export interface HoverRequest { @@ -163,7 +179,9 @@ export class HoverService { // handler then cancels the hover, and the cycle repeats: the tooltip flickers and never settles. // `visibility: hidden` still lays the host out (so it can be measured) but is not hit-tested. host.style.visibility = 'hidden'; - document.body.append(host); + // Render the hover in the document of the target: it may be hosted in a secondary window, + // whose coordinate space is unrelated to the main window's. + target.ownerDocument.body.append(host); if (!host.matches(':popover-open')) { host.showPopover(); } @@ -188,7 +206,7 @@ export class HoverService { } } - await animationFrame(); // Allow the browser to size the host + await this.hostAnimationFrame(target); // Allow the browser to size the host const updatedPosition = this.setHostPosition(target, host, position); // Reveal the host only once it sits at its final position, so it never overlaps the target while // parked at (0,0). Dropping the declaration rather than assigning a value hands `visibility` back @@ -206,14 +224,40 @@ export class HoverService { }); } + /** + * Waits for an animation frame in the window hosting the given target element, + * which may be a secondary window whose rendering is independent of the main window's. + */ + protected hostAnimationFrame(target: HTMLElement): Promise { + const targetWindow = target.ownerDocument.defaultView; + if (!targetWindow || targetWindow === window) { + return animationFrame(); + } + return new Promise(resolve => targetWindow.requestAnimationFrame(() => resolve())); + } + protected setHostPosition(target: HTMLElement, host: HTMLElement, position: HoverPosition): HoverPosition { + const hostDocument = target.ownerDocument; const targetDimensions = target.getBoundingClientRect(); const hostDimensions = host.getBoundingClientRect(); - const documentWidth = document.body.getBoundingClientRect().width; + const documentWidth = hostDocument.body.getBoundingClientRect().width; // document.body.getBoundingClientRect().height doesn't work as expected // scrollHeight will always be accurate here: https://stackoverflow.com/a/44077777 - const documentHeight = document.documentElement.scrollHeight; + const documentHeight = hostDocument.documentElement.scrollHeight; position = HoverPosition.invertIfNecessary(position, targetDimensions, hostDimensions, documentWidth, documentHeight); + if (!HoverPosition.fits(position, targetDimensions, hostDimensions, documentWidth, documentHeight)) { + // The hover fits on neither side of the target in the requested direction, e.g. when the + // target spans the full width of a narrow secondary window. Try the perpendicular direction + // so that the target and the hover both remain visible. If that does not fit either, + // keep the requested direction; the clamping below keeps the hover inside the viewport. + const fallback = HoverPosition.invertIfNecessary( + position === 'left' || position === 'right' ? 'bottom' : 'right', + targetDimensions, hostDimensions, documentWidth, documentHeight + ); + if (HoverPosition.fits(fallback, targetDimensions, hostDimensions, documentWidth, documentHeight)) { + position = fallback; + } + } if (position === 'top' || position === 'bottom') { const targetMiddleWidth = targetDimensions.left + (targetDimensions.width / 2); const middleAlignment = targetMiddleWidth - (hostDimensions.width / 2); @@ -230,9 +274,13 @@ export class HoverService { const middleAlignment = targetMiddleHeight - (hostDimensions.height / 2); const furthestTop = Math.min(documentHeight - hostDimensions.height, middleAlignment); const top = Math.max(0, furthestTop); - const left = position === 'left' + const desiredLeft = position === 'left' ? targetDimensions.left - hostDimensions.width - 5 : targetDimensions.right + 5; + // the hover may not fit on either side of the target, e.g. when the target + // spans the full width of a narrow (secondary) window: keep it in the viewport + const furthestRight = Math.min(documentWidth - hostDimensions.width, desiredLeft); + const left = Math.max(0, furthestRight); host.style.setProperty('--theia-hover-before-position', `${targetMiddleHeight - top - 5}px`); host.style.left = `${left}px`; host.style.top = `${top}px`; @@ -280,13 +328,20 @@ export class HoverService { this.cancelHover(); } }; - document.addEventListener('mousedown', handleMouseDown, true); - this.disposeOnHide.push({ dispose: () => document.removeEventListener('mousedown', handleMouseDown, true) }); + // Listen in the document of the target, which may be hosted in a secondary window + const targetDocument = request.target.ownerDocument; + targetDocument.addEventListener('mousedown', handleMouseDown, true); + this.disposeOnHide.push({ dispose: () => targetDocument.removeEventListener('mousedown', handleMouseDown, true) }); } protected unRenderHover(): void { - if (this.hoverHost.matches(':popover-open')) { - this.hoverHost.hidePopover(); + try { + if (this.hoverHost.matches(':popover-open')) { + this.hoverHost.hidePopover(); + } + } catch { + // hidePopover throws for popovers in documents that are no longer fully active, + // e.g. when the secondary window hosting the hover has been closed in the meantime } this.hoverHost.remove(); this.hoverHost.replaceChildren(); From b99a079cf2079a59da0f7773b2e573889f70070e Mon Sep 17 00:00:00 2001 From: safi Date: Sun, 23 Aug 2026 12:58:05 +0300 Subject: [PATCH 2/2] fix(core): survive secondary window close and dismiss hovers reliably - create the hover host in the document it is shown in and never adopt it across documents; cancel the hover on pagehide of the hosting window and guard against closed windows, so hovers no longer break (or crash the Electron renderer) after closing a secondary window with an open hover - resolve the dismissal listeners and unRenderHover against the host of the current hover instead of recreating a host for the main document, so hovers in a secondary window are dismissed on mouse-out and mousedown instead of piling up - do not let a superseded render reposition, reveal, or leak css classes into the hover that replaced it --- .../core/src/browser/hover-service.spec.ts | 274 +++++++++++++++--- packages/core/src/browser/hover-service.ts | 92 +++++- 2 files changed, 313 insertions(+), 53 deletions(-) diff --git a/packages/core/src/browser/hover-service.spec.ts b/packages/core/src/browser/hover-service.spec.ts index dff3718ccb862..648b0cc47cdbc 100644 --- a/packages/core/src/browser/hover-service.spec.ts +++ b/packages/core/src/browser/hover-service.spec.ts @@ -26,20 +26,34 @@ import { OpenerService } from './opener-service'; disableJSDOM(); +/* eslint-disable @typescript-eslint/no-explicit-any */ + describe('HoverService', () => { let container: Container; let hoverService: HoverService; + let originalMatches: (selectors: string) => boolean; before(() => { disableJSDOM = enableJSDOM(); // The hover service positions its host after waiting for an animation frame. // JSDOM (without pretendToBeVisual) does not provide requestAnimationFrame. - // eslint-disable-next-line @typescript-eslint/no-explicit-any (global as any).requestAnimationFrame = (cb: FrameRequestCallback) => setTimeout(cb, 0); + // JSDOM implements neither the Popover API (showPopover/hidePopover) nor the + // `:popover-open` pseudo-class: stub them, tracking the open state in an attribute. + const elementPrototype = window.HTMLElement.prototype as any; + elementPrototype.showPopover = function (this: HTMLElement): void { this.setAttribute('data-test-popover-open', 'true'); }; + elementPrototype.hidePopover = function (this: HTMLElement): void { this.removeAttribute('data-test-popover-open'); }; + originalMatches = elementPrototype.matches; + elementPrototype.matches = function (this: HTMLElement, selectors: string): boolean { + return selectors === ':popover-open' ? this.hasAttribute('data-test-popover-open') : originalMatches.call(this, selectors); + }; }); after(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any + const elementPrototype = window.HTMLElement.prototype as any; + delete elementPrototype.showPopover; + delete elementPrototype.hidePopover; + elementPrototype.matches = originalMatches; delete (global as any).requestAnimationFrame; disableJSDOM(); }); @@ -47,40 +61,62 @@ describe('HoverService', () => { beforeEach(() => { container = new Container(); container.bind(HoverService).toSelf().inSingletonScope(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any container.bind(PreferenceService).toConstantValue({ get: () => 0 } as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any container.bind(CoreMarkdownRenderer).toConstantValue({ render: () => ({ element: document.createElement('div'), dispose: () => { } }) } as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any container.bind(OpenerService).toConstantValue({} as any); hoverService = container.get(HoverService); - stubPopoverApi(hoverService); }); afterEach(() => { hoverService.cancelHover(); }); - /** - * JSDOM implements neither the Popover API (showPopover/hidePopover) nor the - * `:popover-open` pseudo-class, so stub them on the service's host element. - */ - function stubPopoverApi(service: HoverService): void { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const host: HTMLElement = (service as any).hoverHost; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (host as any).showPopover = () => { }; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (host as any).hidePopover = () => { }; - const originalMatches = host.matches.bind(host); - host.matches = (selectors: string) => selectors === ':popover-open' ? false : originalMatches(selectors); - } - function waitForHover(): Promise { // hover delay (0ms timeout) + animation frame (0ms timeout stub) return new Promise(resolve => setTimeout(resolve, 20)); } + function waitForMouseOutDismissal(): Promise { + // the mouse-out handler re-checks the hover state after quickMouseThresholdMillis (200ms) + return new Promise(resolve => setTimeout(resolve, 250)); + } + + interface FakeSecondaryWindow { + secondaryDocument: Document; + fireEvent(type: string): void; + } + + /** + * Creates a document simulating one hosted in a secondary window: unlike a document from + * `createHTMLDocument`, it has a `defaultView` window on which the hover service can listen + * for the window going away. + */ + function createSecondaryWindowDocument(options?: { closed?: boolean }): FakeSecondaryWindow { + const secondaryDocument = document.implementation.createHTMLDocument('secondary window'); + const listeners = new Map(); + const fakeWindow = { + closed: options?.closed ?? false, + requestAnimationFrame: (cb: FrameRequestCallback) => setTimeout(cb, 0), + addEventListener: (type: string, listener: EventListener) => { + const forType = listeners.get(type) ?? []; + forType.push(listener); + listeners.set(type, forType); + }, + removeEventListener: (type: string, listener: EventListener) => { + const forType = listeners.get(type); + const index = forType?.indexOf(listener) ?? -1; + if (forType && index > -1) { + forType.splice(index, 1); + } + } + }; + Object.defineProperty(secondaryDocument, 'defaultView', { value: fakeWindow, configurable: true }); + return { + secondaryDocument, + fireEvent: type => [...(listeners.get(type) ?? [])].forEach(listener => listener({ type } as Event)) + }; + } + it('renders the hover in the document of the target element', async () => { const target = document.createElement('div'); document.body.appendChild(target); @@ -91,7 +127,7 @@ describe('HoverService', () => { }); it('renders the hover in a secondary window document if the target lives there', async () => { - const secondaryDocument = document.implementation.createHTMLDocument('secondary window'); + const { secondaryDocument } = createSecondaryWindowDocument(); const target = secondaryDocument.createElement('div'); secondaryDocument.body.appendChild(target); hoverService.requestHover({ content: 'secondary window hover', target, position: 'right', skipHoverDelay: true }); @@ -101,6 +137,179 @@ describe('HoverService', () => { target.remove(); }); + it('creates the hover host in the document of the target instead of adopting it across documents', async () => { + const mainTarget = document.createElement('div'); + document.body.appendChild(mainTarget); + hoverService.requestHover({ content: 'main', target: mainTarget, position: 'right', skipHoverDelay: true }); + await waitForHover(); + const mainHost = document.querySelector('.theia-hover'); + expect(mainHost, 'hover should be in the main document').to.exist; + + const { secondaryDocument } = createSecondaryWindowDocument(); + const secondaryTarget = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(secondaryTarget); + hoverService.requestHover({ content: 'secondary', target: secondaryTarget, position: 'right', skipHoverDelay: true }); + await waitForHover(); + const secondaryHost = secondaryDocument.querySelector('.theia-hover'); + expect(secondaryHost, 'hover should be in the secondary document').to.exist; + // moving a host into another document would make it outlive its window; a host must be + // created in the document it is shown in + expect(secondaryHost, 'the secondary host must not be the adopted main host').to.not.equal(mainHost); + expect(secondaryHost!.ownerDocument).to.equal(secondaryDocument); + mainTarget.remove(); + secondaryTarget.remove(); + }); + + it('cancels the hover when the window hosting it is closed', async () => { + const { secondaryDocument, fireEvent } = createSecondaryWindowDocument(); + const target = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(target); + hoverService.requestHover({ content: 'secondary window hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be in the secondary document').to.exist; + + fireEvent('pagehide'); + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be removed when its window closes').to.not.exist; + + // hovers in the main window must keep working afterwards + const mainTarget = document.createElement('div'); + document.body.appendChild(mainTarget); + hoverService.requestHover({ content: 'after window close', target: mainTarget, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(document.querySelector('.theia-hover'), 'hover should be rendered in the main document afterwards').to.exist; + target.remove(); + mainTarget.remove(); + }); + + it('dismisses the hover when the pointer leaves its target in a secondary window', async () => { + const { secondaryDocument } = createSecondaryWindowDocument(); + const target = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(target); + hoverService.requestHover({ content: 'secondary window hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be shown in the secondary document').to.exist; + + target.dispatchEvent(new window.Event('mouseout')); + await waitForMouseOutDismissal(); + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be dismissed after the pointer left its target').to.not.exist; + target.remove(); + }); + + it('dismisses a non-interactive hover on mousedown in a secondary window', async () => { + const { secondaryDocument } = createSecondaryWindowDocument(); + const target = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(target); + hoverService.requestHover({ content: 'secondary window hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be shown in the secondary document').to.exist; + + secondaryDocument.body.dispatchEvent(new window.Event('mousedown')); + expect(secondaryDocument.querySelector('.theia-hover'), 'hover should be dismissed on mousedown outside of it').to.not.exist; + target.remove(); + }); + + it('shows at most one hover box in a secondary window across repeated hovers', async () => { + const { secondaryDocument } = createSecondaryWindowDocument(); + const target = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(target); + for (let i = 0; i < 2; i++) { + hoverService.requestHover({ content: `hover ${i}`, target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + target.dispatchEvent(new window.Event('mouseout')); + await waitForMouseOutDismissal(); + } + hoverService.requestHover({ content: 'final hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(secondaryDocument.querySelectorAll('.theia-hover').length, 'stale hover hosts must not pile up').to.equal(1); + target.remove(); + }); + + it('does not render a hover for a target in an already closed window', async () => { + const { secondaryDocument } = createSecondaryWindowDocument({ closed: true }); + const target = secondaryDocument.createElement('div'); + secondaryDocument.body.appendChild(target); + hoverService.requestHover({ content: 'closed window hover', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + expect(secondaryDocument.querySelector('.theia-hover'), 'no hover should be rendered in a closed window').to.not.exist; + expect(document.querySelector('.theia-hover'), 'no hover should be rendered in the main document either').to.not.exist; + target.remove(); + }); + + it('keeps the hover host hidden until it has been positioned', async () => { + // the host is appended (and the popover shown) at (0, 0) first and only positioned after an + // animation frame; it must not be hittable in the meantime: a visible popover at (0, 0) can + // cover the target, kick it out of the hover chain and retrigger mouseenter hovers in an + // endless show/hide loop (e.g. for tabs at the top-left corner of a secondary window) + const target = document.createElement('div'); + document.body.appendChild(target); + const rendering = (hoverService as any).renderHover({ content: 'positioning', target, position: 'right' }) as Promise; + const host = document.querySelector('.theia-hover') as HTMLElement; + expect(host, 'hover should be appended synchronously').to.exist; + expect(host.style.visibility, 'hover must not be visible before it has been positioned').to.equal('hidden'); + await rendering; + expect(host.style.visibility, 'hover should be revealed once positioned').to.not.equal('hidden'); + target.remove(); + }); + + it('does not reveal a hover that was superseded while waiting to be positioned', async () => { + const target = document.createElement('div'); + document.body.appendChild(target); + const service = hoverService as any; + const first = service.renderHover({ content: 'first', target, position: 'right' }) as Promise; + const second = service.renderHover({ content: 'second', target, position: 'right' }) as Promise; + await first; + const host = document.querySelector('.theia-hover') as HTMLElement; + expect(host.style.visibility, 'the superseded render must not reveal the host').to.equal('hidden'); + await second; + expect(host.style.visibility, 'the latest render reveals the host').to.not.equal('hidden'); + target.remove(); + }); + + it('does not leak css classes from a hover that was superseded while waiting to be positioned', async () => { + const service = hoverService as any; + // keep the first render stuck waiting for its animation frame so that a second hover supersedes it mid-render + const originalAnimationFrame = service.hostAnimationFrame.bind(service); + let releaseFirst: () => void; + let animationFrameCalls = 0; + service.hostAnimationFrame = (element: HTMLElement) => ++animationFrameCalls === 1 + ? new Promise(resolve => { releaseFirst = resolve; }) + : originalAnimationFrame(element); + const target = document.createElement('div'); + document.body.appendChild(target); + hoverService.requestHover({ content: 'first', target, position: 'right', skipHoverDelay: true, cssClasses: ['first-hover-class'] }); + await waitForHover(); + hoverService.requestHover({ content: 'second', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + releaseFirst!(); // let the superseded render finish + await waitForHover(); + const host = document.querySelector('.theia-hover'); + expect(host, 'second hover should be rendered').to.exist; + expect(host!.classList.contains('first-hover-class'), 'the superseded hover must not leak its css classes').to.equal(false); + target.remove(); + }); + + it('recovers if the open hover can no longer be hidden', async () => { + // simulate a hover whose document is no longer fully active, e.g. because the secondary + // window hosting it was closed: hidePopover throws and must not break subsequent hovers + const target = document.createElement('div'); + document.body.appendChild(target); + hoverService.requestHover({ content: 'first', target, position: 'right', skipHoverDelay: true }); + await waitForHover(); + const host = document.querySelector('.theia-hover') as HTMLElement; + expect(host, 'first hover should be rendered').to.exist; + (host as any).hidePopover = () => { throw new Error('InvalidStateError: not fully active'); }; + + const secondTarget = document.createElement('div'); + document.body.appendChild(secondTarget); + hoverService.requestHover({ content: 'second', target: secondTarget, position: 'right', skipHoverDelay: true }); + await waitForHover(); + const secondHost = document.querySelector('.theia-hover'); + expect(secondHost, 'hover should be rendered again in the main document').to.exist; + expect(secondHost!.textContent).to.equal('second'); + target.remove(); + secondTarget.remove(); + }); + describe('position fallback', () => { // simulated window: 400px wide, 600px high const windowWidth = 400; @@ -115,7 +324,6 @@ describe('HoverService', () => { beforeEach(() => { target = document.createElement('div'); document.body.appendChild(target); - // eslint-disable-next-line @typescript-eslint/no-explicit-any const host: HTMLElement = (hoverService as any).hoverHost; host.getBoundingClientRect = () => rect(0, 0, 300, 50); originalBodyRect = document.body.getBoundingClientRect.bind(document.body); @@ -125,13 +333,11 @@ describe('HoverService', () => { afterEach(() => { document.body.getBoundingClientRect = originalBodyRect; - // eslint-disable-next-line @typescript-eslint/no-explicit-any delete (document.documentElement as any).scrollHeight; target.remove(); }); function setHostPosition(position: 'left' | 'right' | 'top' | 'bottom'): string { - // eslint-disable-next-line @typescript-eslint/no-explicit-any const service = hoverService as any; return service.setHostPosition(target, service.hoverHost, position); } @@ -152,28 +358,10 @@ describe('HoverService', () => { }); it('keeps the requested direction when the perpendicular direction does not fit either', () => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any const host: HTMLElement = (hoverService as any).hoverHost; host.getBoundingClientRect = () => rect(0, 0, 300, windowHeight); // hover as tall as the window target.getBoundingClientRect = () => rect(0, 100, windowWidth, 20); expect(setHostPosition('left')).to.equal('right'); }); }); - - it('recovers if the document hosting an open hover is no longer active', async () => { - // simulate a hover host left popover-open in a closed secondary window's document: - // hidePopover then throws 'InvalidStateError' and must not break subsequent hovers - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const host: HTMLElement = (hoverService as any).hoverHost; - host.matches = (selectors: string) => selectors === ':popover-open'; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (host as any).hidePopover = () => { throw new Error('InvalidStateError: not fully active'); }; - const target = document.createElement('div'); - document.body.appendChild(target); - hoverService.requestHover({ content: 'after window close', target, position: 'right', skipHoverDelay: true }); - stubPopoverApi(hoverService); // restore working popover stubs for the new hover - await waitForHover(); - expect(document.querySelector('.theia-hover'), 'hover should be rendered again in the main document').to.exist; - target.remove(); - }); }); diff --git a/packages/core/src/browser/hover-service.ts b/packages/core/src/browser/hover-service.ts index 6fa68171cb5c0..9e15b1ecca4c9 100644 --- a/packages/core/src/browser/hover-service.ts +++ b/packages/core/src/browser/hover-service.ts @@ -118,9 +118,25 @@ export class HoverService { @inject(OpenerService) protected readonly openerService: OpenerService; protected _hoverHost: HTMLElement | undefined; + /** + * The host of the current hover, which may live in a secondary window's document. + * Resolving against the main document here instead would silently replace the host + * whenever the current hover lives in another document, detaching the dismissal + * listeners and `unRenderHover` from the host that is actually rendered. + */ protected get hoverHost(): HTMLElement { - if (!this._hoverHost) { - this._hoverHost = document.createElement('div'); + return this._hoverHost ?? this.getOrCreateHoverHost(document); + } + + /** + * Returns the host element to render hovers into for the given document, creating it if the + * current one belongs to a different document. A host is always created in the document it is + * shown in: adopting a host into another document would let it outlive its window, and touching + * it after that window was closed breaks (and in Electron crashes) the application. + */ + protected getOrCreateHoverHost(targetDocument: Document): HTMLElement { + if (!this._hoverHost || this._hoverHost.ownerDocument !== targetDocument) { + this._hoverHost = targetDocument.createElement('div'); this._hoverHost.classList.add(HoverService.hostClassName); this._hoverHost.style.position = 'absolute'; this._hoverHost.setAttribute('popover', 'hint'); @@ -129,18 +145,44 @@ export class HoverService { } protected pendingTimeout: Disposable | undefined; protected hoverTarget: HTMLElement | undefined; + /** Identifies the latest render so that superseded renders can detect they are stale. */ + protected renderSequence = 0; protected lastHidHover = Date.now(); protected readonly disposeOnHide = new DisposableCollection(); requestHover(request: HoverRequest): void { this.cancelHover(); + const targetWindow = request.target.ownerDocument.defaultView; + if (!targetWindow || targetWindow.closed) { + // the window hosting the target is already gone, e.g. a closed secondary window: + // its document must not be touched anymore + return; + } const delay = request.skipHoverDelay ? 0 : this.getHoverDelay(); this.pendingTimeout = disposableTimeout(() => this.renderHover(request), delay); this.hoverTarget = request.target; + // resolve the host for the target's document up front so that the listeners below attach to the host that will be rendered + this.getOrCreateHoverHost(request.target.ownerDocument); + this.listenForWindowClose(request.target); this.listenForMouseOut(); this.listenForMouseClick(request); } + /** + * Cancels the hover when the window hosting the target element goes away, e.g. when the + * secondary window containing the target is closed: neither the hover host nor any listeners + * may outlive the document they belong to. + */ + protected listenForWindowClose(target: HTMLElement): void { + const targetWindow = target.ownerDocument.defaultView; + if (!targetWindow || targetWindow === window) { + return; + } + const handlePageHide = () => this.cancelHover(); + targetWindow.addEventListener('pagehide', handlePageHide); + this.disposeOnHide.push({ dispose: () => targetWindow.removeEventListener('pagehide', handlePageHide) }); + } + protected getHoverDelay(): number { return Date.now() - this.lastHidHover < quickMouseThresholdMillis ? 0 @@ -148,9 +190,16 @@ export class HoverService { } protected async renderHover(request: HoverRequest): Promise { - const host = this.hoverHost; - let firstChild: HTMLElement | undefined; const { target, content, position, cssClasses, interactive, onHide } = request; + const targetWindow = target.ownerDocument.defaultView; + if (!targetWindow || targetWindow.closed) { + // the window hosting the target is already gone, e.g. a closed secondary window: + // its document must not be touched anymore + return; + } + const host = this.getOrCreateHoverHost(target.ownerDocument); + const sequence = ++this.renderSequence; + let firstChild: HTMLElement | undefined; if (onHide) { this.disposeOnHide.push({ dispose: onHide.bind(request) }); } @@ -207,6 +256,11 @@ export class HoverService { } await this.hostAnimationFrame(target); // Allow the browser to size the host + if (sequence !== this.renderSequence || !host.isConnected) { + // this hover was cancelled or superseded by a newer one while waiting for the animation + // frame: it must neither reposition nor reveal the host + return; + } const updatedPosition = this.setHostPosition(target, host, position); // Reveal the host only once it sits at its final position, so it never overlaps the target while // parked at (0,0). Dropping the declaration rather than assigning a value hands `visibility` back @@ -335,18 +389,36 @@ export class HoverService { } protected unRenderHover(): void { + const host = this._hoverHost; + if (!host) { + return; + } + const hostWindow = host.ownerDocument.defaultView; + if (!hostWindow || hostWindow.closed) { + // the window hosting the hover is already gone, e.g. a closed secondary window: + // its DOM must not be touched anymore; abandon the host and start from scratch + this._hoverHost = undefined; + return; + } try { - if (this.hoverHost.matches(':popover-open')) { - this.hoverHost.hidePopover(); + if (host.matches(':popover-open')) { + host.hidePopover(); } } catch { // hidePopover throws for popovers in documents that are no longer fully active, - // e.g. when the secondary window hosting the hover has been closed in the meantime + // e.g. while the secondary window hosting the hover is being closed } - this.hoverHost.remove(); - this.hoverHost.replaceChildren(); + host.remove(); + host.replaceChildren(); + // drop the classes added for the rendered hover (position and request classes): a render + // aborted because it was superseded must not leak its classes into the next hover + host.className = HoverService.hostClassName; // The host is reused across hovers; drop the transient hidden state set during measurement so a // hover cancelled before it was revealed does not leave the next one invisible. - this.hoverHost.style.removeProperty('visibility'); + host.style.removeProperty('visibility'); + if (host.ownerDocument !== document) { + // never keep a host from another document: it must not outlive its window + this._hoverHost = undefined; + } } }