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

prevent initial focus trap while has a focused element. #44932

Merged
merged 16 commits into from
Jul 16, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
106 changes: 106 additions & 0 deletions patches/focus-trap+7.5.4.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
diff --git a/node_modules/focus-trap/dist/focus-trap.esm.js b/node_modules/focus-trap/dist/focus-trap.esm.js
index 10d56db..a6d76d8 100644
--- a/node_modules/focus-trap/dist/focus-trap.esm.js
+++ b/node_modules/focus-trap/dist/focus-trap.esm.js
@@ -100,8 +100,8 @@ var isKeyForward = function isKeyForward(e) {
var isKeyBackward = function isKeyBackward(e) {
return isTabEvent(e) && e.shiftKey;
};
-var delay = function delay(fn) {
- return setTimeout(fn, 0);
+var delay = function delay(fn, delayTime = 0) {
+ return setTimeout(() => setTimeout(fn, delayTime), 0);
};

// Array.find/findIndex() are not supported on IE; this replicates enough
@@ -283,7 +283,7 @@ var createFocusTrap = function createFocusTrap(elements, userOptions) {
return node;
};
var getInitialFocusNode = function getInitialFocusNode() {
- var node = getNodeForOption('initialFocus');
+ var node = getNodeForOption('initialFocus', state.containers);

// false explicitly indicates we want no initialFocus at all
if (node === false) {
@@ -744,7 +744,7 @@ var createFocusTrap = function createFocusTrap(elements, userOptions) {
// that caused the focus trap activation.
state.delayInitialFocusTimer = config.delayInitialFocus ? delay(function () {
tryFocus(getInitialFocusNode());
- }) : tryFocus(getInitialFocusNode());
+ }, typeof config.delayInitialFocus === 'number' ? config.delayInitialFocus : undefined) : tryFocus(getInitialFocusNode());
doc.addEventListener('focusin', checkFocusIn, true);
doc.addEventListener('mousedown', checkPointerDown, {
capture: true,
@@ -880,7 +880,7 @@ var createFocusTrap = function createFocusTrap(elements, userOptions) {
tryFocus(getReturnFocusNode(state.nodeFocusedBeforeActivation));
}
onPostDeactivate === null || onPostDeactivate === void 0 || onPostDeactivate();
- });
+ }, typeof config.delayInitialFocus === 'number' ? config.delayInitialFocus : undefined);
};
if (returnFocus && checkCanReturnFocus) {
checkCanReturnFocus(getReturnFocusNode(state.nodeFocusedBeforeActivation)).then(finishDeactivation, finishDeactivation);
diff --git a/node_modules/focus-trap/index.d.ts b/node_modules/focus-trap/index.d.ts
index 400db1b..69f4b94 100644
--- a/node_modules/focus-trap/index.d.ts
+++ b/node_modules/focus-trap/index.d.ts
@@ -16,7 +16,7 @@ declare module 'focus-trap' {
* `document.querySelector()` to find the DOM node), `false` to explicitly indicate
* an opt-out, or a function that returns a DOM node or `false`.
*/
- export type FocusTargetOrFalse = FocusTargetValueOrFalse | (() => FocusTargetValueOrFalse);
+ export type FocusTargetOrFalse = FocusTargetValueOrFalse | ((containers?: HTMLElement[]) => FocusTargetValueOrFalse | undefined);

type MouseEventToBoolean = (event: MouseEvent | TouchEvent) => boolean;
type KeyboardEventToBoolean = (event: KeyboardEvent) => boolean;
@@ -185,7 +185,7 @@ declare module 'focus-trap' {
* This prevents elements within the focusable element from capturing
* the event that triggered the focus trap activation.
*/
- delayInitialFocus?: boolean;
+ delayInitialFocus?: boolean | number;
/**
* Default: `window.document`. Document where the focus trap will be active.
* This allows to use FocusTrap in an iFrame context.
diff --git a/node_modules/focus-trap/index.js b/node_modules/focus-trap/index.js
index de8e46a..bfc8b63 100644
--- a/node_modules/focus-trap/index.js
+++ b/node_modules/focus-trap/index.js
@@ -63,8 +63,8 @@ const isKeyBackward = function (e) {
return isTabEvent(e) && e.shiftKey;
};

-const delay = function (fn) {
- return setTimeout(fn, 0);
+const delay = function (fn, delayTime = 0) {
+ return setTimeout(() => setTimeout(fn, delayTime), 0);
};

// Array.find/findIndex() are not supported on IE; this replicates enough
@@ -267,7 +267,7 @@ const createFocusTrap = function (elements, userOptions) {
};

const getInitialFocusNode = function () {
- let node = getNodeForOption('initialFocus');
+ let node = getNodeForOption('initialFocus', state.containers);

// false explicitly indicates we want no initialFocus at all
if (node === false) {
@@ -817,7 +817,7 @@ const createFocusTrap = function (elements, userOptions) {
state.delayInitialFocusTimer = config.delayInitialFocus
? delay(function () {
tryFocus(getInitialFocusNode());
- })
+ }, typeof config.delayInitialFocus === 'number' ? config.delayInitialFocus : undefined)
: tryFocus(getInitialFocusNode());

doc.addEventListener('focusin', checkFocusIn, true);
@@ -989,7 +989,7 @@ const createFocusTrap = function (elements, userOptions) {
tryFocus(getReturnFocusNode(state.nodeFocusedBeforeActivation));
}
onPostDeactivate?.();
- });
+ }, typeof config.delayInitialFocus === 'number' ? config.delayInitialFocus : undefined);
};

if (returnFocus && checkCanReturnFocus) {
27 changes: 8 additions & 19 deletions src/components/FocusTrap/FocusTrapForScreen/index.web.tsx
Original file line number Diff line number Diff line change
@@ -1,23 +1,18 @@
import {useFocusEffect, useIsFocused, useRoute} from '@react-navigation/native';
import {useIsFocused, useRoute} from '@react-navigation/native';
import FocusTrap from 'focus-trap-react';
import React, {useCallback, useMemo} from 'react';
import {useOnyx} from 'react-native-onyx';
import React, {useMemo} from 'react';
import BOTTOM_TAB_SCREENS from '@components/FocusTrap/BOTTOM_TAB_SCREENS';
import getScreenWithAutofocus 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 ONYXKEYS from '@src/ONYXKEYS';
import CONST from '@src/CONST';
import type FocusTrapProps from './FocusTrapProps';

let activeRouteName = '';
function FocusTrapForScreen({children}: FocusTrapProps) {
const isFocused = useIsFocused();
const route = useRoute();
const {isSmallScreenWidth} = useWindowDimensions();
const [isAuthenticated] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => !!session?.authToken});
const screensWithAutofocus = useMemo(() => getScreenWithAutofocus(!!isAuthenticated), [isAuthenticated]);

const isActive = useMemo(() => {
// Focus trap can't be active on bottom tab screens because it would block access to the tab bar.
Expand All @@ -37,13 +32,6 @@ function FocusTrapForScreen({children}: FocusTrapProps) {
return true;
}, [isFocused, isSmallScreenWidth, route.name]);

useFocusEffect(
useCallback(() => {
// eslint-disable-next-line react-compiler/react-compiler
activeRouteName = route.name;
}, [route]),
);

return (
<FocusTrap
active={isActive}
Expand All @@ -52,15 +40,16 @@ function FocusTrapForScreen({children}: FocusTrapProps) {
trapStack: sharedTrapStack,
allowOutsideClick: true,
fallbackFocus: document.body,
// We don't want to ovverride autofocus on these screens.
initialFocus: () => {
if (screensWithAutofocus.includes(activeRouteName)) {
delayInitialFocus: CONST.ANIMATED_TRANSITION,
suneox marked this conversation as resolved.
Show resolved Hide resolved
initialFocus: (focusTrapContainers) => {
const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
if (isFocusedElementInsideContainer) {
return false;
}
return undefined;
},
setReturnFocus: (element) => {
if (screensWithAutofocus.includes(activeRouteName)) {
suneox marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not return focus to the initial screen if we already have a focused element, e.g. in ReportScreen after closing RHP the Composer should remain focused

Screen.Recording.2024-07-09.at.12.17.06.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt I’m still working on setReturnFocus. Do you mean your suggestion also checks if there’s a focused element to skip setReturnFocus?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a component using focus-trap-react unmounts, it calls deactivateTrap, which triggers focusTrap.deactivate. This subsequently calls getReturnFocusNode in focus-trap. Therefore, we can’t get the containers for the next screen because when Screen B unmounts (after navigating from Screen A to Screen B), it sets the return focus node from Screen B’s context.

As a result, we can’t retrieve the containers for Screen A when Screen B unmounts. Instead, we can check if the active element is the default (document body). If it is, we can return false to ensure the focus trap does not interfere with the focus transition to Screen A. Otherwise, we keep the current focused element.

I have updated the code, and it covers the issue you mentioned above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me but let's also check if document.activeElement is null. In that case we want to return the focus as well

if (document.activeElement && document.activeElement !== document.body) {
return false;
}
return element;
Expand Down
29 changes: 0 additions & 29 deletions src/components/FocusTrap/SCREENS_WITH_AUTOFOCUS.ts

This file was deleted.

16 changes: 14 additions & 2 deletions src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {useIsFocused} from '@react-navigation/native';
import {useFocusEffect, useIsFocused} from '@react-navigation/native';
import lodashIsEqual from 'lodash/isEqual';
import React, {memo, useCallback, useEffect, useMemo, useState} from 'react';
import React, {memo, useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
Expand Down Expand Up @@ -737,6 +737,17 @@ function MoneyRequestConfirmationList({
],
);

const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);
const confirmButtonRef = useRef<View>(null);
useFocusEffect(
useCallback(() => {
focusTimeoutRef.current = setTimeout(() => {
confirmButtonRef.current?.focus();
}, CONST.ANIMATED_TRANSITION);
return () => focusTimeoutRef.current && clearTimeout(focusTimeoutRef.current);
}, []),
);

const footerContent = useMemo(() => {
if (isReadOnly) {
return;
Expand Down Expand Up @@ -768,6 +779,7 @@ function MoneyRequestConfirmationList({
/>
) : (
<ButtonWithDropdownMenu
buttonRef={confirmButtonRef}
success
pressOnEnter
isDisabled={shouldDisableButton}
Expand Down
Loading