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

Add focus trap #39520

Merged
merged 34 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ec50a62
fix BaseGenericPressable defocusing after press
adamgrzybowski Apr 3, 2024
623f6ca
add focus-trap-react package
adamgrzybowski Apr 3, 2024
7700359
add focus trap for screens and popovers
adamgrzybowski Apr 3, 2024
ee852a4
fix back button
kosmydel Apr 8, 2024
d64cb37
Merge branch 'main' into add-focus-trap
jnowakow Apr 26, 2024
cfdab9a
Merge branch 'main' into add-focus-trap
jnowakow May 6, 2024
76887e4
Merge branch 'main' into add-focus-trap
jnowakow May 7, 2024
7f25987
Add focus trap to report context menu
jnowakow May 7, 2024
3425512
Merge branch 'main' into add-focus-trap
jnowakow May 8, 2024
9740843
Fix focus and tab navigation in Context Menu
jnowakow May 8, 2024
368610e
Point to new search screen in autofocus
jnowakow May 8, 2024
5ced21b
Fix linter
jnowakow May 9, 2024
9928153
Apply review comment
jnowakow May 9, 2024
12846dd
Merge branch 'main' into add-focus-trap
jnowakow May 13, 2024
41862ad
Fix ts
jnowakow May 13, 2024
eb4b654
Merge branch 'main' into add-focus-trap
jnowakow May 13, 2024
6e3ba8c
Fix prettier
jnowakow May 13, 2024
23f47d4
Merge branch 'main' into add-focus-trap
jnowakow May 14, 2024
1ac3596
useCallback
jnowakow May 14, 2024
bc9e6eb
Merge branch 'main' into add-focus-trap
jnowakow May 15, 2024
296e9a1
Workspace screens in autofocus
jnowakow May 15, 2024
e707d95
Merge branch 'main' into add-focus-trap
jnowakow May 15, 2024
7c78aaa
Fix
jnowakow May 15, 2024
d0d29a2
Merge branch 'main' into add-focus-trap
jnowakow Jun 3, 2024
7c6626b
Merge branch 'main' into add-focus-trap
jnowakow Jun 4, 2024
9c995b5
Merge branch 'main' into add-focus-trap
jnowakow Jun 5, 2024
69b14e3
Replace react-native-web focus trap with our implementation
jnowakow Jun 6, 2024
745f06f
Merge branch 'main' into add-focus-trap
jnowakow Jun 6, 2024
aff1627
Merge branch 'main' into add-focus-trap
jnowakow Jun 7, 2024
11fda35
Merge branch 'main' into add-focus-trap
jnowakow Jun 10, 2024
12b2bae
Fix focus trap in tab navigator and back button in contact details
jnowakow Jun 10, 2024
7f0b86d
Merge branch 'main' into add-focus-trap
jnowakow Jun 11, 2024
a9743ce
Address review feedback
jnowakow Jun 11, 2024
eb3b538
fix prettier
jnowakow Jun 11, 2024
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
28 changes: 28 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
"expo-av": "~13.10.4",
"expo-image": "1.11.0",
"expo-image-manipulator": "11.8.0",
"focus-trap-react": "^10.2.3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request was resolved successfully

"htmlparser2": "^7.2.0",
"idb-keyval": "^6.2.1",
"jest-expo": "50.0.1",
Expand Down Expand Up @@ -248,8 +249,8 @@
"babel-plugin-transform-class-properties": "^6.24.1",
"babel-plugin-transform-remove-console": "^6.9.4",
"clean-webpack-plugin": "^4.0.0",
"copy-webpack-plugin": "^10.1.0",
"concurrently": "^8.2.2",
"copy-webpack-plugin": "^10.1.0",
"css-loader": "^6.7.2",
"diff-so-fancy": "^1.3.0",
"dotenv": "^16.0.3",
Expand Down
5 changes: 5 additions & 0 deletions src/components/FocusTrap/BOTTOM_TAB_SCREENS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import SCREENS from '@src/SCREENS';

const BOTTOM_TAB_SCREENS: string[] = [SCREENS.HOME, SCREENS.SETTINGS.ROOT];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of string[], use BottomTabName[]


export default BOTTOM_TAB_SCREENS;
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type FocusTrapForModalProps = {
children: React.ReactNode;
active: boolean;
};

