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

[$250] Android-Chat-The top of emojis are cut off on applying quote markdown #47899

Open
1 of 6 tasks
IuliiaHerets opened this issue Aug 23, 2024 · 23 comments
Open
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 Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 23, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.24
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4884616
Issue reported by: Applause Internal Team

Action Performed:

  1. Laugh app
  2. Tap on a chat
  3. Enter > 😁😄😃😡😡
  4. Send the message

Expected Result:

The top of emojis must not be cut off on applying quote markdown.

Actual Result:

The top of emojis are cut off on applying quote markdown.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6579849_1724375588392.Screenrecorder-2024-08-23-06-37-31-905_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01550e0f7b3eac50c6
  • Upwork Job ID: 1829238843117327522
  • Last Price Increase: 2024-08-29
  • Automatic offers:
    • hoangzinh | Reviewer | 103831336
Issue OwnerCurrent Issue Owner: @hoangzinh
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 23, 2024
Copy link

melvin-bot bot commented Aug 23, 2024

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@IuliiaHerets
Copy link
Author

@strepanier03 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@melvin-bot melvin-bot bot added the Overdue label Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

@daledah
Copy link
Contributor

daledah commented Aug 28, 2024

Edited by proposal-police: This proposal was edited at 2024-08-28 10:22:02 UTC.

Proposal

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

The top of emojis are cut off on applying quote markdown.

What is the root cause of that problem?

The emoji's lineHeight for comments containing only emojis are 35:

const htmlContent = containsOnlyEmojis ? Str.replaceAll(htmlWithDeletedTag, '<emoji>', '<emoji islarge>') : htmlWithDeletedTag;

const style = 'islarge' in tnode.attributes ? styles.onlyEmojisText : {};

App/src/styles/index.ts

Lines 1704 to 1707 in f15a1e4

onlyEmojisText: {
fontSize: variables.fontSizeOnlyEmojis,
lineHeight: variables.fontSizeOnlyEmojisHeight,
},

fontSizeOnlyEmojisHeight: 35,

While blockquote uses the default lineHeight for HTML renderers, which is smaller than 35:

baseStyle={styles.webViewStyles.baseFontStyle}

App/src/styles/index.ts

Lines 235 to 240 in f15a1e4

baseFontStyle: {
color: theme.text,
fontSize: variables.fontSizeNormal,
...FontUtils.fontFamily.platform.EXP_NEUE,
flex: 1,
lineHeight: variables.fontSizeNormalHeight,

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

This issue already happened with EditedRenderer. The solution was to denote these contains-only-emoji edited tags as islarge and use lineHeight: 35 specifically for them:

const editedTag = fragment?.isEdited ? `<edited ${styleAsDeleted ? 'deleted' : ''} ${containsOnlyEmojis ? 'islarge' : ''}></edited>` : '';

const isLarge = !!(tnode.attributes.islarge !== undefined);

<Text style={isLarge && styles.onlyEmojisTextLineHeight}>

So in this case:

  1. Add islarge attribute to contains-only-emoji blockquote following what we've done with emojis:

const htmlContent = containsOnlyEmojis ? Str.replaceAll(htmlWithDeletedTag, '<emoji>', '<emoji islarge>') : htmlWithDeletedTag;

let htmlContent = htmlWithDeletedTag;
if (containsOnlyEmojis) {
    htmlContent = Str.replaceAll(htmlContent, '<emoji>', '<emoji islarge>');
    htmlContent = Str.replaceAll(htmlContent, '<blockquote>', '<blockquote islarge>');
}
  1. Now let's customize blockquote's style based on islarge attribute above. In the customHTMLElementModels here, define blockquote custom renderer and customize its style using getMixedUAStyles:
blockquote: HTMLElementModel.fromCustomModel({
    tagName: 'blockquote',
    contentModel: HTMLContentModel.block,
    getMixedUAStyles: (tnode) => {
        if (tnode.attributes.islarge === undefined) {
            return;
        }
        return styles.onlyEmojisTextLineHeight;
    },
})
  1. As this problem only occurs on Android, we might need to define platform-specific onlyEmojisBlockquoteLineHeight styles.

Copy link

melvin-bot bot commented Aug 28, 2024

@strepanier03 Huh... This is 4 days overdue. Who can take care of this?

@strepanier03 strepanier03 added the External Added to denote the issue can be worked on by a contributor label Aug 29, 2024
@melvin-bot melvin-bot bot changed the title Android-Chat-The top of emojis are cut off on applying quote markdown [$250] Android-Chat-The top of emojis are cut off on applying quote markdown Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01550e0f7b3eac50c6

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 29, 2024
@strepanier03
Copy link
Contributor

Easy to reproduce with any combo of emoji.

@hoangzinh
Copy link
Contributor

Thanks for proposal @daledah. Can you elaborate on why the issue is only reproducible on Android?

Copy link

melvin-bot bot commented Sep 2, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@hoangzinh
Copy link
Contributor

Hi @daledah. When you have time, can you check my comment here? Thank you

@melvin-bot melvin-bot bot removed the Overdue label Sep 3, 2024
@daledah
Copy link
Contributor

daledah commented Sep 4, 2024

@hoangzinh I think it's a problem with react-native itself. You can check out the demo here: https://snack.expo.dev/b1h4FVhn3ZkBsS6XVQJg6.

@hoangzinh
Copy link
Contributor

hoangzinh commented Sep 5, 2024

Thanks for updates @daledah. I'm thinking about the expectation in this case. When users type emojis with blockquote, should we display emojis in a large style or a normal style?

@hoangzinh
Copy link
Contributor

@Expensify/design Do you have any advice for us in this case? The question is: When users type emojis with blockquote, should we display emojis in a large style or a normal style? Thank you.

Large style Normal style
Screenshot 2024-09-05 at 19 05 50 Screenshot 2024-09-05 at 19 05 34

@shawnborton
Copy link
Contributor

I think we'd follow the same styles that regular messages have. So whatever we do for normal emoji-only messages, we'd do here for blockquotes as well.

@hoangzinh
Copy link
Contributor

Thanks @shawnborton

@hoangzinh
Copy link
Contributor

@daledah's proposal looks good to me

Link to proposal #47899 (comment)

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 5, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 5, 2024
Copy link

melvin-bot bot commented Sep 5, 2024

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Sep 5, 2024

📣 @daledah You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

Copy link

melvin-bot bot commented Sep 6, 2024

@hoangzinh @strepanier03 @aldo-expensify @daledah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 7, 2024
@daledah
Copy link
Contributor

daledah commented Sep 7, 2024

@hoangzinh PR is ready.

@hoangzinh
Copy link
Contributor

PR is deployed to Prod. Awaiting for payment

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 Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests

6 participants