Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problems related with menu bar updates on focus change. #14959

Merged
merged 2 commits into from
Feb 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 31 additions & 29 deletions examples/api-samples/src/browser/menu/sample-menu-contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,36 +225,38 @@ export class SampleCommandContribution implements CommandContribution {
@injectable()
export class SampleMenuContribution implements MenuContribution {
registerMenus(menus: MenuModelRegistry): void {
const subMenuPath = [...MAIN_MENU_BAR, 'sample-menu'];
menus.registerSubmenu(subMenuPath, 'Sample Menu', {
order: '2' // that should put the menu right next to the File menu
});
menus.registerMenuAction(subMenuPath, {
commandId: SampleCommand.id,
order: '0'
});
menus.registerMenuAction(subMenuPath, {
commandId: SampleCommand2.id,
order: '2'
});
const subSubMenuPath = [...subMenuPath, 'sample-sub-menu'];
menus.registerSubmenu(subSubMenuPath, 'Sample sub menu', { order: '2' });
menus.registerMenuAction(subSubMenuPath, {
commandId: SampleCommand.id,
order: '1'
});
menus.registerMenuAction(subSubMenuPath, {
commandId: SampleCommand2.id,
order: '3'
});
const placeholder = new PlaceholderMenuNode([...subSubMenuPath, 'placeholder'].join('-'), 'Placeholder', { order: '0' });
menus.registerMenuNode(subSubMenuPath, placeholder);
setTimeout(() => {
const subMenuPath = [...MAIN_MENU_BAR, 'sample-menu'];
menus.registerSubmenu(subMenuPath, 'Sample Menu', {
order: '2' // that should put the menu right next to the File menu
});
menus.registerMenuAction(subMenuPath, {
commandId: SampleCommand.id,
order: '0'
});
menus.registerMenuAction(subMenuPath, {
commandId: SampleCommand2.id,
order: '2'
});
const subSubMenuPath = [...subMenuPath, 'sample-sub-menu'];
menus.registerSubmenu(subSubMenuPath, 'Sample sub menu', { order: '2' });
menus.registerMenuAction(subSubMenuPath, {
commandId: SampleCommand.id,
order: '1'
});
menus.registerMenuAction(subSubMenuPath, {
commandId: SampleCommand2.id,
order: '3'
});
const placeholder = new PlaceholderMenuNode([...subSubMenuPath, 'placeholder'].join('-'), 'Placeholder', { order: '0' });
menus.registerMenuNode(subSubMenuPath, placeholder);

/**
* Register an action menu with an invalid command (un-registered and without a label) in order
* to determine that menus and the layout does not break on startup.
*/
menus.registerMenuAction(subMenuPath, { commandId: 'invalid-command' });
/**
* Register an action menu with an invalid command (un-registered and without a label) in order
* to determine that menus and the layout does not break on startup.
*/
menus.registerMenuAction(subMenuPath, { commandId: 'invalid-command' });
}, 10000);
}

}
Expand Down
60 changes: 55 additions & 5 deletions packages/core/src/browser/menu/browser-menu-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,20 @@
// *****************************************************************************

import { injectable, inject } from 'inversify';
import { MenuBar, Menu as MenuWidget, Widget } from '@phosphor/widgets';
import { Menu, MenuBar, Menu as MenuWidget, Widget } from '@phosphor/widgets';
import { CommandRegistry as PhosphorCommandRegistry } from '@phosphor/commands';
import { ElementExt } from '@phosphor/domutils';
import {
CommandRegistry, environment, DisposableCollection, Disposable,
MenuModelRegistry, MAIN_MENU_BAR, MenuPath, MenuNode, MenuCommandExecutor, CompoundMenuNode, CompoundMenuNodeRole, CommandMenuNode
MenuModelRegistry, MAIN_MENU_BAR, MenuPath, MenuNode, MenuCommandExecutor, CompoundMenuNode, CompoundMenuNodeRole, CommandMenuNode,
ArrayUtils
} from '../../common';
import { KeybindingRegistry } from '../keybinding';
import { FrontendApplication } from '../frontend-application';
import { FrontendApplicationContribution } from '../frontend-application-contribution';
import { ContextKeyService, ContextMatcher } from '../context-key-service';
import { ContextMenuContext } from './context-menu-context';
import { waitForRevealed } from '../widgets';
import { Message, waitForRevealed } from '../widgets';
import { ApplicationShell } from '../shell';
import { CorePreferences } from '../core-preferences';
import { PreferenceService } from '../preferences/preference-service';
Expand Down Expand Up @@ -82,8 +84,10 @@ export class BrowserMainMenuFactory implements MenuWidgetFactory {
this.keybindingRegistry.onKeybindingsChanged(() => {
this.showMenuBar(menuBar);
}),
this.menuProvider.onDidChange(() => {
this.showMenuBar(menuBar);
this.menuProvider.onDidChange(evt => {
if (ArrayUtils.startsWith(evt.path, MAIN_MENU_BAR)) {
this.showMenuBar(menuBar);
}
})
);
menuBar.disposed.connect(() => disposable.dispose());
Expand Down Expand Up @@ -156,6 +160,10 @@ export class BrowserMainMenuFactory implements MenuWidgetFactory {

}

export function isMenuElement(element: HTMLElement | null): boolean {
return !!element && element.className.includes('p-Menu');
}

export class DynamicMenuBarWidget extends MenuBarWidget {

/**
Expand Down Expand Up @@ -263,6 +271,48 @@ export class DynamicMenuWidget extends MenuWidget {
this.updateSubMenus(this, this.menu, this.options.commands);
}

protected override onAfterAttach(msg: Message): void {
super.onAfterAttach(msg);
this.node.ownerDocument.addEventListener('pointerdown', this, true);
}

protected override onBeforeDetach(msg: Message): void {
this.node.ownerDocument.removeEventListener('pointerdown', this);
super.onAfterDetach(msg);
}

override handleEvent(event: Event): void {
if (event.type === 'pointerdown') {
this.handlePointerDown(event as PointerEvent);
}
super.handleEvent(event);
}

handlePointerDown(event: PointerEvent): void {
// this code is copied from the superclass because we cannot use the hit
// test from the "Private" implementation namespace
if (this['_parentMenu']) {
return;
}

// The mouse button which is pressed is irrelevant. If the press
// is not on a menu, the entire hierarchy is closed and the event
// is allowed to propagate. This allows other code to act on the
// event, such as focusing the clicked element.
if (!this.hitTestMenus(this, event.clientX, event.clientY)) {
this.close();
}
}

private hitTestMenus(menu: Menu, x: number, y: number): boolean {
for (let temp: Menu | null = menu; temp; temp = temp.childMenu) {
if (ElementExt.hitTest(temp.node, x, y)) {
return true;
}
}
return false;
}

public aboutToShow({ previousFocusedElement }: { previousFocusedElement: HTMLElement | undefined }): void {
this.preserveFocusedElement(previousFocusedElement);
this.clearItems();
Expand Down
25 changes: 25 additions & 0 deletions packages/core/src/common/array-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,29 @@ export namespace ArrayUtils {
}
return result;
}

export function shallowEqual<T>(left: readonly T[], right: readonly T[]): boolean {
if (left.length !== right.length) {
return false;
}
for (let i = 0; i < left.length; i++) {
if (left[i] !== right[i]) {
return false;
}
}
return true;
}

export function startsWith<T>(left: readonly T[], right: readonly T[]): boolean {
if (right.length > left.length) {
return false;
}

for (let i = 0; i < right.length; i++) {
if (left[i] !== right[i]) {
return false;
}
}
return true;
}
}
6 changes: 4 additions & 2 deletions packages/core/src/common/menu/composite-menu-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,13 @@ export class CompositeMenuNode implements MutableCompoundMenuNode {
};
}

removeNode(id: string): void {
removeNode(id: string): boolean {
const idx = this._children.findIndex(n => n.id === id);
if (idx >= 0) {
this._children.splice(idx, 1);
return true;
}
return false;
}

updateOptions(options?: SubMenuOptions): void {
Expand Down Expand Up @@ -108,7 +110,7 @@ export class CompositeMenuNodeWrapper implements MutableCompoundMenuNode {

addNode(node: MenuNode): Disposable { return this.wrapped.addNode(node); }

removeNode(id: string): void { return this.wrapped.removeNode(id); }
removeNode(id: string): boolean { return this.wrapped.removeNode(id); }

updateOptions(options: SubMenuOptions): void { return this.wrapped.updateOptions(options); }
}
Loading
Loading