[$500] [MEDIUM] mWeb – Thread – Missing cursor when start a thread in the request preview of workspace chat.

[kbecciv]

On hold for #15992

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.3.94.0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in with any account.
3. Create an expense request in a workspace chat
4. Start a thread in the request preview

Expected Result:

Cursor is active

Actual Result:

Missing cursor when start a thread in the request preview of workspace chat.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6259243_1698835220741.Missing_cursor.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d26fe014dd5d92c1
• Upwork Job ID: 1719695171618611200
• Last Price Increase: 2023-11-15

[melvin-bot]

Triggered auto assignment to @conorpendergrast (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d26fe014dd5d92c1

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[bernhardoj]

It's intentional to not auto-focus a transaction thread. #26702 (comment)

[melvin-bot]

@conorpendergrast, @Santhosh-Sellavel Eep! 4 days overdue now. Issues have feelings too...

[conorpendergrast]

I agree with @bernhardoj; this looks like it's working as-designed.

@kbecciv if you're seeing this as part of a QA test, could you link to that and I'll reopen and check? Thanks!

[FitseTLT]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Compose not auto-focusing when a user presses reply in thread on a request preview

What is the root cause of that problem?

We are already calling the focus function on rely in thread here but the code is doing nothing.

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Lines 138 to 140 in 7b546b0

	ReportActionComposeFocusManager.focus();
	Report.navigateToAndOpenChildReport(lodashGet(reportAction, 'childReportID', '0'), reportAction, reportID);
	});

*** This line of code is not working for all cases whether it is a chat or request preview but the reason it is auto focusing for empty chat thread is the autofocus prop passed to Compose component (shouldAutoFocus) is true because isEmptyChat will be true, here

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

Lines 127 to 128 in 7b546b0

	const shouldAutoFocus = !modal.isVisible && (shouldFocusInputOnScreenFocus || (isEmptyChat && !ReportActionsUtils.isTransactionThread(parentAction))) && shouldShowComposeInput;

This is because focusComposerWithDelay focuses a textInput ref that is passed to it in here

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

Lines 443 to 445 in 7b546b0

	const focus = useCallback((shouldDelay = false) => {
	focusComposerWithDelay(textInputRef.current)(shouldDelay);
	}, []);

which is already set before the user pressed reply in thread and when the user pressed the reply in thread it is navigated to the transaction thread route and the main composer is rerendered leaving the ref passed to focusComposerWithDelay pointing to an already unmounted text input so when the focus function is called by runAfterInteraction in focusComposerWithDelay here

App/src/libs/focusComposerWithDelay.ts

Lines 22 to 25 in b8f91a4

	if (!shouldDelay) {
	textInput.focus();
	return;
	}

the compose will not focus because textInput is refering to the already unmounted text input element belonging to the screen that existed before pressing the reply in thread button 👍 .

What changes do you think we should make in order to solve the problem?

The solution is to use the appropriate ref pointing to the current Compose text input element in side focusComposerWithDelay. And to do that we can use ReportActionComposeFocusManager.composerRef that is properly updated to the current compose text input by the ref setter function in ComposerWithSuggestions here

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

Lines 182 to 192 in 7b546b0

	const setTextInputRef = useCallback(
	(el) => {
	ReportActionComposeFocusManager.composerRef.current = el;
	textInputRef.current = el;
	if (_.isFunction(animatedRef)) {
	animatedRef(el);
	}
	},
	[animatedRef],
	);

So we will change

App/src/libs/focusComposerWithDelay.ts

Lines 9 to 34 in b8f91a4

	function focusComposerWithDelay(textInput: TextInput | null): FocusComposerWithDelay {
	/**
	* Focus the text input
	* @param [shouldDelay] Impose delay before focusing the text input
	*/
	return (shouldDelay = false) => {
	// There could be other animations running while we trigger manual focus.
	// This prevents focus from making those animations janky.
	InteractionManager.runAfterInteractions(() => {
	if (!textInput || EmojiPickerAction.isEmojiPickerVisible()) {
	return;
	}
	if (!shouldDelay) {
	textInput.focus();
	return;
	}
	ComposerFocusManager.isReadyToFocus().then(() => {
	if (!textInput) {
	return;
	}
	textInput.focus();
	});
	});
	};
	}

to

function focusComposerWithDelay(textInputRef: TextInput | null | undefined): FocusComposerWithDelay {
    /**
     * Focus the text input
     * @param [shouldDelay] Impose delay before focusing the text input
     */
    return (shouldDelay = false) => {
        // There could be other animations running while we trigger manual focus.
        // This prevents focus from making those animations janky.
        InteractionManager.runAfterInteractions(() => {
            const textInput = textInputRef || ReportActionComposeFocusManager.composerRef.current;

            if (!textInput || EmojiPickerAction.isEmojiPickerVisible()) {
                return;
            }

            if (!shouldDelay) {
                textInput.focus();
                return;
            }
            ComposerFocusManager.isReadyToFocus().then(() => {
                if (!textInput) {
                    return;
                }
                textInput.focus();
            });
        });
    };
}

