From 18be748d9473a18addc9d37669dbb66927338c8a Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:08:14 -0800 Subject: [PATCH 01/14] Install shim-keyboard-event-key to support older browsers --- package-lock.json | 5 +++++ package.json | 1 + src/setup/platformSetup/index.website.js | 4 ++++ 3 files changed, 10 insertions(+) diff --git a/package-lock.json b/package-lock.json index 3d64add44978..8718532d9388 100644 --- a/package-lock.json +++ b/package-lock.json @@ -38710,6 +38710,11 @@ "dev": true, "optional": true }, + "shim-keyboard-event-key": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/shim-keyboard-event-key/-/shim-keyboard-event-key-1.0.3.tgz", + "integrity": "sha512-PTNRkOxDlZ2+Xz4CbKJJsh/pe1DJdaC+b4HHV02A1aEWNmwh1g9am0ZiU/ktu3uVfQrY3yDHTOVhst3xpLhw2A==" + }, "side-channel": { "version": "1.0.3", "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.0.3.tgz", diff --git a/package.json b/package.json index bc9856cb20e2..4284ac63a39c 100644 --- a/package.json +++ b/package.json @@ -106,6 +106,7 @@ "react-web-config": "^1.0.0", "rn-fetch-blob": "^0.12.0", "save": "^2.4.0", + "shim-keyboard-event-key": "^1.0.3", "smoothscroll-polyfill": "^0.4.4", "underscore": "^1.13.1", "urbanairship-react-native": "^11.0.2" diff --git a/src/setup/platformSetup/index.website.js b/src/setup/platformSetup/index.website.js index 2913d8ea52ea..e8e9c759b100 100644 --- a/src/setup/platformSetup/index.website.js +++ b/src/setup/platformSetup/index.website.js @@ -1,4 +1,8 @@ import {AppRegistry} from 'react-native'; + +// This is a polyfill for InternetExplorer to support the modern KeyboardEvent.key and KeyboardEvent.code instead of KeyboardEvent.keyCode +import 'shim-keyboard-event-key'; + import checkForUpdates from '../../libs/checkForUpdates'; import Config from '../../CONFIG'; import HttpUtils from '../../libs/HttpUtils'; From 7f88aaf5e4833d4547e1b473a5c0fe542bee231c Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:12:40 -0800 Subject: [PATCH 02/14] Rename poorly names eventHandler map --- src/libs/KeyboardShortcut/index.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 8bb508a3e493..de9778446767 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -3,7 +3,7 @@ import lodashGet from 'lodash/get'; import getOperatingSystem from '../getOperatingSystem'; import CONST from '../../CONST'; -const events = {}; +const eventHandlers = {}; const keyboardShortcutMap = {}; /** @@ -20,11 +20,11 @@ function getKeyboardShortcuts() { * @private */ function bindHandlerToKeyupEvent(event) { - if (events[event.keyCode] === undefined) { + if (eventHandlers[event.keyCode] === undefined) { return; } - const eventCallbacks = events[event.keyCode]; + const eventCallbacks = eventHandlers[event.keyCode]; const reversedEventCallbacks = [...eventCallbacks].reverse(); // Loop over all the callbacks @@ -116,7 +116,7 @@ function getKeyCode(key) { */ function unsubscribe(key) { const keyCode = getKeyCode(key); - events[keyCode].pop(); + eventHandlers[keyCode].pop(); } /** @@ -156,10 +156,10 @@ function addKeyToMap(key, modifiers, descriptionKey) { */ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false) { const keyCode = getKeyCode(key); - if (events[keyCode] === undefined) { - events[keyCode] = []; + if (eventHandlers[keyCode] === undefined) { + eventHandlers[keyCode] = []; } - events[keyCode].push({callback, modifiers: _.isArray(modifiers) ? modifiers : [modifiers], captureOnInputs}); + eventHandlers[keyCode].push({callback, modifiers: _.isArray(modifiers) ? modifiers : [modifiers], captureOnInputs}); if (descriptionKey) { addKeyToMap(key, modifiers, descriptionKey); From af52c65cdee455d7d0fc0c5ced5bc9e9b7d04de1 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:20:55 -0800 Subject: [PATCH 03/14] Use key+modifiers as keys in callback map --- src/libs/KeyboardShortcut/index.js | 144 +++++++++++++---------------- 1 file changed, 66 insertions(+), 78 deletions(-) diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index de9778446767..784facbf4be7 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -1,5 +1,6 @@ import _ from 'underscore'; import lodashGet from 'lodash/get'; +import Str from 'expensify-common/lib/str'; import getOperatingSystem from '../getOperatingSystem'; import CONST from '../../CONST'; @@ -14,13 +15,59 @@ function getKeyboardShortcuts() { return _.values(keyboardShortcutMap); } +/** + * Gets modifiers from a keyboard event. + * + * @param {Event} event + * @returns {Array} + */ +function getKeyEventModifiers(event) { + const modifiers = []; + if (event.shiftKey) { + modifiers.push('SHIFT'); + } + if (event.ctrlKey) { + modifiers.push('CONTROL'); + } + if (event.altKey) { + modifiers.push('ALT'); + } + if (event.metaKey) { + modifiers.push('META'); + } + return modifiers; +} + +/** + * Generates the normalized display name for keyboard shortcuts. + * + * @param {String} key + * @param {String|Array} modifiers + * @returns {String} + */ +function getDisplayName(key, modifiers) { + let displayName = [key.toUpperCase()]; + if (_.isString(modifiers)) { + displayName.unshift(modifiers); + } else if (_.isArray(modifiers)) { + displayName = [..._.sortBy(modifiers), ...displayName]; + } + + displayName = _.map(displayName, modifier => lodashGet(CONST.KEYBOARD_SHORTCUT_KEY_DISPLAY_NAME, modifier.toUpperCase(), modifier)); + + return displayName.join(' + '); +} + /** * Checks if an event for that key is configured and if so, runs it. * @param {Event} event * @private */ function bindHandlerToKeyupEvent(event) { - if (eventHandlers[event.keyCode] === undefined) { + const eventModifiers = getKeyEventModifiers(event); + const displayName = getDisplayName(event.key.toUpperCase(), eventModifiers); + + if (!_.has(eventHandlers, displayName)) { return; } @@ -29,44 +76,6 @@ function bindHandlerToKeyupEvent(event) { // Loop over all the callbacks _.every(reversedEventCallbacks, (callback) => { - const pressedModifiers = _.all(callback.modifiers, (modifier) => { - if (modifier === 'shift' && !event.shiftKey) { - return false; - } - if (modifier === 'control' && !event.ctrlKey) { - return false; - } - if (modifier === 'alt' && !event.altKey) { - return false; - } - if (modifier === 'meta' && !event.metaKey) { - return false; - } - return true; - }); - - const extraModifiers = _.difference(['shift', 'control', 'alt', 'meta'], callback.modifiers); - - // returns true if extra modifiers are pressed - const pressedExtraModifiers = _.some(extraModifiers, (extraModifier) => { - if (extraModifier === 'shift' && event.shiftKey) { - return true; - } - if (extraModifier === 'control' && event.ctrlKey) { - return true; - } - if (extraModifier === 'alt' && event.altKey) { - return true; - } - if (extraModifier === 'meta' && event.metaKey) { - return true; - } - return false; - }); - if (!pressedModifiers || pressedExtraModifiers) { - return true; - } - // If configured to do so, prevent input text control to trigger this event if (!callback.captureOnInputs && ( event.target.nodeName === 'INPUT' @@ -91,32 +100,14 @@ document.removeEventListener('keydown', bindHandlerToKeyupEvent, {capture: true} document.addEventListener('keydown', bindHandlerToKeyupEvent, {capture: true}); /** - * Returns keyCode for a given key - * @param {String} key The key to watch, i.e. 'K' or 'Escape' - * @returns {Number} The key's keyCode, i.e. 75 or 27 - * @private - */ -function getKeyCode(key) { - // For keys that have longer names we must catch and return the correct key key.charCodeAt(0) would return the - // key code for 'E' (the letter at index 0 in the string) not 'Escape' - switch (key) { - case 'Enter': - return 13; - case 'Escape': - return 27; - default: - return key.charCodeAt(0); - } -} - -/** - * Unsubscribes to a keyboard event. - * @param {Number} key The key to stop watching + * Unsubscribes a keyboard event handler. + * + * @param {String} displayName The display name for the key combo to stop watching + * @param {String} callbackID The specific ID given to the callback at the time it was added * @private */ -function unsubscribe(key) { - const keyCode = getKeyCode(key); - eventHandlers[keyCode].pop(); +function unsubscribe(displayName, callbackID) { + eventHandlers[displayName] = _.reject(eventHandlers[displayName], callback => callback.id === callbackID); } /** @@ -127,16 +118,7 @@ function unsubscribe(key) { * @param {String} descriptionKey Translation key for shortcut description */ function addKeyToMap(key, modifiers, descriptionKey) { - let displayName = [key]; - if (_.isString(modifiers)) { - displayName.unshift(modifiers); - } else if (_.isArray(modifiers)) { - displayName = [...modifiers, ...displayName]; - } - - displayName = _.map(displayName, modifier => lodashGet(CONST.KEYBOARD_SHORTCUT_KEY_DISPLAY_NAME, modifier.toUpperCase(), modifier)); - - displayName = displayName.join(' + '); + const displayName = getDisplayName(key, modifiers); keyboardShortcutMap[displayName] = { shortcutKey: key, descriptionKey, @@ -155,16 +137,22 @@ function addKeyToMap(key, modifiers, descriptionKey) { * @returns {Function} clean up method */ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false) { - const keyCode = getKeyCode(key); - if (eventHandlers[keyCode] === undefined) { - eventHandlers[keyCode] = []; + const displayName = getDisplayName(key, modifiers); + if (!_.has(eventHandlers, displayName)) { + eventHandlers[displayName] = []; } - eventHandlers[keyCode].push({callback, modifiers: _.isArray(modifiers) ? modifiers : [modifiers], captureOnInputs}); + + const callbackID = Str.guid(); + eventHandlers[displayName].push({ + id: callbackID, + callback, + captureOnInputs, + }); if (descriptionKey) { addKeyToMap(key, modifiers, descriptionKey); } - return () => unsubscribe(key); + return () => unsubscribe(displayName, callbackID); } /** From 6c294988805308d4d6a8f9944551cadf55294fc1 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:24:42 -0800 Subject: [PATCH 04/14] Implement event bubbling --- src/libs/KeyboardShortcut/index.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 784facbf4be7..24b99ffebf7b 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -90,8 +90,12 @@ function bindHandlerToKeyupEvent(event) { } event.preventDefault(); - // Short circuit the loop because the event is triggered - return false; + // If the event should not bubble, short-circuit the loop + let shouldBubble = callback.shouldBubble || false; + if (_.isFunction(callback.shouldBubble)) { + shouldBubble = callback.shouldBubble(); + } + return shouldBubble; }); } @@ -132,11 +136,12 @@ function addKeyToMap(key, modifiers, descriptionKey) { * @param {String} key The key to watch, i.e. 'K' or 'Escape' * @param {Function} callback The callback to call * @param {String} descriptionKey Translation key for shortcut description - * @param {String|Array} modifiers Can either be shift or control - * @param {Boolean} captureOnInputs Should we capture the event on inputs too? + * @param {String|Array} [modifiers] Can either be shift or control + * @param {Boolean} [captureOnInputs] Should we capture the event on inputs too? + * @param {Boolean|Function} [shouldBubble] Should the event bubble? * @returns {Function} clean up method */ -function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false) { +function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false, shouldBubble = false) { const displayName = getDisplayName(key, modifiers); if (!_.has(eventHandlers, displayName)) { eventHandlers[displayName] = []; @@ -147,6 +152,7 @@ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOn id: callbackID, callback, captureOnInputs, + shouldBubble, }); if (descriptionKey) { From 03fcb047dee0e5e4f859a6d16527218ab877bf95 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:27:07 -0800 Subject: [PATCH 05/14] Implement priority --- src/libs/KeyboardShortcut/index.js | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 24b99ffebf7b..82790e8245f8 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -139,16 +139,17 @@ function addKeyToMap(key, modifiers, descriptionKey) { * @param {String|Array} [modifiers] Can either be shift or control * @param {Boolean} [captureOnInputs] Should we capture the event on inputs too? * @param {Boolean|Function} [shouldBubble] Should the event bubble? + * @param {Number} [priority] The position the callback should take in the stack. 0 means top priority, and 1 means less priority than the most recently added. * @returns {Function} clean up method */ -function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false, shouldBubble = false) { +function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false, shouldBubble = false, priority = 0) { const displayName = getDisplayName(key, modifiers); if (!_.has(eventHandlers, displayName)) { eventHandlers[displayName] = []; } const callbackID = Str.guid(); - eventHandlers[displayName].push({ + eventHandlers[displayName].splice(priority, 0, { id: callbackID, callback, captureOnInputs, @@ -179,15 +180,19 @@ function getShortcutModifiers(modifiers) { } /** - * Module storing the different keyboard shortcut + * This module configures a global keyboard event handler. * - * We are using a push/pop model where new event are pushed at the end of an - * array of events. When the event occur, we trigger the callback of the last - * element. This allow us to replace shortcut from a page to a dialog without - * having the page having to handle that logic. + * It uses a stack to store event handlers for each key combination. Some additional details: * - * This is also following the convention of the PubSub module. - * The "subClass" is used by pages to bind /unbind with no worries + * - By default, new handlers are pushed to the top of the stack. If you pass a >0 priority when subscribing to the key event, + * then the handler will get pushed further down the stack. This means that priority of 0 is higher than priority 1. + * + * - When a key event occurs, we trigger callbacks for that key starting from the top of the stack. + * By default, events do not bubble, and only the handler at the top of the stack will be executed. + * Individual callbacks can be configured with the shouldBubble parameter, to allow the next event handler on the stack execute. + * + * - Each handler has a unique callbackID, so calling the `unsubscribe` function (returned from `subscribe`) will unsubscribe the expected handler, + * regardless of its position in the stack. */ const KeyboardShortcut = { subscribe, From 80d4c0ad3981c1b1aef1a8802e1009b9783c5f2e Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:28:20 -0800 Subject: [PATCH 06/14] Fix toUpperCase bug --- src/CONST.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/CONST.js b/src/CONST.js index d340757cd11e..8e81c23c8832 100755 --- a/src/CONST.js +++ b/src/CONST.js @@ -176,9 +176,9 @@ const CONST = { }, }, KEYBOARD_SHORTCUT_KEY_DISPLAY_NAME: { - CONTROL: 'Ctrl', - META: 'Cmd', - SHIFT: 'Shift', + CONTROL: 'CTRL', + META: 'CMD', + SHIFT: 'SHIFT', }, CURRENCY: { USD: 'USD', From 56e717c8709f6b57418b5f3783cfba17442589fa Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:44:04 -0800 Subject: [PATCH 07/14] Move platform-specific key adjustment into KeyboardShortcut lib --- src/CONST.js | 2 +- src/components/KeyboardShortcutsModal.js | 3 +- src/libs/KeyboardShortcut/index.js | 41 ++++++++++--------- src/libs/KeyboardShortcut/index.native.js | 1 - .../Navigation/AppNavigator/AuthScreens.js | 7 +--- src/pages/home/report/ReportActionsView.js | 4 +- 6 files changed, 26 insertions(+), 32 deletions(-) diff --git a/src/CONST.js b/src/CONST.js index 8e81c23c8832..f21bf6426f23 100755 --- a/src/CONST.js +++ b/src/CONST.js @@ -134,7 +134,7 @@ const CONST = { WEB: 'web', DESKTOP: 'desktop', }, - KEYBOARD_SHORTCUT_MODIFIERS: { + PLATFORM_SPECIFIC_KEYS: { CTRL: { DEFAULT: 'control', [PLATFORM_OS_MACOS]: 'meta', diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index f86216d1f9bb..28af324611e2 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -33,10 +33,9 @@ const defaultProps = { class KeyboardShortcutsModal extends React.Component { componentDidMount() { const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL; - const shortcutModifiers = KeyboardShortcut.getShortcutModifiers(shortcutConfig.modifiers); this.unsubscribeShortcutModal = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, () => { KeyboardShortcutsActions.showKeyboardShortcutModal(); - }, shortcutConfig.descriptionKey, shortcutModifiers, true); + }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true); } componentWillUnmount() { diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 82790e8245f8..0cbc6b3f0470 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -131,6 +131,24 @@ function addKeyToMap(key, modifiers, descriptionKey) { }; } +/** + * Return platform specific modifiers for keys like Control (CMD on macOS) + * + * @param {Array} keys + * @returns {Array} + */ +function getPlatformEquivalentForKeys(keys) { + const operatingSystem = getOperatingSystem(); + return _.map(keys, (key) => { + if (!_.has(CONST.PLATFORM_SPECIFIC_KEYS, key)) { + return key; + } + + const platformModifiers = CONST.PLATFORM_SPECIFIC_KEYS[key]; + return lodashGet(platformModifiers, operatingSystem, platformModifiers.DEFAULT || key); + }); +} + /** * Subscribes to a keyboard event. * @param {String} key The key to watch, i.e. 'K' or 'Escape' @@ -143,7 +161,8 @@ function addKeyToMap(key, modifiers, descriptionKey) { * @returns {Function} clean up method */ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOnInputs = false, shouldBubble = false, priority = 0) { - const displayName = getDisplayName(key, modifiers); + const platformAdjustedModifiers = getPlatformEquivalentForKeys(modifiers); + const displayName = getDisplayName(key, platformAdjustedModifiers); if (!_.has(eventHandlers, displayName)) { eventHandlers[displayName] = []; } @@ -157,28 +176,11 @@ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOn }); if (descriptionKey) { - addKeyToMap(key, modifiers, descriptionKey); + addKeyToMap(key, platformAdjustedModifiers, descriptionKey); } return () => unsubscribe(displayName, callbackID); } -/** - * Return platform specific modifiers for keys like Control (Cmd) - * @param {Array} modifiers - * @returns {Array} - */ -function getShortcutModifiers(modifiers) { - const operatingSystem = getOperatingSystem(); - return _.map(modifiers, (modifier) => { - if (!_.has(CONST.KEYBOARD_SHORTCUT_MODIFIERS, modifier)) { - return modifier; - } - - const platformModifiers = CONST.KEYBOARD_SHORTCUT_MODIFIERS[modifier]; - return lodashGet(platformModifiers, operatingSystem, platformModifiers.DEFAULT || modifier); - }); -} - /** * This module configures a global keyboard event handler. * @@ -197,7 +199,6 @@ function getShortcutModifiers(modifiers) { const KeyboardShortcut = { subscribe, getKeyboardShortcuts, - getShortcutModifiers, }; export default KeyboardShortcut; diff --git a/src/libs/KeyboardShortcut/index.native.js b/src/libs/KeyboardShortcut/index.native.js index ae1d49830dce..b76fe956e2eb 100644 --- a/src/libs/KeyboardShortcut/index.native.js +++ b/src/libs/KeyboardShortcut/index.native.js @@ -7,7 +7,6 @@ const KeyboardShortcut = { return () => {}; }, getKeyboardShortcuts() { return []; }, - getShortcutModifiers() { return []; }, }; export default KeyboardShortcut; diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 5b5d72713217..944dcfa52e97 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -153,20 +153,17 @@ class AuthScreens extends React.Component { Timing.end(CONST.TIMING.HOMEPAGE_INITIAL_RENDER); const searchShortcutConfig = CONST.KEYBOARD_SHORTCUTS.SEARCH; - const searchShortcutModifiers = KeyboardShortcut.getShortcutModifiers(searchShortcutConfig.modifiers); - const groupShortcutConfig = CONST.KEYBOARD_SHORTCUTS.NEW_GROUP; - const groupShortcutModifiers = KeyboardShortcut.getShortcutModifiers(groupShortcutConfig.modifiers); // Listen for the key K being pressed so that focus can be given to // the chat switcher, or new group chat // based on the key modifiers pressed and the operating system this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(searchShortcutConfig.shortcutKey, () => { Navigation.navigate(ROUTES.SEARCH); - }, searchShortcutConfig.descriptionKey, searchShortcutModifiers, true); + }, searchShortcutConfig.descriptionKey, searchShortcutConfig.modifiers, true); this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(groupShortcutConfig.shortcutKey, () => { Navigation.navigate(ROUTES.NEW_GROUP); - }, groupShortcutConfig.descriptionKey, groupShortcutModifiers, true); + }, groupShortcutConfig.descriptionKey, groupShortcutConfig.modifiers, true); } shouldComponentUpdate(nextProps) { diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index a92e335a1838..ff0bc8d3f62f 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -151,11 +151,9 @@ class ReportActionsView extends React.Component { Report.fetchActions(this.props.reportID); const copyShortcutConfig = CONST.KEYBOARD_SHORTCUTS.COPY; - const copyShortcutModifiers = KeyboardShortcut.getShortcutModifiers(copyShortcutConfig.modifiers); - this.unsubscribeCopyShortcut = KeyboardShortcut.subscribe(copyShortcutConfig.shortcutKey, () => { this.copySelectionToClipboard(); - }, copyShortcutConfig.descriptionKey, copyShortcutModifiers, false); + }, copyShortcutConfig.descriptionKey, copyShortcutConfig.modifiers, false); } shouldComponentUpdate(nextProps, nextState) { From acbf21294f4b05e3bd9a7be69983996359a999ac Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 11:57:08 -0800 Subject: [PATCH 08/14] Clarify the role of the documented shortcuts map --- src/components/KeyboardShortcutsModal.js | 2 +- src/libs/KeyboardShortcut/index.js | 37 +++++++++--------------- 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index 28af324611e2..7437d187d233 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -72,7 +72,7 @@ class KeyboardShortcutsModal extends React.Component { } render() { - const shortcuts = KeyboardShortcut.getKeyboardShortcuts(); + const shortcuts = KeyboardShortcut.getDocumentedShortcuts(); const modalType = this.props.isSmallScreenWidth ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE; return ( diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 0cbc6b3f0470..66bbf7ed0c0f 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -4,15 +4,17 @@ import Str from 'expensify-common/lib/str'; import getOperatingSystem from '../getOperatingSystem'; import CONST from '../../CONST'; +// Handlers for the various keyboard listeners we set up const eventHandlers = {}; -const keyboardShortcutMap = {}; + +// Documentation information for keyboard shortcuts that are displayed in the keyboard shortcuts informational modal +const documentedShortcuts = {}; /** - * Return the key-value pair for shortcut keys and translate keys * @returns {Array} */ -function getKeyboardShortcuts() { - return _.values(keyboardShortcutMap); +function getDocumentedShortcuts() { + return _.values(documentedShortcuts); } /** @@ -114,23 +116,6 @@ function unsubscribe(displayName, callbackID) { eventHandlers[displayName] = _.reject(eventHandlers[displayName], callback => callback.id === callbackID); } -/** - * Add key to the shortcut map - * - * @param {String} key The key to watch, i.e. 'K' or 'Escape' - * @param {String|String[]} modifiers Can either be shift or control - * @param {String} descriptionKey Translation key for shortcut description - */ -function addKeyToMap(key, modifiers, descriptionKey) { - const displayName = getDisplayName(key, modifiers); - keyboardShortcutMap[displayName] = { - shortcutKey: key, - descriptionKey, - displayName, - modifiers, - }; -} - /** * Return platform specific modifiers for keys like Control (CMD on macOS) * @@ -176,8 +161,14 @@ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOn }); if (descriptionKey) { - addKeyToMap(key, platformAdjustedModifiers, descriptionKey); + documentedShortcuts[displayName] = { + shortcutKey: key, + descriptionKey, + displayName, + modifiers, + }; } + return () => unsubscribe(displayName, callbackID); } @@ -198,7 +189,7 @@ function subscribe(key, callback, descriptionKey, modifiers = 'shift', captureOn */ const KeyboardShortcut = { subscribe, - getKeyboardShortcuts, + getDocumentedShortcuts, }; export default KeyboardShortcut; From 7db67a4ae6a36e9a6257d35055cdf88e97cfd261 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 12:03:37 -0800 Subject: [PATCH 09/14] Make exports match in index.native.js --- src/libs/KeyboardShortcut/index.native.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/KeyboardShortcut/index.native.js b/src/libs/KeyboardShortcut/index.native.js index b76fe956e2eb..8c97f2daf343 100644 --- a/src/libs/KeyboardShortcut/index.native.js +++ b/src/libs/KeyboardShortcut/index.native.js @@ -6,7 +6,7 @@ const KeyboardShortcut = { subscribe() { return () => {}; }, - getKeyboardShortcuts() { return []; }, + getDocumentedShortcuts() { return []; }, }; export default KeyboardShortcut; From d31da7cd8452e615182571ffbd1d0b0fa6e29d11 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 12:37:51 -0800 Subject: [PATCH 10/14] Fix bindHandlerToKeydownEvent --- src/libs/KeyboardShortcut/index.js | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 66bbf7ed0c0f..2ab72435b96a 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -65,19 +65,12 @@ function getDisplayName(key, modifiers) { * @param {Event} event * @private */ -function bindHandlerToKeyupEvent(event) { +function bindHandlerToKeydownEvent(event) { const eventModifiers = getKeyEventModifiers(event); const displayName = getDisplayName(event.key.toUpperCase(), eventModifiers); - if (!_.has(eventHandlers, displayName)) { - return; - } - - const eventCallbacks = eventHandlers[event.keyCode]; - const reversedEventCallbacks = [...eventCallbacks].reverse(); - // Loop over all the callbacks - _.every(reversedEventCallbacks, (callback) => { + _.every(eventHandlers[displayName], (callback) => { // If configured to do so, prevent input text control to trigger this event if (!callback.captureOnInputs && ( event.target.nodeName === 'INPUT' @@ -102,8 +95,8 @@ function bindHandlerToKeyupEvent(event) { } // Make sure we don't add multiple listeners -document.removeEventListener('keydown', bindHandlerToKeyupEvent, {capture: true}); -document.addEventListener('keydown', bindHandlerToKeyupEvent, {capture: true}); +document.removeEventListener('keydown', bindHandlerToKeydownEvent, {capture: true}); +document.addEventListener('keydown', bindHandlerToKeydownEvent, {capture: true}); /** * Unsubscribes a keyboard event handler. From b31d30a313b00feb40befc84816f15feb4fef8da Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 12:44:49 -0800 Subject: [PATCH 11/14] Fix ESC in keyboard shortcut modal --- src/CONST.js | 3 ++- src/libs/KeyboardShortcut/index.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/CONST.js b/src/CONST.js index f21bf6426f23..813436c60526 100755 --- a/src/CONST.js +++ b/src/CONST.js @@ -177,8 +177,9 @@ const CONST = { }, KEYBOARD_SHORTCUT_KEY_DISPLAY_NAME: { CONTROL: 'CTRL', + ESCAPE: 'ESC', META: 'CMD', - SHIFT: 'SHIFT', + SHIFT: 'Shift', }, CURRENCY: { USD: 'USD', diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 2ab72435b96a..44e3cab9613e 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -67,7 +67,7 @@ function getDisplayName(key, modifiers) { */ function bindHandlerToKeydownEvent(event) { const eventModifiers = getKeyEventModifiers(event); - const displayName = getDisplayName(event.key.toUpperCase(), eventModifiers); + const displayName = getDisplayName(event.key, eventModifiers); // Loop over all the callbacks _.every(eventHandlers[displayName], (callback) => { From 5ba48164217147d40bdc28e65b2159e32a3b99c4 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 12:48:21 -0800 Subject: [PATCH 12/14] Improve JSDocs --- src/libs/KeyboardShortcut/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/KeyboardShortcut/index.js b/src/libs/KeyboardShortcut/index.js index 44e3cab9613e..aa485ffa8cd6 100644 --- a/src/libs/KeyboardShortcut/index.js +++ b/src/libs/KeyboardShortcut/index.js @@ -21,7 +21,7 @@ function getDocumentedShortcuts() { * Gets modifiers from a keyboard event. * * @param {Event} event - * @returns {Array} + * @returns {Array} */ function getKeyEventModifiers(event) { const modifiers = []; @@ -112,7 +112,7 @@ function unsubscribe(displayName, callbackID) { /** * Return platform specific modifiers for keys like Control (CMD on macOS) * - * @param {Array} keys + * @param {Array} keys * @returns {Array} */ function getPlatformEquivalentForKeys(keys) { @@ -132,7 +132,7 @@ function getPlatformEquivalentForKeys(keys) { * @param {String} key The key to watch, i.e. 'K' or 'Escape' * @param {Function} callback The callback to call * @param {String} descriptionKey Translation key for shortcut description - * @param {String|Array} [modifiers] Can either be shift or control + * @param {String|Array} [modifiers] Can either be shift or control * @param {Boolean} [captureOnInputs] Should we capture the event on inputs too? * @param {Boolean|Function} [shouldBubble] Should the event bubble? * @param {Number} [priority] The position the callback should take in the stack. 0 means top priority, and 1 means less priority than the most recently added. From b179109d93e25fe9d1a1612eed1070d3c831df8e Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 14:34:15 -0800 Subject: [PATCH 13/14] Allow Buttons to assign priority to their enter key event listener --- src/components/Button.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/components/Button.js b/src/components/Button.js index 6c826576c692..e927e1be3478 100644 --- a/src/components/Button.js +++ b/src/components/Button.js @@ -48,6 +48,10 @@ const propTypes = { /** Call the onPress function when Enter key is pressed */ pressOnEnter: PropTypes.bool, + /** The priority to assign the enter key event listener. 0 is the highest priority. */ + enterKeyEventListenerPriority: PropTypes.number, + + /** Additional styles to add after local styles. Applied to Pressable portion of button */ style: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.object), @@ -92,6 +96,7 @@ const defaultProps = { onPressIn: () => {}, onPressOut: () => {}, pressOnEnter: false, + enterKeyEventListenerPriority: 0, style: [], innerStyles: [], textStyles: [], @@ -124,7 +129,7 @@ class Button extends Component { return; } this.props.onPress(); - }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true); + }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true, false, this.props.enterKeyEventListenerPriority); } componentWillUnmount() { From fe4467ae6ed5372133192b685b9d3cfcd7f2bb12 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Wed, 9 Mar 2022 18:28:28 -0800 Subject: [PATCH 14/14] Remove unnecessary blank line --- src/components/Button.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Button.js b/src/components/Button.js index e927e1be3478..a57a68a1d50d 100644 --- a/src/components/Button.js +++ b/src/components/Button.js @@ -51,7 +51,6 @@ const propTypes = { /** The priority to assign the enter key event listener. 0 is the highest priority. */ enterKeyEventListenerPriority: PropTypes.number, - /** Additional styles to add after local styles. Applied to Pressable portion of button */ style: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.object),