Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent parent view scroll when press space bar and arrow #56435

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const keyInputUpArrow = KeyCommand?.constants?.keyInputUpArrow ?? 'keyInputUpArr
const keyInputDownArrow = KeyCommand?.constants?.keyInputDownArrow ?? 'keyInputDownArrow';
const keyInputLeftArrow = KeyCommand?.constants?.keyInputLeftArrow ?? 'keyInputLeftArrow';
const keyInputRightArrow = KeyCommand?.constants?.keyInputRightArrow ?? 'keyInputRightArrow';
const keyInputSpace = ' ';

// describes if a shortcut key can cause navigation
const KEYBOARD_SHORTCUT_NAVIGATION_TYPE = 'NAVIGATION_SHORTCUT';
Expand Down Expand Up @@ -921,6 +922,14 @@ const CONST = {
shortcutKey: 'Backspace',
modifiers: [],
},
SPACE: {
descriptionKey: null,
shortcutKey: 'Space',
modifiers: [],
trigger: {
DEFAULT: {input: keyInputSpace},
},
},
},
KEYBOARD_SHORTCUTS_TYPES: {
NAVIGATION_SHORTCUT: KEYBOARD_SHORTCUT_NAVIGATION_TYPE,
Expand Down
57 changes: 40 additions & 17 deletions src/components/PopoverMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/* eslint-disable react/jsx-props-no-spreading */
import lodashIsEqual from 'lodash/isEqual';
import type {ReactNode, RefObject} from 'react';
import React, {useLayoutEffect, useState} from 'react';
import type {RefObject} from 'react';
import React, {useCallback, useLayoutEffect, useRef, useState} from 'react';
import {StyleSheet, View} from 'react-native';
import type {StyleProp, TextStyle, ViewStyle} from 'react-native';
// eslint-disable-next-line no-restricted-imports
import type {ScrollView as ScrollViewType, StyleProp, TextStyle, ViewStyle} from 'react-native';
import type {ModalProps} from 'react-native-modal';
import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager';
import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
Expand All @@ -12,8 +13,9 @@ import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as Browser from '@libs/Browser';
import * as Modal from '@userActions/Modal';
import {isSafari} from '@libs/Browser';
import getPlatform from '@libs/getPlatform';
import {close as closeModal} from '@userActions/Modal';
import CONST from '@src/CONST';
import type {AnchorPosition} from '@src/styles';
import type {PendingAction} from '@src/types/onyx/OnyxCommon';
Expand Down Expand Up @@ -135,14 +137,6 @@ type PopoverMenuProps = Partial<PopoverModalProps> & {
testID?: string;
};

const renderWithConditionalWrapper = (shouldUseScrollView: boolean, contentContainerStyle: StyleProp<ViewStyle>, children: ReactNode): React.JSX.Element => {
if (shouldUseScrollView) {
return <ScrollView contentContainerStyle={contentContainerStyle}>{children}</ScrollView>;
}
// eslint-disable-next-line react/jsx-no-useless-fragment
return <>{children}</>;
};

function getSelectedItemIndex(menuItems: PopoverMenuItem[]) {
return menuItems.findIndex((option) => option.isSelected);
}
Expand Down Expand Up @@ -189,8 +183,10 @@ function PopoverMenu({
const currentMenuItemsFocusedIndex = getSelectedItemIndex(currentMenuItems);
const [enteredSubMenuIndexes, setEnteredSubMenuIndexes] = useState<readonly number[]>(CONST.EMPTY_ARRAY);
const {windowHeight} = useWindowDimensions();

const platform = getPlatform();
const isWebOrDesktop = platform === CONST.PLATFORM.WEB || platform === CONST.PLATFORM.DESKTOP;
const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: currentMenuItemsFocusedIndex, maxIndex: currentMenuItems.length - 1, isActive: isVisible});
const scrollViewRef = useRef<HTMLDivElement | ScrollViewType>(null);

const selectItem = (index: number) => {
const selectedItem = currentMenuItems.at(index);
Expand All @@ -202,9 +198,9 @@ function PopoverMenu({
setEnteredSubMenuIndexes([...enteredSubMenuIndexes, index]);
const selectedSubMenuItemIndex = selectedItem?.subMenuItems.findIndex((option) => option.isSelected);
setFocusedIndex(selectedSubMenuItemIndex);
} else if (selectedItem.shouldCallAfterModalHide && !Browser.isSafari()) {
} else if (selectedItem.shouldCallAfterModalHide && !isSafari()) {
onItemSelected?.(selectedItem, index);
Modal.close(
closeModal(
() => {
selectedItem.onSelected?.();
},
Expand Down Expand Up @@ -311,6 +307,23 @@ function PopoverMenu({
},
{isActive: isVisible},
);
const keyboardShortcutSpaceCallback = useCallback(() => {
if (!scrollViewRef.current) {
return;
}
const scrollableElement = scrollViewRef.current as HTMLDivElement;
// If the popover menu is scrollable, allow scrolling with the space bar instead of preventing it.
const clientHeight = scrollableElement.clientHeight;
const scrollTop = scrollableElement.scrollTop;
const scrollHeight = scrollableElement.scrollHeight;
const remainingScrollSpace = scrollHeight - clientHeight - scrollTop;

scrollableElement.scrollBy(0, Math.min(clientHeight, remainingScrollSpace));
}, []);
linhvovan29546 marked this conversation as resolved.
Show resolved Hide resolved

// On web and desktop, pressing the space bar after interacting with the parent view
// can cause the parent view to scroll when the space bar is pressed.
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.SPACE, keyboardShortcutSpaceCallback, {isActive: isWebOrDesktop && isVisible});
linhvovan29546 marked this conversation as resolved.
Show resolved Hide resolved

const onModalHide = () => {
setFocusedIndex(currentMenuItemsFocusedIndex);
Expand Down Expand Up @@ -368,7 +381,17 @@ function PopoverMenu({
<View style={[isSmallScreenWidth ? {maxHeight: windowHeight - 250} : styles.createMenuContainer, containerStyles]}>
{renderHeaderText()}
{enteredSubMenuIndexes.length > 0 && renderBackButtonItem()}
{renderWithConditionalWrapper(shouldUseScrollView, scrollContainerStyle, renderedMenuItems)}
{shouldUseScrollView ? (
linhvovan29546 marked this conversation as resolved.
Show resolved Hide resolved
<ScrollView
contentContainerStyle={scrollContainerStyle}
ref={scrollViewRef as RefObject<ScrollViewType>}
>
{renderedMenuItems}
</ScrollView>
) : (
// eslint-disable-next-line react/jsx-no-useless-fragment
<>{renderedMenuItems}</>
)}
</View>
</FocusTrapForModal>
</PopoverWithMeasuredContent>
Expand Down
4 changes: 4 additions & 0 deletions src/components/ScrollView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ import type {ForwardedRef} from 'react';
// eslint-disable-next-line no-restricted-imports
import {ScrollView as RNScrollView} from 'react-native';
import type {ScrollViewProps} from 'react-native';
import useScrollEnabled from '@hooks/useScrollEnabled';

function ScrollView({children, scrollIndicatorInsets, ...props}: ScrollViewProps, ref: ForwardedRef<RNScrollView>) {
const scrollEnabled = useScrollEnabled();

return (
<RNScrollView
ref={ref}
Expand All @@ -13,6 +16,7 @@ function ScrollView({children, scrollIndicatorInsets, ...props}: ScrollViewProps
// to closest value to 0 fixes this issue, 0 (default) doesn't work
// See: https://github.com/Expensify/App/issues/31441
scrollIndicatorInsets={scrollIndicatorInsets ?? {right: Number.MIN_VALUE}}
scrollEnabled={scrollEnabled}
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
>
Expand Down
3 changes: 3 additions & 0 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
import useKeyboardState from '@hooks/useKeyboardState';
import useLocalize from '@hooks/useLocalize';
import usePrevious from '@hooks/usePrevious';
import useScrollEnabled from '@hooks/useScrollEnabled';
import useSingleExecution from '@hooks/useSingleExecution';
import useStyledSafeAreaInsets from '@hooks/useStyledSafeAreaInsets';
import useThemeStyles from '@hooks/useThemeStyles';
Expand Down Expand Up @@ -135,6 +136,7 @@ function BaseSelectionList<TItem extends ListItem>(
const shouldShowSelectAll = !!onSelectAll;
const activeElementRole = useActiveElementRole();
const isFocused = useIsFocused();
const scrollEnabled = useScrollEnabled();
const [maxToRenderPerBatch, setMaxToRenderPerBatch] = useState(shouldUseDynamicMaxToRenderPerBatch ? 0 : CONST.MAX_TO_RENDER_PER_BATCH.DEFAULT);
const [isInitialSectionListRender, setIsInitialSectionListRender] = useState(true);
const {isKeyboardShown} = useKeyboardState();
Expand Down Expand Up @@ -872,6 +874,7 @@ function BaseSelectionList<TItem extends ListItem>(
listHeaderContent
)
}
scrollEnabled={scrollEnabled}
ListFooterComponent={listFooterContent ?? ShowMoreButtonInstance}
onEndReached={onEndReached}
onEndReachedThreshold={onEndReachedThreshold}
Expand Down
5 changes: 5 additions & 0 deletions src/hooks/useScrollEnabled/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type UseScrollEnabled from './types';

const useScrollEnabled: UseScrollEnabled = () => undefined;

export default useScrollEnabled;
10 changes: 10 additions & 0 deletions src/hooks/useScrollEnabled/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import {useIsFocused} from '@react-navigation/native';
import usePrevious from '@hooks/usePrevious';
import type UseScrollEnabled from './types';

const useScrollEnabled: UseScrollEnabled = () => {
const isFocused = useIsFocused();
const prevIsFocused = usePrevious(isFocused);
return !(prevIsFocused && !isFocused);
linhvovan29546 marked this conversation as resolved.
Show resolved Hide resolved
};
export default useScrollEnabled;
3 changes: 3 additions & 0 deletions src/hooks/useScrollEnabled/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
type UseScrollEnabled = () => boolean | undefined;

export default UseScrollEnabled;
4 changes: 4 additions & 0 deletions src/libs/KeyboardShortcut/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const keyInputUpArrow = KeyCommand?.constants?.keyInputUpArrow?.toString() ?? 'k
const keyInputDownArrow = KeyCommand?.constants?.keyInputDownArrow?.toString() ?? 'keyInputDownArrow';
const keyInputLeftArrow = KeyCommand?.constants?.keyInputLeftArrow?.toString() ?? 'keyInputLeftArrow';
const keyInputRightArrow = KeyCommand?.constants?.keyInputRightArrow?.toString() ?? 'keyInputRightArrow';
const keyInputSpace = ' ';

/**
* Generates the normalized display name for keyboard shortcuts.
Expand All @@ -67,6 +68,9 @@ function getDisplayName(key: string, modifiers: string | string[]): string {
if (key.toLowerCase() === keyInputRightArrow.toLowerCase()) {
return ['ARROWRIGHT'];
}
if (key === keyInputSpace) {
return ['SPACE'];
}
return [key.toUpperCase()];
})();

Expand Down
Loading