From d8e83cfee0680afc8b2d5f8a817a501a0bfb14eb Mon Sep 17 00:00:00 2001 From: Aleck Greenham Date: Thu, 11 Jul 2019 06:31:51 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20#201:=20Always=20allow=20s?= =?UTF-8?q?ubmatches=20for=20combinations=20involving=20command?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../isMatchPossibleBasedOnNumberOfKeys.js | 6 +- src/lib/Configuration.js | 6 ++ .../strategies/AbstractKeyEventStrategy.js | 14 ++-- .../HoldingDownKeySharedByActions.spec.js | 70 +++++++++++++++++++ 4 files changed, 90 insertions(+), 6 deletions(-) create mode 100644 test/GlobalHotKeys/HoldingDownKeySharedByActions.spec.js diff --git a/src/helpers/resolving-handlers/isMatchPossibleBasedOnNumberOfKeys.js b/src/helpers/resolving-handlers/isMatchPossibleBasedOnNumberOfKeys.js index 4306cfce..5e1c4592 100644 --- a/src/helpers/resolving-handlers/isMatchPossibleBasedOnNumberOfKeys.js +++ b/src/helpers/resolving-handlers/isMatchPossibleBasedOnNumberOfKeys.js @@ -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 { /** diff --git a/src/lib/Configuration.js b/src/lib/Configuration.js index d44f7ef2..62b58032 100644 --- a/src/lib/Configuration.js +++ b/src/lib/Configuration.js @@ -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 always 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, diff --git a/src/lib/strategies/AbstractKeyEventStrategy.js b/src/lib/strategies/AbstractKeyEventStrategy.js index 4b0edc34..b74870d1 100644 --- a/src/lib/strategies/AbstractKeyEventStrategy.js +++ b/src/lib/strategies/AbstractKeyEventStrategy.js @@ -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]; @@ -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')) { diff --git a/test/GlobalHotKeys/HoldingDownKeySharedByActions.spec.js b/test/GlobalHotKeys/HoldingDownKeySharedByActions.spec.js new file mode 100644 index 00000000..e4b72e09 --- /dev/null +++ b/test/GlobalHotKeys/HoldingDownKeySharedByActions.spec.js @@ -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 () { + 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( + +
+ , + { 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; + }); + }); + }) + }); + }); +});