export default FocusTrapForModalProps;
9 changes: 9 additions & 0 deletions src/components/FocusTrap/FocusTrapForModal/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type FocusTrapForModalProps from './FocusTrapForModalProps';

function FocusTrapForModal({children}: FocusTrapForModalProps) {
return children;
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
}

FocusTrapForModal.displayName = 'FocusTrapForModal';

export default FocusTrapForModal;
23 changes: 23 additions & 0 deletions src/components/FocusTrap/FocusTrapForModal/index.web.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import FocusTrapOriginal from 'focus-trap-react';
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
import React from 'react';
import sharedTrapStack from '@components/FocusTrap/sharedTrapStack';
import type FocusTrapForModalProps from './FocusTrapForModalProps';

function FocusTrapForModal({children, active}: FocusTrapForModalProps) {
return (
<FocusTrapOriginal
active={active}
focusTrapOptions={{
trapStack: sharedTrapStack,
allowOutsideClick: true,
fallbackFocus: document.body,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should disable the initial focus to prevent default selection on popup menus.#43659

}}
>
{children}
</FocusTrapOriginal>
);
}

FocusTrapForModal.displayName = 'FocusTrapForModal';

export default FocusTrapForModal;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type FocusTrapForScreenProps = {
children: React.ReactNode;
};

export default FocusTrapForScreenProps;
9 changes: 9 additions & 0 deletions src/components/FocusTrap/FocusTrapForScreen/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type FocusTrapProps from './FocusTrapProps';

function FocusTrapView({children}: FocusTrapProps) {
return children;
}

FocusTrapView.displayName = 'FocusTrapView';

export default FocusTrapView;
68 changes: 68 additions & 0 deletions src/components/FocusTrap/FocusTrapForScreen/index.web.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import {useFocusEffect, useIsFocused, useRoute} from '@react-navigation/native';
import FocusTrapOriginal from 'focus-trap-react';
import React, {useMemo, useRef} from 'react';
import BOTTOM_TAB_SCREENS from '@components/FocusTrap/BOTTOM_TAB_SCREENS';
import SCREENS_WITH_AUTOFOCUS from '@components/FocusTrap/SCREENS_WITH_AUTOFOCUS';
import sharedTrapStack from '@components/FocusTrap/sharedTrapStack';
import WIDE_LAYOUT_INACTIVE_SCREENS from '@components/FocusTrap/WIDE_LAYOUT_INACTIVE_SCREENS';
import useWindowDimensions from '@hooks/useWindowDimensions';
import type FocusTrapProps from './FocusTrapProps';

let activeRouteName = '';
Copy link
Contributor

@Skalakid Skalakid May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need activeRouteName variable outside the component? Can't we just use, for example, useRef?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is shared between instances of FocusTrap component


function FocusTrap({children}: FocusTrapProps) {
const isFocused = useIsFocused();
const route = useRoute();
const {isSmallScreenWidth} = useWindowDimensions();

const isActive = useMemo(() => {
// Focus trap can't be active on bottom tab screens because it would block access to the tab bar.
if (BOTTOM_TAB_SCREENS.includes(route.name)) {
return false;
}

// Focus trap can't be active on these screens if the layout is wide because they may be displayed side by side.
if (WIDE_LAYOUT_INACTIVE_SCREENS.includes(route.name) && !isSmallScreenWidth) {
return false;
}
return true;
}, [isSmallScreenWidth, route]);

useFocusEffect(() => {
activeRouteName = route.name;
});

const focusTrapRef = useRef<FocusTrapOriginal | null>(null);

return (
<FocusTrapOriginal
ref={focusTrapRef}
active={isActive}
paused={!isFocused}
focusTrapOptions={{
trapStack: sharedTrapStack,
allowOutsideClick: true,
fallbackFocus: document.body,
// We don't want to ovverride autofocus on these screens.
initialFocus: () => {
if (SCREENS_WITH_AUTOFOCUS.includes(activeRouteName)) {
return false;
}
return undefined;
},
setReturnFocus: (element) => {
if (SCREENS_WITH_AUTOFOCUS.includes(activeRouteName)) {
return false;
}
return element;
},
}}
>
{children}
</FocusTrapOriginal>
);
}

FocusTrap.displayName = 'FocusTrapView';

