From 85a957d8d086e521ef82499b00528516b6dc2941 Mon Sep 17 00:00:00 2001 From: Anton Kosyakov Date: Mon, 20 Apr 2020 13:12:57 +0000 Subject: [PATCH 1/2] added integration tests to reproduce #7619 regression Signed-off-by: Anton Kosyakov --- examples/api-tests/src/menus.spec.js | 45 +++++++++++++++++++ .../core/src/browser/context-menu-renderer.ts | 7 ++- .../menu/browser-context-menu-renderer.ts | 12 ++++- .../src/browser/shell/side-panel-toolbar.ts | 10 +++++ .../src/browser/shell/tab-bar-toolbar.tsx | 13 ++++-- packages/core/src/browser/widgets/widget.ts | 24 ++++++++-- .../menu/electron-context-menu-renderer.ts | 12 ++++- 7 files changed, 109 insertions(+), 14 deletions(-) diff --git a/examples/api-tests/src/menus.spec.js b/examples/api-tests/src/menus.spec.js index 5c8ef3da4e45f..7c46a592be008 100644 --- a/examples/api-tests/src/menus.spec.js +++ b/examples/api-tests/src/menus.spec.js @@ -17,9 +17,15 @@ // @ts-check describe('Menus', function () { + const { assert } = chai; + const { BrowserMenuBarContribution } = require('@theia/core/lib/browser/menu/browser-menu-plugin'); + const { BrowserContextMenuAccess } = require('@theia/core/lib/browser/menu/browser-context-menu-renderer'); const { ApplicationShell } = require('@theia/core/lib/browser/shell/application-shell'); + const { ViewContainer } = require('@theia/core/lib/browser/view-container'); + const { waitForRevealed, waitForHidden } = require('@theia/core/lib/browser/widgets/widget'); const { CallHierarchyContribution } = require('@theia/callhierarchy/lib/browser/callhierarchy-contribution'); + const { EXPLORER_VIEW_CONTAINER_ID } = require('@theia/navigator/lib/browser/navigator-widget'); const { FileNavigatorContribution } = require('@theia/navigator/lib/browser/navigator-contribution'); const { ScmContribution } = require('@theia/scm/lib/browser/scm-contribution'); const { ScmHistoryContribution } = require('@theia/scm-extra/lib/browser/history/scm-history-contribution'); @@ -28,11 +34,19 @@ describe('Menus', function () { const { PluginFrontendViewContribution } = require('@theia/plugin-ext/lib/main/browser/plugin-frontend-view-contribution'); const { ProblemContribution } = require('@theia/markers/lib/browser/problem/problem-contribution'); const { SearchInWorkspaceFrontendContribution } = require('@theia/search-in-workspace/lib/browser/search-in-workspace-frontend-contribution'); + const { HostedPluginSupport } = require('@theia/plugin-ext/lib/hosted/browser/hosted-plugin'); const container = window.theia.container; const shell = container.get(ApplicationShell); const menuBarContribution = container.get(BrowserMenuBarContribution); const menuBar = /** @type {import('@theia/core/lib/browser/menu/browser-menu-plugin').MenuBarWidget} */ (menuBarContribution.menuBar); + const pluginService = container.get(HostedPluginSupport); + + before(async function () { + await pluginService.didStart; + // register views for the explorer view container + await pluginService.activatePlugin('vscode.npm'); + }); for (const contribution of [ container.get(CallHierarchyContribution), @@ -52,4 +66,35 @@ describe('Menus', function () { }); } + it('reveal more context menu in the explorer view container toolbar', async function () { + const viewContainer = await shell.revealWidget(EXPLORER_VIEW_CONTAINER_ID); + if (!(viewContainer instanceof ViewContainer)) { + assert.isTrue(viewContainer instanceof ViewContainer); + return; + } + + const contribution = container.get(FileNavigatorContribution); + const waitForParts = []; + for (const part of viewContainer.getParts()) { + if (part.wrapped.id !== contribution.viewId) { + part.hide(); + waitForParts.push(waitForHidden(part.wrapped)); + } else { + part.show(); + waitForParts.push(waitForRevealed(part.wrapped)); + } + } + await Promise.all(waitForParts); + + const contextMenuAccess = shell.leftPanelHandler.toolBar.showMoreContextMenu({ x: 0, y: 0 }); + if (!(contextMenuAccess instanceof BrowserContextMenuAccess)) { + assert.isTrue(contextMenuAccess instanceof BrowserContextMenuAccess); + return; + } + const contextMenu = contextMenuAccess.menu; + + await waitForRevealed(contextMenu); + assert.notEqual(contextMenu.items.length, 0); + }); + }); diff --git a/packages/core/src/browser/context-menu-renderer.ts b/packages/core/src/browser/context-menu-renderer.ts index ae248d7d48ff2..6fe7381a52dd6 100644 --- a/packages/core/src/browser/context-menu-renderer.ts +++ b/packages/core/src/browser/context-menu-renderer.ts @@ -24,11 +24,14 @@ export function toAnchor(anchor: HTMLElement | { x: number, y: number }): Anchor return anchor instanceof HTMLElement ? { x: anchor.offsetLeft, y: anchor.offsetTop } : anchor; } +export interface ContextMenuAccess { +} + export const ContextMenuRenderer = Symbol('ContextMenuRenderer'); export interface ContextMenuRenderer { - render(options: RenderContextMenuOptions): void; + render(options: RenderContextMenuOptions): ContextMenuAccess; /** @deprecated since 0.7.2 pass `RenderContextMenuOptions` instead */ - render(menuPath: MenuPath, anchor: Anchor, onHide?: () => void): void; + render(menuPath: MenuPath, anchor: Anchor, onHide?: () => void): ContextMenuAccess; } export interface RenderContextMenuOptions { diff --git a/packages/core/src/browser/menu/browser-context-menu-renderer.ts b/packages/core/src/browser/menu/browser-context-menu-renderer.ts index 2a64ffe3f3591..187f10bc3ad27 100644 --- a/packages/core/src/browser/menu/browser-context-menu-renderer.ts +++ b/packages/core/src/browser/menu/browser-context-menu-renderer.ts @@ -18,16 +18,23 @@ import { inject, injectable } from 'inversify'; import { MenuPath } from '../../common/menu'; -import { ContextMenuRenderer, Anchor, RenderContextMenuOptions } from '../context-menu-renderer'; +import { Menu } from '../widgets'; +import { ContextMenuAccess, ContextMenuRenderer, Anchor, RenderContextMenuOptions } from '../context-menu-renderer'; import { BrowserMainMenuFactory } from './browser-menu-plugin'; +export class BrowserContextMenuAccess implements ContextMenuAccess { + constructor( + public readonly menu: Menu + ) { } +} + @injectable() export class BrowserContextMenuRenderer implements ContextMenuRenderer { constructor(@inject(BrowserMainMenuFactory) private menuFactory: BrowserMainMenuFactory) { } - render(arg: MenuPath | RenderContextMenuOptions, arg2?: Anchor, arg3?: () => void): void { + render(arg: MenuPath | RenderContextMenuOptions, arg2?: Anchor, arg3?: () => void): BrowserContextMenuAccess { const { menuPath, anchor, args, onHide } = RenderContextMenuOptions.resolve(arg, arg2, arg3); const contextMenu = this.menuFactory.createContextMenu(menuPath, args); const { x, y } = anchor instanceof MouseEvent ? { x: anchor.clientX, y: anchor.clientY } : anchor!; @@ -35,6 +42,7 @@ export class BrowserContextMenuRenderer implements ContextMenuRenderer { contextMenu.aboutToClose.connect(() => onHide!()); } contextMenu.open(x, y); + return new BrowserContextMenuAccess(contextMenu); } } diff --git a/packages/core/src/browser/shell/side-panel-toolbar.ts b/packages/core/src/browser/shell/side-panel-toolbar.ts index 5f076998c2dfc..8455d0fc403c4 100644 --- a/packages/core/src/browser/shell/side-panel-toolbar.ts +++ b/packages/core/src/browser/shell/side-panel-toolbar.ts @@ -19,6 +19,7 @@ import { TabBarToolbar, TabBarToolbarRegistry, TabBarToolbarFactory } from './ta import { Message } from '@phosphor/messaging'; import { BaseWidget } from '../widgets'; import { Emitter } from '../../common/event'; +import { ContextMenuAccess, Anchor } from '../context-menu-renderer'; export class SidePanelToolbar extends BaseWidget { @@ -101,4 +102,13 @@ export class SidePanelToolbar extends BaseWidget { this.update(); } } + + /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ + showMoreContextMenu(anchor: Anchor): ContextMenuAccess { + if (this.toolbar) { + return this.toolbar.renderMoreContextMenu(anchor); + } + throw new Error(this.id + ' widget is not attached'); + } + } diff --git a/packages/core/src/browser/shell/tab-bar-toolbar.tsx b/packages/core/src/browser/shell/tab-bar-toolbar.tsx index fba28e87ed00a..6f2cc21a12124 100644 --- a/packages/core/src/browser/shell/tab-bar-toolbar.tsx +++ b/packages/core/src/browser/shell/tab-bar-toolbar.tsx @@ -25,7 +25,7 @@ import { CommandRegistry } from '../../common/command'; import { Disposable, DisposableCollection } from '../../common/disposable'; import { ContextKeyService } from '../context-key-service'; import { Event, Emitter } from '../../common/event'; -import { ContextMenuRenderer } from '../context-menu-renderer'; +import { ContextMenuRenderer, Anchor } from '../context-menu-renderer'; import { MenuModelRegistry } from '../../common/menu'; /** @@ -145,6 +145,11 @@ export class TabBarToolbar extends ReactWidget { event.stopPropagation(); event.preventDefault(); + this.renderMoreContextMenu(event.nativeEvent); + }; + + /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ + renderMoreContextMenu(anchor: Anchor): any { const menuPath = ['TAB_BAR_TOOLBAR_CONTEXT_MENU']; const toDisposeOnHide = new DisposableCollection(); for (const [, item] of this.more) { @@ -154,13 +159,13 @@ export class TabBarToolbar extends ReactWidget { when: item.when })); } - this.contextMenuRenderer.render({ + return this.contextMenuRenderer.render({ menuPath, args: [this.current], - anchor: event.nativeEvent, + anchor, onHide: () => toDisposeOnHide.dispose() }); - }; + } shouldHandleMouseEvent(event: MouseEvent): boolean { return event.target instanceof Element && (!!this.inline.get(event.target.id) || event.target.id === '__more__'); diff --git a/packages/core/src/browser/widgets/widget.ts b/packages/core/src/browser/widgets/widget.ts index 6f2c9fea5f6b6..e1af24a6daf5e 100644 --- a/packages/core/src/browser/widgets/widget.ts +++ b/packages/core/src/browser/widgets/widget.ts @@ -261,21 +261,37 @@ export function addClipboardListener(element ); } +/** + * Resolves when the given widget is detached and hidden. + */ export function waitForClosed(widget: Widget): Promise { - return waitForVisible(widget, false); + return waitForVisible(widget, false, false); } +/** + * Resolves when the given widget is attached and visible. + */ export function waitForRevealed(widget: Widget): Promise { + return waitForVisible(widget, true, true); +} + +/** + * Resolves when the given widget is hidden regardless of attachment. + */ +export function waitForHidden(widget: Widget): Promise { return waitForVisible(widget, true); } -function waitForVisible(widget: Widget, visible: boolean): Promise { - if (widget.isAttached === visible && (widget.isVisible === visible || (widget.node.style.visibility !== 'hidden') === visible)) { +function waitForVisible(widget: Widget, visible: boolean, attached?: boolean): Promise { + if ((typeof attached !== 'boolean' || widget.isAttached === attached) && + (widget.isVisible === visible || (widget.node.style.visibility !== 'hidden') === visible) + ) { return new Promise(resolve => window.requestAnimationFrame(() => resolve())); } return new Promise(resolve => { const waitFor = () => window.requestAnimationFrame(() => { - if (widget.isAttached === visible && (widget.isVisible === visible || (widget.node.style.visibility !== 'hidden') === visible)) { + if ((typeof attached !== 'boolean' || widget.isAttached === attached) && + (widget.isVisible === visible || (widget.node.style.visibility !== 'hidden') === visible)) { window.requestAnimationFrame(() => resolve()); } else { waitFor(); diff --git a/packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts b/packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts index 9083c1d5def67..f92ae0052fb73 100644 --- a/packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts +++ b/packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts @@ -16,12 +16,19 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ +import * as electron from 'electron'; import { inject, injectable } from 'inversify'; import { MenuPath } from '../../common'; -import { ContextMenuRenderer, Anchor, RenderContextMenuOptions } from '../../browser'; +import { ContextMenuRenderer, Anchor, RenderContextMenuOptions, ContextMenuAccess } from '../../browser'; import { ElectronMainMenuFactory } from './electron-main-menu-factory'; import { ContextMenuContext } from '../../browser/menu/context-menu-context'; +export class ElectronContextMenuAccess implements ContextMenuAccess { + constructor( + public readonly menu: electron.Menu + ) { } +} + @injectable() export class ElectronContextMenuRenderer implements ContextMenuRenderer { @@ -31,7 +38,7 @@ export class ElectronContextMenuRenderer implements ContextMenuRenderer { constructor(@inject(ElectronMainMenuFactory) private menuFactory: ElectronMainMenuFactory) { } - render(arg: MenuPath | RenderContextMenuOptions, arg2?: Anchor, arg3?: () => void): void { + render(arg: MenuPath | RenderContextMenuOptions, arg2?: Anchor, arg3?: () => void): ElectronContextMenuAccess { const { menuPath, args, onHide } = RenderContextMenuOptions.resolve(arg, arg2, arg3); const menu = this.menuFactory.createContextMenu(menuPath, args); menu.popup({}); @@ -40,6 +47,7 @@ export class ElectronContextMenuRenderer implements ContextMenuRenderer { if (onHide) { menu.once('menu-will-close', () => onHide()); } + return new ElectronContextMenuAccess(menu); } } From c43540709236f008f1f1f6a1cb2129a07b82f02e Mon Sep 17 00:00:00 2001 From: Anton Kosyakov Date: Mon, 20 Apr 2020 13:16:44 +0000 Subject: [PATCH 2/2] fix #7619: don't wrap menu action args into another array Signed-off-by: Anton Kosyakov --- packages/core/src/browser/menu/browser-menu-plugin.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/browser/menu/browser-menu-plugin.ts b/packages/core/src/browser/menu/browser-menu-plugin.ts index c564eda552e56..969b9d11fd605 100644 --- a/packages/core/src/browser/menu/browser-menu-plugin.ts +++ b/packages/core/src/browser/menu/browser-menu-plugin.ts @@ -362,7 +362,7 @@ class MenuCommandRegistry extends PhosphorCommandRegistry { } // eslint-disable-next-line @typescript-eslint/no-explicit-any - registerActionMenu(menu: ActionMenuNode, ...args: any[]): void { + registerActionMenu(menu: ActionMenuNode, args: any[]): void { const { commandId } = menu.action; const { commandRegistry } = this.services; const command = commandRegistry.getCommand(commandId);