[$500] Do not allow emojis to be formatted (ie italics)

[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. Go to android app
2. Go to any chat
3. Try to make emoji in italic form:-Type in : insertemoji For example:⁦:100:, ⁦:purple_heart: , ⁦:partying_face: ,
4. Notice that the top right part of the emoji cuts off. Few Example: the 💯, 💜, 🥳 emoji has its top right section cut , (It happens on android only latest)

Expected Result:

Users should not be able to format emojis, lets keep them with normal style (no bold, italics, strikethrough etc)

Actual Result:

Users can make emoji italic which leads to the emojis to cut off a few parts of the emoji (for some emojis)

Workaround:

unknown

Platforms:

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

• Android / native
• MacOS Desktop
• MacOS Chrome Web

Version Number: 1.2.62-1
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:

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675094271396099

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017d08e0c09284d31f
• Upwork Job ID: 1620563000799895552
• Last Price Increase: 2024-08-08
• Automatic offers:
• eh2077 | Reviewer | 0
• wildan-m | Contributor | 0

Issue Owner

Current Issue Owner: @mallenexpensify / @mallenexpensify

[anmurali]

Confirmed that this happens on Android native app.

Confirmed it does not happen on Mac/Chrome. But related, when I try the same reproduction steps on iOS native, iOS Chrome and Mac Safari - I cannot italicize emojis at all. Is that expected?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017d08e0c09284d31f

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[alexxxwork]

Proposal

Quick fix:

diff --git a/src/styles/styles.js b/src/styles/styles.js
index b826ed336f..84286591e9 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -63,6 +63,7 @@ const webViewStyles = {
         em: {
             fontFamily: fontFamily.EXP_NEUE,
             fontStyle: 'italic',
+            width: variables.fontSizeNormalHeight,
         },
 
         del: {

[flodnv]

I don't think we should be able to italicize emojis, asked here: https://expensify.slack.com/archives/C01GTK53T8Q/p1675266083150699

[JmillsExpensify]

Agree with @flodnv.

[tienifr]

Proposal

The Emoji is italicized because it is contained in the <em></em> tag (same as when we make a text italic). We can observe the behavior on both Chrome web and Android.

In order to remove the ability to italicize emojis, I'll wrap any emoji that are inside the <em></em> with a expensify-emoji custom HTML tag, and give it a fontStyle: 'normal' style. This is the same way that Github does it.

(The emoji regex below is taken from the CONST.REGEX.EMOJIS in Expensify app)

In https://github.com/Expensify/expensify-common

diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js
index 7f94ffe..057e87c 100644
--- a/lib/ExpensiMark.js
+++ b/lib/ExpensiMark.js
@@ -134,6 +134,11 @@ export default class ExpensiMark {
                 regex: /(?!_blank")\b_((?!\s).*?\S)_\b(?![^<]*(<\/pre>|<\/code>|<\/a>|_blank))/g,
                 replacement: '<em>$1</em>',
             },
+            {
+                name: 'emoji',
+                regex: /(<em>.*?)([\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3)(.*?<\/em>)/gu,
+                replacement: '$1<expensify-emoji>$2</expensify-emoji>$3',
+            },
             {
                 // Use \B in this case because \b doesn't match * or ~.
                 // \B will match everything that \b doesn't, so it works
                 

In https://github.com/Expensify/App

diff --git a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
index 456cd1ecf3..0bf75ea9d2 100755
--- a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
+++ b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
@@ -39,6 +39,10 @@ const customHTMLElementModels = {
         tagName: 'email-comment',
         mixedUAStyles: {whiteSpace: 'normal'},
     }),
+    'expensify-emoji': defaultHTMLElementModels.span.extend({
+        tagName: 'expensify-emoji',
+        mixedUAStyles: {fontStyle: 'normal'},
+    }),
 };
 
 const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText]};

There're small possible variations based on our preference:

• We don't have to use a custom expensify-emoji tag but just use a span tag with a style. I prefer using a custom tag to make it clear

There're some further minor fixes required:

• when only there's only the emoji inside the em, no other text, the emoji will not be italicized but it will also not have the onlyEmojisText style, we can fix that with another regex.
• add logic to htmlToMarkdownRules to remove the Expensify-emoji tag

I'll add those if we want to go with this direction.

Pls note we have to fix the same in the BE as well, in order to test the above fix we need to enable offline mode.

[tienifr]

working well after the fix:

Screen.Recording.2023-02-02.at.18.08.29.mov

[s77rt]

@tienifr Your approach looks solid however the regex is not correct as it will only match one emoji, try sending hi 😁 there 😁, it will wrap the first emoji but not the second one. Also it will only work with em tags and I think it would be better to wrap the emojis all the time (this will also cover the bold case).

Here are my suggestions:

1. Replace the regex part with:

            {
                name: 'emoji',
                regex: /[\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu,
                replacement: match => `<expensify-emoji>${match}</expensify-emoji>`,
            },

2. Move the regex emoji to be before italic and after autolink (Not necessary, just a personal opinion as I like to preserve the order of italic, bold, strikethrough regex rules)
3. In mixedUAStyles: {fontStyle: 'normal'} add fontWeight: 'normal'

[tienifr]

Thanks @s77rt for your suggestion!

Updated proposal

Adding to the original proposal, Here's the my updated diff for the regex so that it matches all rather than just one.

diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js
index c535048..62b183f 100644
--- a/lib/ExpensiMark.js
+++ b/lib/ExpensiMark.js
@@ -242,6 +242,11 @@ export default class ExpensiMark {
                 regex: /<(em|i)(?:"[^"]*"|'[^']*'|[^'">])*>(.*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,
                 replacement: '_$2_',
             },
+            {
+                name: 'emoji',
+                regex: /([\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3)/gu,
+                replacement: '<expensify-emoji>$1</expensify-emoji>',
+            },
             {
                 name: 'bold',
                 regex: /<(b|strong)(?:"[^"]*"|'[^']*'|[^'">])*>([\s\S]*?)<\/\1>(?![^<]*(<\/pre>|<\/code>))/gi,

[tienifr]

@s77rt Regarding the fontWeight: 'normal', I don't feel strongly about since it doesn't seem that the bold fontWeight affects how the emoji looks.

[s77rt]

@tienifr

• You can use the same regex as provided, there is no need to create a capturing group that captures the whole match (unnecessary doubling memory usage)
• If you bold the emoji it will be bold but it's really hard to notice the difference, the outer circle shadow will appear a bit thicker. Adding fontWeight: 'normal' ensures we are rendering the emoji correctly

[tienifr]

@s77rt I tried to zoom the emojis in but can't recognize any difference between an emoji that has fontWeight: 'bold' and a normal one. But we surely can add that if we want to be extra safe in that bold case (Github also does it).

Thanks for the input!

diff --git a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
index 456cd1ecf3..0bf75ea9d2 100755
--- a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
+++ b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
@@ -39,6 +39,10 @@ const customHTMLElementModels = {
         tagName: 'email-comment',
         mixedUAStyles: {whiteSpace: 'normal'},
     }),
+    'expensify-emoji': defaultHTMLElementModels.span.extend({
+        tagName: 'expensify-emoji',
+        mixedUAStyles: {fontStyle: 'normal', fontWeight: 'normal'},
+    }),
 };
 
 const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText]};