export default FocusTrap;
15 changes: 15 additions & 0 deletions src/components/FocusTrap/SCREENS_WITH_AUTOFOCUS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import SCREENS from '@src/SCREENS';

const SCREENS_WITH_AUTOFOCUS: string[] = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty manual/brittle. Is there a way we can derive which screens have auto-focus or not, rather than having a const we need to remember to update?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is any other way to configure this. Maybe only by rewriting configuration in src/SCREENS.ts and enforcing this information there.
cc @adamgrzybowski

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly I tried two different approaches:

  1. Check if the input is focused -> That caused many problems because autofocusing is often applied with a little delay. e.g. after the screen stops animating. I couldn't get a satisfying solution this way

  2. There is a hook useAutofocusInput. I thought that we could check the name of the screen here and add it to the array. After some investigation (correct me if I am wrong) it turned out that it's not the only way we handle autofocus. In some places, it's just hardcoded without some general hook.

When I created this PR, the array with const seemed as the simplest and most reliable solution.

If we unify how we handle autofocus, we could once again consider option nr. 2. Or maybe there is another option I haven't consider

SCREENS.WORKSPACE_SWITCHER.ROOT,
SCREENS.SEARCH_ROOT,
SCREENS.REPORT,
SCREENS.REPORT_DESCRIPTION_ROOT,
SCREENS.PRIVATE_NOTES.EDIT,
SCREENS.SETTINGS.PROFILE.STATUS,
SCREENS.SETTINGS.PROFILE.PRONOUNS,
SCREENS.NEW_TASK.DETAILS,
SCREENS.MONEY_REQUEST.CREATE,
];

export default SCREENS_WITH_AUTOFOCUS;
36 changes: 36 additions & 0 deletions src/components/FocusTrap/WIDE_LAYOUT_INACTIVE_SCREENS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import NAVIGATORS from '@src/NAVIGATORS';
import SCREENS from '@src/SCREENS';

const WIDE_LAYOUT_INACTIVE_SCREENS: string[] = [
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
NAVIGATORS.BOTTOM_TAB_NAVIGATOR,
SCREENS.HOME,
SCREENS.SETTINGS.ROOT,
SCREENS.REPORT,
SCREENS.SETTINGS.PROFILE.ROOT,
SCREENS.SETTINGS.PREFERENCES.ROOT,
SCREENS.SETTINGS.SECURITY,
SCREENS.SETTINGS.WALLET.ROOT,
SCREENS.SETTINGS.ABOUT,
SCREENS.SETTINGS.WORKSPACES,

SCREENS.WORKSPACE.INITIAL,

SCREENS.WORKSPACE.PROFILE,
SCREENS.WORKSPACE.CARD,
SCREENS.WORKSPACE.WORKFLOWS,
SCREENS.WORKSPACE.WORKFLOWS_APPROVER,
SCREENS.WORKSPACE.WORKFLOWS_AUTO_REPORTING_FREQUENCY,
SCREENS.WORKSPACE.WORKFLOWS_AUTO_REPORTING_MONTHLY_OFFSET,
SCREENS.WORKSPACE.REIMBURSE,
SCREENS.WORKSPACE.BILLS,
SCREENS.WORKSPACE.INVOICES,
SCREENS.WORKSPACE.TRAVEL,
SCREENS.WORKSPACE.MEMBERS,
SCREENS.WORKSPACE.CATEGORIES,
SCREENS.WORKSPACE.MORE_FEATURES,
SCREENS.WORKSPACE.TAGS,
SCREENS.WORKSPACE.TAXES,
SCREENS.WORKSPACE.DISTANCE_RATES,
];

export default WIDE_LAYOUT_INACTIVE_SCREENS;
5 changes: 5 additions & 0 deletions src/components/FocusTrap/sharedTrapStack.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type {FocusTrap as FocusTrapHandler} from 'focus-trap';

const trapStack: FocusTrapHandler[] = [];
roryabraham marked this conversation as resolved.
Show resolved Hide resolved

