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: Copying a message with a reply doesn't refocus on the composer #44399

Merged
merged 7 commits into from
Jul 9, 2024
7 changes: 7 additions & 0 deletions src/components/FocusTrap/FocusTrapForModal/index.web.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import FocusTrap from 'focus-trap-react';
import React from 'react';
import sharedTrapStack from '@components/FocusTrap/sharedTrapStack';
import ReportActionComposeFocusManager from '@libs/ReportActionComposeFocusManager';
import type FocusTrapForModalProps from './FocusTrapForModalProps';

function FocusTrapForModal({children, active}: FocusTrapForModalProps) {
Expand All @@ -12,6 +13,12 @@ function FocusTrapForModal({children, active}: FocusTrapForModalProps) {
clickOutsideDeactivates: true,
initialFocus: false,
fallbackFocus: document.body,
setReturnFocus: (element) => {
Copy link
Contributor Author

@gijoe0295 gijoe0295 Jun 25, 2024

Choose a reason for hiding this comment

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

Problem

  1. Right press the message sender/report preview/money request preview/task checkbox or any Pressable within a ReportActionItem
  2. Press any context menu item
  3. The composer gains focus briefly then loses it

RCA

The context menu activates a focus trap that would return focus to to the element that was previously focused when it unmounts, which can be any of the above Pressables. So when the context menu closes, the composer would gain focus first, then the focus trap moves the focus to the Pressable right after.

This doesn't happen when you right click the ReportActionItem background thanks to the withoutFocusOnSecondaryInteraction prop.

withoutFocusOnSecondaryInteraction

Solution

If the composer is already focused, we won't allow the focus trap to return focus.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

if (ReportActionComposeFocusManager.isFocused()) {
return false;
}
return element;
},
}}
>
{children}
Expand Down
17 changes: 17 additions & 0 deletions src/libs/Navigation/isReportOpenInRHP.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import type {NavigationState} from '@react-navigation/native';
import NAVIGATORS from '@src/NAVIGATORS';
import SCREENS from '@src/SCREENS';

const isReportOpenInRHP = (state: NavigationState | undefined): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the useIsReportOpenInRHP hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we couldn't. I'm not sure why the activeRoute returned by that hook is always RightModalNavigator when used in ReportActionComposeFocusManager but should be Search_Report_RHP instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WojtekBoman could you check on this please? I'd rather not have two ways of checking if a report is open on RHP

const lastRoute = state?.routes?.at(-1);
if (!lastRoute) {
return false;
}
const params = lastRoute.params;
if (params && 'screen' in params && typeof params.screen === 'string' && params.screen === SCREENS.RIGHT_MODAL.SEARCH_REPORT) {
return true;
}
return !!(lastRoute.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR && lastRoute.state?.routes?.some((route) => route?.name === SCREENS.RIGHT_MODAL.SEARCH_REPORT));
};
Copy link
Contributor Author

@gijoe0295 gijoe0295 Jun 25, 2024

Choose a reason for hiding this comment

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

The params check mentioned by the other proposal only solves the issue when we navigate to report RHP from the search expenses page but not when we navigate to this page by direct URL. In the latter case, we should use state.routes check.

Reference:

const isReportInRhpOpened = lastRoute?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR && lastRoute?.state?.routes?.some((route) => route?.name === SCREENS.RIGHT_MODAL.SEARCH_REPORT);


export default isReportOpenInRHP;
3 changes: 2 additions & 1 deletion src/libs/Navigation/linkTo/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type {NavigationContainerRef, NavigationState, PartialState} from '@react
import {findFocusedRoute} from '@react-navigation/native';
import {omitBy} from 'lodash';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import isReportOpenInRHP from '@libs/Navigation/isReportOpenInRHP';
import extractPolicyIDsFromState from '@libs/Navigation/linkingConfig/extractPolicyIDsFromState';
import isCentralPaneName from '@libs/NavigationUtils';
import shallowCompare from '@libs/ObjectUtils';
Expand Down Expand Up @@ -68,7 +69,7 @@ export default function linkTo(navigation: NavigationContainerRef<RootStackParam

const action: StackNavigationAction = getActionFromState(stateFromPath, linkingConfig.config);

const isReportInRhpOpened = lastRoute?.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR && lastRoute?.state?.routes?.some((route) => route?.name === SCREENS.RIGHT_MODAL.SEARCH_REPORT);
const isReportInRhpOpened = isReportOpenInRHP(rootState);

// If action type is different than NAVIGATE we can't change it to the PUSH safely
if (action?.type === CONST.NAVIGATION.ACTION_TYPE.NAVIGATE) {
Expand Down
11 changes: 7 additions & 4 deletions src/libs/ReportActionComposeFocusManager.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import React from 'react';
import type {MutableRefObject} from 'react';
import type {TextInput} from 'react-native';
import ROUTES from '@src/ROUTES';
import Navigation from './Navigation/Navigation';
import SCREENS from '@src/SCREENS';
import getTopmostRouteName from './Navigation/getTopmostRouteName';
import isReportOpenInRHP from './Navigation/isReportOpenInRHP';
import navigationRef from './Navigation/navigationRef';

type FocusCallback = (shouldFocusForNonBlurInputOnTapOutside?: boolean) => void;

Expand Down Expand Up @@ -31,8 +33,9 @@ function onComposerFocus(callback: FocusCallback | null, isMainComposer = false)
* Request focus on the ReportActionComposer
*/
function focus(shouldFocusForNonBlurInputOnTapOutside?: boolean) {
/** Do not trigger the refocusing when the active route is not the report route, */
if (!Navigation.isActiveRoute(ROUTES.REPORT_WITH_ID.getRoute(Navigation.getTopmostReportId() ?? '-1'))) {
/** Do not trigger the refocusing when the active route is not the report screen */
const navigationState = navigationRef.getState();
if (!navigationState || (!isReportOpenInRHP(navigationState) && getTopmostRouteName(navigationState) !== SCREENS.REPORT)) {
return;
}

Expand Down
5 changes: 5 additions & 0 deletions tests/perf-test/SidebarLinks.perf-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ jest.mock('../../src/libs/Navigation/Navigation', () => ({
isNavigationReady: jest.fn(() => Promise.resolve()),
isDisplayedInModal: jest.fn(() => false),
}));
jest.mock('../../src/libs/Navigation/navigationRef', () => ({
getState: () => ({
routes: [],
}),
}));
jest.mock('@components/Icon/Expensicons');

jest.mock('@react-navigation/native');
Expand Down
Loading