[HOLD for payment 2023-05-23] [$1000] Copy pasting suggestion triggering text from sent message does not trigger suggestion emojis

[kavimuru]

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:

1. Open the app
2. Open any report
3. Send any suggestion triggering text like: ':pizza'
4. Select and copy the sent message and paste it in compose box

Expected Result:

App should trigger suggestion emojis as it does on slack and as it does normally for copy to clipboard text

Actual Result:

App does not trigger suggestion emojis when we copy paste suggestion triggering text from previous messages

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.8.8
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
Notes/Photos/Videos: Any additional supporting documentation

copy.paste.text.emoji.suggestion.does.not.trigger.mp4

Recording.436.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682930654074229

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011ee31bf40fe883ce
• Upwork Job ID: 1654137510019043328
• Last Price Increase: 2023-05-04

[MelvinBot]

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

[MelvinBot]

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

[bernhardoj]

Proposal

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

Copying and pasting an emoji code from chat message does not show the emoji suggestion.

What is the root cause of that problem?

Every chat message is appended with LTR unicode.

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

Lines 129 to 131 in 4d4d484

	style={[EmojiUtils.containsOnlyEmojis(text) ? styles.onlyEmojisText : undefined, styles.ltr, ...props.style]}
	>
	{StyleUtils.convertToLTR(Str.htmlDecode(text))}

App/src/styles/StyleUtils.js

Lines 728 to 736 in 4d4d484

	/**
	* Android only - convert RTL text to a LTR text using Unicode controls.
	* https://www.w3.org/International/questions/qa-bidi-unicode-controls
	* @param {String} text
	* @returns {String}
	*/
	function convertToLTR(text) {
	return `\u2066${text}`;
	}

It is to force the text to becomes LTR because the style from styles.ltr uses writingDirection which is not supported on native (the comment mentioned Android only but looks like iOS doesn't support that style too).

Because now we have the extra character (\u2066), when we select a chat message and copy it, it will also copy the LTR unicode. You can validate it by copy the text and decode it here https://www.online-toolz.com/tools/text-unicode-entities-convertor.php.

So, when we paste the text (let say :pizza) to composer, it will be
\u2006:pizza

Currently, an emoji suggestion won't show if there is a character before the colon. There is already a report of this here https://expensify.slack.com/archives/C049HHMV9SM/p1678947124050359 which marked as not a bug.

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

This issue will be fixed once we allow the emoji suggestion to show when there is a character before a colon, but I also think the LTR unicode is redundant for web because we already use the writingDirection: ltr style. So, we can simply apply the convertToLTR only for native.

1. I don't think it belongs to StyleUtils.js, so let's move convertToLTR from StyleUtils.js to a new platform specific file in libs to have a different logic.

libs
 |--- convertToLTR
         |--- index.js
         |--- index.native.js

2. In index.js, we simply return the text passed as the param, while in index.native.js, we use the same logic as we have now, which is to append with \u2066

[slafortune]

Issue looks good!

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~011ee31bf40fe883ce

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @cead22 (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[cead22]

Every chat message is appended with LTR unicode. ... It is to force the text to becomes LTR

Out of curiosity, what happens if we don't do this?

[bernhardoj]

It would messed up the text position because of the bidi algorithm when the text contains both RTL and LTR characters.

This is how it looks when we force it to be LTR

[cead22]

This issue will be fixed once we allow the emoji suggestion to show when there is a character before a colon,

I agree with this statement, but that doesn't feel like a solution to this issue

but I also think the LTR unicode is redundant for web because we already use the writingDirection: ltr style. So, we can simply apply the convertToLTR only for native.

I also agree with this, and since there's a difference in behavior between platforms, I think it makes sense to use different logic here. Is the idea to make StyleUtils.convertToLTR simply return the text passed as param for web/desktop, and keep the current behavior for native? If so, can you update your proposal to reflect that?

That sounds ideal to me because

• I don't think we should be prepending anything to what users send. Said differently, it seems like Bad UX that we're prepending \u2066, cause I wasn't expecting it, and I bet most users won't expect to be copying that when selecting text from a message all the way to the beginning, and then pasting it.
• However on native, you can't copy \u2066 because you can't really select the text up to the beginning, cause there's no cursor to drag for a selection
• I tested long pressing > copy to clipboard on a message containing :pizza on iOS, and then pasted it, and the emoji autocomplete dialog showed up, which makes me think when using the context menu item copy to clipboard, we're NOT copying the \u2066 character
• That means we'd be adding \u2066 only on native but in a way you can't copy it, and we won't be adding it on web/desktop where you would be able to copy it

[bernhardoj]

@cead22 Totally agree with you. Updated my proposal to be more detail.

[thesahindia]

The proposal makes sense to me. But since this is only needed for android let's use index.android.js

C+ reviewed 🎀👀🎀

cc: @cead22

[cead22]

(the comment mentioned Android only but looks like iOS doesn't support that style too).

Agreed, but let's confirm. If this applies to both iOS and Android, the index.native.js file makes sense. Otherwise let's go with index.android.js

@slafortune can you hire @bernhardoj for this please?

[bernhardoj]

Just tested on iOS. It supports writingDirection, so we only need index.android.js.
https://reactnative.dev/docs/text-style-props#writingdirection-ios

[bernhardoj]

@cead22 Am I allow to open the PR now? Or wait to get assigned?

[melvin-bot]

📣 @bernhardoj You have been assigned to this job by @cead22!
Please apply to this job in Upwork 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 📖

[cead22]

Yup, sorry for the delay here

[bernhardoj]

No worries. PR is ready

cc: @thesahindia

[melvin-bot]

@cead22 @slafortune @bernhardoj @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[cead22]

The PR for this is on staging

[melvin-bot]

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

• Append LTR unicode on Android only #18768

If no regressions arise, payment will be issued on 2023-05-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

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

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

[slafortune]

Offers sent to @thesahindia @bernhardoj @dhairyasenjaliya 👍
@thesahindia can you complete the above check list?

[dhanashree-sawant]

Thanks @slafortune, I have accepted the offer.

[slafortune]

Ah shoot! @dhairyasenjaliya - I need to type more letters than @dha before hitting enter!

[dhairyasenjaliya]

Seems like no payment due for me here all good @slafortune

[slafortune]

@thesahindia - Shout when you accept the offer. I am OOO but finishing this.

[thesahindia]

@slafortune, accepted!

[melvin-bot]

@cead22, @slafortune, @bernhardoj, @thesahindia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[slafortune]

complete