Fix #166: Fix over-zealous modifier flag discrepancy correction #185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
greena13
commented
May 30, 2019
| @@ -1111,17 +1111,28 @@ class AbstractKeyEventStrategy { | |||
| * Synchronises the key combination history to match the modifier key flag attributes | |||
| * on new key events | |||
| * @param {KeyboardEvent} event - Event to check the modifier flags for | |||
| * @param {String} key - Name of key that events relates to | |||
| * @param {KeyEventBitmapIndex} keyEventBitmapIndex - Index of event type | |||
There was a problem hiding this comment.
✅ Improve this parameter description
| /** | ||
| * When a modifier key is being released (keyup), it sets its own modifier flag | ||
| * to false. (e.g. On the keyup event for Command, the metaKey attribute is false). | ||
| * If this the case, we want to handle it using the main algorithm. |
There was a problem hiding this comment.
✅ Expand this comment:
... and skip the reconciliation algorithm
| @@ -301,7 +301,7 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { | |||
| return true; | |||
| } | |||
|
|
|||
| const responseAction = this._howToHandleKeyDownEvent(event, | |||
| const responseAction = this._howToHandleKeyEvent(event, | |||
There was a problem hiding this comment.
🐥Renamed this function to more accurately reflect its conceptual level of abstraction (it doesn't just deal with keydown events)
| this.targetElement.focus(); | ||
| }); | ||
|
|
||
| it('then simulates the cmd keypress event and the non-modifier key\'s keypress event (https://github.com/greena13/react-hotkeys/issues/166)', function () { |
There was a problem hiding this comment.
✅ Fix this test description. (Copy and paste error.)
| import FocusableElement from '../support/FocusableElement'; | ||
|
|
||
| describe('Binding to keys with simulated keypress events:', function () { | ||
| context('when HotKeys has actions for a the keydown and keyup event', () => { |
There was a problem hiding this comment.
✅ Improve this context description - could be more specific.
| @@ -28,26 +28,26 @@ export default class FocusableElement { | |||
| } | |||
|
|
|||
| keyDown(key, options = {}) { | |||
| this.element.simulate('keyDown', {key}); | |||
| this.element.simulate('keyDown', {key, ...options}); | |||
There was a problem hiding this comment.
🐥 Allows passing additional KeyboardEvent attributes when writing tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
There is a subroutine that is run on the
keyupevent that checks the currentKeyBoardEvent's modifier key flags (e.g.metaKey) and compares it against the key state thatreact-hotkeysis maintaining. It then corrects for any discrepancies (missed key events, most likely due to a focus loss) in thereact-hotkeysrecord of key state.On the
keyupevent for a modifier key (e.g. Command/Meta), its own corresponding modifier key flag (e.g.metaKey) is false. This was causing registering the key as being in thekeyupstate twice, resulting in #166 whereby handlers bound to thekeyupevent of theMetakey were not being called.This pull request
keyupstate, and skips running the subroutine