[s77rt]

@tienifr While seaching I found this image.

The middle emoji is in bold but it's hard to notice also it may depend on the font. Adding fontWeight: 'normal' is more a safety step.

[MelvinBot]

@flodnv, @anmurali, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

[MelvinBot]

@flodnv, @anmurali, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

[aimane-chnaif]

reviewing proposals shortly

[stitesExpensify]

@BartoszGrajdek where are we at with prioritization these days?

[melvin-bot]

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

[BartoszGrajdek]

BartoszGrajdek where are we at with prioritization these days?

We have much more time to handle things now, sorry for the wait 🙌

@Skalakid decided to investigate if there's a cleaner way to handle this problem. If that won't be the case we'll follow through with the PR created by Robert after making some adjustments.

[stitesExpensify]

@Skalakid could you please provide an update?

[Skalakid]

@stitesExpensify sorry, I had to prioritize the Live Markdown refactor PR and inline images feature. However, I prepared a small fix for the web. We can just add some styles to emoji default styles that will overwrite italic and other ones, like fontStyle: normal. Now we have to investigate this issue on the Android native side, since iOS appear to work properly now

[stitesExpensify]

Hey @Skalakid any more updates? :)

[stitesExpensify]

Bump :)

[Skalakid]

Hello, unfortunately we don't have any capacity to work on this issue now, because we are focused on fixing issues with higher priority

[mallenexpensify]

Thanks @Skalakid , I agree this isn't the highest priority. I was going to do something then realized we have 300 comments (which I didn't read all of before posting this) on this issue so I wanted to pause and reevaluate. Should we

1. Bump this to monthly and the SWM can maybe get to it at some point
2. Add Help Wanted to see if a contributor will want to work on
3. Add Design label to get feedback from them (ie. if they don't care, we could close this)
4. Close it for now and maybe readdress later.

cc @Skalakid , @BartoszGrajdek , @stitesExpensify @eh2077 @wildan-m

[Skalakid]

@mallenexpensify I think we can go with option number 1, we are finishing our current work and might have some time to investigate this issue on the Android native side

[mallenexpensify]

Will keep checking back ~monthly.

[stitesExpensify]

Hey @Skalakid any chance you got around to it this month?

[Skalakid]

Hello, I will take a look at it 👀

[tomekzaw]

FYI we added emoji range in live-markdown parser in Expensify/react-native-live-markdown#233

[Skalakid]

Hi, here is the PR with the fix 😄

[stitesExpensify]

nice, that's awesome!!