-
Notifications
You must be signed in to change notification settings - Fork 27.9k
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.
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
add functionality to hide open context menu #64099
Conversation
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
event.sender.send(CONTEXT_MENU_CLOSE_CHANNEL, contextMenuId); | ||
} | ||
}); | ||
}); | ||
|
||
ipcMain.on(CONTEXT_MENU_HIDE_CHANNEL, (event: Event, contextMenuId: number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contextMenuId
is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten ok then I would remove the variable
actionRunner: this.menuOptions ? this.menuOptions.actionRunner : null, | ||
anchorAlignment: this.menuOptions.anchorAlignment | ||
}); | ||
|
||
setTimeout(() => this._contextMenuProvider.hideContextMenu(), 5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like debug code forgotten to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep testing the scenario I described above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbatten that should then be removed
@@ -248,19 +252,24 @@ export class DropdownMenu extends BaseDropdown { | |||
getActionItem: action => this.menuOptions && this.menuOptions.actionItemProvider ? this.menuOptions.actionItemProvider(action) : null, | |||
getKeyBinding: action => this.menuOptions && this.menuOptions.getKeyBinding ? this.menuOptions.getKeyBinding(action) : null, | |||
getMenuClassName: () => this.menuClassName, | |||
onHide: () => this.onHide(), | |||
onHide: () => this.hideMenu.schedule(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this suddenly async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -214,6 +217,7 @@ export class DropdownMenu extends BaseDropdown { | |||
this.actions = options.actions || []; | |||
this.actionProvider = options.actionProvider; | |||
this.menuClassName = options.menuClassName || ''; | |||
this.hideMenu = new RunOnceScheduler(() => this.onHide(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a scheduler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
constructor( | ||
action: GlobalActivityAction, | ||
colors: (theme: ITheme) => ICompositeBarColors, | ||
@IThemeService themeService: IThemeService, | ||
@IContextMenuService protected contextMenuService: IContextMenuService | ||
) { | ||
super(action, { draggable: false, colors, icon: true }, themeService); | ||
|
||
this.menuVisible = false; | ||
this.hideMenu = new RunOnceScheduler(() => this.hideContextMenu(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the scheduler needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
@@ -131,36 +137,56 @@ export class GlobalActivityActionItem extends ActivityActionItem { | |||
this._register(DOM.addDisposableListener(this.container, DOM.EventType.MOUSE_DOWN, (e: MouseEvent) => { | |||
DOM.EventHelper.stop(e, true); | |||
|
|||
const event = new StandardMouseEvent(e); | |||
this.showContextMenu({ x: event.posx, y: event.posy }); | |||
if (this.menuVisible) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is repeated 3 times and should be extracted to a method?
const globalAction = this._action as GlobalActivityAction; | ||
const activity = globalAction.activity as IGlobalActivity; | ||
const actions = activity.getActions(); | ||
const containerPosition = DOM.getDomNodePagePosition(this.container); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a fix that should be made separately, it seems to be around positioning the menu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be separated if needed. I was in the code area and the same menu had 2 issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
Moving to December. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
@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 |
@joaomoreno since you mentioned waiting to review, I will still not merge until I have heard from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Edge seems to follow this, too, so sounds good. |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const globalAction = this._action as GlobalActivityAction; | ||
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 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the new placement, but am open to hear alternatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the context menus (right-click) for activity bar viewlet icons match the new positioning. do you suggest changing them all?
@@ -78,6 +78,12 @@ export class ContextMenuHandler { | |||
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) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about an invisible div that will block the hover animations as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see latest version
assigned @RMacfarlane to test an issue in the PR extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
ping @joaomoreno |
* 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
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 :) |
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