kavimuru commented 1 year ago

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:

Open the app and login with user A
Open user B chat and send a message
Open user B email
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:

https://user-images.githubusercontent.com/43996225/216351996-a259a36c-cd7d-4260-9e78-79d3714f275e.mp4

https://user-images.githubusercontent.com/43996225/216352049-c590c637-ddfb-46e4-9696-e002ff6bb357.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: 2023-02-06

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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 commented 1 year ago

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

MelvinBot commented 1 year ago

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

mallenexpensify commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

Prince-Mendiratta commented 1 year ago

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":"expertsquarrel@gmail.com"}},{"onyxMethod":"merge","key":"reportActions_7264088055740850","value":{"49":{"person":[{"type":"TEXT","style":"strong","text":"expert"}],"actorEmail":"expertsquarrel@gmail.com","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 - https://github.com/Expensify/App/blob/37e93444ff414b7cb6de81933dbe194ce7f787fa/src/pages/home/report/ReportActionItemFragment.js#L101-L119

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

https://user-images.githubusercontent.com/54077356/217096550-3b4b836e-655d-4abd-b25b-f175e58815f0.mp4

Prince-Mendiratta commented 1 year ago

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 commented 1 year ago

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.