[$1000] Chat - Cursor does not remain the same place when Edit message #25413

lanitochka17 · 2023-08-17T15:16:40Z

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

Action Performed:

Go to staging.new.expensify.com.
Send a message that includes several emojis
Edit the message with the several emojis
Remove some emoji and click on emoji picker inside the edit box
Add new emoji

Expected Result:

New emoji should be added in field where the cursor was pointed

Actual Result:

Cursor does not remain the same place when Edit message. New emoji is added at the end of the message instead on the place where the cursor was last pointed

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.55-0

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6168233_Recording__749.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01b02b907f187853cd
Upwork Job ID: 1692313936551825408
Last Price Increase: 2023-08-17
Automatic offers:

Nikhil-Vats | Contributor | 26187092
situchan | Contributor | 26187097

OSBotify · 2023-08-17T15:17:34Z

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

melvin-bot · 2023-08-17T15:17:37Z

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

getusha · 2023-08-17T17:13:53Z

Proposal

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

Cursor is not remaining on it's place when inserting emoji

What is the root cause of that problem?

there is a prop to enable the calculation of the caret position, and it utilizes renderElementForCaretPosition text to calculate the caret position, when we use useRef the default value is null, when we reach here:

App/src/components/Composer/index.js

Lines 207 to 208 in b7fdbcb

    
           positionX: textRef.current.offsetLeft - CONST.SPACE_CHARACTER_WIDTH, 
        
           positionY: textRef.current.offsetTop,

we are trying to access a value from null which will prevent onSelectionChange from being executed below

App/src/components/Composer/index.js

Line 450 in 9474292

{shouldCalculateCaretPosition && renderElementForCaretPosition}

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

We should add shouldCalculateCaretPosition prop to Composer inside ReportActionItemMessageEditcomponent

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

Line 346 in 9474292

<Composer

Alternatively we can add a check if textRef is defined and calculate the position accordingly to prevent null value.
Will be something like this:

        const selectionValue = {
            ...
            positionX: textRef.current ? textRef.current.offsetLeft - CONST.SPACE_CHARACTER_WIDTH : undefined,
            positionY: textRef.current ? textRef.current.offsetTop : undefined,
        };

What alternative solutions did you explore? (Optional)

situchan · 2023-08-17T17:20:13Z

there is a prop to enable the calculation of the caret position, but in recent migration to function component the default value of the prop is set to false

@getusha shouldCalculateCaretPosition default value was already false before function migration. If your root cause is correct, do you know why it didn't happen before?

getusha · 2023-08-17T17:28:27Z

@situchan updated my proposal explaining the root cause further
#25413 (comment)

Nikhil-Vats · 2023-08-17T17:42:17Z

Proposal

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

Chat - Cursor does not remain the same place when Edit message

What is the root cause of that problem?

The logic of this function was changed -

App/src/components/Composer/index.js

Lines 195 to 216 in 5a1b35c

    
           const addCursorPositionToSelectionChange = (event) => { 
        
               if (shouldCalculateCaretPosition) { 
        
                   // we do flushSync to make sure that the valueBeforeCaret is updated before we calculate the caret position to receive a proper position otherwise we will calculate position for the previous state 
        
                   flushSync(() => { 
        
                       setValueBeforeCaret(event.target.value.slice(0, event.nativeEvent.selection.start)); 
        
                       setCaretContent(getNextChars(value, event.nativeEvent.selection.start)); 
        
                   }); 
        
               } 
        
               const selectionValue = { 
        
                   start: event.nativeEvent.selection.start, 
        
                   end: event.nativeEvent.selection.end, 
        
                   positionX: textRef.current.offsetLeft - CONST.SPACE_CHARACTER_WIDTH, 
        
                   positionY: textRef.current.offsetTop, 
        
               }; 
        
               onSelectionChange({ 
        
                   nativeEvent: { 
        
                       selection: selectionValue, 
        
                   }, 
        
               }); 
        
               setSelection(selectionValue); 
        
           };

Earlier, if shouldCalculateCaretPosition was false then we just used the event parameter without changing it to a custom event and called onSelectionChange function from prop passing event.

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

We should use the same logic, change the function to -

