Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Web - Chat - Some emojis change when there is markdown in the message #32833

Closed
1 of 6 tasks
kbecciv opened this issue Dec 11, 2023 · 36 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Dec 11, 2023

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


Version Number: v1.4.11-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Log in with any account
  2. Navigate to any chat
  3. Send a message which consists of a few emojis and any markdown

Expected Result:

Emojis should match the emojis selected when drafting the message.

Actual Result:

Some emojis change when there is a markdown in the message.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6309294_1702313214146.2023-12-11_17-39-41.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015728063279fccae6
  • Upwork Job ID: 1734259059703267328
  • Last Price Increase: 2024-07-09
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 11, 2023
@melvin-bot melvin-bot bot changed the title Web - Chat - Some emojis change when there is markdown in the message [$500] Web - Chat - Some emojis change when there is markdown in the message Dec 11, 2023
Copy link

melvin-bot bot commented Dec 11, 2023

Job added to Upwork: https://www.upwork.com/jobs/~015728063279fccae6

Copy link

melvin-bot bot commented Dec 11, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 11, 2023
Copy link

melvin-bot bot commented Dec 11, 2023

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

Copy link

melvin-bot bot commented Dec 11, 2023

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

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Dec 11, 2023

Proposal

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

Web - Chat - Some emojis change when there is markdown in the message

What is the root cause of that problem?

This happens only on Windows due to the difference in the the rendering engine used on bold and normal text. Bold text uses TRenderEngineProvider, which uses singleFontFamily, and normal text uses RNText, which multiFontFamily.

Why such difference in renderer?

Becuase of this line in TextCommentFragment

const differByLineBreaksOnly = Str.replaceAll(html, '<br />', '\n') === text;

This is true only for plain text. This makes the difference in engine

Why only Windows?

In

if (getOperatingSystem() === CONST.OS.WINDOWS) {
Object.keys(fontFamily).forEach((key) => {
fontFamily[key as keyof FontFamilyStyles] = fontFamily[key as keyof FontFamilyStyles].replace('Segoe UI Emoji', 'Windows Segoe UI Emoji');
});
}

We changed Segoe UI Emoji to Windows Segoe UI Emoji as the former does not support all emojis properly in the Windows platform.

But we haven't taken this consideration for singleFontFamily yet.

Due to this, we aren't passing Windows Segoe UI Emoji in system fonts which can be picked up by the TRenderEngine for rendering emojis.

That's why the bold text emojis default to Segoe UI Emoji, which breaks the emojis

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

Add Windows Segoe UI Emoji for Windows platform in systemFonts in TRenderEngineProvider. And maybe add Noto Color Emoji too.

Result

Screenshot 2023-12-12 at 12 12 25 AM

What alternative solutions did you explore? (Optional)

@jliexpensify
Copy link
Contributor

@kbecciv I'm on Staging v1.4.11-6 and can't repro this.

Some emojis change when there is a markdown in the message.

Can you define some? Are there specific emojis that always change? Does the change always occur?

Also, you're missing a step - there should be something like: 4. Click Edit to remove the * around the markdown and save the message.

@jliexpensify jliexpensify added the Needs Reproduction Reproducible steps needed label Dec 11, 2023
@it-education-md
Copy link

I can't repro this issue.

@shubham1206agra
Copy link
Contributor

@jliexpensify This can be repro on Windows platform only

@dragnoir
Copy link
Contributor

dragnoir commented Dec 13, 2023

Proposal

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

Windows emojis change when there is markdown in the message.

What is the root cause of that problem?

When there's any HTML (fragments/markdown..) on the comment, the TextCommentFragment will use RenderHTMLSource to render the element.

The TRenderEngineProvider from RenderHTMLSource use baseStyle to render the element inline style.

<TRenderEngineProvider
customHTMLElementModels={customHTMLElementModels}
baseStyle={styles.webViewStyles.baseFontStyle}
tagsStyles={styles.webViewStyles.tagStyles}
enableCSSInlineProcessing={false}
systemFonts={_.values(singleFontFamily)}

Until here we have 2 issues:

  • The font missing Windows Segoe UI Emoji, is not listed inside systemFonts props

image

  • The TRE will pick the first match as mentioned here
    If we provide ⇒ font-family: 'ExpensifyNeue-Regular, Windows Segoe UI Emoji, Noto Color Emoji',
    then the TRE will pick the 1st available font wih is ExpensifyNeue-Regular and will ignore the rest.

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

We can use CSS font-family: inherit; to solve the issue. Because RenderHTMLSource only use the 1st validate font passed throw fontFamily, we just use inherit.
inherit will grap the font from body tag oh the HTML.

1- Set the defualt font at body style

App/web/index.html

Lines 37 to 41 in 8bef4bb

body {
overflow: hidden;
overscroll-behavior: none;
touch-action: none;
}

to:

