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.

Sign up for GitHub

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 - mWeb auto focus on reply in thread #37929

Merged
merged 21 commits into from
Apr 5, 2024
Merged

Fix - mWeb auto focus on reply in thread #37929

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5e7121a
fix auto focus on reply in thread
FitseTLT Feb 23, 2024
1450307
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 6, 2024
b7f306e
refactor on the implementation
FitseTLT Mar 6, 2024
d7abc9d
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 7, 2024
f69df2a
pass text input ref
FitseTLT Mar 7, 2024
69e6022
revert ref variable change
FitseTLT Mar 7, 2024
0d37448
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 7, 2024
c51602f
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 8, 2024
9e2c1d1
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 20, 2024
78d815b
minor fix
FitseTLT Mar 20, 2024
1b9163c
minor fix
FitseTLT Mar 20, 2024
aa4fe30
minor fix
FitseTLT Mar 20, 2024
4c34830
minor fix
FitseTLT Mar 20, 2024
31db23e
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 21, 2024
297cd03
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Mar 26, 2024
2bf777e
fix auto focus for natives
FitseTLT Mar 26, 2024
822cf62
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Apr 1, 2024
c27d74e
Updated comment
FitseTLT Apr 3, 2024
4a40236
added comment
FitseTLT Apr 3, 2024
335b5fa
Merge branch 'main' into fix-auto-focus-on-reply-in-thread
FitseTLT Apr 5, 2024
6fd4517
changed variable names
FitseTLT Apr 5, 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
21 changes: 14 additions & 7 deletions src/components/Composer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,13 +385,20 @@ function Composer(
disabled={isDisabled}
onKeyPress={handleKeyPress}
onFocus={(e) => {
ReportActionComposeFocusManager.onComposerFocus(() => {
if (!textInput.current) {
return;
}

textInput.current.focus();
});
if (isReportActionCompose) {
ReportActionComposeFocusManager.onComposerFocus(null);
} else {
// While a user was editing a comment and if they open on LHN menu we want the focus to return
// to the ReportActionItemMessageEdit compose after they click on the menu (for e.g. mark as read)
// so we assign the focus callback here.
FitseTLT marked this conversation as resolved.
Show resolved Hide resolved
ReportActionComposeFocusManager.onComposerFocus(() => {
if (!textInput.current) {
return;
}

textInput.current.focus();
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (isReportActionCompose) {
ReportActionComposeFocusManager.onComposerFocus(null);
} else {
// While a user was editing a comment and if they open on LHN menu we want the focus to return
// to the ReportActionItemMessageEdit compose after they click on the menu (for e.g. mark as read)
// so we assign the focus callback here.
ReportActionComposeFocusManager.onComposerFocus(() => {
if (!textInput.current) {
return;
}
textInput.current.focus();
});
}
ReportActionComposeFocusManager.onComposerFocus(() => {
/*
While a user was editing a comment and if they open on LHN menu we want the focus to return
to the ReportActionItemMessageEdit compose after they click on the menu (for e.g. mark as read)
so we assign the focus callback here.
*/
if (!textInput.current || isReportActionCompose) {
return;
}
textInput.current.focus();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. What we are trying to avoid for main composer is overriding of the focus callback; onComposerFocus is callback assigner so we should avoid overriding of mainComposerFocusCallback by focusCallback so when isReportActionCompose is true we reset it to null.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't get the reasoning behind this. Why can't we just return early in the callback if isReportActionCompose is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we want to achieve is when the focus is called from reply in thread we want to focus the compose in the newly created thread and we can do that via mainComposerFocusCallback set here

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx

Lines 611 to 617 in 822cf62

ReportActionComposeFocusManager.onComposerFocus((shouldFocusForNative = false) => {
if ((!willBlurTextInputOnTapOutside && !shouldFocusForNative) || !isFocused) {
return;
}
focus(false);
}, true);

But mainComposerFocusCallback are only called when focusCallback is not set.

App/src/libs/ReportActionComposeFocusManager.ts

Lines 38 to 43 in 822cf62

if (typeof focusCallback !== 'function') {
if (typeof mainComposerFocusCallback !== 'function') {
return;
}
mainComposerFocusCallback(shouldFocusForNative);

So here what we want is to reset focusCallback to null so that our mainComposerFocusCallback will be called. But early returning, as U suggest, means we leave (without reseting) what was already set on focusCallback therefore our mainComposerFocusCallback will not be called.


props.onFocus?.(e);
}}
Expand Down
8 changes: 4 additions & 4 deletions src/libs/ReportActionComposeFocusManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type {TextInput} from 'react-native';
import ROUTES from '@src/ROUTES';
import Navigation from './Navigation/Navigation';

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

const composerRef = React.createRef<TextInput>();
const editComposerRef = React.createRef<TextInput>();
Expand All @@ -18,7 +18,7 @@ let mainComposerFocusCallback: FocusCallback | null = null;
*
* @param callback callback to register
*/
function onComposerFocus(callback: FocusCallback, isMainComposer = false) {
function onComposerFocus(callback: FocusCallback | null, isMainComposer = false) {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
if (isMainComposer) {
mainComposerFocusCallback = callback;
} else {
Expand All @@ -29,7 +29,7 @@ function onComposerFocus(callback: FocusCallback, isMainComposer = false) {
/**
* Request focus on the ReportActionComposer
*/
function focus() {
function focus(shouldFocusForNative?: boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a shouldFocusForNative param seems to encourages code that is not cross platform.

Can we rename this to indicate the intended behavior regardless of what platform we are on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we rename this to indicate the intended behavior regardless of what platform we are on?

That isn't clear @marcaaron. Because we are early returning here

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx

Lines 612 to 613 in a4d01a8

if (!willBlurTextInputOnTapOutside || !isFocused) {
return;

willBlurTextInputOnTapOutside is false only for natives so we are normally preventing focus for natives but in our case we have to manually focus it for natives too that's why I came up with the variable. What other name could we give?

Copy link
Contributor

@allroundexperts allroundexperts Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Naming it shouldReturnEarly should be fine @FitseTLT. We do not support code that this written just for native platforms mixed together with the rest of the code. If we need to do something specific to the native platform, we usually create a separate file .native.ts extension to keep proper separation of concerns.

/** Do not trigger the refocusing when the active route is not the report route, */
if (!Navigation.isActiveRoute(ROUTES.REPORT_WITH_ID.getRoute(Navigation.getTopmostReportId() ?? ''))) {
return;
Expand All @@ -40,7 +40,7 @@ function focus() {
return;
}

mainComposerFocusCallback();
mainComposerFocusCallback(shouldFocusForNative);
return;
}

Expand Down
5 changes: 4 additions & 1 deletion src/pages/home/report/ContextMenu/ContextMenuActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import type {MutableRefObject} from 'react';
import React from 'react';
import {InteractionManager} from 'react-native';
// eslint-disable-next-line no-restricted-imports
import type {GestureResponderEvent, Text, View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
Expand Down Expand Up @@ -200,7 +201,9 @@ const ContextMenuActions: ContextMenuAction[] = [
onPress: (closePopover, {reportAction, reportID}) => {
if (closePopover) {
hideContextMenu(false, () => {
ReportActionComposeFocusManager.focus();
InteractionManager.runAfterInteractions(() => {
ReportActionComposeFocusManager.focus(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here to explain why we are passing true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added but can u check the comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid creating platform specific language.

Can you answer this question without referencing "native":

What behavior are we giving the callback by passing true?

});
Report.navigateToAndOpenChildReport(reportAction?.childReportID ?? '0', reportAction, reportID);
});
return;
Expand Down
4 changes: 2 additions & 2 deletions ...pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,8 +608,8 @@ function ComposerWithSuggestions(

const setUpComposeFocusManager = useCallback(() => {
// This callback is used in the contextMenuActions to manage giving focus back to the compose input.
ReportActionComposeFocusManager.onComposerFocus(() => {
if (!willBlurTextInputOnTapOutside || !isFocused) {
ReportActionComposeFocusManager.onComposerFocus((shouldFocusForNative = false) => {
if ((!willBlurTextInputOnTapOutside && !shouldFocusForNative) || !isFocused) {
return;
}

Expand Down
Loading