const addCursorPositionToSelectionChange = (event) => {
    if (shouldCalculateCaretPosition) {
        // we do flushSync to make sure that the valueBeforeCaret is updated before we calculate the caret position to receive a proper position otherwise we will calculate position for the previous state
        flushSync(() => {
            setValueBeforeCaret(event.target.value.slice(0, event.nativeEvent.selection.start));
            setCaretContent(getNextChars(value, event.nativeEvent.selection.start));
        });

        const selectionValue = {
            start: event.nativeEvent.selection.start,
            end: event.nativeEvent.selection.end,
            positionX: textRef.current.offsetLeft - CONST.SPACE_CHARACTER_WIDTH,
            positionY: textRef.current.offsetTop,
        };
        onSelectionChange({
            nativeEvent: {
                selection: selectionValue,
            },
        });
        setSelection(selectionValue);
        return;
    }

    onSelectionChange(event);
    setSelection(event.nativeEvent.selection);
};

What alternative solutions did you explore? (Optional)

Alternatively, we can pass shouldCalculateCaretPosition in ReportActionMessageEdit.

Nikhil-Vats · 2023-08-17T17:43:39Z

Hey @situchan, the logic of that function was changed which changed the behaviour of composer when shouldCalculateCaretPosition was set to false which resulted in the regression. shouldCalculateCaretPosition was always false in case of message edit.

You can check here that when shouldCalculateCaretPosition was false the logic of the function is completely different and we should change it back to that as it works for both cases.

getusha · 2023-08-17T17:56:02Z

@Nikhil-Vats when textRef is null it means it's not rendered and shouldCalculateCaretPosition is false which makes your proposal same as mine with slight style change.

Nikhil-Vats · 2023-08-17T18:08:25Z

I disagree @getusha, I actually don't agree with your RCA completely, part of it is correct (although difficult to understand) and the other part seems wrong -

we are trying to access a value from null which will prevent onSelectionChange from being executed below in addition, the calculation will be null - 4 which will become -4.

if ref is null, shouldn't we get an error something like - Cannot read properties of null.

Anyway, whatever @situchan decides is fine with me.

getusha · 2023-08-17T18:15:04Z

if ref is null, shouldn't we get an error something like - Cannot read properties of null.

You can confirm the value being null by logging it to the console.

what you proposed as a solution is equivalent to mine which is:

        const selectionValue = {
            ...
            positionX: textRef.current ? textRef.current.offsetLeft - CONST.SPACE_CHARACTER_WIDTH : undefined,
            positionY: textRef.current ? textRef.current.offsetTop : undefined,
        };

instead of checking textRef we could also check the prop which is equivalent thing.

and you haven't explained why it's not executing onSelectionChange in your RCA

getusha · 2023-08-17T18:18:38Z

Example: added this check

and we you can confirm the execution is stopped right after assigning selectionValue

perunt · 2023-08-17T18:20:22Z

Hey guys, I think I know where the problem is. So here is my proposal-fix

The issue is reproducible because the onSelectionChange has never been fired. Also, position calculation fields should be defined only when the prop onSelectionChange is true.

    const addCursorPositionToSelectionChange = (event) => {
        const selectionValue = {start: 0, end: 0, positionX: 0, positionY: 0};
        if (shouldCalculateCaretPosition) {
            // we do flushSync to make sure that the valueBeforeCaret is updated before we calculate the caret position to receive a proper position otherwise we will calculate position for the previous state
            flushSync(() => {
                setValueBeforeCaret(event.target.value.slice(0, event.nativeEvent.selection.start));
                setCaretContent(getNextChars(value, event.nativeEvent.selection.start));
            });
            selectionValue.positionX = textRef.current.offsetLeft - CONST.SPACE_CHARACTER_WIDTH;
            selectionValue.positionY = textRef.current.offsetTop;
        }

        selectionValue.start = event.nativeEvent.selection.start;
        selectionValue.end = event.nativeEvent.selection.end;

        onSelectionChange({
            nativeEvent: {
                selection: selectionValue,
            },
        });
        setSelection(selectionValue);
    };

perunt · 2023-08-17T18:25:12Z

Untitled.mov

getusha · 2023-08-17T18:26:42Z

The issue is reproducible because the onSelectionChange has never fired

@perunt yes, i think the reason behind that is we are trying to access null value. your solution is also equivalent to mine here

perunt · 2023-08-17T18:51:37Z

Yes, you are right, it has the same root
But with a few points

for now, edit input shouldn't use shouldCalculateCaretPosition as if used for future inline autosuggestion features and use flushSync, which is quite expensive from a performance perspective
we shouldn't react to textRef.current but shouldCalculateCaretPosition.
also, we shouldn't skip calling setSelection. (it's from another proposal, but I need to mention it here)

For the one whose proposal will be chosen, please ensure that positionX and positionY will come without delay in one frame. It is essential, thanks.

situchan · 2023-08-17T19:34:42Z