body {
	overflow: hidden;
	overscroll-behavior: none;
	touch-action: none;
	font-family: 'ExpensifyNeue-Regular, Windows Segoe UI Emoji, Segoe UI Emoji, Noto Color Emoji';
}

2- Add inherit to systemFonts here

// new emojiRenderfonts adding inherit
const  emojiRenderfonts  = ['inherit', 'Windows Segoe UI Emoji', ..._.values(singleFontFamily)];
....
systemFonts={_.values(emojiRenderfonts)}
}

3- Set new base style here

baseStyle={styles.webViewStyles.htmlRenderFontStyle}

where htmlRenderFontStyle is equal to:

htmlRenderFontStyle: {
	fontFamily:  'inherit',
	color:  theme.text,
	fontSize:  variables.fontSizeNormal,
	flex:  1,
	lineHeight:  variables.fontSizeNormalHeight,
	...writingDirection.ltr,
},

4- remove fontFamily: fontFamily.EXP_NEUE from tagStyles here

This solution with inherit will make the element style grap the font-family from the body tag. Where we can put all fonts we need without issues.

I tested only on Windows, we can add OS validation for the new style props oassed to TRenderEngineProvider where only this update apply on windows and we keep old value for the other platforms.

@dragnoir
Copy link
Contributor

@jliexpensify here's the full list of icons: https://emojipedia.org/microsoft
Those are icons supported by Windows. Windows OS style those icons using the default Windows Segoe UI Emoji font (there's two styles windows10 and windows 11). Same fonts are styled in another way in other platforms.

@melvin-bot melvin-bot bot added the Overdue label Dec 14, 2023
@jliexpensify jliexpensify removed the Needs Reproduction Reproducible steps needed label Dec 14, 2023
@jliexpensify
Copy link
Contributor

Thanks @dragnoir - @abdulrahuman5196 we have some proposals for review now

@melvin-bot melvin-bot bot removed the Overdue label Dec 14, 2023
@abdulrahuman5196
Copy link
Contributor

Hi, Will review today

@shubham1206agra
Copy link
Contributor

@abdulrahuman5196 Please hold this on #31389

@jliexpensify jliexpensify changed the title [$500] Web - Chat - Some emojis change when there is markdown in the message [HOLD] [$500] Web - Chat - Some emojis change when there is markdown in the message Dec 14, 2023
@jliexpensify jliexpensify changed the title [HOLD] [$500] Web - Chat - Some emojis change when there is markdown in the message [HOLD FOR #31389] [$500] Web - Chat - Some emojis change when there is markdown in the message Dec 14, 2023
@jliexpensify
Copy link
Contributor

Thanks @shubham1206agra - have updated the title. Let us know when the other GH is resolved, cheers!

@jliexpensify jliexpensify added Weekly KSv2 and removed Daily KSv2 labels Dec 18, 2023
Copy link

melvin-bot bot commented Dec 18, 2023

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

@mallenexpensify mallenexpensify changed the title [HOLD FOR #31389] [$500] Web - Chat - Some emojis change when there is markdown in the message [$500] Web - Chat - Some emojis change when there is markdown in the message Jun 10, 2024
@mallenexpensify mallenexpensify added Daily KSv2 and removed Monthly KSv2 labels Jun 10, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2024
@mallenexpensify mallenexpensify added the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label Jun 10, 2024
@mallenexpensify
Copy link
Contributor

Off hold, the below hit production a bit ago.
@jliexpensify @abdulrahuman5196 , please attempt reproduction. I'ave also added retest-weekly

Found this cuz I have this other emoji+markdown issue but I don't think it's directly related

Copy link

melvin-bot bot commented Jun 11, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jun 13, 2024
@jliexpensify
Copy link
Contributor

jliexpensify commented Jun 14, 2024

Hmm I can't seem to replicate this issue, all my emojis seem like they are correct:
image

What I do notice is that the emojis start off as large in the chat viewer, but as soon as you apply the first *, they change in size. I assume that's intended? EDIT: confirmed it is intended. So I can't replicate this then!

image

goes to

image

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 14, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

@jliexpensify, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Jun 18, 2024

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

@jliexpensify
Copy link
Contributor

Not overdue, waiting for @kbecciv to re-test/Applause to re-test weekly.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 19, 2024
@jliexpensify jliexpensify added Weekly KSv2 and removed Daily KSv2 labels Jun 24, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jun 24, 2024
@jliexpensify
Copy link
Contributor

Not overdue, waiting for a retest from Applause.

Copy link

melvin-bot bot commented Jun 25, 2024

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

1 similar comment
Copy link

melvin-bot bot commented Jul 2, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jul 2, 2024
@jliexpensify
Copy link
Contributor

Waiting for a retest

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

Copy link

melvin-bot bot commented Jul 9, 2024

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

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@jliexpensify
Copy link
Contributor

Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants