-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add focus trap #39520
Changes from all commits
ec50a62
623f6ca
7700359
ee852a4
d64cb37
cfdab9a
76887e4
7f25987
3425512
9740843
368610e
5ced21b
9928153
12846dd
41862ad
eb4b654
6e3ba8c
23f47d4
1ac3596
bc9e6eb
296e9a1
e707d95
7c78aaa
d0d29a2
7c6626b
9c995b5
69b14e3
745f06f
aff1627
11fda35
12b2bae
7f0b86d
a9743ce
eb3b538
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 |
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; |
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import FocusTrap from 'focus-trap-react'; | ||
import React from 'react'; | ||
import sharedTrapStack from '@components/FocusTrap/sharedTrapStack'; | ||
import type FocusTrapForModalProps from './FocusTrapForModalProps'; | ||
|
||
function FocusTrapForModal({children, active}: FocusTrapForModalProps) { | ||
return ( | ||
<FocusTrap | ||
active={active} | ||
focusTrapOptions={{ | ||
trapStack: sharedTrapStack, | ||
allowOutsideClick: true, | ||
fallbackFocus: document.body, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
</FocusTrap> | ||
); | ||
} | ||
|
||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import type FocusTrapProps from './FocusTrapProps'; | ||
|
||
function FocusTrapForScreen({children}: FocusTrapProps) { | ||
return children; | ||
} | ||
|
||
FocusTrapForScreen.displayName = 'FocusTrapForScreen'; | ||
|
||
export default FocusTrapForScreen; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import {useFocusEffect, useIsFocused, useRoute} from '@react-navigation/native'; | ||
import FocusTrap 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 = ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This variable is shared between instances of |
||
function FocusTrapForScreen({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 ( | ||
<FocusTrap | ||
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} | ||
</FocusTrap> | ||
); | ||
} | ||
|
||
FocusTrapForScreen.displayName = 'FocusTrapForScreen'; | ||
|
||
export default FocusTrapForScreen; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import {CENTRAL_PANE_WORKSPACE_SCREENS} from '@libs/Navigation/AppNavigator/Navigators/FullScreenNavigator'; | ||
import SCREENS from '@src/SCREENS'; | ||
|
||
const SCREENS_WITH_AUTOFOCUS: string[] = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remember correctly I tried two different approaches:
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.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; |
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import NAVIGATORS from '@src/NAVIGATORS'; | ||
import SCREENS from '@src/SCREENS'; | ||
|
||
// Screens that should not have active focus trap when rendered on wide screen in order to allow Tab navigation in LHP and RHP | ||
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, | ||
SCREENS.SEARCH.CENTRAL_PANE, | ||
]; | ||
|
||
export default WIDE_LAYOUT_INACTIVE_SCREENS; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import type {FocusTrap as FocusTrapHandler} from 'focus-trap'; | ||
|
||
// focus-trap is capable of managing many traps. It's necessary for RHP and modals | ||
const trapStack: FocusTrapHandler[] = []; | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export default trapStack; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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. | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this view necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's necessary because There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
- <View>
+ <View style={[styles.w100]}> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jnowakow @roryabraham This is a real blocker There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this (image after removing) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request to add library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request was resolved successfully