[HOLD for payment 2024-08-29] [$250] Composer box- position cursor jumps to begining of message when copying as link

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: v9.0.11-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): negasofonias+7241@gmail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Login to staging new dot
2. Go to any chat with history and type anything in the composer
3. While still in edit mode long press any message from chat and select copy as link
4. observe that the position cursor in composer box jumps to the begining of the content in the composer box

Expected Result:

Position cursor should stay at the end of the message being typed

Actual Result:

Position cursor jumps to the front of the content being typed

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

Bug6550361_1721724352651.Screen_Recording_20240723_113515.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01dc75491f30967e9c
• Upwork Job ID: 1816958403568861454
• Last Price Increase: 2024-08-09
• Automatic offers:
• c3024 | Reviewer | 103552362

Issue Owner

Current Issue Owner: @c3024

[melvin-bot]

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2024-08-06 14:20:45 UTC.

Proposal

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

The cursor position of the composer go to the start after selecting option from the context menu.

What is the root cause of that problem?

When we select an option from the context menu such as copy link, we will refocus to the composer.

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

Line 459 in bf69d5d

hideContextMenu(true, ReportActionComposeFocusManager.focus);

ReportActionComposeFocusManager.focus will trigger ReportActionComposeFocusManager.onComposerFocus callback that is set up in ComposerWithSuggestions and Composer/index.tsx.

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

Lines 589 to 595 in bf69d5d

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

App/src/components/Composer/index.tsx

Lines 376 to 386 in bf69d5d

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

I don't have the context on why we have 2 onComposerFocus, but the 2nd onComposerFocus is only set-up when the composer is focused and also being used for both main and edit composer.

When the composer (live markdown input) is focused, it will trigger onFocus of the live markdown div element (which triggers handleFocus) then set the cursor position based on the selection value.
https://github.com/Expensify/react-native-live-markdown/blob/ff55579f673c049a6eb6cdf38ffc223315309b07/src/MarkdownTextInput.web.tsx#L502-L516

However, in our case, focusing after closing the context menu doesn't trigger the onFocus event so the cursor position is never set. I found that it's caused by the focus trap. When we try to focus the composer (from onModalHide callback), the modal isn't completely hidden and the focus trap isn't completely deactivated yet.

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

We need to wait for the modal to completely hidden before focusing the composer. If the modal is completely hidden, the children will be unmounted including the focus trap too, so it's guaranteed the focus trap is gone. To do that, we need to focus to the composer using focusComposerWithDelay and enable the "delay". The "delay" is not a timeout but rather a promise indicating whether it's ready to focus on the composer or not. (note: the old implementation rely on timeout)

App/src/libs/focusComposerWithDelay/index.ts

