From 74b3073a8b695b7696268e4d84223141eb4df05c Mon Sep 17 00:00:00 2001 From: Aleck Greenham Date: Sun, 2 Jun 2019 17:30:24 +0100 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20Fix=20#187:=20Add=20root=20p?= =?UTF-8?q?rop=20to=20HotKeys=20to=20allow=20root=20HotKeys=20components?= =?UTF-8?q?=20to=20capture=20all=20events?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 17 +++++ index.d.ts | 5 ++ .../strategies/FocusOnlyKeyEventStrategy.js | 40 ++++++++-- src/withHotKeys.js | 11 ++- test/HotKeys/focus-only/RootProp.spec.js | 73 +++++++++++++++++++ 5 files changed, 136 insertions(+), 10 deletions(-) create mode 100644 test/HotKeys/focus-only/RootProp.spec.js diff --git a/README.md b/README.md index 4e449fe4..4239966b 100644 --- a/README.md +++ b/README.md @@ -131,6 +131,7 @@ export default MyNode; - [Other keyboard event listeners are no longer being triggered](#other-keyboard-event-listeners-are-no-longer-being-triggered) - [Actions aren't being triggered when using withHotKeys](#actions-arent-being-triggered-when-using-withhotkeys) - [Actions aren't being triggered for HotKeys](#actions-arent-being-triggered-for-hotkeys) + - [React Hotkeys thinks I'm holding down a key I've released](#react-hotkeys-thinks-im-holding-down-a-key-ive-released) - [Blue border appears around children of HotKeys](#blue-border-appears-around-children-of-hotkeys) - [Stability & Maintenance](#stability--maintenance) - [Contribute, please!](#contribute-please) @@ -453,6 +454,12 @@ However, it [does require that its children be wrapped in a DOM-mounted node](#H * to get a reference to the node, so you can call .focus() on it */ innerRef: {undefined} + + /** + * Whether this is the root HotKeys node - this enables some special + * behaviour + */ + root={false} > /** @@ -1220,6 +1227,16 @@ Also make sure your React application is not calling `stopPropagation()` on the Finally, make sure your key event are not coming from one of the [tags ignored by react-hotkeys](#Ignoring-events). +#### React Hotkeys thinks I'm holding down a key I've released + +This can happen when you have an action handler that either unmounts the `` component, or focuses an area of the application where there is no parent ``. The solution to this is to add a `` to the top of your application that renders it as children - or at least high enough to *not* be unmounted or unfocused by your action handler. + +```javascript + + // The parts of your application that are re-rendered or unfocused here + +``` + #### Blue border appears around children of HotKeys `react-hotkeys` adds a `
` around its children with a `tabindex="-1"` to allow them to be programmatically focused. This can result in browsers rendering a blue outline around them to visually indicate that they are the elements in the document that is currently in focus. diff --git a/index.d.ts b/index.d.ts index 28b250bb..927a2be9 100644 --- a/index.d.ts +++ b/index.d.ts @@ -53,6 +53,11 @@ export interface HotKeysEnabledProps extends GlobalHotKeysProps { * Function to call when this component loses focus in the browser */ onBlur?: () => void; + + /** + * Whether this is the root HotKeys node - this enables some special behaviour + */ + root?: boolean; } /** diff --git a/src/lib/strategies/FocusOnlyKeyEventStrategy.js b/src/lib/strategies/FocusOnlyKeyEventStrategy.js index 61a780c8..cabe147b 100644 --- a/src/lib/strategies/FocusOnlyKeyEventStrategy.js +++ b/src/lib/strategies/FocusOnlyKeyEventStrategy.js @@ -309,6 +309,14 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { KeyEventBitmapIndex.keydown ); + /** + * We need to record the position of the component that is currently dealing with + * the event, in case the component defines a handler for that event that changes + * the focus or content in the render tree, causing the component to be de-registered + * and have its position lost + */ + const componentPosition = this._getComponentPosition(componentId); + if (responseAction === EventResponse.handled) { const keyInCurrentCombination = !!this._getCurrentKeyState(_key); @@ -323,7 +331,7 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { this._simulateKeyPressesMissingFromBrowser(event, _key, focusTreeId, componentId, options); - this._updateEventPropagationHistory(componentId); + this._updateEventPropagationHistory(componentId, componentPosition); return false; } @@ -417,6 +425,14 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { ); } + /** + * We need to record the position of the component that is currently dealing with + * the event, in case the component defines a handler for that event that changes + * the focus or content in the render tree, causing the component to be de-registered + * and have its position lost + */ + const componentPosition = this._getComponentPosition(componentId); + /** * We attempt to find a handler of the event, only if it has not already * been handled and should not be ignored @@ -431,7 +447,7 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { ); } - this._updateEventPropagationHistory(componentId); + this._updateEventPropagationHistory(componentId, componentPosition); return shouldDiscardFocusTreeId; } @@ -486,6 +502,14 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { ); } + /** + * We need to record the position of the component that is currently dealing with + * the event, in case the component defines a handler for that event that changes + * the focus or content in the render tree, causing the component to be de-registered + * and have its position lost + */ + const componentPosition = this._getComponentPosition(componentId); + /** * We attempt to find a handler of the event, only if it has not already * been handled and should not be ignored @@ -500,7 +524,7 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { */ this._simulateKeyUpEventsHiddenByCmd(event, _key, focusTreeId, componentId, options); - this._updateEventPropagationHistory(componentId); + this._updateEventPropagationHistory(componentId, componentPosition); return shouldDiscardFocusId; } @@ -544,10 +568,12 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { _ignoreEvent(event, componentId) { this.currentEvent.ignored = true; + const componentPosition = this._getComponentPosition(componentId); + if(this._stopEventPropagationAfterIgnoringIfEnabled(event, componentId)) { - this._updateEventPropagationHistory(componentId, { forceReset: true }); + this._updateEventPropagationHistory(componentId, componentPosition, { forceReset: true }); } else { - this._updateEventPropagationHistory(componentId); + this._updateEventPropagationHistory(componentId, componentPosition); } } @@ -577,11 +603,11 @@ class FocusOnlyKeyEventStrategy extends AbstractKeyEventStrategy { return previousComponentPosition === -1 || previousComponentPosition >= this._getComponentPosition(componentId); } - _updateEventPropagationHistory(componentId, options = { forceReset: false }) { + _updateEventPropagationHistory(componentId, componentPosition, options = { forceReset: false }) { if (options.forceReset || this._isFocusTreeRoot(componentId)) { this._clearEventPropagationState(); } else { - this.eventPropagationState.previousComponentPosition = this._getComponentPosition(componentId); + this.eventPropagationState.previousComponentPosition = componentPosition; } } diff --git a/src/withHotKeys.js b/src/withHotKeys.js index 2b0e6119..cf44e6f7 100644 --- a/src/withHotKeys.js +++ b/src/withHotKeys.js @@ -113,7 +113,12 @@ function withHotKeys(Component, hotKeysOptions = {}) { * component mounts. If false, changes to the keyMap and handlers * props will be ignored */ - allowChanges: PropTypes.bool + allowChanges: PropTypes.bool, + + /** + * Whether this is the root HotKeys node - this enables some special behaviour + */ + root: PropTypes.bool }; constructor(props) { @@ -152,7 +157,7 @@ function withHotKeys(Component, hotKeysOptions = {}) { * Props used by HotKeys that should not be passed down to its focus trap * component */ - keyMap, handlers, allowChanges, + keyMap, handlers, allowChanges, root, ...props } = this.props; @@ -181,7 +186,7 @@ function withHotKeys(Component, hotKeysOptions = {}) { _shouldBindKeyListeners() { const keyMap = getKeyMap(this.props); - return !isEmpty(keyMap) || ( + return !isEmpty(keyMap) || this.props.root || ( Configuration.option('enableHardSequences') && this._handlersIncludeHardSequences(keyMap, getHandlers(this.props)) ); } diff --git a/test/HotKeys/focus-only/RootProp.spec.js b/test/HotKeys/focus-only/RootProp.spec.js new file mode 100644 index 00000000..baff57ff --- /dev/null +++ b/test/HotKeys/focus-only/RootProp.spec.js @@ -0,0 +1,73 @@ +import React from 'react'; +import {mount} from 'enzyme'; +import {expect} from 'chai'; +import sinon from 'sinon'; + +import FocusableElement from '../../support/FocusableElement'; +import KeyEventManager from '../../../src/lib/KeyEventManager'; +import KeyCode from '../../support/Key'; + +import {HotKeys} from '../../../src/'; + +describe('HotKeys root prop:', function () { + describe('when a HotKeys component has a root prop value of true', function () { + beforeEach(function () { + this.keyMap = { + 'NEXT': 'a', + }; + + this.nextHandler = sinon.spy(); + + const handlers = { + 'NEXT': () => { + this.secondElement.focus(); + + this.nextHandler(); + } + }; + + this.wrapper = mount( + + +
+ + +
+ + ); + + this.firstElement = new FocusableElement(this.wrapper, '.firstChildElement'); + this.secondElement = new FocusableElement(this.wrapper, '.secondChildElement'); + + this.firstElement.focus(); + + expect(this.firstElement.isFocused()).to.equal(true); + }); + + describe('and nested HotKeys component has a handler that changes focus to another element bound to keydown', function () { + it('then the root HotKeys still records the keyup event', function() { + this.firstElement.keyDown(KeyCode.A); + + expect(this.nextHandler).to.have.been.calledOnce; + + expect(this.secondElement.isFocused()).to.equal(true); + + this.secondElement.keyPress(KeyCode.A); + this.secondElement.keyUp(KeyCode.A); + + expect(KeyEventManager.getInstance()._focusOnlyEventStrategy.keyCombinationHistory).to.eql([ + { + 'keys': { + 'a': [ + [true, true, false], + [true, true, true] + ] + }, + 'ids': ['a'], + 'keyAliases': {} + } + ]); + }); + }); + }); +}); From 60ae3b99f171cc7c9b30a39040ecdc3da2af71ec Mon Sep 17 00:00:00 2001 From: Aleck Greenham Date: Sun, 2 Jun 2019 17:46:36 +0100 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=92=AC=20=F0=9F=93=9A=20Improve=20tro?= =?UTF-8?q?ubleshooting=20step=20description?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4239966b..d08a905a 100644 --- a/README.md +++ b/README.md @@ -1229,7 +1229,7 @@ Finally, make sure your key event are not coming from one of the [tags ignored b #### React Hotkeys thinks I'm holding down a key I've released -This can happen when you have an action handler that either unmounts the `` component, or focuses an area of the application where there is no parent ``. The solution to this is to add a `` to the top of your application that renders it as children - or at least high enough to *not* be unmounted or unfocused by your action handler. +This can happen when you have an action handler that either unmounts the `` component, or focuses an area of the application where there is no ancestor ``. The solution is to add a `` component with the `root` prop to the top of your application - or at least high enough to *not* be unmounted or unfocused by your action handler. ```javascript