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 31 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.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,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
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
diff --git a/node_modules/react-native-web/dist/exports/Modal/index.js b/node_modules/react-native-web/dist/exports/Modal/index.js
index d5df021..e2c46cf 100644
--- a/node_modules/react-native-web/dist/exports/Modal/index.js
+++ b/node_modules/react-native-web/dist/exports/Modal/index.js
@@ -86,13 +86,11 @@ var Modal = /*#__PURE__*/React.forwardRef((props, forwardedRef) => {
onDismiss: onDismissCallback,
onShow: onShowCallback,
visible: visible
- }, /*#__PURE__*/React.createElement(ModalFocusTrap, {
- active: isActive
}, /*#__PURE__*/React.createElement(ModalContent, _extends({}, rest, {
active: isActive,
onRequestClose: onRequestClose,
ref: forwardedRef,
transparent: transparent
- }), children))));
+ }), children)));
});
export default Modal;
\ No newline at end of file
2 changes: 2 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3456,6 +3456,8 @@ const CONST = {
TIMER: 'timer',
/** Use for toolbars containing action buttons or components. */
TOOLBAR: 'toolbar',
/** Use for navigation elements */
NAVIGATION: 'navigation',
},
TRANSLATION_KEYS: {
ATTACHMENT: 'common.attachment',
Expand Down
5 changes: 5 additions & 0 deletions src/components/ContextMenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ type ContextMenuItemProps = {

/** Handles what to do when the item is focused */
onFocus?: () => void;

/** Handles what to do when the item loose focus */
onBlur?: () => void;
};

type ContextMenuItemHandle = {
Expand All @@ -74,6 +77,7 @@ function ContextMenuItem(
shouldPreventDefaultFocusOnPress = true,
buttonRef = {current: null},
onFocus = () => {},
onBlur = () => {},
}: ContextMenuItemProps,
ref: ForwardedRef<ContextMenuItemHandle>,
) {
Expand Down Expand Up @@ -130,6 +134,7 @@ function ContextMenuItem(
focused={isFocused}
interactive={isThrottledButtonActive}
onFocus={onFocus}
onBlur={onBlur}
/>
);
}
Expand Down
6 changes: 6 additions & 0 deletions src/components/FocusTrap/BOTTOM_TAB_SCREENS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import type {BottomTabName} from '@libs/Navigation/types';
import SCREENS from '@src/SCREENS';

const BOTTOM_TAB_SCREENS: BottomTabName[] = [SCREENS.HOME, SCREENS.SETTINGS.ROOT];

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;
73 changes: 73 additions & 0 deletions src/components/FocusTrap/FocusTrapForScreen/index.web.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import {useFocusEffect, useIsFocused, useRoute} from '@react-navigation/native';
import FocusTrapOriginal from 'focus-trap-react';
import React, {useCallback, useMemo} 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 TOP_TAB_SCREENS from '@components/FocusTrap/TOP_TAB_SCREENS';
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.find((screen) => screen === route.name)) {
return false;
}

// in top tabs only focus trap for currently shown tab should be active
if (TOP_TAB_SCREENS.find((screen) => screen === route.name)) {
return isFocused;
}

// 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;
}, [isFocused, isSmallScreenWidth, route.name]);

useFocusEffect(
useCallback(() => {
activeRouteName = route.name;
}, [route]),
);

return (
<FocusTrapOriginal
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;
16 changes: 16 additions & 0 deletions src/components/FocusTrap/SCREENS_WITH_AUTOFOCUS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import {CENTRAL_PANE_WORKSPACE_SCREENS} from '@libs/Navigation/AppNavigator/Navigators/FullScreenNavigator';
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

...Object.keys(CENTRAL_PANE_WORKSPACE_SCREENS),
SCREENS.SEARCH.CENTRAL_PANE,
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;
5 changes: 5 additions & 0 deletions src/components/FocusTrap/TOP_TAB_SCREENS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import CONST from '@src/CONST';

const TOP_TAB_SCREENS: string[] = [CONST.TAB.NEW_CHAT, CONST.TAB.NEW_ROOM, CONST.TAB_REQUEST.DISTANCE, CONST.TAB_REQUEST.MANUAL, CONST.TAB_REQUEST.SCAN];

export default TOP_TAB_SCREENS;
34 changes: 34 additions & 0 deletions src/components/FocusTrap/WIDE_LAYOUT_INACTIVE_SCREENS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
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;
2 changes: 1 addition & 1 deletion src/components/HeaderWithBackButton/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ function HeaderWithBackButton({
}
}}
style={[styles.touchableButtonImage]}
role="button"
role={CONST.ROLE.BUTTON}
accessibilityLabel={translate('common.back')}
id={CONST.BACK_BUTTON_NATIVE_ID}
>
Expand Down
6 changes: 5 additions & 1 deletion src/components/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ type MenuItemBaseProps = {
/** Handles what to do when the item is focused */
onFocus?: () => void;

/** Handles what to do when the item loose focus */
onBlur?: () => void;

/** Optional account id if it's user avatar or policy id if it's workspace avatar */
avatarID?: number | string;
};
Expand Down Expand Up @@ -365,6 +368,7 @@ function MenuItem(
isPaneMenu = false,
shouldPutLeftPaddingWhenNoIcon = false,
onFocus,
onBlur,
avatarID,
}: MenuItemProps,
ref: PressableRef,
Expand Down Expand Up @@ -462,7 +466,7 @@ function MenuItem(
};