export default trapStack;
61 changes: 32 additions & 29 deletions src/components/PopoverMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import useWindowDimensions from '@hooks/useWindowDimensions';
import CONST from '@src/CONST';
import type {AnchorPosition} from '@src/styles';
import type AnchorAlignment from '@src/types/utils/AnchorAlignment';
import FocusTrapForModal from './FocusTrap/FocusTrapForModal';
import * as Expensicons from './Icon/Expensicons';
import type {MenuItemProps} from './MenuItem';
import MenuItem from './MenuItem';
Expand Down Expand Up @@ -189,35 +190,37 @@ function PopoverMenu({
withoutOverlay={withoutOverlay}
shouldSetModalVisibility={shouldSetModalVisibility}
>
<View style={isSmallScreenWidth ? {} : styles.createMenuContainer}>
{!!headerText && <Text style={[styles.createMenuHeaderText, styles.ml3]}>{headerText}</Text>}
{enteredSubMenuIndexes.length > 0 && renderBackButtonItem()}
{currentMenuItems.map((item, menuIndex) => (
<MenuItem
key={item.text}
icon={item.icon}
iconWidth={item.iconWidth}
iconHeight={item.iconHeight}
iconFill={item.iconFill}
contentFit={item.contentFit}
title={item.text}
shouldCheckActionAllowedOnPress={false}
description={item.description}
numberOfLinesDescription={item.numberOfLinesDescription}
onPress={() => selectItem(menuIndex)}
focused={focusedIndex === menuIndex}
displayInDefaultIconColor={item.displayInDefaultIconColor}
shouldShowRightIcon={item.shouldShowRightIcon}
shouldPutLeftPaddingWhenNoIcon={item.shouldPutLeftPaddingWhenNoIcon}
label={item.label}
isLabelHoverable={item.isLabelHoverable}
floatRightAvatars={item.floatRightAvatars}
floatRightAvatarSize={item.floatRightAvatarSize}
shouldShowSubscriptRightAvatar={item.shouldShowSubscriptRightAvatar}
disabled={item.disabled}
/>
))}
</View>
<FocusTrapForModal active={isVisible}>
<View style={isSmallScreenWidth ? {} : styles.createMenuContainer}>
{!!headerText && <Text style={[styles.createMenuHeaderText, styles.ml3]}>{headerText}</Text>}
{enteredSubMenuIndexes.length > 0 && renderBackButtonItem()}
{currentMenuItems.map((item, menuIndex) => (
<MenuItem
key={item.text}
icon={item.icon}
iconWidth={item.iconWidth}
iconHeight={item.iconHeight}
iconFill={item.iconFill}
contentFit={item.contentFit}
title={item.text}
shouldCheckActionAllowedOnPress={false}
description={item.description}
numberOfLinesDescription={item.numberOfLinesDescription}
onPress={() => selectItem(menuIndex)}
focused={focusedIndex === menuIndex}
displayInDefaultIconColor={item.displayInDefaultIconColor}
shouldShowRightIcon={item.shouldShowRightIcon}
shouldPutLeftPaddingWhenNoIcon={item.shouldPutLeftPaddingWhenNoIcon}
label={item.label}
isLabelHoverable={item.isLabelHoverable}
floatRightAvatars={item.floatRightAvatars}
floatRightAvatarSize={item.floatRightAvatarSize}
shouldShowSubscriptRightAvatar={item.shouldShowSubscriptRightAvatar}
disabled={item.disabled}
/>
))}
</View>
</FocusTrapForModal>
</PopoverWithMeasuredContent>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,11 @@ function GenericPressable(
if (shouldUseHapticsOnLongPress) {
HapticFeedback.longPress();
}
if (ref && 'current' in ref) {
if (ref && 'current' in ref && nextFocusRef) {
ref.current?.blur();
Accessibility.moveAccessibilityFocus(nextFocusRef);
}
onLongPress(event);

Accessibility.moveAccessibilityFocus(nextFocusRef);
},
[shouldUseHapticsOnLongPress, onLongPress, nextFocusRef, ref, isDisabled],
);
Expand All @@ -106,11 +105,11 @@ function GenericPressable(
if (shouldUseHapticsOnPress) {
HapticFeedback.press();
}
if (ref && 'current' in ref) {
if (ref && 'current' in ref && nextFocusRef) {
ref.current?.blur();
Accessibility.moveAccessibilityFocus(nextFocusRef);
}
const onPressResult = onPress(event);
Accessibility.moveAccessibilityFocus(nextFocusRef);
return onPressResult;
},
[shouldUseHapticsOnPress, onPress, nextFocusRef, ref, isDisabled],
Expand Down
Loading
Loading