Lines 30 to 34 in bf69d5d

	Promise.all([ComposerFocusManager.isReadyToFocus(), isWindowReadyToFocus()]).then(() => {
	if (!textInput) {
	return;
	}
	textInput.focus();

ReportActionComposeFocusManager.onComposerFocus(() => {
    ...
    focusComposerWithDelay(textInput.current)(true);
});

isWindowReadyToFocus is android only, so we can ignore it. The ComposerFocusManager.isReadyToFocus() is resolved when the modal is completely hidden. It's resolved from the onDismiss callback here.

App/src/components/Modal/BaseModal.tsx

Line 232 in bf69d5d

onDismiss={handleDismissModal}

App/src/components/Modal/BaseModal.tsx

Lines 145 to 147 in bf69d5d

	const handleDismissModal = () => {
	ComposerFocusManager.setReadyToFocus(uniqueModalId);
	};

However, in strict mode, the onDismiss is triggered when the modal becomes visible, so isReadyToFocus is already resolved. To fix that, we need to add a onModalWillShow prop which calls saveFocusState to ModalContent too. Then, in ModalContent, call onModalWillShow on mount.

<ModalContent 
    onModalWillShow={saveFocusState}
    onDismiss={handleDismissModal}
>

ModalContent.tsx

React.useEffect(() => {
    onModalWillShow();
    return onDismiss;
}, []);

This will fix the issue when we previously focus on the composer, open the context menu, and select copy link. However, it's still broken if we open the chat without focusing on the composer, open the context menu, and select copy link. As I mentioned in my proposal, the cursor position is set based on the selection value. The main composer initial selection value is 0, so without ever focusing on the composer, selecting copy link from the context menu will still put the cursor at the start. To fix it, we need to set the initial selection to the value length.

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

Line 301 in bf69d5d

const [selection, setSelection] = useState<TextSelection>(() => ({start: 0, end: 0, positionX: 0, positionY: 0}));

const [selection, setSelection] = useState(() => ({start: value.length, end: value.length, positionX: 0, positionY: 0}));

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

@c3024 , can you attempt reproduction? I just tried on Safari, iOS, with no luck. If you're able to , can you review @bernhardoj 's proposal?

[mallenexpensify]

Added to #vip-vsb, this is a small bug and a bit of an edge case. @bernhardoj , do you know if this bug exists elsewhere? Or if this fix would help other instances?

[bernhardoj]

@mallenexpensify there are 2 fixes in my proposal. The first fix is specific to refocusing logic after selecting an item from the context menu (copy link, mark as unread, etc.). The second one fixes the composer initial selection value which is a broader fix, but I'm not aware of any issue that needs the same fix.

[c3024]

can you attempt reproduction?

I could reproduce this. I will update on the proposal.

[c3024]

@bernhardoj

When we try to focus the composer (from onModalHide callback), the modal isn't completely hidden and the focus trap isn't completely deactivated yet.

Is this an issue with react-native-modal? The onModalHide callback should be called after the modal is completely hidden, right?

[bernhardoj]

Yes, it's a problem we are facing in #23959 too and in my proposal there, I already fix it on the rn-web, but stuck on native.

[mallenexpensify]

@bernhardoj @c3024 , what do you propose is the best next step here? I added Help Wanted.

[bernhardoj]

I think we are waiting for @c3024 review

[c3024]

so without ever focusing on the composer, selecting copy link from the context menu will still put the cursor at the start.

How can we select copy link from a comment without ever focusing on the composer? When we open a report the composer gets automatically focused, right?

[bernhardoj]

When we open a report the composer gets automatically focused, right?

This only happens if the chat is empty (on small screen).

[melvin-bot]

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

[c3024]

On Desktop Web, even for smaller screens, I see composer gets focused for all cases including the case when there is a comment in the report. Here, focusing composer with delay fix is enough.

cursor.mp4

But for mWeb Safari, I see this

This only happens if the chat is empty (on small screen).

but this is not fixed even after making both the changes.

cursormWeb.mp4

[bernhardoj]

On Desktop Web, even for smaller screens, I see composer gets focused for all cases

By small screen, I mean the mobile device.

But for mWeb Safari, I see this
but this is not fixed even after making both the changes.

I just tested and you're right. When I turned off the strict mode, then it works. From my finding, because of the strict mode, when the modal becomes visible, the onDismiss is triggered because the useEffect is mounted and remounted.

App/src/components/Modal/BaseModal.tsx

Line 258 in 41e27c9

<ModalContent onDismiss={handleDismissModal}>

App/src/components/Modal/ModalContent.tsx

Lines 16 to 19 in 41e27c9

	function ModalContent({children, onDismiss = () => {}}: ModalContentProps) {
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	React.useEffect(() => () => onDismiss?.(), []);
	return children;

App/src/components/Modal/BaseModal.tsx

Lines 145 to 148 in 41e27c9

	const handleDismissModal = () => {
	ComposerFocusManager.setReadyToFocus(uniqueModalId);
	};

Because of that, ComposerFocusManager.isReadyToFocus() is already resolved. I have updated my proposal to fix that.

[melvin-bot]

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[MariaHCD]

@bernhardoj 's proposal looks good to me 👍🏼

[bernhardoj]

There is another case I found in which the issue is still happening and the focus doesn't wait until the modal is closed (even with setTimeout). I'll continue the investigation and open the PR tomorrow.

[bernhardoj]

PR is ready

cc: @c3024

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.23-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix composer selection is at start after when copying link to a message #47596

If no regressions arise, payment will be issued on 2024-08-29. 🎊

For reference, here are some details about the assignees on this issue:

• @bernhardoj requires payment through NewDot Manual Requests
• @c3024 requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR:
• [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@c3024] Determine if we should create a regression test for this bug.
• [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mallenexpensify]

Contributor: @bernhardoj due $250 via NewDot
Contributor+: @c3024 paid $250 via Upwork

@c3024 plz complete the BZ checklist. thx

[bernhardoj]

Requested in ND.

[c3024]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR: Fix jumping composer when entering emojis or markdown text #40128
• [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: e572f6d#r146059726
• [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No discussion was started because this could not have been identified earlier.
• [@c3024] Determine if we should create a regression test for this bug. Yes
• [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test proposal

1. Open a chat with existing messages.
2. Type any text in the composer.
3. (This step is for mWeb only) Go back and reopen the chat again. The composer shouldn't be auto-focused.
4. Long-press/right-click on any existing message.
5. Select "Copy link".
6. Verify the composer is focused with the cursor at the end of the text in the composer.

[JmillsExpensify]

$250 approved for @bernhardoj

[mallenexpensify]

Thanks @c3024 , test case created

• https://github.com/Expensify/Expensify/issues/425308