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

Fix remapped shortcuts not getting activated in succession #4712

Merged

Conversation

arjunbalgovind
Copy link
Contributor

Summary of the Pull Request

This PR fixes a scenario where a user could remap Alt+A->Ctrl+A, Alt+V->Ctrl+V and if you press Alt+A, release A and press V, Ctrl+V was not getting invoked. This was happening because when we fixed Alt+Tab style shortcuts in #3965, we changed the behavior such that we release the remapped modifier only after a key apart from the invoked shortcut's keys are pressed. When this would happen, we would reset the keyboard state to whichever keys should be physically pressed at that time, however we were sending all the keys including the latest pressed key (V in the example), with the KEYBOARDMANAGER_SHORTCUT_FLAG, because of which when the system received Alt+V it would not remap it to Ctrl+V. The fix involves removing the flag for that particular key event. We still keep the shortcut flag for all the key events before that so that we don't reinvoke the initial shortcut and cause an infinite recursion.
Comments have been updated to clarify the flag usage and two test cases have been added to capture the scenario.

PR Checklist

Validation Steps Performed

Added test cases and manually validated with the repro steps in the issue

@arjunbalgovind arjunbalgovind added the Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager label Jul 2, 2020
@arjunbalgovind arjunbalgovind requested a review from a team July 2, 2020 22:25
@saahmedm saahmedm added the Hot Fix Items we will product an out-of-band release for label Jul 2, 2020
Copy link
Contributor

@alekhyareddy28 alekhyareddy28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Validated that it fires the new shortcut which has been remapped.

@arjunbalgovind
Copy link
Contributor Author

@enricogior should I merge this normally? The first commit is the fix and the second commit is for test cases. I think the second commit depends on another PR in master so it would be better to only cherry-pick the first commit to stable.

@enricogior
Copy link
Contributor

@arjunbalgovind
you can use "Rebase and merge", it will preserve the two commits.

@arjunbalgovind arjunbalgovind merged commit cffe019 into microsoft:master Jul 3, 2020
@arjunbalgovind
Copy link
Contributor Author

Merged. ac2a9de is the commit to be cherry-picked

@enricogior
Copy link
Contributor

@arjunbalgovind
done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hot Fix Items we will product an out-of-band release for Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants