[$1000] Emoji size from email reply is smaller then emoji size sent via normal chat and emoji does not use user selected skin color

[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. Open the app and login with user A
2. Open user B chat and send a message
3. Open user B email
4. Reply to user A message by email and send only emojis

Expected Result:

Size of emojis sent via email reply should be same as size of emoji sent via normal chat, emoji skin color should also match to the one used by the user

Actual Result:

Size of emojis is smaller then size of emojis sent via normal chat and skin color too does not match

Workaround:

unknown

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: v1.2.63-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

emoji.send.email.reply.mp4

Recording.1426.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0174e50816828c7960
• Upwork Job ID: 1622671435996983296
• Last Price Increase: 2024-12-18

Issue Owner

Current Issue Owner: @pecanoro

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

Testing, not getting emails for all chats sent from my test account.

[MelvinBot]

@mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mallenexpensify]

ah ha! Was able to reproduce, nice find @dhanashree-sawant
Pretty sure this can be external.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0174e50816828c7960

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[Prince-Mendiratta]

Proposal

RCA

Whenever a comment is sent from the email, the update is of this format -

[{"onyxMethod":"merge","key":"report_7264088055740850","value":{"reportID":"7264088055740850","maxSequenceNumber":49,"lastActionCreated":"2023-02-06 21:10:18.038","lastReadTime":"2023-02-06 21:10:08.928","lastMessageText":"👴👴","lastActorEmail":"[email protected]"}},{"onyxMethod":"merge","key":"reportActions_7264088055740850","value":{"49":{"person":[{"type":"TEXT","style":"strong","text":"expert"}],"actorEmail":"[email protected]","actorAccountID":13064488,"message":[{"type":"COMMENT","html":"<div dir=\"ltr\">👴👴</div>","text":"👴👴","isEdited":false}],"originalMessage":{"html":"<div dir=\"ltr\">👴👴</div>","source":"email"},"avatar":"https://d1wpcgnaa73g0y.cloudfront.net/497a9e0151014341987046ea878258b3833ec0b7_128.jpeg","created":"2023-02-06 21:10:18.038","timestamp":1675717818,"reportActionTimestamp":1675717818038,"automatic":false,"sequenceNumber":49,"actionName":"ADDCOMMENT","shouldShow":true,"clientID":"","reportActionID":"7249547654175174326","isAttachment":false},"":null},"shouldNotify":true}]

From the backend, the emojis are enclosed with <div dir="ltr">{emojis}</div>. As per this code, we check if the text is to be rendered as HTML or Text component -

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

Lines 101 to 119 in 37e9344

	const {html, text} = props.fragment;
	// If the only difference between fragment.text and fragment.html is <br /> tags
	// we render it as text, not as html.
	// This is done to render emojis with line breaks between them as text.
	const differByLineBreaksOnly = Str.replaceAll(html, '<br />', '\n') === text;
	// Only render HTML if we have html in the fragment
	if (!differByLineBreaksOnly) {
	const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
	const htmlContent = html + editedTag;
	return (
	<RenderHTML
	html={props.source === 'email'
	? `<email-comment>${htmlContent}</email-comment>`
	: `<comment>${htmlContent}</comment>`}
	/>
	);
	}

We only check for the existence of emoji only texts if the text is being rendered using the Text component and not as an HTML.

Solution

We can add conditions to check that if the text obtained from an email comment contains only emojis and if it does, apply the onlyEmojisText style to the element by enclosing it with a custom tag.

diff --git a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
index 456cd1ecf3..9402cd8314 100755
--- a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
+++ b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
@@ -35,6 +35,10 @@ const customHTMLElementModels = {
         tagName: 'comment',
         mixedUAStyles: {whiteSpace: 'pre'},
     }),