Thanks for suggestions @perunt
@neil-marcellini I think we can assign @getusha to fix this deploy blocker. (proposal: #25413 (comment)).
The code @perunt proposed is perfect.

neil-marcellini · 2023-08-17T19:57:31Z

I don't understand the root cause explained in @getusha's proposal. I like @Nikhil-Vats' root cause explanation and proposal better. Thanks for your thoughts @perunt. Do you suggest any changes to @Nikhil-Vats' proposal?

Does that sound good @situchan? The root cause is the most important part of the proposal so that's why I prefer the other proposal.

getusha · 2023-08-17T20:00:02Z

@neil-marcellini the root cause is that we are trying to access a key from a ref which has a value of null.
the execution stops here, showed an example: #25413 (comment)

situchan · 2023-08-17T20:05:22Z

@getusha's root cause is correct (thought not sure why console doesn't show error).

Regarding solution, I am fine either way. One proposes to pass filtered custom event always, while another one proposes to pass raw event when shouldCalculateCaretPosition is false.

Nikhil-Vats · 2023-08-17T20:18:37Z

@situchan @neil-marcellini I can implement @perunt's solution or mine right away if you agree.

Otherwise, the only issue @perunt mentioned in my solution was that I need to call setSelection (it is part of state) too. I will do that along with the proposed change in my proposal.

also, we shouldn't skip calling setSelection. (it's from another proposal, but I need to mention it here)

@perunt pointed that we shouldn't set shouldCalculateCaretPosition and we shouldn't react to textRef.input which is part of the other proposal

for now, edit input shouldn't use shouldCalculateCaretPosition as if used for future inline autosuggestion features and use flushSync, which is quite expensive from a performance perspective
we shouldn't react to textRef.current but shouldCalculateCaretPosition.

getusha · 2023-08-17T20:21:04Z

I explained that the solution is equivalent to mine and your proposal didn't explain why it's not executing onSelectionChange in the root cause #25413 (comment)

getusha · 2023-08-17T21:10:58Z

