Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.33k stars 2.76k forks source link

[$250] mWeb - Chat - Entering spade and diamond makes spade symbol smaller in compose #47675

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 weeks 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!


Version Number: 9.0.22 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/4871799 Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Tap on a chat
  3. Enter ♡◇

Expected Result:

Entering spade and diamond must not make spade symbol smaller in compose

Actual Result:

Entering spade and diamond makes spade symbol smaller in compose

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/b665b265-c628-4559-80d1-1e9b19f74e7b

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aa453839e3c5dec4
  • Upwork Job ID: 1826736805004685557
  • Last Price Increase: 2024-09-12
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @johncschuster (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.

lanitochka17 commented 3 weeks ago

We think that this bug might be related to #vip- vsp

lanitochka17 commented 3 weeks ago

@johncschuster 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

tsa321 commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-20 03:50:33 UTC.

Proposal

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

Spade/ geometry shapes are not detected as emojis.

What is the root cause of that problem?

In here:

https://github.com/Expensify/App/blob/6334d36a049d01cfa9a445f3747125d79a00bc98/src/CONST.ts#L2335-L2337

https://github.com/Expensify/expensify-common/blob/e14dff64438b19fc9b6aa6937502b64870e0be86/lib/CONST.ts#L365

we don't include the geometrical shapes characters

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

We could add the geometrical shapes (\u{25A0}-\u{25FF}) charater in the regex. The regex could be:


/[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|[\u{25A0}-\u{25FF}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu

and must add the regex in this two lines:

https://github.com/Expensify/App/blob/6334d36a049d01cfa9a445f3747125d79a00bc98/src/CONST.ts#L2335-L2337

So the emojis could be:

EMOJIS: /[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{25A0}-\u{25FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu,

Also if we want to add other characters as well we could include it in the regex.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks 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 1 week ago

@johncschuster @mollfpr 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[bot] commented 1 week ago

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

johncschuster commented 1 week ago

@mollfpr can you review the proposal above when you get a moment?

wildan-m commented 3 days ago

Proposal

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

Entering a spade and diamond in chat makes the spade symbol smaller when composing messages in mWeb.

What is the root cause of that problem?

We are adding some unlisted symbol as emoji ♡ and excluding others ◇, This create inconsistencies.

https://github.com/Expensify/App/blob/6334d36a049d01cfa9a445f3747125d79a00bc98/src/CONST.ts#L2335-L2337

https://github.com/Expensify/expensify-common/blob/e14dff64438b19fc9b6aa6937502b64870e0be86/lib/CONST.ts#L365

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

Emoji and non-emoji text were handled differently. If the text consisted only of emoji, the font size and line height could vary, and hovering over an emoji would trigger a pop-up. Unknown symbols might be classified under Extended_Pictographic, leading to inconsistencies and unexpected outcomes.

Changing Extended_Pictographic to Emoji_Presentation in the regex is recommended to only treat typical emojis.

        // eslint-disable-next-line max-len, no-misleading-character-class
        EMOJI: /[\p{Emoji_Presentation}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu,
        // eslint-disable-next-line max-len, no-misleading-character-class
        EMOJIS: /[\p{Emoji_Presentation}](\u200D[\p{Emoji_Presentation}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu,

And also in expensify-common

Slack seems to treat characters in Emoji_Presentation differently than unknown symbols in Extended_Pictographic or other symbols.

Quick test:

https://github.com/user-attachments/assets/955699d7-346c-413e-a412-fa879a3c23d4

What alternative solutions did you explore? (Optional)

Only treat differently for character that matches registered emojis assets/emojis/common.ts

rough snippet:

// Step 1: Define the list of Unicode code points
const codePoints = [
    0x1F600, // 😀
    0x1F601, // 😁
    0x1F602, // 😂
    0x1F603, // 😃
    0x1F604, // 😄
    0x1F605, // 😅
    0x1F606, // 😆
    0x1F607, // 😇
    0x1F608, // 😈
    0x1F609, // 😉
    // Add more code points as needed
];

// Step 2: Convert each code point to its hexadecimal representation
const hexCodePoints = codePoints.map(cp => cp.toString(16).toUpperCase());

// Step 3: Create a regex pattern that matches any of these code points
const regexPattern = `\\u{${hexCodePoints.join('}|\\u{')}}`;

// Step 4: Create the regex object
const regex = new RegExp(regexPattern, 'u');

console.log(regex); // Output: /\u{1F600}|\u{1F601}|\u{1F602}|\u{1F603}|\u{1F604}|\u{1F605}|\u{1F606}|\u{1F607}|\u{1F608}|\u{1F609}/u
melvin-bot[bot] commented 1 day ago

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

johncschuster commented 1 day ago

Bumped in Slack for proposal review