add functionality to hide open context menu#64099

sbatten · 2018-12-01T01:20:58Z

fix toggling of ... menu on windows
fix toggling of global activity bar menu on windows
fix position of global activity bar menu on windows
fixes #62413
fixes #61766

fix toggling of ... menu on windows fix toggling of global activity bar menu on windows fix position of global activity bar menu on windows fixes #62413 fixes #61766

bpasero · 2018-12-02T10:28:45Z

src/vs/base/parts/contextmenu/electron-main/contextmenu.ts

 		});
 	});
+
+	ipcMain.on(CONTEXT_MENU_HIDE_CHANNEL, (event: Event, contextMenuId: number) => {


contextMenuId is unused?

oops, yes, currently with this approach, the hideContextMenu call will hide any context menu that is shown, even if someone else drew it. This was experimenting with adding an id so we knew we were dismissing the right menu. it was too much code change for probably not a lot of value yet

@sbatten ok then I would remove the variable

bpasero · 2018-12-02T10:29:18Z

src/vs/base/browser/ui/dropdown/dropdown.ts

 			anchorAlignment: this.menuOptions.anchorAlignment
 		});
+
+		setTimeout(() => this._contextMenuProvider.hideContextMenu(), 5000);


This seems like debug code forgotten to be removed?

yep testing the scenario I described above

@sbatten that should then be removed

bpasero · 2018-12-02T10:29:37Z

src/vs/base/browser/ui/dropdown/dropdown.ts

 			getKeyBinding: action => this.menuOptions && this.menuOptions.getKeyBinding ? this.menuOptions.getKeyBinding(action) : null,
 			getMenuClassName: () => this.menuClassName,
-			onHide: () => this.onHide(),
+			onHide: () => this.hideMenu.schedule(),


Why is this suddenly async?

the onhide calls all became async because this callback happens before we get the click event on the button in the native case. That means the button will get a click after the menu is not shown anymore and just reshow it which is exactly what we are trying to prevent. alternative proposal/solutions accepted.

bpasero · 2018-12-02T10:29:42Z

src/vs/base/browser/ui/dropdown/dropdown.ts

 		this.actions = options.actions || [];
 		this.actionProvider = options.actionProvider;
 		this.menuClassName = options.menuClassName || '';
+		this.hideMenu = new RunOnceScheduler(() => this.onHide(), 0);


Why a scheduler?

bpasero · 2018-12-02T10:30:58Z

src/vs/workbench/browser/parts/activitybar/activitybarActions.ts

 		super(action, { draggable: false, colors, icon: true }, themeService);
+
+		this.menuVisible = false;
+		this.hideMenu = new RunOnceScheduler(() => this.hideContextMenu(), 0);


Why is the scheduler needed?

bpasero · 2018-12-02T10:31:09Z

src/vs/workbench/browser/parts/activitybar/activitybarActions.ts


