Fix ElectronContextMenuRenderer to use passed in anchor XY values

[kenneth-marut-work]

Signed-off-by: Kenneth Marut [email protected]
Signed-off-by: Colin Grant [email protected]

What it does

Fixes: #7722

This PR addresses #7722 which was uncovered in #7105 where the ElectronContextMenuRenderer did not utilize the passed in anchor parameter to render the menu at a given X/Y location. This caused the gear-icon and folder-scope context menus to be rendered at the mouse location rather than the passed-in anchor location. This issue is fixed by adding an identical MouseEvent check as in:

theia/packages/core/src/browser/menu/browser-context-menu-renderer.ts

Line 30 in cea6233

const { x, y } = anchor instanceof MouseEvent ? { x: anchor.clientX, y: anchor.clientY } : anchor;

And also by rounding anchor XY values to integer forms (due to a conversion error down the line) and passing the arguments into menu.popup({ x: Math.round(x), y: Math.round(y) }); in the electron-menu-context-renderer.ts

How to test

1. Pull latest changes from Added Preferences UI widget #7105, build, and run in Electron
2. Open new preferences UI with multiroot workspace
3. Click on gear-icon and/or folder scope tab in preferences UI, observe bad behavior of context menu
4. Pull changes from this PR, and rebuild in Electron
5. Relaunch and run steps 2-3
6. Observe context menu behaior

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

I tested the changes against #7105 and it works correctly for me 👍

For reference I also took some screenshots:

Before:

After:

[akosyakov]

@kittaakos maybe you can help to test it a bit against Electron

[kittaakos]

I have tried it, clicked around the application: I did not notice any regression. Very nice contribution, thank you! 👍 (Note: I haven't had any issues with the context menu before either.)

@kenneth-marut-work, I do not get why you have to round the coordinates? MouseEvent.clientX has a double floating-point value. Have you experienced any malfunction?

Even if the numbers are not integers, the MouseEvent is part of the contextmenu event, why do not we pass x, y further as is? I do not see any reason for the rounding.

[kenneth-marut-work]

Hey @kittaakos, thanks for the comment. I am not sure why this is necessary either, however if the number is not rounded I receive this error when testing (this has been the case on my colleague's machine as well):

[kittaakos]

I could reproduce it with this change:

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 b93ae2e67..e1a4f03b5 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
@@ -46,7 +46,7 @@ export class ElectronContextMenuRenderer extends ContextMenuRenderer {
         const menu = this.menuFactory.createContextMenu(menuPath, args);
         const { x, y } = anchor instanceof MouseEvent ? { x: anchor.clientX, y: anchor.clientY } : anchor!;
         // x and y values must be Ints or else there is a conversion error
-        menu.popup({ x: Math.round(x), y: Math.round(y) });
+        menu.popup({ x: x + 0.125, y });
         // native context menu stops the event loop, so there is no keyboard events
         this.context.resetAltPressed();
         if (onHide) {

Uncaught Error: Could not call remote function 'anonymous'. Check that the function signature is correct. Underlying error: Error processing argument at index 1, conversion failure from 607.125

Error: Could not call remote function 'anonymous'. Check that the function signature is correct. Underlying error: Error processing argument at index 1, conversion failure from 607.125
    at callFunction (/Users/akos.kitta/gi…r/rpc-server.js:258)
    at /Users/akos.kitta/gi…r/rpc-server.js:409
    at EventEmitter.ipcMain.on.args (/Users/akos.kitta/gi…r/rpc-server.js:273)
    at EventEmitter.emit (events.js:182)
    at WebContents.<anonymous> (/Users/akos.kitta/gi…web-contents.js:368)
    at WebContents.emit (events.js:182)
    at callFunction (/Users/akos.kitta/gi…r/rpc-server.js:258)
    at /Users/akos.kitta/gi…r/rpc-server.js:409
    at EventEmitter.ipcMain.on.args (/Users/akos.kitta/gi…r/rpc-server.js:273)
    at EventEmitter.emit (events.js:182)
    at WebContents.<anonymous> (/Users/akos.kitta/gi…web-contents.js:368)
    at WebContents.emit (events.js:182)
    at errors.ts:26

The error comes from electron. I am OK with the rounding.

[kittaakos]

@akosyakov, do you have any remarks on this PR? Otherwise, I'm going to merge it.
The electron guys have completely rewritten the rpc-server, I think this x, y rounding becomes obsolete once we bump up the electron version:

• 4.2.x: https://github.com/electron/electron/blob/4-2-x/lib/browser/rpc-server.js
• master: https://github.com/electron/electron/blob/master/lib/browser/rpc-server.js