[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [HOLD for payment 2024-06-10] [HOLD on #4114/#17767] [$500] iOS - Chat - When moving to another line, the word and emoji are close to each other.

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:

Launch the app
Log in with any account
In the compose box enter message with have lengthier letters like (j) and emoji

Expected Result:

Text entered with letters and emojis on the next line must not be close to each other

Actual Result:

When moving to another line, the word and emoji are close to each other

Workaround:

Unknown

Platforms:

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

[x] iOS / native

Version Number: 1.2.57.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

https://user-images.githubusercontent.com/93399543/213815233-c2908d14-2042-4571-9196-09fbb3de6bd2.mp4

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/~01ebb233eae82dffea
Upwork Job ID: 1622658479891505152
Last Price Increase: 2024-03-05

Issue Owner

Current Issue Owner: @twisterdotcom

twisterdotcom commented 1 year ago

Sorry for the delay, I was getting my Android phone working as well to test it there too.

My first few of these, so taking the template from StackOverflowTeams: https://stackoverflowteams.com/c/expensify/questions/14418

[ ] The "bug" is actually a bug.
[x] The bug is not a duplicate report of an existing GH
[ ] The bug is reproducible, following the reproduction steps.
[ ] The GH template is filled out as fully as possible
[x] The GH was created by an Expensify employee or a QA tester.
[x] The bug is an /App issue

So, as we can see from my checklist, I don't see this still occurring.

iOS v12-58.4

https://user-images.githubusercontent.com/9133401/214638575-051e0d0d-0d2e-49f1-b753-12de6e56e6a8.MP4

Eugh, and my Android device still isn't working. Okay, in the meantime - I think this has self-resolved somehow. Could you confirm @kbecciv?

twisterdotcom commented 1 year ago

Okay, I tested this again today and I think this is a non-issue/resolved. Closing it.

kbecciv commented 1 year ago

@twisterdotcom Issue is still reproduced with build 1.2.64.3 Please type the message with lengthier letters like (j) and emoji, so you will be able to reproduce it.

repro ios 0302 (2)

MelvinBot commented 1 year ago

Current assignee @twisterdotcom is eligible for the Bug assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

twisterdotcom commented 1 year ago

Okay, you're right? Yes, if it's a lower case letter than typically falls below the bottom line, it interferes ever so slightly with the next line, especially emoji. So q, j, g, y, and p I think: IMG_A7E855E2C6FE-1

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01ebb233eae82dffea

MelvinBot commented 1 year ago

Current assignee @twisterdotcom 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 - @eVoloshchak (External)

MelvinBot commented 1 year ago

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

fedirjh commented 1 year ago

Proposal

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

The issue at hand is related to formatting and spacing when moving to a new line. The problem is that words and emojis are too close to each other. The goal is to find a solution that separates the word and emoji, creating clear and visually appealing formatting.

What is the root cause of that problem?

In iOS, it render emojis with line height equals to the text line height (20px), resulting in zero spacing between lines. On the other hand, Chrome renders emojis at 19.5 pixels, causing a 0.5 pixel spacing between lines.

IOS	WEB

As observed, emojis in iOS appear larger than on the web, occupying the full line height in iOS. In contrast, on the web, there is a spacing at the top and bottom, indicating that emojis do not fully cover the line height.

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

We should use different lineHeight for text that contains emojis

Add the following function to EmojiUtils:

function containsEmoji(text) {
    return CONST.REGEX.EMOJIS.test(text);
}

Add this line in ReportActionItemFragment

    EmojiUtils.containsEmoji(text) ? styles.textEmojis : undefined,

Add this to Styles

textEmojis: {
    lineHeight: variables.emojiLineHeight, // we can define new variable textEmojiLineHeight
},

Optional : Add new textEmojiLineHeight to variables : We should discuss what value we should set.

    textEmojiLineHeight: getValueUsingPixelRatio(24, 28),

What alternative solutions did you explore? (Optional)

We can increase text line Height with 2 or 4 px. Here is the difference with different line Height in IOS

Actual 20px	22px	24px

C+ Questions

I think different line heights for different lines (that contain emojis and not) aren't going to look good.

We are actually using this approach with onlyEmojisText , it has a different fontSize and lineHeight . to make it consistent we can increase the text LineHeight with 2 or 4 px .

Why are emojis bigger than text? Why is this happening only on iOS?

The emoji rendering is not consistent on all platform , there is a small difference between IOS and Web

eVoloshchak commented 1 year ago

@fedirjh, thank you for your proposal, I think you might be right about the wrong lineHeight. However, I think different line heights for different lines (that contain emojis and not) aren't going to look good. Why are emojis bigger than text? Why is this happening only on iOS?

Also, could you please update your proposal following the proposal template here?

fedirjh commented 1 year ago

@eVoloshchak updated !!

MelvinBot commented 1 year ago

@eVoloshchak @twisterdotcom @ctkochan22 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!

MelvinBot commented 1 year ago

@eVoloshchak, @twisterdotcom, @ctkochan22 Huh... This is 4 days overdue. Who can take care of this?

MelvinBot commented 1 year ago

@eVoloshchak, @twisterdotcom, @ctkochan22 Eep! 4 days overdue now. Issues have feelings too...

twisterdotcom commented 1 year ago

Are we going with @fedirjh's proposal @eVoloshchak and @ctkochan22? If so, let's get them assigned!

redstar504 commented 1 year ago

Proposal

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

In a multi-line comment, emojis are overlapping character descenders of the previous line on iOS.

What is the root cause of that problem?

Emojis rendered with the ExpensifyNeue font on iOS are not respecting the line height of regular baseline characters from the same font family. The inclusion of them seems to shift the baseline up resulting in them overlapping the descenders of the previous line.

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

Split text comments into individual characters and apply the EMOJI_TEXT_FONT selectively to emoji characters, eg:

{_.map([...text], char => (char.match(/[^ -~]+/g)
  ? <Text family="EMOJI_TEXT_FONT">{char}</Text>
  : char))}

Set the EMOJI_TEXT_FONT to System on iOS, so emoji characters respect the declared font size and line height relative to characters in the ExpensifyNeue font family. Create a new file src/styles/fontFamily/emoji/index.ios.js:

const emojiFont = 'System';

export default emojiFont;

eVoloshchak commented 1 year ago

@twisterdotcom, I think we should HOLD until https://github.com/Expensify/App/pull/15088 is merged. It resolves https://github.com/Expensify/App/issues/4114 and adds logic for rendering emojis with styles different from text (which is a mix of @fedirjh's and @redstar504's proposals)

twisterdotcom commented 1 year ago

Cool, sounds good. I'll add a HOLD.

ctkochan22 commented 1 year ago

Holding makes sense

ctkochan22 commented 1 year ago

Still on hold: https://github.com/Expensify/App/pull/15088

twisterdotcom commented 1 year ago

Still on hold: https://github.com/Expensify/App/pull/15088

ctkochan22 commented 1 year ago

Still on hold I believe. @twisterdotcom please re-apply the engineering label for re-assignment when off hold (or before, just a suggestion)

twisterdotcom commented 1 year ago

Still held.

twisterdotcom commented 1 year ago

Held on https://github.com/Expensify/App/pull/15611

eVoloshchak commented 1 year ago

Not overdue, holding for https://github.com/Expensify/App/pull/15611, which is in review

twisterdotcom commented 1 year ago

Still held on https://github.com/Expensify/App/pull/15611

twisterdotcom commented 1 year ago

Making Monthly because that issue is moving slowly.

twisterdotcom commented 1 year ago

Still held but I do see movement.

twisterdotcom commented 1 year ago

FB addressed some comments here: https://github.com/facebook/hermes/pull/968#pullrequestreview-1458032263

twisterdotcom commented 1 year ago

Last update from @fbmal7 here: https://github.com/facebook/hermes/pull/968#issuecomment-1599335001

twisterdotcom commented 1 year ago

Same as above.

twisterdotcom commented 1 year ago

Waiting for RN 0.73: https://github.com/facebook/hermes/pull/968#issuecomment-1633011552

twisterdotcom commented 1 year ago

Okay, @eVoloshchak can we unhold this now?

eVoloshchak commented 1 year ago

@twisterdotcom, not yet, https://github.com/facebook/hermes/pull/968 will be included in RN 0.73 (it isn't out yet)

eVoloshchak commented 11 months ago

Not overdue, still waiting for RN 0.73

twisterdotcom commented 10 months ago

Still held on https://github.com/Expensify/App/issues/31381

twisterdotcom commented 9 months ago

Still held.

twisterdotcom commented 8 months ago

Oh wow, not held anymore!

melvin-bot[bot] commented 8 months ago

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

twisterdotcom commented 8 months ago

Issue still exists. WhatsApp does handle it, so let's still try it but... gonna move to vipvsb and make it FUTURE and set it as $500 again.

https://github.com/Expensify/App/assets/9133401/6152a831-5de7-479d-bee6-e0822ed8c236

melvin-bot[bot] commented 8 months ago

Upwork job price has been updated to $500

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

twisterdotcom commented 7 months ago

Discussing closing this here: https://expensify.slack.com/archives/C01SKUP7QR0/p1709660985424209

twisterdotcom commented 7 months ago

@fabOnReact - we held this previously on https://github.com/Expensify/App/issues/31381 thinking it would help, but it didn't. @cead22 tells me that maybe https://github.com/Expensify/App/issues/17767 will fix it. @roryabraham says maybe #4114 might. Do you think it might? If so, I'll HOLD it again.

fabOnReact commented 7 months ago

Thanks. I'll work on this next week.

melvin-bot[bot] commented 7 months ago

📣 @fabOnReact! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

Expensify / App