From f07e3ade004f92f46da3c0d2051af5e7747a79c0 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Thu, 4 May 2023 11:14:20 -0700 Subject: [PATCH 1/5] Only subscribe shortcuts when modal is open --- src/components/KeyboardShortcutsModal.js | 63 ++++++++++++++++-------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index 7dd2adf72585..9dbfa4097880 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -39,30 +39,53 @@ class KeyboardShortcutsModal extends React.Component { ModalActions.close(); KeyboardShortcutsActions.showKeyboardShortcutModal(); }, openShortcutModalConfig.descriptionKey, openShortcutModalConfig.modifiers, true); + } - // Allow closing the modal with the both Enter and Escape keys - // Both callbacks have the lowest priority (0) to ensure that they are called before any other callbacks - // and configured so that event propagation is stopped after the callback is called (only when the modal is open) - const closeShortcutEscapeModalConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE; - this.unsubscribeCloseEscapeModal = KeyboardShortcut.subscribe(closeShortcutEscapeModalConfig.shortcutKey, () => { - ModalActions.close(); - KeyboardShortcutsActions.hideKeyboardShortcutModal(); - }, closeShortcutEscapeModalConfig.descriptionKey, closeShortcutEscapeModalConfig.modifiers, true, true); + componentDidUpdate(prevProps) { + if (!prevProps.isShortcutsModalOpen && this.props.isShortcutsModalOpen) { + // Modal is opening, add keyboard shortcuts + // Allow closing the modal with the both Enter and Escape keys + // Both callbacks have the lowest priority (0) to ensure that they are called before any other callbacks + // and configured so that event propagation is stopped after the callback is called (only when the modal is open) + const closeShortcutEscapeModalConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE; + this.unsubscribeCloseEscapeModal = KeyboardShortcut.subscribe(closeShortcutEscapeModalConfig.shortcutKey, () => { + ModalActions.close(); + KeyboardShortcutsActions.hideKeyboardShortcutModal(); + }, closeShortcutEscapeModalConfig.descriptionKey, closeShortcutEscapeModalConfig.modifiers, true, true); - const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER; - this.unsubscribeCloseEnterModal = KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => { - ModalActions.close(); - KeyboardShortcutsActions.hideKeyboardShortcutModal(); - }, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true, () => !this.props.isShortcutsModalOpen); + const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER; + this.unsubscribeCloseEnterModal = KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => { + ModalActions.close(); + KeyboardShortcutsActions.hideKeyboardShortcutModal(); + }, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true); - // Intercept arrow up and down keys to prevent scrolling ArrowKeyFocusManager while this modal is open - const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP; - this.unsubscribeArrowUpKey = KeyboardShortcut.subscribe(arrowUpConfig.shortcutKey, () => { - }, arrowUpConfig.descriptionKey, arrowUpConfig.modifiers, true, () => !this.props.isShortcutsModalOpen); + // Intercept arrow up and down keys to prevent scrolling ArrowKeyFocusManager while this modal is open + const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP; + this.unsubscribeArrowUpKey = KeyboardShortcut.subscribe(arrowUpConfig.shortcutKey, () => { + }, arrowUpConfig.descriptionKey, arrowUpConfig.modifiers, true); - const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN; - this.unsubscribeArrowDownKey = KeyboardShortcut.subscribe(arrowDownConfig.shortcutKey, () => { - }, arrowDownConfig.descriptionKey, arrowDownConfig.modifiers, true, () => !this.props.isShortcutsModalOpen); + const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN; + this.unsubscribeArrowDownKey = KeyboardShortcut.subscribe(arrowDownConfig.shortcutKey, () => { + }, arrowDownConfig.descriptionKey, arrowDownConfig.modifiers, true); + } else if (prevProps.isShortcutsModalOpen && !this.props.isShortcutsModalOpen) { + // Modal is closing, remove keyboard shortcuts + if (this.unsubscribeCloseEscapeModal) { + this.unsubscribeCloseEscapeModal(); + this.unsubscribeCloseEscapeModal = undefined; + } + if (this.unsubscribeCloseEnterModal) { + this.unsubscribeCloseEnterModal(); + this.unsubscribeCloseEnterModal = undefined; + } + if (this.unsubscribeArrowUpKey) { + this.unsubscribeArrowUpKey(); + this.unsubscribeArrowUpKey = undefined; + } + if (this.unsubscribeArrowDownKey) { + this.unsubscribeArrowDownKey(); + this.unsubscribeArrowDownKey = undefined; + } + } } componentWillUnmount() { From a834ca01fd990dfd597b459432923e5b6983680a Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Thu, 4 May 2023 11:34:47 -0700 Subject: [PATCH 2/5] Refactor / register shortcuts if modal starts open --- src/components/KeyboardShortcutsModal.js | 98 +++++++++++------------- 1 file changed, 46 insertions(+), 52 deletions(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index 9dbfa4097880..fe3fbe867af1 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -35,56 +35,24 @@ const defaultProps = { class KeyboardShortcutsModal extends React.Component { componentDidMount() { const openShortcutModalConfig = CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL; + this.subscribedOpenModalShortcuts = []; this.unsubscribeShortcutModal = KeyboardShortcut.subscribe(openShortcutModalConfig.shortcutKey, () => { ModalActions.close(); KeyboardShortcutsActions.showKeyboardShortcutModal(); }, openShortcutModalConfig.descriptionKey, openShortcutModalConfig.modifiers, true); + + if (this.props.isShortcutsModalOpen) { + // The modal started open, which can happen if you reload the page when the modal is open. + this.subscribeOpenModalKeyboardShortcuts(); + } } componentDidUpdate(prevProps) { if (!prevProps.isShortcutsModalOpen && this.props.isShortcutsModalOpen) { - // Modal is opening, add keyboard shortcuts - // Allow closing the modal with the both Enter and Escape keys - // Both callbacks have the lowest priority (0) to ensure that they are called before any other callbacks - // and configured so that event propagation is stopped after the callback is called (only when the modal is open) - const closeShortcutEscapeModalConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE; - this.unsubscribeCloseEscapeModal = KeyboardShortcut.subscribe(closeShortcutEscapeModalConfig.shortcutKey, () => { - ModalActions.close(); - KeyboardShortcutsActions.hideKeyboardShortcutModal(); - }, closeShortcutEscapeModalConfig.descriptionKey, closeShortcutEscapeModalConfig.modifiers, true, true); - - const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER; - this.unsubscribeCloseEnterModal = KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => { - ModalActions.close(); - KeyboardShortcutsActions.hideKeyboardShortcutModal(); - }, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true); - - // Intercept arrow up and down keys to prevent scrolling ArrowKeyFocusManager while this modal is open - const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP; - this.unsubscribeArrowUpKey = KeyboardShortcut.subscribe(arrowUpConfig.shortcutKey, () => { - }, arrowUpConfig.descriptionKey, arrowUpConfig.modifiers, true); - - const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN; - this.unsubscribeArrowDownKey = KeyboardShortcut.subscribe(arrowDownConfig.shortcutKey, () => { - }, arrowDownConfig.descriptionKey, arrowDownConfig.modifiers, true); + this.subscribeOpenModalKeyboardShortcuts(); } else if (prevProps.isShortcutsModalOpen && !this.props.isShortcutsModalOpen) { // Modal is closing, remove keyboard shortcuts - if (this.unsubscribeCloseEscapeModal) { - this.unsubscribeCloseEscapeModal(); - this.unsubscribeCloseEscapeModal = undefined; - } - if (this.unsubscribeCloseEnterModal) { - this.unsubscribeCloseEnterModal(); - this.unsubscribeCloseEnterModal = undefined; - } - if (this.unsubscribeArrowUpKey) { - this.unsubscribeArrowUpKey(); - this.unsubscribeArrowUpKey = undefined; - } - if (this.unsubscribeArrowDownKey) { - this.unsubscribeArrowDownKey(); - this.unsubscribeArrowDownKey = undefined; - } + this.unsubscribeOpenModalShortcuts(); } } @@ -92,18 +60,44 @@ class KeyboardShortcutsModal extends React.Component { if (this.unsubscribeShortcutModal) { this.unsubscribeShortcutModal(); } - if (this.unsubscribeCloseEscapeModal) { - this.unsubscribeCloseEscapeModal(); - } - if (this.unsubscribeCloseEnterModal) { - this.unsubscribeCloseEnterModal(); - } - if (this.unsubscribeArrowUpKey) { - this.unsubscribeArrowUpKey(); - } - if (this.unsubscribeArrowDownKey) { - this.unsubscribeArrowDownKey(); - } + this.unsubscribeOpenModalShortcuts(); + } + + /* + * Subscribe shortcuts that only are used when the modal is open + */ + subscribeOpenModalKeyboardShortcuts() { + // Allow closing the modal with the both Enter and Escape keys + // Both callbacks have the lowest priority (0) to ensure that they are called before any other callbacks + // and configured so that event propagation is stopped after the callback is called (only when the modal is open) + const closeShortcutEscapeModalConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE; + this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(closeShortcutEscapeModalConfig.shortcutKey, () => { + ModalActions.close(); + KeyboardShortcutsActions.hideKeyboardShortcutModal(); + }, closeShortcutEscapeModalConfig.descriptionKey, closeShortcutEscapeModalConfig.modifiers, true, true)); + + const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER; + this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => { + ModalActions.close(); + KeyboardShortcutsActions.hideKeyboardShortcutModal(); + }, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true)); + + // Intercept arrow up and down keys to prevent scrolling ArrowKeyFocusManager while this modal is open + const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP; + this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(arrowUpConfig.shortcutKey, () => { + }, arrowUpConfig.descriptionKey, arrowUpConfig.modifiers, true)); + + const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN; + this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(arrowDownConfig.shortcutKey, () => { + }, arrowDownConfig.descriptionKey, arrowDownConfig.modifiers, true)); + } + + /* + * Unsubscribe all shortcuts that were subscribed when the modal opened + */ + unsubscribeOpenModalShortcuts() { + this.subscribedOpenModalShortcuts.forEach(unsubscribe => unsubscribe()); + this.subscribedOpenModalShortcuts = []; } /** From 4650155a1e45788ec48f24ae999a118ab0f7ef9e Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Thu, 4 May 2023 11:47:32 -0700 Subject: [PATCH 3/5] Make function name more consistent --- src/components/KeyboardShortcutsModal.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index fe3fbe867af1..b5719d4968a8 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -34,8 +34,9 @@ const defaultProps = { class KeyboardShortcutsModal extends React.Component { componentDidMount() { - const openShortcutModalConfig = CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL; this.subscribedOpenModalShortcuts = []; + + const openShortcutModalConfig = CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL; this.unsubscribeShortcutModal = KeyboardShortcut.subscribe(openShortcutModalConfig.shortcutKey, () => { ModalActions.close(); KeyboardShortcutsActions.showKeyboardShortcutModal(); @@ -43,13 +44,13 @@ class KeyboardShortcutsModal extends React.Component { if (this.props.isShortcutsModalOpen) { // The modal started open, which can happen if you reload the page when the modal is open. - this.subscribeOpenModalKeyboardShortcuts(); + this.subscribeOpenModalShortcuts(); } } componentDidUpdate(prevProps) { if (!prevProps.isShortcutsModalOpen && this.props.isShortcutsModalOpen) { - this.subscribeOpenModalKeyboardShortcuts(); + this.subscribeOpenModalShortcuts(); } else if (prevProps.isShortcutsModalOpen && !this.props.isShortcutsModalOpen) { // Modal is closing, remove keyboard shortcuts this.unsubscribeOpenModalShortcuts(); @@ -66,7 +67,7 @@ class KeyboardShortcutsModal extends React.Component { /* * Subscribe shortcuts that only are used when the modal is open */ - subscribeOpenModalKeyboardShortcuts() { + subscribeOpenModalShortcuts() { // Allow closing the modal with the both Enter and Escape keys // Both callbacks have the lowest priority (0) to ensure that they are called before any other callbacks // and configured so that event propagation is stopped after the callback is called (only when the modal is open) From 3258a1e388c922d9b8aca00023a707cfd448c478 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Garay <87341702+aldo-expensify@users.noreply.github.com> Date: Thu, 4 May 2023 12:28:34 -0700 Subject: [PATCH 4/5] Use _.each instead of .forEach Co-authored-by: Marc Glasser --- src/components/KeyboardShortcutsModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index b5719d4968a8..dfe6602e648b 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -97,7 +97,7 @@ class KeyboardShortcutsModal extends React.Component { * Unsubscribe all shortcuts that were subscribed when the modal opened */ unsubscribeOpenModalShortcuts() { - this.subscribedOpenModalShortcuts.forEach(unsubscribe => unsubscribe()); + _.each(this.subscribedOpenModalShortcuts, unsubscribe); this.subscribedOpenModalShortcuts = []; } From 1bb88173b4898391b932caeaea555fe95a62b159 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Thu, 4 May 2023 12:30:05 -0700 Subject: [PATCH 5/5] Call unsubscribe correctly --- src/components/KeyboardShortcutsModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index dfe6602e648b..845a8779a731 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -97,7 +97,7 @@ class KeyboardShortcutsModal extends React.Component { * Unsubscribe all shortcuts that were subscribed when the modal opened */ unsubscribeOpenModalShortcuts() { - _.each(this.subscribedOpenModalShortcuts, unsubscribe); + _.each(this.subscribedOpenModalShortcuts, unsubscribe => unsubscribe()); this.subscribedOpenModalShortcuts = []; }