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 #150: Reset global handler resolution state when a new GlobalHo… #178

Merged
merged 2 commits into from
May 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/lib/strategies/AbstractKeyEventStrategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,14 @@ class AbstractKeyEventStrategy {
* @protected
*/
_initHandlerResolutionState() {
if (this.keyMaps === null) {
/**
* If this.keyMaps is already set to null, then the state has already been reset
* and we need not do it again
*/
return;
}

/**
* List of mappings from key sequences to handlers that is constructed on-the-fly
* as key events propagate up the render tree
Expand Down
5 changes: 5 additions & 0 deletions src/lib/strategies/GlobalKeyEventStrategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ class GlobalKeyEventStrategy extends AbstractKeyEventStrategy {

this._updateDocumentHandlers();

/**
* Reset handler resolution state
*/
this._initHandlerResolutionState();
Copy link
Owner Author

@greena13 greena13 May 18, 2019

Choose a reason for hiding this comment

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

✅⚔️Check the behaviour of this._updateDocumentHandlers() to make sure calling these methods in this sequence will always work as intended.

Copy link
Owner Author

@greena13 greena13 May 18, 2019

Choose a reason for hiding this comment

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

✅ 🚀See if there is a way of wrapping the resetting of state in this._initHandlerResolutionState() in a condition that will abort resetting the state if it's apparent handler resolution hasn't actually taken place yet, and the state is already set to the default values.


this.logger.debug(
this._logPrefix(componentId, {eventId: false}),
'Mounted.',
Expand Down
106 changes: 106 additions & 0 deletions test/GlobalHotKeys/MatchingKeyMapAfterRemount.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import {mount} from 'enzyme';
import {expect} from 'chai';
import sinon from 'sinon';
import simulant from 'simulant';
import React, {Component} from 'react'

import KeyCode from '../support/Key';
import {GlobalHotKeys} from '../../src';

describe('Matching key map after remount for a GlobalHotKeys component:', function () {
beforeEach(function () {
const keyMap = this.keyMap = {
'ACTION_A': 'a',
};

const keyMap2 = this.keyMap2 = {
'ACTION_B': 'b',
};

this.handler = sinon.spy();
this.handler2 = sinon.spy();

const handlers = this.handlers = {
'ACTION_A': this.handler,
};

const handlers2 = this.handlers2 = {
'ACTION_B': this.handler2,
};

class ToggleComponent extends Component {
render(){
const { secondKeyMapActive } = this.props;

return (
<div>
<GlobalHotKeys keyMap={keyMap} handlers={handlers} />
{ secondKeyMapActive && <GlobalHotKeys keyMap={keyMap2} handlers={handlers2} />}
</div>
);
}
}

this.reactDiv = document.createElement('div');
document.body.appendChild(this.reactDiv);

this.wrapper = mount(
<ToggleComponent secondKeyMapActive={true} />,
{ attachTo: this.reactDiv }
);
});

after(function() {
document.body.removeChild(this.reactDiv);
});

describe('when two GlobalHotKeys components are mounted, unmounted and remounted', () => {
it('then both of their key maps work while they are mounted and not, when they aren\'t (BUG: https://github.com/greena13/react-hotkeys/issues/150)', function() {
simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.A });
simulant.fire(this.reactDiv, 'keypress', { key: KeyCode.A });
simulant.fire(this.reactDiv, 'keyup', { key: KeyCode.A });

expect(this.handler).to.have.been.calledOnce;

simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.B });
simulant.fire(this.reactDiv, 'keypress', { key: KeyCode.B });
simulant.fire(this.reactDiv, 'keyup', { key: KeyCode.B });

expect(this.handler2).to.have.been.calledOnce;

/**
* Unmount the second GlobalHotKeys component
*/
this.wrapper.setProps({ secondKeyMapActive: false });

simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.A });
simulant.fire(this.reactDiv, 'keypress', { key: KeyCode.A });
simulant.fire(this.reactDiv, 'keyup', { key: KeyCode.A });

expect(this.handler).to.have.been.calledTwice;

simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.B });
simulant.fire(this.reactDiv, 'keypress', { key: KeyCode.B });
simulant.fire(this.reactDiv, 'keyup', { key: KeyCode.B });

expect(this.handler2).to.have.been.calledOnce;

/**
* Re-mount the second GlobalHotKeys component
*/
this.wrapper.setProps({ secondKeyMapActive: true });

simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.A });
simulant.fire(this.reactDiv, 'keypress', { key: KeyCode.A });
simulant.fire(this.reactDiv, 'keyup', { key: KeyCode.A });

expect(this.handler).to.have.been.calledThrice;

simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.B });
simulant.fire(this.reactDiv, 'keypress', { key: KeyCode.B });
simulant.fire(this.reactDiv, 'keyup', { key: KeyCode.B });

expect(this.handler2).to.have.been.calledTwice;
});
});
});