[HOLD for payment 2023-06-21] [$1000] Web - Reaction Emoji- Some emoji as reactions are displayed incorrectly

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

Go to URL https://staging.new.expensify.com/
Navigate to a conversation that already has some messages.
Hover over any of the suggested emojis.
Verify there's a button to select an emoji to react the message.
Click on the button.
Verify the emoji picker opens.
Select emoji: :relaxed: or :elevator: or any flag.

Expected Result:

Each selected emoji should be displayed.

Actual Result:

Some emoji as reactions are displayed incorrectly.

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] Web / Chrome
[ ] MacOS / Desktop

Version Number: 1.3.11.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01149274d6e399baa4
Upwork Job ID: 1656821076429647872
Last Price Increase: 2023-05-24

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

therealsujitk commented 1 year ago

Proposal

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

The correct emoji is being displayed here, however the issue is it's not being displayed properly.

What is the root cause of that problem?

The cause here is very simple, the right font-family is not being used for displaying the emojis in reactions, this is required for web browsers.

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

To prevent this from happening in future, we could add the emoji font to all the font families in /src/styles/fontFamily/index.js as suggested by @rushatgabhane.

const fontFamily = {
    EXP_NEUE_ITALIC: 'ExpensifyNeue-Italic, Segoe UI Emoji, Noto Color Emoji',
    EXP_NEUE_BOLD: bold,
    EXP_NEUE: 'ExpensifyNeue-Regular, Segoe UI Emoji, Noto Color Emoji',
    EXP_NEW_KANSAS_MEDIUM: 'ExpensifyNewKansas-Medium, Segoe UI Emoji, Noto Color Emoji',
    EXP_NEW_KANSAS_MEDIUM_ITALIC: 'ExpensifyNewKansas-MediumItalic, Segoe UI Emoji, Noto Color Emoji',
    SYSTEM: 'System',
    MONOSPACE: 'ExpensifyMono-Regular, Segoe UI Emoji, Noto Color Emoji',
    MONOSPACE_ITALIC: 'ExpensifyMono-Regular, Segoe UI Emoji, Noto Color Emoji',
    MONOSPACE_BOLD: 'ExpensifyMono-Bold, Segoe UI Emoji, Noto Color Emoji',
    MONOSPACE_BOLD_ITALIC: 'ExpensifyMono-Bold, Segoe UI Emoji, Noto Color Emoji',
};

Screencast from 07-05-23 03:37:52 PM IST.webm

What alternative solutions did you explore? (Optional)

The correct font-family should be added to the inline styles of the reaction bubble and the tooltip text.

emojiReactionBubbleText: {
    fontFamily: fontFamily.EMOJI_TEXT_FONT,   // This was missing
    textAlignVertical: 'center',
},

...

reactionEmojiTitle: {
    fontFamily: fontFamily.EMOJI_TEXT_FONT,   // This was missing
    fontSize: variables.iconSizeLarge,
    lineHeight: variables.iconSizeXLarge,
},

mallenexpensify commented 1 year ago

I'm unable to reproduce, the reaction emoji looks correct/accurate. @kbecciv @therealsujitk is there something I'm not doing correctly? I'm on Mac - Chrome, latest staging instance.

@kbecciv can you please add a screen image or video of what you're seeing to the OP?

therealsujitk commented 1 year ago

@mallenexpensify I believe it has to do with the emoji's available in system fonts, do you have a windows or linux PC/VM you can test this on? I tested it on Ubuntu 22.04. Since the current font family doesn't take care of emojis, it defaults/falls-back to the system font. In your case on mac those emoijs happen to be available.

mallenexpensify commented 1 year ago

ah... that makes sense. Thanks @therealsujitk .

I was able to test, is this what you're seeing?

I wonder if this is more of an issue with all emoji and how they're displayed differently. I'm pretty sure we've discussed them before, might have to do some digging

therealsujitk commented 1 year ago

@mallenexpensify yup, and to fix this we simply have to set the font family as mentioned in my proposal.

Also, yeah it was probably discussed before as it is also mention in the comments of your code here. I'm guessing you guys just forgot to apply it to the reactions.

https://github.com/Expensify/App/blob/45c3212061983394e0919839248512874c2af83f/src/styles/fontFamily/emoji/index.js#L1-L5