return (
<View>
<View onBlur={onBlur}>
{!!label && !isLabelHoverable && (
<View style={[styles.ph5, labelStyle]}>
<Text style={StyleUtils.combineStyles([styles.sidebarLinkText, styles.optionAlternateText, styles.textLabelSupporting, styles.pre])}>{label}</Text>
Expand Down
21 changes: 13 additions & 8 deletions src/components/Modal/BaseModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {forwardRef, useCallback, useEffect, useMemo, useRef} from 'react'
import {View} from 'react-native';
import ReactNativeModal from 'react-native-modal';
import ColorSchemeWrapper from '@components/ColorSchemeWrapper';
import FocusTrapForModal from '@components/FocusTrap/FocusTrapForModal';
import useKeyboardState from '@hooks/useKeyboardState';
import usePrevious from '@hooks/usePrevious';
import useSafeAreaInsets from '@hooks/useSafeAreaInsets';
Expand Down Expand Up @@ -214,7 +215,7 @@ function BaseModal(
// a conflict between RN core and Reanimated shadow tree operations
// position absolute is needed to prevent the view from interfering with flex layout
collapsable={false}
style={[styles.pAbsolute]}
style={[styles.pAbsolute, {zIndex: 1}]}
>
<ReactNativeModal
// Prevent the parent element to capture a click. This is useful when the modal component is put inside a pressable.
Expand Down Expand Up @@ -251,14 +252,18 @@ function BaseModal(
avoidKeyboard={avoidKeyboard}
customBackdrop={shouldUseCustomBackdrop ? <Overlay onPress={handleBackdropPress} /> : undefined}
>
<ModalContent onDismiss={handleDismissModal}>
<View
style={[styles.defaultModalContainer, modalPaddingStyles, modalContainerStyle, !isVisible && styles.pointerEventsNone]}
ref={ref}
>
<ColorSchemeWrapper>{children}</ColorSchemeWrapper>
<FocusTrapForModal active={isVisible}>
<View>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this view necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's necessary because FocusTrap creates ref on it's first child. Without this View there's error saying that function components cannot be given refs. It's safer to add this View rather than to try to forward refs through ModalContent

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, let's add a comment explaining why the view is necessary.

Copy link
Contributor

@fedirjh fedirjh Jun 13, 2024

Choose a reason for hiding this comment

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

@jnowakow It seems like this extra view has caused a regression with FAB popover

Screenshot 2024-06-13 at 9 58 00 AM

possible fix :

- <View>
+ <View style={[styles.w100]}>

Copy link
Contributor

Choose a reason for hiding this comment

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

@jnowakow @roryabraham This is a real blocker

Screenshot 2024-06-13 at 1 20 32 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this <View> component really necessary? The one wrapping the <ModalContent>

(image after removing)
image
What do you think @fedirjh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm I found the thread with explanation 😄

<ModalContent onDismiss={handleDismissModal}>
<View
style={[styles.defaultModalContainer, modalPaddingStyles, modalContainerStyle, !isVisible && styles.pointerEventsNone]}
ref={ref}
>
<ColorSchemeWrapper>{children}</ColorSchemeWrapper>
</View>
</ModalContent>
</View>
</ModalContent>
</FocusTrapForModal>
</ReactNativeModal>
</View>
</ModalContext.Provider>
Expand Down
Loading
Loading