(thought not sure why console doesn't show error).

@situchan @neil-marcellini the reason it's not throwing an error is a bug from react-native-web

https://github.com/necolas/react-native-web/blob/ef95fc0c2b40cc0d3743df4b56485a16a8442e5f/packages/react-native-web/src/exports/TextInput/index.js#L328-L337

it's contained in a try and catch block, which prevents the error from being shown. if you change the catch block to log the error inside node_modules/@expensify/react-native-web/dist/exports/TextInput/index.js, you can see what the actual error is underneath.

melvin-bot · 2023-08-17T23:15:11Z

📣 @Nikhil-Vats 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

melvin-bot · 2023-08-17T23:15:30Z

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

neil-marcellini · 2023-08-17T23:16:11Z

Ok @situchan is the C+ and @Nikhil-Vats will be implementing it. I'm also removing the blocker label because it's a small regression.

getusha · 2023-08-18T01:31:57Z

It's quite disappointing decision after I identified the correct Root cause #25413 (comment), while the selected lacks the correct root cause and has the same proposal as mine. @neil-marcellini i respect your decision 🙂

Nikhil-Vats · 2023-08-18T04:54:40Z

PR is up for review.

melvin-bot · 2023-08-18T18:08:36Z

🎯 ⚡️ Woah @situchan / @Nikhil-Vats, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

when @Nikhil-Vats got assigned: 2023-08-17 23:15:11 Z
when the PR got merged: 2023-08-18 18:08:35 UTC

On to the next one 🚀

melvin-bot · 2023-08-19T13:12:49Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Nikhil-Vats · 2023-09-06T08:47:38Z

@JmillsExpensify @neil-marcellini i think there's something wrong with the automation here. The overdue label is not added and the payment date is also not mentioned in title despite the PR being deployed to prod 13 days ago.

Nikhil-Vats · 2023-09-12T18:42:21Z

Hi @JmillsExpensify bump on this since labels are not being added here.

Cc @neil-marcellini

Nikhil-Vats · 2023-09-19T20:45:10Z

Hey @JmillsExpensify, this one is due for payment.

neil-marcellini · 2023-09-22T02:35:22Z

I'll get Jason on it. It's best to message him directly in NewDot.

JmillsExpensify · 2023-09-25T08:50:18Z

Thanks for reaching out. Catching up and processing payments.

JmillsExpensify · 2023-09-25T08:51:23Z

Payment summary:

Issue reporter: N/A
Contributor: @Nikhil-Vats $1,500 (incl. urgency bonus)
Contributor+: @situchan $1,500 (incl. urgency bonus).

JmillsExpensify · 2023-09-25T08:57:52Z

Contributor payment issued. @situchan do you mind completing the BZ checklist before I issue payment?

situchan · 2023-09-25T10:06:41Z

I have offer accepted already - #25413 (comment)

situchan · 2023-09-25T10:14:38Z

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: Migrate Composer. web #23359 (comment)
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.
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.

This was caught by Applause team, which means always exists in Testrail.
So no need further regression test.

JmillsExpensify · 2023-09-25T21:58:23Z

Great thanks! I don't see any outstanding contracts for you for this job, so I went ahead and extended one.

JmillsExpensify · 2023-10-08T14:09:52Z

My records are showing that everyone has been paid for this issue. Closing! Please reach out if that's incorrect.

lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Aug 17, 2023

OSBotify mentioned this issue Aug 17, 2023

Deploy Checklist: New Expensify 2023-08-17 #25160

Closed

75 tasks

github-actions bot added Engineering Hourly KSv2 labels Aug 17, 2023

melvin-bot bot assigned neil-marcellini Aug 17, 2023

ShogunFire mentioned this issue Aug 17, 2023

Migrate Composer. web #23359

Merged

58 tasks

melvin-bot bot added Daily KSv2 and removed Hourly KSv2 labels Aug 17, 2023

neil-marcellini assigned Nikhil-Vats and unassigned cubuspl42 and Nikhil-Vats Aug 17, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 17, 2023

neil-marcellini assigned situchan Aug 17, 2023

neil-marcellini removed the DeployBlockerCash This issue or pull request should block deployment label Aug 17, 2023

Nikhil-Vats mentioned this issue Aug 18, 2023

Fix cursor shifting to end while editing messages #25456

Merged

59 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 18, 2023

Nikhil-Vats mentioned this issue Aug 19, 2023

Adding emoji by text in edit shifts cursor #25526

Closed

6 tasks

JmillsExpensify closed this as completed Oct 8, 2023

[$1000] Chat - Cursor does not remain the same place when Edit message #25413

[$1000] Chat - Cursor does not remain the same place when Edit message #25413

Comments

lanitochka17 commented Aug 17, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

OSBotify commented Aug 17, 2023

melvin-bot bot commented Aug 17, 2023

getusha commented Aug 17, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

situchan commented Aug 17, 2023 • edited Loading

getusha commented Aug 17, 2023

Nikhil-Vats commented Aug 17, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Nikhil-Vats commented Aug 17, 2023 • edited Loading

getusha commented Aug 17, 2023

Nikhil-Vats commented Aug 17, 2023

getusha commented Aug 17, 2023

getusha commented Aug 17, 2023

perunt commented Aug 17, 2023 • edited Loading

perunt commented Aug 17, 2023

getusha commented Aug 17, 2023

perunt commented Aug 17, 2023

situchan commented Aug 17, 2023

neil-marcellini commented Aug 17, 2023

getusha commented Aug 17, 2023

situchan commented Aug 17, 2023

Nikhil-Vats commented Aug 17, 2023 • edited Loading

getusha commented Aug 17, 2023

getusha commented Aug 17, 2023

melvin-bot bot commented Aug 17, 2023

melvin-bot bot commented Aug 17, 2023

neil-marcellini commented Aug 17, 2023

getusha commented Aug 18, 2023

Nikhil-Vats commented Aug 18, 2023 • edited Loading

melvin-bot bot commented Aug 18, 2023

melvin-bot bot commented Aug 19, 2023

Nikhil-Vats commented Sep 6, 2023

Nikhil-Vats commented Sep 12, 2023

Nikhil-Vats commented Sep 19, 2023

neil-marcellini commented Sep 22, 2023

JmillsExpensify commented Sep 25, 2023

JmillsExpensify commented Sep 25, 2023

JmillsExpensify commented Sep 25, 2023

situchan commented Sep 25, 2023

situchan commented Sep 25, 2023

JmillsExpensify commented Sep 25, 2023

JmillsExpensify commented Oct 8, 2023

lanitochka17 commented Aug 17, 2023 •

edited by melvin-bot bot

Loading

getusha commented Aug 17, 2023 •

edited

Loading

situchan commented Aug 17, 2023 •

edited

Loading

Nikhil-Vats commented Aug 17, 2023 •

edited

Loading

Nikhil-Vats commented Aug 17, 2023 •

edited

Loading

perunt commented Aug 17, 2023 •

edited

Loading

Nikhil-Vats commented Aug 17, 2023 •

edited

Loading

Nikhil-Vats commented Aug 18, 2023 •

edited

Loading