therealsujitk commented 1 year ago

I found another similar issue, look at how the emoji is displayed in the message under Chats on the left panel. This should also be fixed in the same way.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01149274d6e399baa4

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

mallenexpensify commented 1 year ago

Thanks @therealsujitk , I'm unfamiliar with fonts, their functionality, libraries and how they work across platforms/OSs. I added the internal label to get get eyes from a C+. Rushat, what cha think?

rushatgabhane commented 1 year ago

@mallenexpensify i beileve @therealsujitk is right.

When we go to any webpage, the browser travel downs a list of font-family values until it finds a font available on the system it is running on or downloads them. Because we haven't specified a font, this list is empty and it defaults to some system font.

rushatgabhane commented 1 year ago

@therealsujitk but a better fix would be to apply this to all html, right? This way, this kind of bug will never happen.

html {
    font-family: "exfy font"
}

therealsujitk commented 1 year ago

@rushatgabhane normally that would work for web, however react native is more strict about it.

Basically in react native we have to set the fontFamily for every <Text /> component we use. The best way to do this is to create your own component overriding react native's text component and setting some default properties such as the fontFamily. This is what is done here.

Now similar to what you said, we could set the emoji font in this component so it will be applied everywhere, however I believe this was not done so that unnecessary fonts are not loaded especially on native platforms giving it a slight performance improvement.

rushatgabhane commented 1 year ago

however I believe this was not done so that unnecessary fonts are not loaded especially on native platforms giving it a slight performance improvement

i would prefer correct font 1000 times over a slight performance improvement

font files are just a few KBs in size. the performance improvement shouldn't be measurable

therealsujitk commented 1 year ago

@rushatgabhane sure we can change the base font, can you just confirm with the author of the code? There could be some other reason he/she made it this way.

rushatgabhane commented 1 year ago

sure, can you send a new proposal for it? it'll be easier for me to link to it then

therealsujitk commented 1 year ago

@rushatgabhane I've updated my proposal, kindly take a look. In the code I've written I believe we can apply the emoji font everywhere only on web therefore keeping the performance improvement on native.

rushatgabhane commented 1 year ago

@therealsujitk before making any performance claims, you need to measure the numbers.

@therealsujitk let's keep it simple and apply for all platforms! or we'll wonder why the font is on web but not native

code readability >> performance in this case

therealsujitk commented 1 year ago

@rushatgabhane alright :+1:

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @aldo-expensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

mallenexpensify commented 1 year ago

@aldo-expensify , can you take a look at the above? I'm unfamiliar with how we're managing fonts cross-platform and I'm wanting to do any sort of 'quick fix' that isn't aligned with any guidelines we might have for emoji (if you know where any guidelines are, please share a link, I can look into getting them added to our testing guidelines).

aldo-expensify commented 1 year ago

I'm a bit confused about what the problem is... the actual results section is not very descriptive and there is not screenshot/video.

For me this is working well in staging:

https://github.com/Expensify/App/assets/87341702/992755bb-e9eb-4dab-9d08-38f5c0dd31db

Update: ahh, it is this right?

https://github.com/Expensify/App/issues/18531#issuecomment-1541129523

aldo-expensify commented 1 year ago

@aldo-expensify , can you take a look at the above? I'm unfamiliar with how we're managing fonts cross-platform and I'm wanting to do any sort of 'quick fix' that isn't aligned with any guidelines we might have for emoji (if you know where any guidelines are, please share a link, I can look into getting them added to our testing guidelines).

Sorry, I don't know of any guidelines, I just know that we have seen this problem in the past.

The proposed solution looks very clean to me. I wonder if it would solve this issue for all platforms? 🤷

@stitesExpensify , emoji expert, can I get your opinion here? 🙏

stitesExpensify commented 1 year ago

@therealsujitk what font family are you referring to specifically? We ran into a similar issue here and decided to do nothing because adding an entire font for such a small segment of users was not worth it.

mallenexpensify commented 1 year ago

@therealsujitk what font family are you referring to specifically?

When you see this @therealsujitk (pinging to remove overdue too)

therealsujitk commented 1 year ago

@stitesExpensify two font families are used to render emojis properly in the EmojiPicker i.e. Segoe UI Emoji, Noto Color Emoji. You can refer to src/styles/fontFamily/emoji/.

I initially suggested adding them to the required components but there was a possibility that this issue could arise again with some other component.

We talked about adding these font families everywhere, I don't believe doing this on the web would cause any performance issues as the font should get downloaded only once, correct me if I'm wrong.

These font families were not added for native platforms because they weren't required, but it wouldn't hurt to add them there too.

We ran into a similar issue here and...

As for this issue, I'm not sure what it was supposed to be fixing because the emojis in the picker render properly for me, Ubuntu 22.04. Did Ubuntu add these fonts sometime over the last 6 months?

I didn't realise we used these fonts from the system, I assumed they were downloaded on the browser. In that case there should definitely not be any performance issue (I mentioned above).

melvin-bot[bot] commented 1 year ago

@mallenexpensify @rushatgabhane @aldo-expensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

mallenexpensify commented 1 year ago

@stitesExpensify can you chime in, I'm not well-versed in the emoji world so I'm unable to propose what might be the best path forward here. Thx

aldo-expensify commented 1 year ago

Sorry, but I'm also pretty ignorant when it comes to fonts, I think we can reduce the problem to this:

Does solving this require downloading extra fonts? Do nothing, we would require all users to download extra fonts just to fix it for some specific OS, and this problem will keep coming back.

Is the fix just adding a font family correctly in styles.js (see proposal) without downloading extra font files? If this is the case, I think its fine to proceed.

Also, is this issue only happening in Web (desktop/mobile)? or does it also happen in native? If it is only in web, what @rushatgabhane said here makes sense to me: https://github.com/Expensify/App/issues/18531#issuecomment-1545191484

therealsujitk commented 1 year ago

Does solving this require downloading extra fonts? Do nothing, we would require all users to download extra fonts just to fix it for some specific OS, and this problem will keep coming back.

Nope, nothing of that sort.

Is the fix just adding a font family correctly in styles.js (see https://github.com/Expensify/App/issues/18531#issuecomment-1537377165) without downloading extra font files? If this is the case, I think its fine to proceed.

Yes, this fixes it.

Also, is this issue only happening in Web (desktop/mobile)? or does it also happen in native? If it is only in web, what @rushatgabhane said here makes sense to me: https://github.com/Expensify/App/issues/18531#issuecomment-1545191484

This issue is only for desktop yes, however react native doesn't work like that. I've explained it here. Hence the reason we'll have to do it through JS. We could also make use of index.native.js or index.web.js if you'd like so we apply these only to web.

aldo-expensify commented 1 year ago

Also, is this issue only happening in Web (desktop/mobile)? or does it also happen in native? If it is only in web, what @rushatgabhane said here makes sense to me: https://github.com/Expensify/App/issues/18531#issuecomment-1545191484

This issue is only for desktop yes, however react native doesn't work like that. I've explained it https://github.com/Expensify/App/issues/18531#issuecomment-1545217678. Hence the reason we'll have to do it through JS. We could also make use of index.native.js or index.web.js if you'd like so we apply these only to web.

Yeah, I saw that, sorry about asking again, I thought that there was a way to define <html blabla="" for web, but after searching around I think we can't. I see you point now :P

@rushatgabhane I think we can proceed with https://github.com/Expensify/App/issues/18531#issuecomment-1537377165, right?

@therealsujitk could you summarize the pros/cons of the main proposal here vs the alternative solution? Do you both (@rushatgabhane and you) think that the best is the main proposed solution?

therealsujitk commented 1 year ago

Main Proposal

PRO: This approach would prevent this issue from occurring again. Ever.

I was initially worried of there being performance issues adding fonts unnecessarily to components that don't need them, however after some research I found out that this impact is negligible.

Loading and Caching: React Native uses native font handling mechanisms to load and cache fonts. When you add a new font family, React Native will load that specific font only when it's required by a component. The font will then be cached, so subsequent uses of the same font will not incur additional loading time.

Minimal Memory Impact: The additional font family information stored in memory is minimal, as it only includes the font file data and related metadata. Unless you're using a large number of different font families simultaneously, the impact on memory usage should be negligible.

Render Performance: The rendering performance impact of adding an additional font family is typically negligible. React Native renders components efficiently, and the time taken to render a specific component with a different font family should not be significant, especially if it's only a few components.

Alternative Proposal

PRO: That negligible performance impact won't be there.
CON: There's a possibility that a component will come up in future that has the same issue because of the missing font family. Example, I found another occurrence of this issue.

I personally think we should go with the main proposal, it's up to you guys if you want me to add this only for web or for all platforms. I personally like the web option (index.website.js), but there still won't be any impact on performance adding it for all platforms.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @aldo-expensify is eligible for the External assigner, not assigning anyone new.

aldo-expensify commented 1 year ago

Thanks for the summary.

@rushatgabhane thoughts? do you approved the proposal?

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @aldo-expensify is eligible for the External assigner, not assigning anyone new.

aldo-expensify commented 1 year ago

Friendly bump @rushatgabhane https://github.com/Expensify/App/issues/18531#issuecomment-1561582829

rushatgabhane commented 1 year ago

@therealsujitk can we add it all platforms? I'm a bit lost with the comments.

Can you please repost a proposal that adds it to all platforms

therealsujitk commented 1 year ago

@rushatgabhane Yeah, there's no problem adding it to all platforms.

My proposal's main solution already adds the fonts to all platforms.

Since the code is useless on other platforms, I gave you an option not to add it everywhere unnecessarily (I recommend this method as it's currently done for emojiFont, and it's easy to understand).

`index.js`

import bold from '../bold'; 

const fontFamily = {
    EXP_NEUE_ITALIC: 'ExpensifyNeue-Italic, Segoe UI Emoji, Noto Color Emoji',
    EXP_NEUE_BOLD: bold,
    EXP_NEUE: 'ExpensifyNeue-Regular, Segoe UI Emoji, Noto Color Emoji',
    EXP_NEW_KANSAS_MEDIUM: 'ExpensifyNewKansas-Medium, Segoe UI Emoji, Noto Color Emoji',
    EXP_NEW_KANSAS_MEDIUM_ITALIC: 'ExpensifyNewKansas-MediumItalic, Segoe UI Emoji, Noto Color Emoji',
    SYSTEM: 'System',
    MONOSPACE: 'ExpensifyMono-Regular, Segoe UI Emoji, Noto Color Emoji',
    MONOSPACE_ITALIC: 'ExpensifyMono-Regular, Segoe UI Emoji, Noto Color Emoji',
    MONOSPACE_BOLD: 'ExpensifyMono-Bold, Segoe UI Emoji, Noto Color Emoji',
    MONOSPACE_BOLD_ITALIC: 'ExpensifyMono-Bold, Segoe UI Emoji, Noto Color Emoji',
};

export default fontFamily;

`index.native.js`

import bold from '../bold'; 

 const fontFamily = { 
     EXP_NEUE_ITALIC: 'ExpensifyNeue-Italic', 
     EXP_NEUE_BOLD: bold, 
     EXP_NEUE: 'ExpensifyNeue-Regular', 
     EXP_NEW_KANSAS_MEDIUM: 'ExpensifyNewKansas-Medium', 
     EXP_NEW_KANSAS_MEDIUM_ITALIC: 'ExpensifyNewKansas-MediumItalic', 
     SYSTEM: 'System', 
     MONOSPACE: 'ExpensifyMono-Regular', 
     MONOSPACE_ITALIC: 'ExpensifyMono-Regular', 
     MONOSPACE_BOLD: 'ExpensifyMono-Bold', 
     MONOSPACE_BOLD_ITALIC: 'ExpensifyMono-Bold',
 }; 

 export default fontFamily;

therealsujitk commented 1 year ago

Also, for some reason the bold folder is in styles, I'd like to move it inside fontFamily if that's ok.

melvin-bot[bot] commented 1 year ago

@mallenexpensify @rushatgabhane @aldo-expensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@mallenexpensify, @rushatgabhane, @aldo-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

aldo-expensify commented 1 year ago

No overdue, I'm giving some time for @rushatgabhane to review.

rushatgabhane commented 1 year ago

@aldo-expensify i like @therealsujitk's proposal here - https://github.com/Expensify/App/issues/18531#issuecomment-1537377165

What do you think?

therealsujitk commented 1 year ago

@rushatgabhane so this way is a no? If it's alright can I know why exactly? I'll do whatever you're comfortable with in the end.

Expensify / App