And change

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

Line 444 in 7b546b0

focusComposerWithDelay(textInputRef.current)(shouldDelay);

to

        focusComposerWithDelay()(shouldDelay);

as we no longer need to pass the textInput ref in this case. but we will need to pass for ReportActionItemMessageEdit as in here

App/src/pages/home/report/ReportActionItemMessageEdit.js

Lines 361 to 362 in 7b546b0

	const focus = focusComposerWithDelay(textInputRef.current);

Lastly we will change

App/src/components/Composer/index.js

Lines 473 to 478 in 7b546b0

	onFocus={(e) => {
	ReportActionComposeFocusManager.onComposerFocus(() => {
	if (!textInput.current) {
	return;
	}

to

 onFocus={(e) => {
                    if (isMainComposer) ReportActionComposeFocusManager.onComposerFocus(null);
                    else
                        ReportActionComposeFocusManager.onComposerFocus(() => {
                            if (!textInput.current) {
                                return;
                            }

                            textInput.current.focus();
                        });

This onComposerFocus is called to set the focus callback to fix this issue which is related to focus problem on web in whichReportActionItemMessageEdit composer losing focus to main composer after pressing LHN menu item. So we will pass isMainComposer prop (as true) to Composer for the main composer (ComposerWithSuggestions) and whenever the composer is focused if it is main composer we will reset the focus callback to null because we will use mainComposerFocusCallback for the main composer and only set the focus fallback for the ReportActionItemMessageEdit compsoer to preserve the fix for that issue. But the reason we add this condition is because with the existing code whenever the main compose is focused this function is always called changing the focus callback to other callback than the mainComposerFocusCallback (which focuses the correct compose text input ref). Hence, relpy in thread will still not auto focus if we have focused on the composer anytime after our latest reload of the page as the focus callback set here will execute which tries to focuses the wrong textInput ref which is the unmounted text input 👍 .
POC

2023-11-06_18-18-55.mp4

What alternative solutions did you explore? (Optional)

[FitseTLT]

@conorpendergrast @bernhardoj @Santhosh-Sellavel the case @bernhardoj commented is right when user presses on a request preview because they most probably want to set some fields and the auto focus will have a distrubing UX effect (b/c of the keyboard popup) but when they select reply in thread they definitely definitely 💯 want to reply with a message on the transaction thread and they expect the compose to be auto focused. And also as I mentioned it in my proposal, the focus code here is just doing nothing

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Lines 138 to 140 in 7b546b0

	ReportActionComposeFocusManager.focus();
	Report.navigateToAndOpenChildReport(lodashGet(reportAction, 'childReportID', '0'), reportAction, reportID);
	});

so the auto focusing is only working by this line that determines the auto focus prop passed to the Compose component.

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

Lines 127 to 128 in 7b546b0

	const shouldAutoFocus = !modal.isVisible && (shouldFocusInputOnScreenFocus || (isEmptyChat && !ReportActionsUtils.isTransactionThread(parentAction))) && shouldShowComposeInput;

So auto focusing is not working for mWeb and native when we press reply in thread if it is a tranaction thread or non empty chat.
I think in both cases the expected behaviour is to auto-focus. And I strongly believe this issue should be fixed 💯 👍 .

[conorpendergrast]

Hmm, yeah, this is a good point:

when they select reply in thread they definitely definitely 💯 want to reply with a message on the transaction thread and they expect the compose to be auto focused

Ok, thanks - I agree that this is a behaviour we should change when they choose Reply in thread

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@conorpendergrast, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@conorpendergrast, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[conorpendergrast]

@Santhosh-Sellavel Given my rapid about-turn, does this proposal work?

[Santhosh-Sellavel]

Sorry, I missed this one also unassigning this one due to less bandwidth!

[muttmuure]

@FitseTLT please raise your PR when you are ready

[FitseTLT]

Pr will be ready in 2 days

[muttmuure]

@FitseTLT how are we doing?

[FitseTLT]

Sorry @muttmuure will make it ready by EOD

[melvin-bot]

This issue has not been updated in over 15 days. @allroundexperts, @marcaaron, @FitseTLT, @muttmuure eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[muttmuure]

Old job expired. @FitseTLT I've sent you a new offer here: https://www.upwork.com/nx/wm/offer/102214608

[muttmuure]

$500 to @allroundexperts for C+

[FitseTLT]

@muttmuure Accepted the new offer

[FitseTLT]

bump @muttmuure

[FitseTLT]

bump for payment @muttmuure

[FitseTLT]

bump for payment @muttmuure

[FitseTLT]

bump @muttmuure for my payment pls

[muttmuure]

Sorry, handling now

[muttmuure]

Paid up

[JmillsExpensify]

$500 approved for @allroundexperts