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.

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 window.disableMenuBarAltBehavior option #73258

Merged
merged 5 commits into from
Jun 14, 2019
Merged

add window.disableMenuBarAltBehavior option #73258

merged 5 commits into from
Jun 14, 2019

Conversation

risen228
Copy link
Contributor

@risen228 risen228 commented May 3, 2019

Add window.disableMenuBarAltBehavior option

Background:

Having installed Ubuntu 19.04, I ran into the issue that with a custom MenuBar, pressing Alt key caused the focus to be shifted to the Menu Bar. Thus, there was a loss of focus in the editor. I could not find an option to fix this error (window.enableMenuBarMnemonics set to 'false' did not help, and I couldn’t hide the Menu Bar - I use it often).

Implementation:

Add a new option 'window.disableMenuBarAltBehavior' that allows to disable the Alt key interaction with the Menu Bar. This option is like 'window.enableMenuBarMnemonics' set to 'false' but prevents some Menu Bar events to be continued at all when the main key used in event is Alt.

@msftclas
Copy link

msftclas commented May 3, 2019

CLA assistant check
All CLA requirements met.

@sbatten
Copy link
Member

sbatten commented May 6, 2019

Hi @risenforces, Thanks for the PR. I do want to add this feature but we want to make sure that Alt key still works when the menubar is set to toggle. I'm torn between 2 versions of this setting:

  1. A boolean value like you have. True means that alt will not focus the menubar unless it is set to toggle, in which case the menubar will appear and be focused. If they don't want the menubar to ever appear, they should switch to hidden.

  2. An enum value where the options are 'always', 'toggleOnly', and 'never'. Always will enable alt always, 'toggleOnly' will work like above, and 'never' will work like your PR.

Let me know your thoughts

@sbatten sbatten added this to the May 2019 milestone May 6, 2019
@risen228
Copy link
Contributor Author

risen228 commented May 8, 2019

@sbatten At first, I thought that the enum version is more logical, but when I started to implement it to check in action, I realized that for the most part, the options 'always' and 'toggleOnly' are identical if 'window.menuBarVisibility' is set to 'toggle' .

They would be different if 'toggleOnly' option was forcing the menu bar to appear, but was preventing focus to change. But this version is a bit confusing, so it's not a good variant.

Therefore, it seems to me that it is better to leave the boolean option, but add an additional check this.menuBarVisibility !== 'toggle'.

If the user wants to disable the Alt key interaction with the Menu Bar, he will not set the 'toggle' mode anyway, because, in its nature, it implies the use of the Alt key, which exactly the user wants to avoid. So, these options cannot live together.

So, in my opinion, the first version is better. What do you think?

@sbatten
Copy link
Member

sbatten commented May 20, 2019

@risenforces I agree that this way makes sense. One other concern is that this setting only applies to the custom menu bar. I see you have documented this in the settings description, but perhaps the setting name should also reflect this?

…uBarAltBehavior' and change its description
@risen228
Copy link
Contributor Author

risen228 commented May 21, 2019

@sbatten Is it better?

@sbatten
Copy link
Member

sbatten commented May 28, 2019

@risenforces I will merge this next week for June milestone so that we can have a full month of trying out the setting in insiders

@sbatten sbatten modified the milestones: May 2019, June 2019 May 28, 2019
@sbatten
Copy link
Member

sbatten commented Jun 14, 2019

@risenforces I updated the PR for merge conflicts and notices that this change was also disabling mnemonics. Since there is already a setting for this, I think this new setting should only affect the focusing of the menu bar. I tweaked your PR for this. I'm going to merge, but if you have any additional comments, we can work on those after.

@sbatten sbatten merged commit 1151dab into microsoft:master Jun 14, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants