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.

Sign up for GitHub

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 ElectronContextMenuRenderer to use passed in anchor XY values #7735

Merged
merged 1 commit into from
May 7, 2020
+3 −1
Merged

Fix ElectronContextMenuRenderer to use passed in anchor XY values #7735

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion packages/core/src/electron-browser/menu/electron-context-menu-renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ export class ElectronContextMenuRenderer extends ContextMenuRenderer {

protected doRender({ menuPath, anchor, args, onHide }: RenderContextMenuOptions): ElectronContextMenuAccess {
const menu = this.menuFactory.createContextMenu(menuPath, args);
menu.popup({});
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) });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// native context menu stops the event loop, so there is no keyboard events
this.context.resetAltPressed();
if (onHide) {
Expand Down