-			const event = new StandardMouseEvent(e);
-			this.showContextMenu({ x: event.posx, y: event.posy });
+			if (this.menuVisible) {


This code is repeated 3 times and should be extracted to a method?

bpasero · 2018-12-02T10:31:55Z

src/vs/workbench/browser/parts/activitybar/activitybarActions.ts

 		const globalAction = this._action as GlobalActivityAction;
 		const activity = globalAction.activity as IGlobalActivity;
 		const actions = activity.getActions();
+		const containerPosition = DOM.getDomNodePagePosition(this.container);


Is this a fix that should be made separately, it seems to be around positioning the menu?

it can be separated if needed. I was in the code area and the same menu had 2 issues

joaomoreno

I would definitely not merge this in the last dev day of the milestone.

Will wait to review after @sbatten addresses @bpasero's feedback.

code updated

sbatten · 2018-12-03T15:13:07Z

@joaomoreno really? I know there is indeed risk to late changes, but these are a bit smaller with very few actual callers to the new method meaning a small code path tested widely since dropdown menus are common throughout the UI. I am not saying we definitely need to merge this, just making a case.

code changes

bpasero · 2018-12-03T16:14:44Z

Moving to December.

joaomoreno · 2018-12-03T16:38:53Z

@sbatten Makes sense. The changes do seem localised to the context view. But since the issues aren't new or severe and the changes do impact core UX, I feel a couple weeks insider usage would be great not only to catch issues but to also get feedback on the new behaviour.

bpasero

@sbatten is there a reason the global activity bar action needs special treatment, but other dropdowns with context menu do not? For example the "..." menu in the SCM view:

sbatten · 2018-12-10T16:36:11Z

@bpasero most (all but the custom menu bar) of the more menus in the workbench are made from dropdownMenus which was updated in this PR for hiding on second click. As for positioning, this is also handled by the dropdownMenu

sbatten · 2018-12-10T18:58:05Z

@joaomoreno since you mentioned waiting to review, I will still not merge until I have heard from you.

joaomoreno

I also feel a bit uneasy about the need for having hide be async. Here's how we can fix that:

So far, since the introduction of custom menus, one can still interact with the page while a dropdown menu is open, eg. you can still place a cursor in the editor while a dropdown menu is open, the menu just disappears as soon as you do so. I believe this was accidental...? This feels weird to me since you can't seem to do that at all natively on Linux or macOS. It seems that Windows actually allows the user to do so, but in an incredible display of lack of good UX since all hover feedback is missing from all UI elements other than the dropdown menu, so the user doesn't realize it has the power to click on affordances outside the menu until it is too late.

I suggest to mimic Linux and macOS on this and simply avoid any outside mouse interaction while the dropdown menu is open. This not only aligns with Linux and macOS but also drops the case for async hide, since any click outside the menu would simply close the menu and nothing more. It also makes the whole menu lifecycle easier to reason about and avoids other weird issues which aren't that hard to find:

Quickly testing this shows certain mouse clicks neither opening nor closing the dropdown menu:

Not necessarily related to this PR, but: after so much work into getting rid of raw message passing and wrapping everything into promises/events for the main process in the past, it's pretty sad to see raw IPC message passing gaining ground once again. I'd like to see a proper service around all this message passing, which would make it much easier to reason about the data flow surrounding context menus. Right now I have to jump through N files to fully understand it, when a simple interface would suffice. cc @bpasero

sbatten · 2018-12-13T19:45:49Z

@joaomoreno

I suggest to mimic Linux and macOS on this and simply avoid any outside mouse interaction while the dropdown menu is open.

Edge seems to follow this, too, so sounds good.

sbatten · 2018-12-14T00:51:00Z

@joaomoreno with the change to just absorb the dismissal click, we don't really need any changes to the context menu service except for the native case. Since it isn't the default, we could just forgo those changes

outdated

joaomoreno

I still get the following behavior of having to press click more that three times to open, close and re-open a dropdown menu:

A good example of how that should work is the bottom left activity bar gear icon.

joaomoreno · 2019-01-16T09:23:25Z

src/vs/workbench/browser/parts/activitybar/activitybarActions.ts

 		const activity = globalAction.activity as IGlobalActivity;
 		const actions = activity.getActions();
+		const containerPosition = DOM.getDomNodePagePosition(this.container);
+		const location = { x: containerPosition.left + containerPosition.width / 2, y: containerPosition.top };


The bottom left activity bar gear icon now has a strange menu placement due to the / 2:

It's also not pretty in master because it is currently based on mouse position. But if we touch this code we should make it good everywhere.

I prefer the new placement, but am open to hear alternatives

Oh, I notice I forgot to suggest what I expected. 😆

I expected it to align to one of the corners: so either be above and left-aligned or to the right and bottom-aligned.

the context menus (right-click) for activity bar viewlet icons match the new positioning. do you suggest changing them all?

joaomoreno · 2019-01-16T09:23:54Z

src/vs/platform/contextview/browser/contextMenuHandler.ts

 				const actionRunner = delegate.actionRunner || new ActionRunner();
 				actionRunner.onDidBeforeRun(this.onActionRun, this, menuDisposables);
 				actionRunner.onDidRun(this.onDidActionRun, this, menuDisposables);
+				domEvent(document.body, EventType.MOUSE_DOWN, true)((e: MouseEvent) => {


The change to absorb works in most cases but not in those which listen to mouseup: tree rows, actions (tree actions, tab close actions, activity bar icons), etc. Let's prevent that from happening as well.

how about an invisible div that will block the hover animations as well?

see latest version

sbatten · 2019-01-16T19:30:12Z

assigned @RMacfarlane to test an issue in the PR extension

joaomoreno

Be careful when doing this down in the context view layer. There are features using context views besides menus. The feedback widget is one example, and it happens not to hide when it loses focus... so this change causes an ugly issue in which the only way to exit the feedback widget is to click its X icon. Another example is the input box's validation messages (type ( in the search viewlet, while having regex enabled); this example actually works because the message hides when the input loses focus, but it is coincidental.

Because of that I'd do this only for menus.

Additionally, it would be great if we could do this without blocking the workbench with a dom node. Have you tried catching document.body mouseup events on the capture phase and ignore the very first one which happens on a target which isn't a descendant of your menu? That should fix the issue.

sbatten · 2019-01-18T18:07:01Z

@joaomoreno thanks, I actually debated on doing it in the context menu handler, which I will now do instead now that I know there are other things using it. The beauty of an overlay is that it also prevents mouse hover events which makes it feel nearly identical to macOS. With just absorbing the events, you can still hover activity bar icons and think that clicking will do something, but it wouldn't.

sbatten · 2019-02-21T11:05:55Z

ping @joaomoreno

fixes #67407

* add functionality to hide open context menu fix toggling of ... menu on windows fix toggling of global activity bar menu on windows fix position of global activity bar menu on windows fixes microsoft#62413 fixes microsoft#61766 * addressing feedback * linting error * updating to eat mouse click to dismiss menu * remove native context menu changes as unnecessary * use an invisible div to block mouse * move logic into context menu handler * sanitize SNAP variables * rename update.channel to update.mode fixes microsoft#67407 * dervie status background from editor widget background

ghiscoding · 2019-03-02T05:33:27Z

Hello, please allow me to provide some feedback here. For the gear icon (bottom left), my first impression is similar to @joaomoreno, I would have expected the menu to align with 1 of the corner (top) of the icon. Previously, I could click anywhere on the gear and by moving couple of pixels I could do update (which I do every day on insiders) but with the latest version, if I'm at the bottom of the gear, I have to move the mouse a lot more to do the same exercise of clicking "Update". An action that I previously was able to do by moving my mouse roughly 10px, now takes me 75px to do the same... isn't that a little too far off?

Below you can see I clicked at the bottom of the gear and the menu shows far off from the pointer. I have to move my mouse by about 75px to go over hover 1st sub-menu, wow that's a bit far isn't it?

Compared with the old menu, just a few pixel away from pointer. Though I agree that the menu shouldn't cover the icon but having it aligned on a corner would be better I think.

Then if I try the Git "..." icon, we have another new gap but not as bad as the Gear though

There's no such gap, or white space, when opening a navbar menu. I hope there never will be either. So what is the reasoning in adding these extra gaps everywhere else?

This is just my personal observation, but so far I have to do much more mouse move (in distance) to do the same action from any of these new menu position. Which I personally think it isn't that great. I agree that the previous position over the gear wasn't great, but now it's so much further away that I don't understand why it's so far out.

On a totally different note, I love that VSCode keeps improving and it's been my main driver for couple years already. I'm just giving my personal feeling about the new position, if I'm the only one then please disregard but I have a big feeling that i won't be. Cheers :)

add functionality to hide open context menu

68d6b12

fix toggling of ... menu on windows fix toggling of global activity bar menu on windows fix position of global activity bar menu on windows fixes #62413 fixes #61766

sbatten added this to the November 2018 milestone Dec 1, 2018

sbatten self-assigned this Dec 1, 2018

sbatten requested review from bpasero and joaomoreno December 1, 2018 01:20

bpasero self-assigned this Dec 1, 2018

bpasero previously requested changes Dec 2, 2018

View reviewed changes

bpasero removed their assignment Dec 2, 2018

joaomoreno previously requested changes Dec 3, 2018

View reviewed changes

sbatten added 2 commits December 3, 2018 07:02

addressing feedback

7dcbf8a

linting error

9674882

bpasero modified the milestones: November 2018, December 2018 Dec 3, 2018

bpasero self-assigned this Dec 3, 2018

bpasero approved these changes Dec 10, 2018

View reviewed changes

bpasero removed their assignment Dec 10, 2018

joaomoreno previously requested changes Dec 13, 2018

View reviewed changes

sbatten added 2 commits December 13, 2018 11:57

merge conflicts

c198c0f

updating to eat mouse click to dismiss menu

5088bc9

sbatten added 2 commits January 15, 2019 14:30

Merge branch 'master' into toggleContextMenu

3d6c172

remove native context menu changes as unnecessary

65c5819

joaomoreno requested changes Jan 16, 2019

View reviewed changes

sbatten assigned RMacfarlane Jan 16, 2019

use an invisible div to block mouse

99e50d5

joaomoreno added ux User experience issues workbench-menu dropdown DropDown (SelectBox widget) native and custom issues labels Jan 18, 2019

joaomoreno requested changes Jan 18, 2019

View reviewed changes

sbatten modified the milestones: December/January 2019, February 2019 Jan 31, 2019

sbatten unassigned RMacfarlane Jan 31, 2019

sbatten mentioned this pull request Feb 1, 2019

Menu stays open in this case #61145

Closed

sbatten added 2 commits February 7, 2019 15:54

Merge branch 'master' into toggleContextMenu

7f823cb

move logic into context menu handler

21ebf54

joaomoreno and others added 3 commits February 21, 2019 16:42

sanitize SNAP variables

6dd6b44

rename update.channel to update.mode

f690d5e

fixes #67407

dervie status background from editor widget background

60e586d

sbatten modified the milestones: February 2019, March 2019 Feb 21, 2019

Merge branch 'master' into toggleContextMenu

9d7f752

joaomoreno approved these changes Feb 21, 2019

View reviewed changes

sbatten merged commit d96141b into microsoft:master Feb 21, 2019

sbatten deleted the toggleContextMenu branch February 21, 2019 16:59

mattmasson mentioned this pull request Mar 12, 2019

Object doesn't support property or method 'remove' in 0.16.0 on IE microsoft/monaco-editor#1357

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

sbatten commented Dec 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaomoreno left a comment

Choose a reason for hiding this comment

Uh oh!

sbatten commented Dec 3, 2018

Uh oh!

bpasero commented Dec 3, 2018

Uh oh!

joaomoreno commented Dec 3, 2018

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

sbatten commented Dec 10, 2018

Uh oh!

sbatten commented Dec 10, 2018

Uh oh!

joaomoreno left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbatten commented Dec 13, 2018

Uh oh!

sbatten commented Dec 14, 2018

Uh oh!

joaomoreno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbatten commented Jan 16, 2019

Uh oh!

joaomoreno left a comment

Choose a reason for hiding this comment

Uh oh!

sbatten commented Jan 18, 2019

joaomoreno left a comment •

edited

Loading