From 21bc3e6574c415913cf9f38101a646f1279052f8 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 13:09:04 -0800 Subject: [PATCH 01/23] Use useArrowKeyFocusManager hook instead of ArrowKeyFocusManager component --- .../OptionsSelector/BaseOptionsSelector.js | 82 +++++++------------ 1 file changed, 28 insertions(+), 54 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 23451b8e1a09..0dc0b0414a8f 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -4,7 +4,6 @@ import PropTypes from 'prop-types'; import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; import {ScrollView, View} from 'react-native'; import _ from 'underscore'; -import ArrowKeyFocusManager from '@components/ArrowKeyFocusManager'; import Button from '@components/Button'; import FixedFooter from '@components/FixedFooter'; import FormHelpMessage from '@components/FormHelpMessage'; @@ -12,6 +11,7 @@ import OptionsList from '@components/OptionsList'; import ReferralProgramCTA from '@components/ReferralProgramCTA'; import ShowMoreButton from '@components/ShowMoreButton'; import TextInput from '@components/TextInput'; +import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; import useThemeStyles from '@hooks/useThemeStyles'; @@ -58,27 +58,6 @@ function BaseOptionsSelector(props) { const {translate} = useLocalize(); const themeStyles = useThemeStyles(); - const getInitiallyFocusedIndex = useCallback( - (allOptions) => { - let defaultIndex; - if (props.shouldTextInputAppearBelowOptions) { - defaultIndex = allOptions.length; - } else if (props.focusedIndex >= 0) { - defaultIndex = props.focusedIndex; - } else { - defaultIndex = props.selectedOptions.length; - } - if (_.isUndefined(props.initiallyFocusedOptionKey)) { - return defaultIndex; - } - - const indexOfInitiallyFocusedOption = _.findIndex(allOptions, (option) => option.keyForList === props.initiallyFocusedOptionKey); - - return indexOfInitiallyFocusedOption; - }, - [props.shouldTextInputAppearBelowOptions, props.initiallyFocusedOptionKey, props.selectedOptions.length, props.focusedIndex], - ); - const isWebOrDesktop = [CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform()); const accessibilityRoles = _.values(CONST.ROLE); @@ -137,7 +116,31 @@ function BaseOptionsSelector(props) { // eslint-disable-next-line react-hooks/exhaustive-deps const initialAllOptions = useMemo(() => flattenSections(), []); const [allOptions, setAllOptions] = useState(initialAllOptions); - const [focusedIndex, setFocusedIndex] = useState(getInitiallyFocusedIndex(initialAllOptions)); + + const initialFocusedIndex = useMemo(() => { + if (!_.isUndefined(props.initiallyFocusedOptionKey)) { + return _.findIndex(allOptions, (option) => option.keyForList === props.initiallyFocusedOptionKey); + } + + let defaultIndex; + if (props.shouldTextInputAppearBelowOptions) { + defaultIndex = allOptions.length; + } else if (props.focusedIndex >= 0) { + defaultIndex = props.focusedIndex; + } else { + defaultIndex = props.selectedOptions.length; + } + return defaultIndex; + // eslint-disable-next-line react-hooks/exhaustive-deps -- this value is only used to initialize state so only ever needs to be computed on the first render + }, []); + const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({ + initialFocusedIndex, + disabledIndexes: disabledOptionsIndexes, + maxIndex: allOptions.length - 1, + isActive: !props.disableArrowKeysActions, + disableHorizontalKeys: true, + }); + const [focusedOption, setFocusedOption] = useState(allOptions[focusedIndex]); /** @@ -472,29 +475,6 @@ function BaseOptionsSelector(props) { const debouncedUpdateSearchValue = _.debounce(updateSearchValue, CONST.TIMING.SEARCH_OPTION_LIST_DEBOUNCE_TIME); - /** - * Calculates all currently visible options based on the sections that are currently being shown - * and the number of items of those sections. - * - * @returns {Number} - */ - const calculateAllVisibleOptionsCount = useCallback(() => { - let count = 0; - - _.forEach(sections, (section) => { - count += lodashGet(section, 'data.length', 0); - }); - - return count; - }, [sections]); - - /** - * @param {Number} index - */ - const updateFocusedIndex = useCallback((index) => { - setFocusedIndex(index); - }, []); - /** * Completes the follow-up action after clicking on multiple select button * @param {Object} option @@ -619,13 +599,7 @@ function BaseOptionsSelector(props) { ); return ( - {} : updateFocusedIndex} - shouldResetIndexOnEndReached={false} - > + <> {/* * The OptionsList component uses a SectionList which uses a VirtualizedList internally. @@ -684,7 +658,7 @@ function BaseOptionsSelector(props) { {props.footerContent} )} - + ); } From 5c814047d6bc7726b7794e78da12a6c208e9c74b Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 13:26:24 -0800 Subject: [PATCH 02/23] Remove unnecessary focusedOption state variable --- .../OptionsSelector/BaseOptionsSelector.js | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 0dc0b0414a8f..dbc1e08cced2 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -14,6 +14,7 @@ import TextInput from '@components/TextInput'; import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; +import usePrevious from '@hooks/usePrevious'; import useThemeStyles from '@hooks/useThemeStyles'; import getPlatform from '@libs/getPlatform'; import KeyboardShortcut from '@libs/KeyboardShortcut'; @@ -116,6 +117,7 @@ function BaseOptionsSelector(props) { // eslint-disable-next-line react-hooks/exhaustive-deps const initialAllOptions = useMemo(() => flattenSections(), []); const [allOptions, setAllOptions] = useState(initialAllOptions); + const prevOptions = usePrevious(allOptions); const initialFocusedIndex = useMemo(() => { if (!_.isUndefined(props.initiallyFocusedOptionKey)) { @@ -141,8 +143,6 @@ function BaseOptionsSelector(props) { disableHorizontalKeys: true, }); - const [focusedOption, setFocusedOption] = useState(allOptions[focusedIndex]); - /** * Maps sections to render only allowed count of them per section. * @@ -399,11 +399,6 @@ function BaseOptionsSelector(props) { // eslint-disable-next-line react-hooks/exhaustive-deps }, [paginationPage]); - useEffect(() => { - setFocusedOption(allOptions[focusedIndex]); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [focusedIndex]); - // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { // Screen coming back into focus, for example @@ -429,12 +424,12 @@ function BaseOptionsSelector(props) { } const newFocusedIndex = props.selectedOptions.length; - const prevFocusedOption = _.find(newOptions, (option) => focusedOption && option.keyForList === focusedOption.keyForList); - const prevFocusedOptionIndex = prevFocusedOption ? _.findIndex(newOptions, (option) => focusedOption && option.keyForList === focusedOption.keyForList) : undefined; + const prevFocusedOption = prevOptions[focusedIndex]; + const indexOfPrevFocusedOptionInCurrentList = _.findIndex(newOptions, (option) => prevFocusedOption && option.keyForList === prevFocusedOption.keyForList); setSections(newSections); setAllOptions(newOptions); - setFocusedIndex(prevFocusedOptionIndex || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex)); + setFocusedIndex(indexOfPrevFocusedOptionInCurrentList || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex)); // we want to run this effect only when the sections change // eslint-disable-next-line react-hooks/exhaustive-deps }, [props.sections]); From af46a8a14eb3c2055351b88ad2dd5bb31d352356 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 13:30:39 -0800 Subject: [PATCH 03/23] Remove unnecessary useMemo, initialize state with function --- src/components/OptionsSelector/BaseOptionsSelector.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index dbc1e08cced2..c38b77c5ed95 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -114,9 +114,7 @@ function BaseOptionsSelector(props) { return calcAllOptions; }, [props.sections]); - // eslint-disable-next-line react-hooks/exhaustive-deps - const initialAllOptions = useMemo(() => flattenSections(), []); - const [allOptions, setAllOptions] = useState(initialAllOptions); + const [allOptions, setAllOptions] = useState(() => flattenSections()); const prevOptions = usePrevious(allOptions); const initialFocusedIndex = useMemo(() => { From 27488299fbaa4e965bc307f6e56f178bc5243897 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 13:41:44 -0800 Subject: [PATCH 04/23] Use memo instead of state for allOptions --- .../OptionsSelector/BaseOptionsSelector.js | 40 ++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index c38b77c5ed95..4c931af7f643 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -84,37 +84,34 @@ function BaseOptionsSelector(props) { useImperativeHandle(props.forwardedRef, () => textInputRef.current); /** - * Flattens the sections into a single array of options. + * Flatten the sections into a single array of options. * Each object in this array is enhanced to have: * * 1. A `sectionIndex`, which represents the index of the section it came from * 2. An `index`, which represents the index of the option within the section it came from. - * - * @returns {Array} */ - const flattenSections = useCallback(() => { - const calcAllOptions = []; - const calcDisabledOptionsIndexes = []; + const allOptions = useMemo(() => { + const options = []; + const calcDisabledOptionIndexes = []; let index = 0; _.each(props.sections, (section, sectionIndex) => { _.each(section.data, (option, optionIndex) => { - calcAllOptions.push({ - ...option, - sectionIndex, - index: optionIndex, - }); + // eslint-disable-next-line no-param-reassign -- using param reassignment to avoid unnecessary copy of every option that would happen if we used spread instead + option.sectionIndex = sectionIndex; + // eslint-disable-next-line no-param-reassign + option.index = optionIndex; + options.push(option); + if (section.isDisabled || option.isDisabled) { - calcDisabledOptionsIndexes.push(index); + calcDisabledOptionIndexes.push(index); } - index += 1; + + index++; }); }); - - setDisabledOptionsIndexes(calcDisabledOptionsIndexes); - return calcAllOptions; + setDisabledOptionsIndexes(calcDisabledOptionIndexes); + return options; }, [props.sections]); - - const [allOptions, setAllOptions] = useState(() => flattenSections()); const prevOptions = usePrevious(allOptions); const initialFocusedIndex = useMemo(() => { @@ -412,25 +409,22 @@ function BaseOptionsSelector(props) { useEffect(() => { const newSections = sliceSections(); - const newOptions = flattenSections(); if (prevLocale.current !== props.preferredLocale) { prevLocale.current = props.preferredLocale; - setAllOptions(newOptions); setSections(newSections); return; } const newFocusedIndex = props.selectedOptions.length; const prevFocusedOption = prevOptions[focusedIndex]; - const indexOfPrevFocusedOptionInCurrentList = _.findIndex(newOptions, (option) => prevFocusedOption && option.keyForList === prevFocusedOption.keyForList); + const indexOfPrevFocusedOptionInCurrentList = _.findIndex(allOptions, (option) => prevFocusedOption && option.keyForList === prevFocusedOption.keyForList); setSections(newSections); - setAllOptions(newOptions); setFocusedIndex(indexOfPrevFocusedOptionInCurrentList || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex)); // we want to run this effect only when the sections change // eslint-disable-next-line react-hooks/exhaustive-deps - }, [props.sections]); + }, [allOptions]); useEffect(() => { // If we just toggled an option on a multi-selection page or cleared the search input, scroll to top From 5f1b6e34a37675d62162a4be1fc856758e568a89 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 13:58:12 -0800 Subject: [PATCH 05/23] use memo not state for paginated sections --- .../OptionsSelector/BaseOptionsSelector.js | 54 ++++++------------- 1 file changed, 17 insertions(+), 37 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 4c931af7f643..114940bcc786 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -63,7 +63,6 @@ function BaseOptionsSelector(props) { const accessibilityRoles = _.values(CONST.ROLE); const [disabledOptionsIndexes, setDisabledOptionsIndexes] = useState([]); - const [sections, setSections] = useState(); const [shouldDisableRowSelection, setShouldDisableRowSelection] = useState(false); const [errorMessage, setErrorMessage] = useState(''); const [value, setValue] = useState(''); @@ -83,6 +82,23 @@ function BaseOptionsSelector(props) { useImperativeHandle(props.forwardedRef, () => textInputRef.current); + /** + * Paginate props.sections to only allow a certain number of items per section. + */ + const sections = useMemo( + () => + _.map(props.sections, (section) => { + if (_.isEmpty(section.data)) { + return section; + } + + // eslint-disable-next-line no-param-reassign + section.data = section.data.slice(0, CONST.MAX_OPTIONS_SELECTOR_PAGE_LENGTH * (paginationPage || 1)); + return section; + }), + [paginationPage, props.sections], + ); + /** * Flatten the sections into a single array of options. * Each object in this array is enhanced to have: @@ -138,28 +154,6 @@ function BaseOptionsSelector(props) { disableHorizontalKeys: true, }); - /** - * Maps sections to render only allowed count of them per section. - * - * @returns {Objects[]} - */ - const sliceSections = useCallback( - () => - _.map(props.sections, (section) => { - if (_.isEmpty(section.data)) { - return section; - } - - const pagination = paginationPage || 1; - - return { - ...section, - data: section.data.slice(0, CONST.MAX_OPTIONS_SELECTOR_PAGE_LENGTH * pagination), - }; - }), - [paginationPage, props.sections], - ); - /** * Completes the follow-up actions after a row is selected * @@ -384,16 +378,6 @@ function BaseOptionsSelector(props) { // eslint-disable-next-line react-hooks/exhaustive-deps }, [props.isFocused]); - useEffect(() => { - const newSections = sliceSections(); - - if (prevPaginationPage.current !== paginationPage) { - prevPaginationPage.current = paginationPage; - setSections(newSections); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [paginationPage]); - // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { // Screen coming back into focus, for example @@ -408,11 +392,8 @@ function BaseOptionsSelector(props) { }, [isFocused, props.autoFocus]); useEffect(() => { - const newSections = sliceSections(); - if (prevLocale.current !== props.preferredLocale) { prevLocale.current = props.preferredLocale; - setSections(newSections); return; } @@ -420,7 +401,6 @@ function BaseOptionsSelector(props) { const prevFocusedOption = prevOptions[focusedIndex]; const indexOfPrevFocusedOptionInCurrentList = _.findIndex(allOptions, (option) => prevFocusedOption && option.keyForList === prevFocusedOption.keyForList); - setSections(newSections); setFocusedIndex(indexOfPrevFocusedOptionInCurrentList || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex)); // we want to run this effect only when the sections change // eslint-disable-next-line react-hooks/exhaustive-deps From bbd1ce3d59afb31bf0cae7bc231467588c9afd88 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 13:58:54 -0800 Subject: [PATCH 06/23] Get rid of unnecessary prevLocale and prevPaginationPage --- src/components/OptionsSelector/BaseOptionsSelector.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 114940bcc786..7bcfada9d8dd 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -75,8 +75,6 @@ function BaseOptionsSelector(props) { const enterSubscription = useRef(); const CTRLEnterSubscription = useRef(); const focusTimeout = useRef(); - const prevLocale = useRef(props.preferredLocale); - const prevPaginationPage = useRef(paginationPage); const prevSelectedOptions = useRef(props.selectedOptions); const prevValue = useRef(value); @@ -392,11 +390,6 @@ function BaseOptionsSelector(props) { }, [isFocused, props.autoFocus]); useEffect(() => { - if (prevLocale.current !== props.preferredLocale) { - prevLocale.current = props.preferredLocale; - return; - } - const newFocusedIndex = props.selectedOptions.length; const prevFocusedOption = prevOptions[focusedIndex]; const indexOfPrevFocusedOptionInCurrentList = _.findIndex(allOptions, (option) => prevFocusedOption && option.keyForList === prevFocusedOption.keyForList); From 269b813ab4e9243e493687fc375809c74ccc393b Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 14:06:02 -0800 Subject: [PATCH 07/23] Use ref not state for shouldDisableRowSelection --- .../OptionsSelector/BaseOptionsSelector.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 7bcfada9d8dd..267d215c5889 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -63,12 +63,12 @@ function BaseOptionsSelector(props) { const accessibilityRoles = _.values(CONST.ROLE); const [disabledOptionsIndexes, setDisabledOptionsIndexes] = useState([]); - const [shouldDisableRowSelection, setShouldDisableRowSelection] = useState(false); const [errorMessage, setErrorMessage] = useState(''); const [value, setValue] = useState(''); const [paginationPage, setPaginationPage] = useState(1); const [disableEnterShortCut, setDisableEnterShortCut] = useState(false); + const shouldDisableRowSelection = useRef(false); const relatedTarget = useRef(null); const listRef = useRef(); const textInputRef = useRef(); @@ -110,7 +110,7 @@ function BaseOptionsSelector(props) { let index = 0; _.each(props.sections, (section, sectionIndex) => { _.each(section.data, (option, optionIndex) => { - // eslint-disable-next-line no-param-reassign -- using param reassignment to avoid unnecessary copy of every option that would happen if we used spread instead + // eslint-disable-next-line no-param-reassign option.sectionIndex = sectionIndex; // eslint-disable-next-line no-param-reassign option.index = optionIndex; @@ -196,8 +196,8 @@ function BaseOptionsSelector(props) { if (props.canSelectMultipleOptions) { selectRow(localFocusedOption); - } else if (!shouldDisableRowSelection) { - setShouldDisableRowSelection(true); + } else if (!shouldDisableRowSelection.current) { + shouldDisableRowSelection.current = true; let result = selectRow(localFocusedOption); if (!(result instanceof Promise)) { @@ -206,12 +206,12 @@ function BaseOptionsSelector(props) { setTimeout(() => { result.finally(() => { - setShouldDisableRowSelection(false); + shouldDisableRowSelection.current = false; }); }, 500); } }, - [props.canSelectMultipleOptions, focusedIndex, allOptions, isFocused, selectRow, shouldDisableRowSelection], + [props.canSelectMultipleOptions, focusedIndex, allOptions, isFocused, selectRow], ); const handleFocusIn = () => { From b747acf63d32f44222b5d7c7349cf30a7d75d154 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 14:21:06 -0800 Subject: [PATCH 08/23] Remove duplicate keyboard logic for enter and ctrl+enter --- .../OptionsSelector/BaseOptionsSelector.js | 79 ++----------------- 1 file changed, 6 insertions(+), 73 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 267d215c5889..21b5c11c1567 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -17,7 +17,6 @@ import useLocalize from '@hooks/useLocalize'; import usePrevious from '@hooks/usePrevious'; import useThemeStyles from '@hooks/useThemeStyles'; import getPlatform from '@libs/getPlatform'; -import KeyboardShortcut from '@libs/KeyboardShortcut'; import setSelection from '@libs/setSelection'; import CONST from '@src/CONST'; import {defaultProps as optionsSelectorDefaultProps, propTypes as optionsSelectorPropTypes} from './optionsSelectorPropTypes'; @@ -72,8 +71,6 @@ function BaseOptionsSelector(props) { const relatedTarget = useRef(null); const listRef = useRef(); const textInputRef = useRef(); - const enterSubscription = useRef(); - const CTRLEnterSubscription = useRef(); const focusTimeout = useRef(); const prevSelectedOptions = useRef(props.selectedOptions); const prevValue = useRef(value); @@ -231,51 +228,6 @@ function BaseOptionsSelector(props) { document.addEventListener('focusout', handleFocusOut); }; - const subscribeToEnterShortcut = () => { - const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER; - enterSubscription.current = KeyboardShortcut.subscribe( - enterConfig.shortcutKey, - selectFocusedOption, - enterConfig.descriptionKey, - enterConfig.modifiers, - true, - () => !allOptions[focusedIndex], - ); - }; - - const subscribeToCtrlEnterShortcut = () => { - const CTRLEnterConfig = CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER; - CTRLEnterSubscription.current = KeyboardShortcut.subscribe( - CTRLEnterConfig.shortcutKey, - () => { - if (props.canSelectMultipleOptions) { - props.onConfirmSelection(); - return; - } - - const localFocusedOption = allOptions[focusedIndex]; - if (!localFocusedOption) { - return; - } - - selectRow(localFocusedOption); - }, - CTRLEnterConfig.descriptionKey, - CTRLEnterConfig.modifiers, - true, - ); - }; - - const unSubscribeFromKeyboardShortcut = () => { - if (enterSubscription.current) { - enterSubscription.current(); - } - - if (CTRLEnterSubscription.current) { - CTRLEnterSubscription.current(); - } - }; - const selectOptions = useCallback(() => { if (props.canSelectMultipleOptions) { props.onConfirmSelection(); @@ -295,8 +247,12 @@ function BaseOptionsSelector(props) { useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, { shouldBubble: !allOptions[focusedIndex], captureOnInputs: true, + isActive: !disableEnterShortCut, + }); + useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, selectOptions, { + captureOnInputs: true, + isActive: isFocused, }); - useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, selectOptions, {captureOnInputs: true}); /** * Scrolls to the focused index within the SectionList @@ -334,11 +290,9 @@ function BaseOptionsSelector(props) { ); useEffect(() => { - subscribeToEnterShortcut(); - subscribeToCtrlEnterShortcut(); subscribeActiveElement(); - if (props.isFocused && props.autoFocus && textInputRef.current) { + if (isFocused && props.autoFocus && textInputRef.current) { focusTimeout.current = setTimeout(() => { textInputRef.current.focus(); }, CONST.ANIMATED_TRANSITION); @@ -350,32 +304,11 @@ function BaseOptionsSelector(props) { if (focusTimeout.current) { clearTimeout(focusTimeout.current); } - - unSubscribeFromKeyboardShortcut(); }; // we want to run this effect only once, when the component is mounted // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - useEffect(() => { - // Unregister the shortcut before registering a new one to avoid lingering shortcut listener - enterSubscription.current(); - if (!disableEnterShortCut) { - subscribeToEnterShortcut(); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [disableEnterShortCut]); - - useEffect(() => { - if (props.isFocused) { - subscribeToEnterShortcut(); - subscribeToCtrlEnterShortcut(); - } else { - unSubscribeFromKeyboardShortcut(); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [props.isFocused]); - // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { // Screen coming back into focus, for example From ab6d66c0c604102078487da2480480e850af358d Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 14:35:54 -0800 Subject: [PATCH 09/23] Improve typing of ActiveElementRoleProvider --- src/components/ActiveElementRoleProvider/index.tsx | 6 +++--- src/components/ActiveElementRoleProvider/types.ts | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/components/ActiveElementRoleProvider/index.tsx b/src/components/ActiveElementRoleProvider/index.tsx index 630af8618c08..8ba5df3b9e8f 100644 --- a/src/components/ActiveElementRoleProvider/index.tsx +++ b/src/components/ActiveElementRoleProvider/index.tsx @@ -1,15 +1,15 @@ import React, {useEffect, useState} from 'react'; -import type {ActiveElementRoleContextValue, ActiveElementRoleProps} from './types'; +import type {ActiveElementRoleContextValue, ActiveElementRoleProps, AriaRole} from './types'; const ActiveElementRoleContext = React.createContext({ role: null, }); function ActiveElementRoleProvider({children}: ActiveElementRoleProps) { - const [activeRoleRef, setRole] = useState(document?.activeElement?.role ?? null); + const [activeRoleRef, setRole] = useState((document?.activeElement?.role as AriaRole) ?? null); const handleFocusIn = () => { - setRole(document?.activeElement?.role ?? null); + setRole((document?.activeElement?.role as AriaRole) ?? null); }; const handleFocusOut = () => { diff --git a/src/components/ActiveElementRoleProvider/types.ts b/src/components/ActiveElementRoleProvider/types.ts index f22343b12550..e8d2703148e6 100644 --- a/src/components/ActiveElementRoleProvider/types.ts +++ b/src/components/ActiveElementRoleProvider/types.ts @@ -1,9 +1,14 @@ +import type {ValueOf} from 'type-fest'; +import type CONST from '@src/CONST'; + +type AriaRole = ValueOf; + type ActiveElementRoleContextValue = { - role: string | null; + role: AriaRole | null; }; type ActiveElementRoleProps = { children: React.ReactNode; }; -export type {ActiveElementRoleContextValue, ActiveElementRoleProps}; +export type {AriaRole, ActiveElementRoleContextValue, ActiveElementRoleProps}; From 2d59cf30d66626e5205efc54bea81dccb153fb82 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 14:47:34 -0800 Subject: [PATCH 10/23] Use useActiveElementRole hook directly --- .../OptionsSelector/BaseOptionsSelector.js | 30 ++++--------------- 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 21b5c11c1567..4ec940d08f99 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -11,6 +11,7 @@ import OptionsList from '@components/OptionsList'; import ReferralProgramCTA from '@components/ReferralProgramCTA'; import ShowMoreButton from '@components/ShowMoreButton'; import TextInput from '@components/TextInput'; +import useActiveElementRole from '@hooks/useActiveElementRole'; import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; @@ -59,13 +60,11 @@ function BaseOptionsSelector(props) { const themeStyles = useThemeStyles(); const isWebOrDesktop = [CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform()); - const accessibilityRoles = _.values(CONST.ROLE); const [disabledOptionsIndexes, setDisabledOptionsIndexes] = useState([]); const [errorMessage, setErrorMessage] = useState(''); const [value, setValue] = useState(''); const [paginationPage, setPaginationPage] = useState(1); - const [disableEnterShortCut, setDisableEnterShortCut] = useState(false); const shouldDisableRowSelection = useRef(false); const relatedTarget = useRef(null); @@ -211,23 +210,6 @@ function BaseOptionsSelector(props) { [props.canSelectMultipleOptions, focusedIndex, allOptions, isFocused, selectRow], ); - const handleFocusIn = () => { - const activeElement = document.activeElement; - setDisableEnterShortCut(activeElement && accessibilityRoles.includes(activeElement.role) && activeElement.role !== CONST.ROLE.PRESENTATION); - }; - - const handleFocusOut = () => { - setDisableEnterShortCut(false); - }; - - const subscribeActiveElement = () => { - if (!isWebOrDesktop) { - return; - } - document.addEventListener('focusin', handleFocusIn); - document.addEventListener('focusout', handleFocusOut); - }; - const selectOptions = useCallback(() => { if (props.canSelectMultipleOptions) { props.onConfirmSelection(); @@ -244,10 +226,11 @@ function BaseOptionsSelector(props) { // eslint-disable-next-line react-hooks/exhaustive-deps }, [allOptions, focusedIndex, props.canSelectMultipleOptions, props.onConfirmSelection, selectRow]); + const activeElementRole = useActiveElementRole(); useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, { shouldBubble: !allOptions[focusedIndex], captureOnInputs: true, - isActive: !disableEnterShortCut, + isActive: !activeElementRole || activeElementRole === CONST.ROLE.PRESENTATION, }); useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, selectOptions, { captureOnInputs: true, @@ -290,8 +273,6 @@ function BaseOptionsSelector(props) { ); useEffect(() => { - subscribeActiveElement(); - if (isFocused && props.autoFocus && textInputRef.current) { focusTimeout.current = setTimeout(() => { textInputRef.current.focus(); @@ -301,9 +282,10 @@ function BaseOptionsSelector(props) { scrollToIndex(props.selectedOptions.length ? 0 : focusedIndex, false); return () => { - if (focusTimeout.current) { - clearTimeout(focusTimeout.current); + if (!focusTimeout.current) { + return; } + clearTimeout(focusTimeout.current); }; // we want to run this effect only once, when the component is mounted // eslint-disable-next-line react-hooks/exhaustive-deps From bc3f69538830a79a9c4ed8a8874dfc81432e5d74 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 15:05:19 -0800 Subject: [PATCH 11/23] use forwardRef directly --- .../OptionsSelector/BaseOptionsSelector.js | 22 +++++-------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 4ec940d08f99..a14f8855b0e6 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -1,7 +1,7 @@ import {useIsFocused} from '@react-navigation/native'; import lodashGet from 'lodash/get'; import PropTypes from 'prop-types'; -import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; +import React, {forwardRef, useCallback, useEffect, useMemo, useRef, useState} from 'react'; import {ScrollView, View} from 'react-native'; import _ from 'underscore'; import Button from '@components/Button'; @@ -54,7 +54,7 @@ const defaultProps = { ...optionsSelectorDefaultProps, }; -function BaseOptionsSelector(props) { +function BaseOptionsSelector(props, textInputRef) { const isFocused = useIsFocused(); const {translate} = useLocalize(); const themeStyles = useThemeStyles(); @@ -69,13 +69,10 @@ function BaseOptionsSelector(props) { const shouldDisableRowSelection = useRef(false); const relatedTarget = useRef(null); const listRef = useRef(); - const textInputRef = useRef(); const focusTimeout = useRef(); const prevSelectedOptions = useRef(props.selectedOptions); const prevValue = useRef(value); - useImperativeHandle(props.forwardedRef, () => textInputRef.current); - /** * Paginate props.sections to only allow a certain number of items per section. */ @@ -537,17 +534,8 @@ function BaseOptionsSelector(props) { ); } -BaseOptionsSelector.defaultProps = defaultProps; BaseOptionsSelector.propTypes = propTypes; +BaseOptionsSelector.defaultProps = defaultProps; +BaseOptionsSelector.displayName = 'BaseOptionsSelector'; -const BaseOptionsSelectorWithRef = forwardRef((props, ref) => ( - -)); - -BaseOptionsSelectorWithRef.displayName = 'BaseOptionsSelectorWithRef'; - -export default BaseOptionsSelectorWithRef; +export default forwardRef(BaseOptionsSelector); From e40231707cab10c843b0737cd2925b3a58701117 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 15:09:34 -0800 Subject: [PATCH 12/23] Use usePrevious utility hook --- src/components/OptionsSelector/BaseOptionsSelector.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index a14f8855b0e6..7e7e1c7df2a6 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -66,12 +66,13 @@ function BaseOptionsSelector(props, textInputRef) { const [value, setValue] = useState(''); const [paginationPage, setPaginationPage] = useState(1); + const prevSelectedOptions = usePrevious(props.selectedOptions); + const prevValue = usePrevious(value); + const shouldDisableRowSelection = useRef(false); const relatedTarget = useRef(null); const listRef = useRef(); const focusTimeout = useRef(); - const prevSelectedOptions = useRef(props.selectedOptions); - const prevValue = useRef(value); /** * Paginate props.sections to only allow a certain number of items per section. @@ -313,9 +314,7 @@ function BaseOptionsSelector(props, textInputRef) { useEffect(() => { // If we just toggled an option on a multi-selection page or cleared the search input, scroll to top - if (props.selectedOptions.length !== prevSelectedOptions.current.length || (!!prevValue.current && !value)) { - prevSelectedOptions.current = props.selectedOptions; - prevValue.current = value; + if (props.selectedOptions.length !== prevSelectedOptions.length || (!!prevValue && !value)) { scrollToIndex(0); return; } From 4c05b9e8951e5b3d3d698c0d9ec2c6e8027ca5be Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 15:20:59 -0800 Subject: [PATCH 13/23] Undo bad ref change --- .../OptionsSelector/BaseOptionsSelector.js | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 7e7e1c7df2a6..9a750ac67b0d 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -1,7 +1,7 @@ import {useIsFocused} from '@react-navigation/native'; import lodashGet from 'lodash/get'; import PropTypes from 'prop-types'; -import React, {forwardRef, useCallback, useEffect, useMemo, useRef, useState} from 'react'; +import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; import {ScrollView, View} from 'react-native'; import _ from 'underscore'; import Button from '@components/Button'; @@ -54,7 +54,7 @@ const defaultProps = { ...optionsSelectorDefaultProps, }; -function BaseOptionsSelector(props, textInputRef) { +function BaseOptionsSelector(props) { const isFocused = useIsFocused(); const {translate} = useLocalize(); const themeStyles = useThemeStyles(); @@ -72,8 +72,11 @@ function BaseOptionsSelector(props, textInputRef) { const shouldDisableRowSelection = useRef(false); const relatedTarget = useRef(null); const listRef = useRef(); + const textInputRef = useRef(); const focusTimeout = useRef(); + useImperativeHandle(props.forwardedRef, () => textInputRef.current); + /** * Paginate props.sections to only allow a certain number of items per section. */ @@ -533,8 +536,17 @@ function BaseOptionsSelector(props, textInputRef) { ); } -BaseOptionsSelector.propTypes = propTypes; BaseOptionsSelector.defaultProps = defaultProps; -BaseOptionsSelector.displayName = 'BaseOptionsSelector'; +BaseOptionsSelector.propTypes = propTypes; + +const BaseOptionsSelectorWithRef = forwardRef((props, ref) => ( + +)); + +BaseOptionsSelectorWithRef.displayName = 'BaseOptionsSelectorWithRef'; -export default forwardRef(BaseOptionsSelector); +export default BaseOptionsSelectorWithRef; From 91c956ba96b46fee5fbd0c63eec24596c24af11e Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 15:22:27 -0800 Subject: [PATCH 14/23] Help reduce diff a bit --- src/components/OptionsSelector/BaseOptionsSelector.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 9a750ac67b0d..f1c6a0c10b56 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -66,15 +66,15 @@ function BaseOptionsSelector(props) { const [value, setValue] = useState(''); const [paginationPage, setPaginationPage] = useState(1); - const prevSelectedOptions = usePrevious(props.selectedOptions); - const prevValue = usePrevious(value); - const shouldDisableRowSelection = useRef(false); const relatedTarget = useRef(null); const listRef = useRef(); const textInputRef = useRef(); const focusTimeout = useRef(); + const prevSelectedOptions = usePrevious(props.selectedOptions); + const prevValue = usePrevious(value); + useImperativeHandle(props.forwardedRef, () => textInputRef.current); /** From 684be84898c5cf1b05ef64687096aa1de562591b Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 15:29:33 -0800 Subject: [PATCH 15/23] don't re-calculate focusedIndex unless options actually change --- src/components/OptionsSelector/BaseOptionsSelector.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index f1c6a0c10b56..171af07141dc 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -306,10 +306,13 @@ function BaseOptionsSelector(props) { }, [isFocused, props.autoFocus]); useEffect(() => { + if (_.isEqual(allOptions, prevOptions)) { + return; + } + const newFocusedIndex = props.selectedOptions.length; const prevFocusedOption = prevOptions[focusedIndex]; const indexOfPrevFocusedOptionInCurrentList = _.findIndex(allOptions, (option) => prevFocusedOption && option.keyForList === prevFocusedOption.keyForList); - setFocusedIndex(indexOfPrevFocusedOptionInCurrentList || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex)); // we want to run this effect only when the sections change // eslint-disable-next-line react-hooks/exhaustive-deps From 6641c003b7b0b821090174e1c764394081daf21e Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 15:51:25 -0800 Subject: [PATCH 16/23] Use canFocusInputOnScreenFocus rather than getPlatform --- .../OptionsSelector/BaseOptionsSelector.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 171af07141dc..238e631a667b 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -17,7 +17,7 @@ import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; import usePrevious from '@hooks/usePrevious'; import useThemeStyles from '@hooks/useThemeStyles'; -import getPlatform from '@libs/getPlatform'; +import canFocusInputOnScreenFocus from '@libs/canFocusInputOnScreenFocus'; import setSelection from '@libs/setSelection'; import CONST from '@src/CONST'; import {defaultProps as optionsSelectorDefaultProps, propTypes as optionsSelectorPropTypes} from './optionsSelectorPropTypes'; @@ -54,13 +54,13 @@ const defaultProps = { ...optionsSelectorDefaultProps, }; +const shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus(); + function BaseOptionsSelector(props) { const isFocused = useIsFocused(); const {translate} = useLocalize(); const themeStyles = useThemeStyles(); - const isWebOrDesktop = [CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform()); - const [disabledOptionsIndexes, setDisabledOptionsIndexes] = useState([]); const [errorMessage, setErrorMessage] = useState(''); const [value, setValue] = useState(''); @@ -292,17 +292,16 @@ function BaseOptionsSelector(props) { // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { // Screen coming back into focus, for example // when doing Cmd+Shift+K, then Cmd+K, then Cmd+Shift+K. - // Only applies to platforms that support keyboard shortcuts - if (isWebOrDesktop && isFocused && props.autoFocus && textInputRef.current) { - setTimeout(() => { - textInputRef.current.focus(); - }, CONST.ANIMATED_TRANSITION); + if (!shouldFocusInputOnScreenFocus || !isFocused || !props.autoFocus || !textInputRef.current) { + return; } - // eslint-disable-next-line react-hooks/exhaustive-deps + + setTimeout(() => { + textInputRef.current.focus(); + }, CONST.ANIMATED_TRANSITION); }, [isFocused, props.autoFocus]); useEffect(() => { From e3f13dd918195099cb5e8ff2b75208f63d4533a9 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 16:29:45 -0800 Subject: [PATCH 17/23] Use useAutoFocusInput hook --- .../OptionsSelector/BaseOptionsSelector.js | 38 +++---------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 238e631a667b..d4faf810989a 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -13,6 +13,7 @@ import ShowMoreButton from '@components/ShowMoreButton'; import TextInput from '@components/TextInput'; import useActiveElementRole from '@hooks/useActiveElementRole'; import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; +import useAutoFocusInput from '@hooks/useAutoFocusInput'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; import usePrevious from '@hooks/usePrevious'; @@ -76,6 +77,7 @@ function BaseOptionsSelector(props) { const prevValue = usePrevious(value); useImperativeHandle(props.forwardedRef, () => textInputRef.current); + const {inputCallbackRef} = useAutoFocusInput(); /** * Paginate props.sections to only allow a certain number of items per section. @@ -273,37 +275,6 @@ function BaseOptionsSelector(props) { [allOptions, sections], ); - useEffect(() => { - if (isFocused && props.autoFocus && textInputRef.current) { - focusTimeout.current = setTimeout(() => { - textInputRef.current.focus(); - }, CONST.ANIMATED_TRANSITION); - } - - scrollToIndex(props.selectedOptions.length ? 0 : focusedIndex, false); - - return () => { - if (!focusTimeout.current) { - return; - } - clearTimeout(focusTimeout.current); - }; - // we want to run this effect only once, when the component is mounted - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); - - useEffect(() => { - // Screen coming back into focus, for example - // when doing Cmd+Shift+K, then Cmd+K, then Cmd+Shift+K. - if (!shouldFocusInputOnScreenFocus || !isFocused || !props.autoFocus || !textInputRef.current) { - return; - } - - setTimeout(() => { - textInputRef.current.focus(); - }, CONST.ANIMATED_TRANSITION); - }, [isFocused, props.autoFocus]); - useEffect(() => { if (_.isEqual(allOptions, prevOptions)) { return; @@ -386,7 +357,10 @@ function BaseOptionsSelector(props) { const textInput = ( { + textInputRef.current = el; + inputCallbackRef(el); + }} label={props.textInputLabel} accessibilityLabel={props.textInputLabel} role={CONST.ROLE.PRESENTATION} From 1ffd5c644f71d6661ba376a33bbb2cb6f4556bf5 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 16:32:29 -0800 Subject: [PATCH 18/23] Remove unnecessary canFocusInputOnScreenFocus --- src/components/OptionsSelector/BaseOptionsSelector.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index d4faf810989a..b2d910c7a9a7 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -18,7 +18,6 @@ import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; import usePrevious from '@hooks/usePrevious'; import useThemeStyles from '@hooks/useThemeStyles'; -import canFocusInputOnScreenFocus from '@libs/canFocusInputOnScreenFocus'; import setSelection from '@libs/setSelection'; import CONST from '@src/CONST'; import {defaultProps as optionsSelectorDefaultProps, propTypes as optionsSelectorPropTypes} from './optionsSelectorPropTypes'; @@ -55,8 +54,6 @@ const defaultProps = { ...optionsSelectorDefaultProps, }; -const shouldFocusInputOnScreenFocus = canFocusInputOnScreenFocus(); - function BaseOptionsSelector(props) { const isFocused = useIsFocused(); const {translate} = useLocalize(); From e6ee35f1e5ca7460ad585a4bc2c4d48686cfb0ef Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 27 Feb 2024 16:33:39 -0800 Subject: [PATCH 19/23] Remove unused focusTimeout ref --- src/components/OptionsSelector/BaseOptionsSelector.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index b2d910c7a9a7..0fb380fe5e5d 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -68,7 +68,6 @@ function BaseOptionsSelector(props) { const relatedTarget = useRef(null); const listRef = useRef(); const textInputRef = useRef(); - const focusTimeout = useRef(); const prevSelectedOptions = usePrevious(props.selectedOptions); const prevValue = usePrevious(value); From 9990129641746ff7a6de15e747fda29885e7cf18 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 28 Feb 2024 09:52:18 -0800 Subject: [PATCH 20/23] make sure enter keyboard shortcut is only active if the screen is focused --- src/components/OptionsSelector/BaseOptionsSelector.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index c4c2a12df969..60ed4d137c84 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -228,7 +228,7 @@ function BaseOptionsSelector(props) { useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, { shouldBubble: !allOptions[focusedIndex], captureOnInputs: true, - isActive: !activeElementRole || activeElementRole === CONST.ROLE.PRESENTATION, + isActive: isFocused && (!activeElementRole || activeElementRole === CONST.ROLE.PRESENTATION), }); useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, selectOptions, { captureOnInputs: true, From 5288a654bee120b76c0f88e2f9cc922187c46f69 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 28 Feb 2024 12:30:37 -0800 Subject: [PATCH 21/23] Add back accidentally removed hook import --- src/components/OptionsSelector/BaseOptionsSelector.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 60ed4d137c84..191983de5f45 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -16,6 +16,7 @@ import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; import useAutoFocusInput from '@hooks/useAutoFocusInput'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import useLocalize from '@hooks/useLocalize'; +import usePrevious from '@hooks/usePrevious'; import useThemeStyles from '@hooks/useThemeStyles'; import setSelection from '@libs/setSelection'; import CONST from '@src/CONST'; From 43b82664f0b0dac702efd2aadfc3af837d6c6f2b Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 28 Feb 2024 12:32:47 -0800 Subject: [PATCH 22/23] Remove adjustedSectionIndex --- .../OptionsSelector/BaseOptionsSelector.js | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 191983de5f45..6f6cbaf20e2c 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -256,17 +256,7 @@ function BaseOptionsSelector(props) { return; } - // Note: react-native's SectionList automatically strips out any empty sections. - // So we need to reduce the sectionIndex to remove any empty sections in front of the one we're trying to scroll to. - // Otherwise, it will cause an index-out-of-bounds error and crash the app. - let adjustedSectionIndex = sectionIndex; - for (let i = 0; i < sectionIndex; i++) { - if (_.isEmpty(lodashGet(sections, `[${i}].data`))) { - adjustedSectionIndex--; - } - } - - listRef.current.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated}); + listRef.current.scrollToLocation({sectionIndex, itemIndex, animated}); }, [allOptions, sections], ); From b031d1e19b3b837186579a0c798ded1e23360fb6 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 28 Feb 2024 12:54:17 -0800 Subject: [PATCH 23/23] Use paginated sections to generate allOptions --- src/components/OptionsSelector/BaseOptionsSelector.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/OptionsSelector/BaseOptionsSelector.js b/src/components/OptionsSelector/BaseOptionsSelector.js index 6f6cbaf20e2c..6a08b32800e5 100755 --- a/src/components/OptionsSelector/BaseOptionsSelector.js +++ b/src/components/OptionsSelector/BaseOptionsSelector.js @@ -103,7 +103,7 @@ function BaseOptionsSelector(props) { const options = []; const calcDisabledOptionIndexes = []; let index = 0; - _.each(props.sections, (section, sectionIndex) => { + _.each(sections, (section, sectionIndex) => { _.each(section.data, (option, optionIndex) => { // eslint-disable-next-line no-param-reassign option.sectionIndex = sectionIndex; @@ -120,7 +120,7 @@ function BaseOptionsSelector(props) { }); setDisabledOptionsIndexes(calcDisabledOptionIndexes); return options; - }, [props.sections]); + }, [sections]); const prevOptions = usePrevious(allOptions); const initialFocusedIndex = useMemo(() => {