Fix context menu inaccurate positioning

[hsfzxjy]

This PR should fix #113175, fix #187111.

Previously VSCode adopts a formula like clientX = Math.floor(pageX * zoom) to calculate the coordiate on screen for showing up the context menu. The formula leads to numeric loss when zoom != 1, causing nonnegligible shift of context menu in some cases, and finally presented as user annoyance as described in abovementioned issues.

However, for most of the time the desired position for context menu is the current position of cursor, which actually need not to be calculated by ourselves. Electron Docs states that leaving the x and y arguments as undefined will indicate the context menu popping up at current mouse cursor position.

Therefore, this PR utilizes the Electron API for more accurate context menu popup positioning, and hopefully should fix these related issues, if there's no other defeat in upstream Electron.

[deepak1556]

Thanks for the PR! Have you been able to verify if the change addresses the mentioned issue ? I have not been able to repro them on my machine so cannot validate this change. But agree that Electron calculates the cursor coordinates when not explicitly configured at https://github.com/electron/electron/blob/93024be3b26ff66a7978ba278990a0a0f55bc9d3/shell/browser/api/electron_api_menu_views.cc#L33

Also a follow-up question, when would isCurrentCursor not be needed ? I wondering if we should always use current cursor position for desktop. Maybe @sbatten can chime in on this.

[hsfzxjy]

Thanks for the PR! Have you been able to verify if the change addresses the mentioned issue ?

Yes. It addresses the issue that the menu shifting from the current cursor position.

So now if keeping cursor unmoved during right-click, there won't be accidental click.

But if cursor is moved during right-click, there's still a chance of accidental click since Linux DE will acknowledge an menu item on mouseup -- this is a problem of Linux DE and we can't interfere with.

I have not been able to repro them on my machine

It might be easier to repro with

1. Linux desktop
2. zoom factor > 1
3. menu with long list of items, such that menu spawns at a position where cursor is at the middle of one of its vertical edges instead of at the corner.

when would isCurrentCursor not be needed ?

I don't know. But since this is a public API, I would rather ensure its forward compatibility so as not to break existing code. Adding an optional argument sounds reasonable.

[deepak1556]

Thanks for testing, I have tried different variants documented in the upstream issue electron/electron#31641, unable to repro with my current display setup.

I am good with merging this change once API changes are approved by @sbatten. We can get feedback from the affected users with insiders build.

[bpasero]

and hopefully should fix these related issues

Yeah, I am not 100% convinced it does. electron/electron#31641 is reported with a standalone sample that does seem to rely on Electron positioning the menu properly (code pointer).

Nevertheless, I think having a way to signal back to the Electron context menu to position it where the mouse is is an interesting idea. But the adoption is a bit challenging because from the outside we always have to pass in x and y for our custom menu to position properly which has no other way of getting to the cursor location otherwise.
On top of that, we seem to be having cases where - even though the context menu should open from mouse interaction - we put it to a slightly different location by a few pixels (for example here).

I think from the internal API I would expect another or type to signal the mouse position is wanted, such as:

HTMLElement | { x: number; y: number; width?: number; height?: number; } | { mouseX: number; mouseY: number }

To make it very explicit that in the one case a x/y position is wanted and in the other case the mouse location is preferred.

Finally, - given my first point - I wonder if a pragmatic workaround for VS Code could be to always offset the menu by 1px on Linux at least it the cursor location is wanted.

[hsfzxjy]

Yeah, I am not 100% convinced it does. electron/electron#31641 is reported with a standalone sample that does seem to rely on Electron positioning the menu properly (code pointer).

Something I have observed:

1. I cannot reproduce the bug with the linked gist.
2. I ran into this bug with VSCode previously, but after applying this PR the bug is gone.

So here's my guessing -- this bug on VSCode has two causes

1. Some misbehaviors in Electron, which might triggered under specific hardware settings (e.g., the settings for author of electron/electron#31641, but not mine)
2. Some misbehaviors in VSCode, which is the numerical loss during transforming coordinates.

I think this PR fixes cause 2, and should reduce the possibility to trigger this bug on VSCode for certain users -- but sadly I have no approach to prove this. Perhaps this should be tested by those affected by this bug and collect their responses.

---

Regarding your proposed alternatives:

I wonder if a pragmatic workaround for VS Code could be to always offset the menu by 1px on Linux at least it the cursor location is wanted.

This won't work for two reasons

1. We don't know the context menu will be popped up at the left side or right side of the cursor. Thus we don't know the offset should be +1px or -1px.
2. Most importantly, the bug is caused by the numerical loss during * zoom_factor. This can be proven by the popular comment in Linux: native context menu runs action under mouse when opened #113175 (comment) . When zoom factor does not equal to 1, the effect of offseting by 1px might be eliminated, nor would it solve the problem of numerical loss.

I think from the internal API I would expect another or type to signal the mouse position is wanted, such as:

HTMLElement | { x: number; y: number; width?: number; height?: number; } | { mouseX: number; mouseY: number }

This is also not possible, unfortunately. Although MouseEvent.screenX and .screenY are irrelavant to zoom factor, they are relative to the top-left corner of the screen instead of the top-left corner of the webview. So before passing them to Electron API, we should subtract them by the left and top position of the webview on the screen, which is however not accessible.

So overall, I think this PR is a more pragmatic solution.

[bpasero]

@hsfzxjy I started coding something up in https://github.com/microsoft/vscode/tree/ben/context-menu-mouse-position to make the mouse cursor location more explicit. I would take that as a first step, making sure that in all locations where we want the mouse location, we explicitly set it. Adoption is actually not so trivial because of the loose types, will clean the branch up going over all locations next.

As a second step, I would like to experiment with letting Electron position the menu in those cases where the mouse location is preferred, but probably behind a setting and not enabled by default to get some feedback for whether this truly addresses the issue for all. This could be a PR from your end that is thus then much smaller and easier to get in.

I agree with the sentiment that patching the location from our end is not clever and using undefined in Electron for the location is the ideal.

PS: I found that only for the editor context menu we do some pixel pushing:

vscode/src/vs/editor/contrib/contextmenu/browser/contextmenu.ts

Line 134 in a964308

anchor = { x: e.event.posx - 1, width: 2, y: e.event.posy - 1, height: 2 };

That is why I had asked in #113175 (comment) if this issue reproduces in other menus outside the editor where we do not apply this offset.

[bpasero]

I think #188674 is in good shape, running a build now to give people to test. Given the chance of breakage, this can only be merged early in a new release cycle, such as next week (this week we are winding down for endgame).

Builds should also be used to test various things:

• this Linux issue most importantly
• custom menus still work nicely (e.g. on Windows)
• with / without zooming

[bpasero]

The change is in via #188674. Sorry for not having continued here but I wanted to get the change in asap because we do have a testing day today where this change is covered in #188709

I will make sure to have this PR mentioned in our thank you notes for our upcoming release 🙏

[hsfzxjy]

@bpasero It's OK. Thanks for the follow up to this long-term issue!