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 8 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 @@ -345,13 +345,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.
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
2 changes: 1 addition & 1 deletion src/libs/ReportActionComposeFocusManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
if (isMainComposer) {
mainComposerFocusCallback = callback;
} else {
Expand Down
1 change: 1 addition & 0 deletions src/libs/focusComposerWithDelay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ function focusComposerWithDelay(textInput: TextInput | null): FocusComposerWithD
return (shouldDelay = false) => {
// There could be other animations running while we trigger manual focus.
// This prevents focus from making those animations janky.

if (!textInput || EmojiPickerAction.isEmojiPickerVisible()) {
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 @@ -199,7 +200,9 @@ const ContextMenuActions: ContextMenuAction[] = [
onPress: (closePopover, {reportAction, reportID}) => {
if (closePopover) {
hideContextMenu(false, () => {
ReportActionComposeFocusManager.focus();
InteractionManager.runAfterInteractions(() => {
ReportActionComposeFocusManager.focus();
});
Report.navigateToAndOpenChildReport(reportAction?.childReportID ?? '0', reportAction, reportID);
});
return;
Expand Down
Loading