[hold for app #12854] [$2000] Shows incorrect emoji while combining the manual and emoji picker selected emoji within the text

[kavimuru]

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

---

Action Performed:

1. Load new.expensify.com.
2. Open a chat.
3. type Te:smile :st in the text bar.
4. remove the space so the smile emoji renders inside of the word test. leave your cursor flush with the emoji.
5. click the emoji picker.
6. select an emoji.
7. confirm that the new emoji loads in between the two question mark characters. This is the bug.

Expected Result:

Should show both emoji adjacently after selecting the second one.

Actual Result:

Shows a weird emoji, and removes the original emoji, incorrectly.

Workaround:

Select emojis via the emoji picker.

Platform:

Where is this issue occurring?

• Web

Version Number: v1.2.32-1
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
7c67d298-274a-4ea8-a781-e28384f08007.webm
https://user-images.githubusercontent.com/43996225/204145304-bf1f2078-3b3f-46c5-9ac0-26b4d8bcf01e.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Pujan92
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669391872652859

View all open jobs on GitHub

Recording.1012.mp4

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016d120322c0f448f0
• Upwork Job ID: 1597336781605519360
• Last Price Increase: 2023-04-17

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016d120322c0f448f0
• Upwork Job ID: 1597336781605519360
• Last Price Increase: 2023-04-17

[melvin-bot]

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

[Pujan92]

Proposal

As we are removing the character between the emoji string(: smile:) I think prevState selection logic won't work in the following code.

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

Lines 392 to 396 in 17c34e3

	const remainder = prevState.value.slice(prevState.selection.end).length;
	newState.selection = {
	start: newComment.length - remainder,
	end: newComment.length - remainder,
	};

const index = this.getLastUnmatchedCharacterIndex(comment, newComment) + 1;
newState.selection = {
    start: index,
    end: index,
};

getLastUnmatchedCharacterIndex(text, modifiedText) {
    for (let i = text.length - 1, j = modifiedText.length - 1; i >= 0; i--, j--) {
        if (text[i] !== modifiedText[j]) {
             return j;
         }
     }
}

The above solution will fix 12325 too which was earlier fixed with prevState selection logic.

[s77rt]

Proposal

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 2335864f91..fc7b5fad51 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -7,6 +7,7 @@ import {
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
+import lodashClamp from 'lodash/clamp';
 import {withOnyx} from 'react-native-onyx';
 import lodashIntersection from 'lodash/intersection';
 import styles from '../../../styles/styles';
@@ -389,10 +390,11 @@ class ReportActionCompose extends React.Component {
                 value: newComment,
             };
             if (comment !== newComment) {
-                const remainder = prevState.value.slice(prevState.selection.end).length;
+                const offset = newComment.length - comment.length;
+                const compensation = Math.max(comment.length - prevState.value.length, 0);
                 newState.selection = {
-                    start: newComment.length - remainder,
-                    end: newComment.length - remainder,
+                    start: lodashClamp(prevState.selection.start + offset + compensation, 0, newComment.length),
+                    end: lodashClamp(prevState.selection.end + offset + compensation, 0, newComment.length),
                 };
             }
             return newState;

Details:

The current implemented code does not take into account compensation and overflow.
In my proposal I added a compensation variable to fix the current issue and used lodashClamp to cover other cases (e.g. write : wave:hi then remove the space before wave, the calculated position is negative (invalid), this has been fixed as well)

[joekaufmanexpensify]

Triaging this now:

From this SO: https://stackoverflow.com/c/expensify/questions/14418

• The "bug" is actually a bug:
• The bug is not a duplicate report of an existing GH (close the GH and add any novel details to the original GH instead):
• The bug is reproducible, following the reproduction steps:
• If you're unable to reproduce the bug, add the Needs reproduction label. Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.:
• The GH template is filled out as fully as possible -- this means the GH body and title are clear (ie. could an external contributor understand it and work on it?):
• The GH was created by an Expensify employee or a QA tester:
• If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it's better to wait, close the GH & provide this justification:
• If there's a link to Slack, check the discussion to see if we decided not to fix it:
• Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label:

[joekaufmanexpensify]

I can reproduce this on web. Added some more detail to reproduction steps. This is a niche bug, however it can be reproduced, so I think we proceed with fixing it.

[joekaufmanexpensify]

My thinking is this should be external. Assigning to engineering to confirm!

[melvin-bot]

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

[johnmlee101]

Yep, confirmed. At first I thought it was the skin-tone modifiers for emoji but it seems to be injecting it in a way that splits the unicode incorrectly.

[melvin-bot]

Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016d120322c0f448f0

[melvin-bot]

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

[melvin-bot]

Current assignee @johnmlee101 is eligible for the External assigner, not assigning anyone new.

[Pujan92]

As we are removing the character between the emoji string(: smile:) I think prevState selection logic won't work in the following code.

Initial proposal screencast

ff99a282-339f-4945-b1b8-6bbed6009d52.webm

[thesahindia]

@johnmlee101 @joekaufmanexpensify, I won't be able to get to this today. Please re-assign.

[melvin-bot]

Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[joekaufmanexpensify]

Same

[joekaufmanexpensify]

Still on hold.

[joekaufmanexpensify]

Same

[hannojg]

Hey,

#12854 has been first put on hold for a now closed and dismissed PR. Afterwards it has been put on hold for another PR, but this PR is only addressing an android issue.

So I'd say this issue can be taken off hold, and we can again look into proposals!

[joekaufmanexpensify]

Sounds good! @johnmlee101 and @sobitneupane does that sound good to y'all as well?

[sobitneupane]

Testing if it is still reproducible: Yes. It is reproducible.

I don't think we can take off hold. We would want the issue to be fixed on android and IOS. We should still wait for #12854 to fix issue on android.

@hannojg Is there any issue open that you know that fixes the selection issue on IOS?

[hannojg]

The issue right now is on hold for another issue that fixes a bug on android, while this issue is a web issue. They are in that sense not related.
The android fix will change nothing about this issue, so it doesn't make sense to me to have it on hold for #12854.

For iOS there is this issue:

• [$1000] IOS - IOUAmount - When entering/deleting numbers quickly, the cursor doubles #15856

I confirmed that the issue will be fixed once we upgrade the app to fabric renderer.

[joekaufmanexpensify]

@johnmlee101 curious what you think here?

[johnmlee101]

As long as both issues don't overlap or conflict with each other, I think we can remove off HOLD.

[sobitneupane]

This issue exists on all platform not only Web. And to confirm the solution works well, the selection issues on IOS and android should be handled first.

[joekaufmanexpensify]

Got it. @sobitneupane that would mean in fact leaving this on hold until 12854 is done. Is that right?

[joekaufmanexpensify]

Still waiting for confirmation

[sobitneupane]

@joekaufmanexpensify yes.

[joekaufmanexpensify]

Got it, ty!

[joekaufmanexpensify]

Still on hold

[joekaufmanexpensify]

Same

[joekaufmanexpensify]

Still on hold

[joekaufmanexpensify]

Same

[joekaufmanexpensify]

Same.

[joekaufmanexpensify]

Held

[joekaufmanexpensify]

Same

[joekaufmanexpensify]

Same

[joekaufmanexpensify]

Same.

[joekaufmanexpensify]

Same

[joekaufmanexpensify]

Held

[joekaufmanexpensify]

Just retested this and its no longer reproducible. Closing.