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

Fix menubar toggle alt key detection on focus #12167

Merged
merged 1 commit into from Mar 9, 2018

Conversation

Projects
None yet
5 participants
@lyallh
Contributor

lyallh commented Mar 8, 2018

Reset alt keypress flag on window blur so switching window via
Alt+* desktop / window manager keybindings can't incedentally trigger
annoying menubar toggles.

This issue is a recurring complaint among some Atom and Vscode editor users. Especially in Vscode where you can't remap the Alt key.
(Microsoft/vscode#35010, Microsoft/vscode#41070, atom/atom#6037)

A description of the problem was given in PR #3218 but no fix made.

@lyallh lyallh requested a review from electron/reviewers as a code owner Mar 8, 2018

@welcome

This comment has been minimized.

welcome bot commented Mar 8, 2018

💖 Thanks for opening this pull request! 💖

typing cat

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
@lyallh

This comment has been minimized.

Contributor

lyallh commented Mar 8, 2018

Needs review as I'm not exactly sure what the conditions are under OnWidgetActivationStateChanged. Guessing just focus/blur change.

Fix menubar toggle alt key detection on focus
Reset alt keypress flag on window blur so switching window via
Alt+* window manager keybindings can't incedentally trigger
annoying menubar toggles
@codebytere

looks good to me, thanks!

@codebytere

This comment has been minimized.

Member

codebytere commented Mar 8, 2018

i'll leave this up for more review though from someone who would better know conditions leading to OnWidgetActivationStateChanged

@zcbenz

zcbenz approved these changes Mar 9, 2018

I'm good with this change, I'm not entirely sure whether it would have side effects, but it looks fine.

@zcbenz zcbenz merged commit 44c66fc into electron:master Mar 9, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins: macOS/pr-head This commit looks good
Details
@welcome

This comment has been minimized.

welcome bot commented Mar 9, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@lyallh

This comment has been minimized.

Contributor

lyallh commented Mar 9, 2018

Thanks @codebytere and @zcbenz

Flag seems to be used only for menu toggling and that now works as intended so should be fine.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Mar 9, 2018

Should we backport it to the 2-0-x to get feedback sooner?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Mar 13, 2018

#trop run backport

@trop-devel

This comment has been minimized.

trop-devel bot commented Mar 13, 2018

The backport process for this PR has been manually initiated, here we go :D

@trop-devel

This comment has been minimized.

trop-devel bot commented Mar 13, 2018

An error occurred while attempting to backport this PR to "2-0-x", you will need to perform this backport manually

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Mar 13, 2018

#trop run backport

@trop-devel

This comment has been minimized.

trop-devel bot commented Mar 13, 2018

The backport process for this PR has been manually initiated, here we go :D

@trop-devel

This comment has been minimized.

trop-devel bot commented Mar 13, 2018

We have automatically backported this PR to "2-0-x", please check out #12235

@trop-devel trop-devel bot added merged/2-0-x and removed target/2-0-x labels Mar 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment