[Awaiting checklist] [$250] Composer - Cursor Position Resets to Start of Message on Page Refresh

[lanitochka17]

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: 9.0.1-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: https://expensify.testrail.io/index.php?/tests/view/4661747&group_by=cases:section_id&group_order=asc&group_id=296775
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Open a chat
2. Write text in composer box
3. Refresh the page

Expected Result:

Cursor Position Should Not Resets to Start of Message on Page Refresh

Actual Result:

Cursor Position Resets to Start of Message on Page Refresh

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

Bug6522559_1719208729219.2024-06-24_10-56-46.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0121d7a8303e8c6df6
• Upwork Job ID: 1805917059078868913
• Last Price Increase: 2024-07-03
• Automatic offers:
• hoangzinh | Reviewer | 103037350

Issue Owner

Current Issue Owner: @hoangzinh

[melvin-bot]

Triggered auto assignment to @twisterdotcom (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.

[lanitochka17]

@twisterdotcom FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[BartoszGrajdek]

This issue is possibly related to live markdown, please add it to the project 🙏🏻 cc @thienlnam

[BartoszGrajdek]

@twisterdotcom I believe this issue could be resolved by external contributors. can we open it up for proposals?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0121d7a8303e8c6df6

[melvin-bot]

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

[grogou]

Proposal

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

Cursor position reverts to the beginning of the message when the page is refreshed.

What is the root cause of that problem?

The root cause is that the input field’s state, including the selection state for cursor position, resets on page refresh. Without explicit management using setSelection, the cursor defaults to the beginning.

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

We need to check if value exist at first render and move cursor to the end

if (value && value.length > 0) {
    setSelection({
         start: value.length,
         end: value.length
    });
}

Here we can add this

App/src/components/Composer/index.tsx

Lines 235 to 250 in 4ee43a3

	useEffect(() => {
	if (!textInput.current) {
	return;
	}
	const debouncedSetPrevScroll = lodashDebounce(() => {
	if (!textInput.current) {
	return;
	}
	setPrevScroll(textInput.current.scrollTop);
	}, 100);
	textInput.current.addEventListener('scroll', debouncedSetPrevScroll);
	return () => {
	textInput.current?.removeEventListener('scroll', debouncedSetPrevScroll);
	};
	}, []);

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01631ccf59d885b1c2

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[iCodePrograms]

Proposal

Please re-state the problem that we are trying to solve in this issue.
The position of the caret(vertical blinking line which determines position of text) needs to move to the end of the text when the page refreshes

What is the root cause of that problem?
The position of the caret is determined by the browser which happens before, after which the text is added in the input box, this results in cursor position to be in the beginning rather than the end.

What changes do you think we should make in order to solve the problem?
In order to solve this problem, we would have to set the position of the cursor dynamically to the end of the text whichever is added later(if it exists) for which we can use the Selection API and trigger this in one of the useEffects already in place which handles page load functionalities.

Selection.collapseToEnd()

this will ensure that the caret is always placed in the end of the text. There would no need to add another useEffect or store the position of the caret everytime when the data changes in the page.

[hoangzinh]

@grogou @iCodePrograms Thanks for your proposal. I don't think your RCA is correct. We already trigger scroll and set selection on mount here

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

Lines 708 to 712 in e78c38e

	useEffect(() => {
	// Scrolls the composer to the bottom and sets the selection to the end, so that longer drafts are easier to edit
	updateMultilineInputRange(textInputRef.current, !!shouldAutoFocus);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

[bernhardoj]

Proposal

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

Cursor/selection position resets to the start of the message when refreshing.

What is the root cause of that problem?

When the composer mounts, we already call a function to set the selection based on the text length.

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

Lines 708 to 712 in 4622e19

	useEffect(() => {
	// Scrolls the composer to the bottom and sets the selection to the end, so that longer drafts are easier to edit
	updateMultilineInputRange(textInputRef.current, !!shouldAutoFocus);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

App/src/libs/updateMultilineInputRange/index.ts

Lines 17 to 24 in 4622e19

	if ('value' in input && typeof input.value === 'string' && input.setSelectionRange) {
	const length = input.value.length;
	if (shouldAutoFocus) {
	(input as HTMLInputElement).setSelectionRange(length, length);
	}
	// eslint-disable-next-line no-param-reassign
	input.scrollTop = input.scrollHeight;
	}

However, setSelectionRange is missing. The function doesn't exist in live markdown input.

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

Add setSelectionRange to the live markdown. We can add it when setting up the ref here.

r.setSelectionRange = (start, end) => {
  const newSelection = {start, end};
  contentSelection.current = newSelection;
  updateRefSelectionVariables(newSelection);
  CursorUtils.setCursorPosition(currentRef, newSelection.start, newSelection.end);
}

The code that we will add is actually the same as this one, but I just simplify it for testing purpose. I think we can create a new function for that to make it reusable.

But this isn't enough because there is a problem with the app composer code. There is a useEffect that runs every time the composer selection prop is updated. It's basically to sync the selection prop and the selection state.

App/src/components/Composer/index.tsx

Lines 117 to 124 in 4622e19

	useEffect(() => {
	setSelection((prevSelection) => {
	if (!!prevSelection && selectionProp.start === prevSelection.start && selectionProp.end === prevSelection.end) {
	return;
	}
	return selectionProp;
	});
	}, [selectionProp]);

The selectionProp is initially both 0 (start and end), but the prevSelection already contains the updated selection from setSelectionRange even though we initialize the selection state with the selectionProp, so the selection will be updated to the selection prop which is 0.

App/src/components/Composer/index.tsx

Lines 90 to 101 in 4622e19

	const [selection, setSelection] = useState<
	| {
	start: number;
	end?: number;
	positionX?: number;
	positionY?: number;
	}
	| undefined
	>({
	start: selectionProp.start,
	end: selectionProp.end,
	});

Based on the react doc, using a set state function updater, the updater will be queued. Instead of doing that, we can just do it like this:

useEffect(() => {
    if (!!selection && selectionProp.start === selection.start && selectionProp.end === selection.end) {
        return;
    }
    setSelection(selectionProp);
}, [selectionProp]);

[melvin-bot]

@twisterdotcom, @hoangzinh Whoops! This issue is 2 days overdue. Let's get this updated quick!

[twisterdotcom]

One more proposal from @bernhardoj here @hoangzinh.

[hoangzinh]

The selectionProp is initially both 0 (start and end), but the prevSelection already contains the updated selection from setSelectionRange even though we initialize the selection state with the selectionProp

@bernhardoj can you help me understand this? When I put console.log here, it's true that prevSelection already contains the updated selection but why selectionProp is initially both 0? Doesn't selectionProp must be equal with prevSelection? Thank you. Moreover, if I only apply your solution related to useEffect, it works without define setSelectionRange to the live markdown input

[melvin-bot]

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

[bernhardoj]

but why selectionProp is initially both 0?

Because we set the initial state of the selection of the parent component (ComposerWithSuggestions) to 0.

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

Line 302 in 2784712

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

It's coming from #32168 to fix a safari-only issue, so we need to handle the selection manually, but the safari issue is already fixed and the workaround is removed in #40295.

Moreover, if I only apply your solution related to useEffect, it works without define setSelectionRange to the live markdown input

Oh, you're right. I found that the onSelectionChange of live-markdown is triggered on focus which updates the selection state. Not sure if that's expected or not, but if it's, then I think we only need to fix the useEffect since the safari workaround isn't needed anymore too.

[hoangzinh]

then I think we only need to fix the useEffect since the safari workaround isn't needed anymore too.

HI @bernhardoj, do you mind updating your proposal with this approach to clarify what you mean? (you can put it as an alternative solution). Thank you.

[bernhardoj]

Oh sorry, the useEffect that I meant is this one which is already in my proposal.

[bernhardoj]

PR is ready

cc: @hoangzinh

[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.7-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix cursor position resets to start of message on page refresh #45052

If no regressions arise, payment will be issued on 2024-07-24. 🎊

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

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

[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:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] 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:
• [@hoangzinh] 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:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] 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.
• [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mallenexpensify]

Have another 'cursor position isn't in right place issue`. If it's related to this or, or if this might fix it, plz comment on the issue

• [$250] Room - Pasting long text in description not scrolled to bottom & cursor shown at middle. #45885

[bernhardoj]

@mallenexpensify I don't think it's related because the components being used are different (the PR for this issue is already merged too)

[twisterdotcom]

Payment Summary:

• @hoangzinh paid $250 here (Reviewer)
• @bernhardoj paid $250 via NewDot Manual Request

@hoangzinh please complete the checklist.

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[hoangzinh]

Regression Test Proposal

1. Sign in to NewDot
2. Open any chat
3. Type a message in the composer
4. Navigate to another chat
5. Navigate back to the chat in step 2
6. Verify that the composer is focused and the cursor is at the end of the draft message

Do we agree 👍 or 👎

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: Migrate Composer. web #23359
• 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: https://github.com/Expensify/App/pull/23359/files#r1692999112
• 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: N/A
• Determine if we should create a regression test for this bug: ✅ Yes we should

[twisterdotcom]

please propose the regression test steps to ensure the same bug will not reach production again.

[hoangzinh]

I posted the Regression Test Proposal here #44259 (comment)

[mallenexpensify]

Test case created

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

[perunt]

Hey folks!

I came from this PR as I was mentioned here.

I tested this before the fix on dev(and also on staging, but today, it potentially already has those changes) and noticed that the focus was at the end of the text input.
After the fix, I tested it again and found that after a refresh, the focus always moves to the end. I'm just curious if an option to preserve the cursor position has been discussed rather than moving it to the end @lanitochka17? If not, what do you think about implementing this feature? @hoangzinh

[hoangzinh]

If not, what do you think about implementing this feature?

Hi @perunt, It's an interesting idea for me. But I'm unsure if we will prioritize it right now. I think you can post it in Slack as a feature request to collect opinions.