+    'emojis-only': defaultHTMLElementModels.div.extend({
+        tagName: 'emojis-only',
+        mixedUAStyles: {...styles.onlyEmojisText},
+    }),
     'email-comment': defaultHTMLElementModels.div.extend({
         tagName: 'email-comment',
         mixedUAStyles: {whiteSpace: 'normal'},

diff --git a/src/pages/home/report/ReportActionItemFragment.js b/src/pages/home/report/ReportActionItemFragment.js
index c4f593517d..a700b2a0e4 100644
--- a/src/pages/home/report/ReportActionItemFragment.js
+++ b/src/pages/home/report/ReportActionItemFragment.js
@@ -108,7 +108,8 @@ const ReportActionItemFragment = (props) => {
             // Only render HTML if we have html in the fragment
             if (!differByLineBreaksOnly) {
                 const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
-                const htmlContent = html + editedTag;
+                const emojiOnlyTag = EmojiUtils.containsOnlyEmojis(text) ? `<emojis-only>${html}</emojis-only>` : html;
+                const htmlContent = emojiOnlyTag + editedTag;
                 return (
                     <RenderHTML
                         html={props.source === 'email'

Regarding the emoji skin color issue, I believe it is not a bug and we should not handle that. When a user responds using email, they have specifically used the default emoji tone. If they were to instead use custom emoji skin colors, such as 🖐🏻🖐🏿🖐🏻, then when it renders in expensify, it renders correctly as 🖐🏻🖐🏿🖐🏻. If were to instead change all the emojis to the default skin color of the user, it would convert it to 🖐🏻🖐🏻🖐🏻, if the default tone was white. This would lead to a loss of information and the best way to deal with this is to do nothing. If the user chooses to send default skin tone, then we display default skin tone.

Results

2023-02-07.03-22-22.mp4

[Prince-Mendiratta]

Proposal 2

Solution

We can simply check if the text contains only emojis. If it does, then display it using Text instead. The result is same as in my above proposal.

diff --git a/src/pages/home/report/ReportActionItemFragment.js b/src/pages/home/report/ReportActionItemFragment.js
index c4f593517d..3e4cb3915b 100644
--- a/src/pages/home/report/ReportActionItemFragment.js
+++ b/src/pages/home/report/ReportActionItemFragment.js
@@ -106,7 +106,7 @@ const ReportActionItemFragment = (props) => {
             const differByLineBreaksOnly = Str.replaceAll(html, '<br />', '\n') === text;
 
             // Only render HTML if we have html in the fragment
-            if (!differByLineBreaksOnly) {
+            if (!differByLineBreaksOnly && !EmojiUtils.containsOnlyEmojis(text)) {
                 const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
                 const htmlContent = html + editedTag;
                 return (

Proposal 3

Why are we adding the extra div in the backend? I'd say we can directly remove the div from the backend since then the component would use the Text component and it applies the ltr style anyways.

[s77rt]

Proposal

diff --git a/src/pages/home/report/ReportActionItemFragment.js b/src/pages/home/report/ReportActionItemFragment.js
index c4f593517d..b4ced62e10 100644
--- a/src/pages/home/report/ReportActionItemFragment.js
+++ b/src/pages/home/report/ReportActionItemFragment.js
@@ -104,9 +104,10 @@ const ReportActionItemFragment = (props) => {
             // we render it as text, not as html.
             // This is done to render emojis with line breaks between them as text.
             const differByLineBreaksOnly = Str.replaceAll(html, '<br />', '\n') === text;
+            const differByDivTagsOnly = Str.replaceAll(html, /<div[^>]*>|<\/div>/g, '') === text;
 
             // Only render HTML if we have html in the fragment
-            if (!differByLineBreaksOnly) {
+            if (!differByLineBreaksOnly && !differByDivTagsOnly) {
                 const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
                 const htmlContent = html + editedTag;
                 return (

RCA

Sending messages from email will most likely be sent as html and that's the case here where emojis are sent as <div dir="ltr">😁</div>. Since the message is technically html it will be rendered using our HTML render engine. However as you can see there is nothing special about that message to treat it as html and we can instead just treat as a text.

We follow this approach in another similar case where the difference between the html message and the plaintext message is just line breaks (differByLineBreaksOnly) and I propose to do the same here by checking if the difference between the two messages is just those "useless" div tags (differByDivTagsOnly).

So, we strip the html message from those useless div tags and if it's the same as the text then just treat it as text.

---

• We can extend the regex to cover other "useless" tags such as span, p, etc.
• Why those tags are "useless"? because they produce no difference in the final markdown format.

[bernhardoj]

I think this issue will eventually be fixed by this improvement.

[pecanoro]

@bernhardoj I think you are right. That issue encompasses a lot of fixes for emojis by making them consistent so I am going to add a HOLD on this one in benefit of that one. @mananjadhav What do you think? Or do you think we should fix this one separately?

[pecanoro]

I was looking into this and I am not sure how we could handle it. Gmail is the one that ends up wrapping that emoji between the . We could try to parse it in the back-end to replace the div for the emoji tag but it's a bit hacky and I am not sure if it's high priority enough.

[melvin-bot]

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

[melvin-bot]

@mananjadhav, @pecanoro, @mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

[pecanoro]

I am going to move to monthly because it's an edge case and I have a few more important internal ones to handle first

[mallenexpensify]

👍 @pecanoro . I think the larger issue might be the skin color being inconsistent vs the emoji size. I wonder why google/gmail handles them differently

[pecanoro]

I will test the skin color tomorrow to see what's happening there

[mananjadhav]

@pecanoro Any updates on this?

[pecanoro]

The tone seems to work well, it's only the size:

I could ask internally to see if anyone has a better idea on how to fix this, I couldn't figure it out after a decent a mount of time and since it's really low priority, I feel I should be focusing on other stuff

[mananjadhav]

There were two promising proposals earlier. Should we try to fix it on the frontend to render the emojis without the custom <emoji> tags?

[pecanoro]

We could also try that or do nothing, @mallenexpensify what do you think? Both solutions are a bit weird since it's gmail the one wrapping the emojis in divs.

[mallenexpensify]

@mananjadhav how about posting in #expensify-open-source to get more 👀 and feedback on our options? I don't love fixing it only for gmail but... so many people use gmail.

[mananjadhav]

Okay sure. Started a slack thread.

[mallenexpensify]

@mananjadhav , @s77rt replied in thread

I'm still inclined for the frontend approach and use shouldRenderAsText as we'd be using an already existing solution on how we handle such cases. Otherwise :donothing:.

FWIW, I don't think this should be fixed in the BE because I suspect that the BE does not do any text processing on the email content and add that processing just for this case is not worth it. However IF the BE does process email text then fixing this in BE is probably best.

Do you agree?

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[s77rt]

This is not reproducible from my end too

[pecanoro]

Oh wow yes, it has been magically fixed lol

[pecanoro]

Gmail still wraps the emoji with <div dir="ltr"> so we must have changed something on our end.

[pecanoro]

@mallenexpensify If you can't reproduce it either, let's close it!

[mallenexpensify]

Wow, I'll take magic fixes anytime! Thx y'all

[mallenexpensify]

@mananjadhav due $500 via NewDot for C+ work on this issue, ie. 50% of job price. As discussed here

[mananjadhav]

Requested on NewDot.

[garrettmknight]

$500 approved for @mananjadhav