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 #201: Always allow submatches for combinations involving command #207

Merged
merged 1 commit into from
Jul 11, 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
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,17 @@ import Configuration from '../../lib/Configuration';
* in the key event.
* @param {ActionConfiguration} combinationMatcher Matcher to compare to the
* key state
* @param {boolean} keyupIsHiddenByCmd Whether current combination involves the
* cmd key and keys for which it hides their keyup event
* @returns {boolean} True if the key state has the right amount of keys for a
* match with combinationMatcher to be possible
* @private
*/
function isMatchPossibleBasedOnNumberOfKeys(keyState, combinationMatcher) {
function isMatchPossibleBasedOnNumberOfKeys(keyState, combinationMatcher, keyupIsHiddenByCmd) {
const keyStateKeysNo = Object.keys(keyState.keys).length;
const combinationKeysNo = Object.keys(combinationMatcher.keyDictionary).length;

if (Configuration.option('allowCombinationSubmatches')) {
if (keyupIsHiddenByCmd || Configuration.option('allowCombinationSubmatches')) {
return keyStateKeysNo >= combinationKeysNo;
} else {
/**
Expand Down
6 changes: 6 additions & 0 deletions src/lib/Configuration.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ const _defaultConfiguration = {
* Whether to allow combination submatches - e.g. if there is an action bound to
* cmd, pressing shift+cmd will *not* trigger that action when
* allowCombinationSubmatches is false.
*
* @note This option is ignored for combinations involving command (Meta) and
* submatches are <i>always</i> allowed because Meta hides keyup events
* of other keys, so until Command is released, it's impossible to know
* if one of the keys that has also been pressed has been released.
* @see https://github.com/greena13/react-hotkeys/pull/207
* @type {Boolean}
*/
allowCombinationSubmatches: false,
Expand Down
14 changes: 10 additions & 4 deletions src/lib/strategies/AbstractKeyEventStrategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -789,14 +789,13 @@ class AbstractKeyEventStrategy {
return !keyHasNativeKeypress || (keyHasNativeKeypress && this._keyIsCurrentlyDown('Meta'));
} else if (eventType === KeyEventRecordIndex.keyup) {
return (keyupIsHiddenByCmd(keyName) && keyIsCurrentlyTriggeringEvent(
this._getCurrentKeyState('Meta'),
KeyEventRecordIndex.keyup)
this._getCurrentKeyState('Meta'),
KeyEventRecordIndex.keyup)
);
}

return false
}

_cloneAndMergeEvent(event, extra) {
const eventAttributes = Object.keys(ModifierFlagsDictionary).reduce((memo, eventAttribute) => {
memo[eventAttribute] = event[eventAttribute];
Expand Down Expand Up @@ -1069,7 +1068,14 @@ class AbstractKeyEventStrategy {
const combinationId = combinationOrder[combinationIndex];
const combinationMatcher = matchingSequence.combinations[combinationId];

if (isMatchPossibleBasedOnNumberOfKeys(currentKeyState, combinationMatcher)) {
const cmdKeyIsPressed =
this._getCurrentKeyState('Meta') && !keyIsCurrentlyTriggeringEvent(this._getCurrentKeyState('Meta'), KeyEventRecordIndex.keyup);

const keyupIsHidden = cmdKeyIsPressed && Object.keys(combinationMatcher.keyDictionary).some((keyName) => {
return keyupIsHiddenByCmd(keyName);
});

if (isMatchPossibleBasedOnNumberOfKeys(currentKeyState, combinationMatcher, keyupIsHidden)) {
if (this._combinationMatchesKeys(normalizedKeyName, currentKeyState, combinationMatcher, eventRecordIndex)) {

if (Configuration.option('allowCombinationSubmatches')) {
Expand Down
70 changes: 70 additions & 0 deletions test/GlobalHotKeys/HoldingDownKeySharedByActions.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import React from 'react';
import {mount} from 'enzyme';
import {expect} from 'chai';
import sinon from 'sinon';
import simulant from 'simulant';

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

describe('Holding down key shared by actions:', function () {
Copy link
Owner Author

Choose a reason for hiding this comment

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

⬜️ Make this test specific to Cmd

after(function() {
configure({allowCombinationSubmatches: false });
});

[true, false].forEach((allowCombinationSubmatches) => {
describe(`when allowCombinationSubmatches is ${allowCombinationSubmatches}`, () => {
before(function(){
configure({allowCombinationSubmatches: allowCombinationSubmatches });
});

describe('and there are two actions with combinations that involve cmd (cmd+a and cmd+b) (https://github.com/greena13/react-hotkeys/issues/201)', function () {
beforeEach(function () {
this.keyMap = {
'ACTION1': 'cmd+a',
'ACTION2': 'cmd+b',
};

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

const handlers = {
'ACTION1': this.handler1,
'ACTION2': this.handler2
};

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

this.wrapper = mount(
<GlobalHotKeys keyMap={ this.keyMap } handlers={ handlers }>
<div className="childElement" />
</GlobalHotKeys>,
{ attachTo: this.reactDiv }
);
});

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

describe('and cmd and \'a\' are pressed, and \'a\' is released and \'b\' is pressed instead', function () {
it('then both actions are triggered', function() {
simulant.fire(this.reactDiv, 'keydown', { key: KeyCode.COMMAND });

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

expect(this.handler1).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.COMMAND });

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