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

Fixes #14002 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>
eclipse-theia · Feb 20, 2025 · f067116 · f067116
1 parent f26b515
commit f067116
Show file tree

Hide file tree

Showing 6 changed files with 201 additions and 56 deletions.
diff --git a/examples/api-samples/src/browser/menu/sample-menu-contribution.ts b/examples/api-samples/src/browser/menu/sample-menu-contribution.ts
@@ -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);
     }
 
 }

diff --git a/packages/core/src/browser/menu/browser-menu-plugin.ts b/packages/core/src/browser/menu/browser-menu-plugin.ts
@@ -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';
@@ -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());
@@ -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 {
 
     /**
@@ -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();

diff --git a/packages/core/src/common/array-utils.ts b/packages/core/src/common/array-utils.ts
@@ -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;
+    }
 }
diff --git a/packages/core/src/common/menu/composite-menu-node.ts b/packages/core/src/common/menu/composite-menu-node.ts
@@ -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 {
@@ -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); }
 }