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] Chat - Emoji codes overlapping for some emoji suggestions #41779

Closed
1 of 6 tasks
izarutskaya opened this issue May 7, 2024 · 46 comments
Closed
1 of 6 tasks

[$250] Chat - Emoji codes overlapping for some emoji suggestions #41779

izarutskaya opened this issue May 7, 2024 · 46 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 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

Comments

@izarutskaya
Copy link

izarutskaya commented May 7, 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: 1.4.71-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+1117ck@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Launch ND app & log in
  2. Navigate to any chat
  3. Type ":tr" or ":arr" ::hea" to trigger emoji suggestions
  4. Scroll down & observe emoji codes overlapping

Expected Result:

Emojis codes should should be clearly shown in emoji suggestions list & all suggestions should be displayed in the list

Actual Result:

  1. Emoji codes overlapping for some emoji suggestions (for eg - 💓 & 💞, ⏭️ & ⏮️, 🔃 & 🔄)
  2. Overlapped emoji suggestion missing from list

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

Bug6473794_1715090200917.az_recorder_20240507_094307.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0121813b34b025e1d6
  • Upwork Job ID: 1788031980674232320
  • Last Price Increase: 2024-06-05
  • Automatic offers:
    • DylanDylann | Contributor | 102518894
Issue OwnerCurrent Issue Owner: @c3024
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 7, 2024
Copy link

melvin-bot bot commented May 7, 2024

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

@izarutskaya
Copy link
Author

We think this issue might be related to the #vip-vsb.

@slafortune slafortune added the External Added to denote the issue can be worked on by a contributor label May 8, 2024
@melvin-bot melvin-bot bot changed the title Chat - Emoji codes overlapping for some emoji suggestions [$250] Chat - Emoji codes overlapping for some emoji suggestions May 8, 2024
Copy link

melvin-bot bot commented May 8, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0121813b34b025e1d6

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

melvin-bot bot commented May 8, 2024

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

@dominictb
Copy link
Contributor

dominictb commented May 9, 2024

Proposal

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

Emoji codes overlapping for some emoji suggestions (for eg - 💓 & 💞, ⏭️ & ⏮️, 🔃 & 🔄)

What is the root cause of that problem?

The estimated width being set here

width: (isLargeScreenWidth ? windowWidth - variables.sideBarWidth : windowWidth) - CONST.CHAT_FOOTER_HORIZONTAL_PADDING,
is wrong.

The autoCompleteSuggestionsContainer here also has a border

borderWidth: 1,

Which takes space (2), so the width of the list will need to be subtracted 2 to get the correct value.

This mismatch can be confirmed by adding logs in onLayout of the FlashList and compare the actual width with the width of the estimatedListSize.

When the estimatedListSize width is wrong, it leads to FlashList calculating the element position incorrectly, causing items to shuffle around and overlap.

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

When calculating the estimatedListSize in

width: (isLargeScreenWidth ? windowWidth - variables.sideBarWidth : windowWidth) - CONST.CHAT_FOOTER_HORIZONTAL_PADDING,

We need to subtract the total border width (2 * borderWidth) from the estimated width

width: (isLargeScreenWidth ? windowWidth - variables.sideBarWidth : windowWidth) - CONST.CHAT_FOOTER_HORIZONTAL_PADDING - 2,

We should set the border with of the autosuggestion container to a constant and reuse in container styles, and in the calculation above, so later if we change the border width, the estimated width of the list will be calculated correctly.

What alternative solutions did you explore? (Optional)

Another observation:
Although it doesn't yet cause any issues, the height being calculated here is also wrong, it doesn't take into account the borders same as width, and also it's calculating the height of all elements, while it should calculate the heights of visible elements only
Screenshot 2024-05-09 at 6 10 46 PM

It should be calculated using the measureHeightOfSuggestionRows which gets height of visible part. We can optionally clean this up too.

@c3024
Copy link
Contributor

c3024 commented May 10, 2024

This is not reproducible on main.

emojiOverlap.mp4

@dominictb
Copy link
Contributor

dominictb commented May 13, 2024

@c3024 I just tried again on latest main and it's definitely still reproducible. Could you try with the chat with Concierge, for a fresh new account? Also do try on a physical device too.

ImportedPhoto_1715618133694

@melvin-bot melvin-bot bot added the Overdue label May 13, 2024
@c3024
Copy link
Contributor

c3024 commented May 13, 2024

Thanks for the bump. I'll check.

@melvin-bot melvin-bot bot removed the Overdue label May 13, 2024
@c3024
Copy link
Contributor

c3024 commented May 14, 2024

Is this specific to some devices? This is on a POCO M4 for a new account. It looks fine here. When the popup first appears the letters overlap but after it fully opens I don't see the bug mentioned.

Screenrecorder-2024-05-14-12-28-48-711.mp4

@dominictb
Copy link
Contributor

@c3024 I tested on a Samsung S22 and can always reproduce it

When the popup first appears the letters overlap

This is also due to the same root cause and needs to be fixed, if you apply my solution here, you'll see that it no longer overlaps when first appearing.

Perhaps the list item size being calculated incorrectly will cause slightly different problems in different device, but you can see that the issue is happening in your device When the popup first appears the letters overlap

Copy link

melvin-bot bot commented May 15, 2024

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

Copy link

melvin-bot bot commented May 17, 2024

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

@melvin-bot melvin-bot bot added the Overdue label May 17, 2024
@slafortune
Copy link
Contributor

I had been able to reproduce this, I also have a Samsung - Galaxy Z Flip 4 - I can't reproduce this any more. I did have a mobile update yesterday.

@dominictb are you still able to reproduce this?

@c3024
Copy link
Contributor

c3024 commented May 18, 2024

Let me check on another physical device as well.

@melvin-bot melvin-bot bot removed the Overdue label May 18, 2024
@c3024
Copy link
Contributor

c3024 commented May 18, 2024

@dominictb

I tested again with the change you suggested.

I still see this

When the popup first appears the letters overlap

widthOfMentionContainer-compressed.mp4

@dominictb
Copy link
Contributor

When the popup first appears the letters overlap

@c3024 Can you point out a screenshot where you see it's overlapping? I don't see it overlapping in the video, just that when the list rendering it will show a bit of white empty space, which is normal.

Meanwhile, in your earlier video, here's the screenshot of the clear overlap:
Screenshot 2024-05-22 at 12 13 31 AM

And you'll see it every time you try type some new suggestion.

@melvin-bot melvin-bot bot added the Overdue label May 21, 2024
@dominictb
Copy link
Contributor

@dominictb are you still able to reproduce this?

@slafortune Yes I can still reproduce a majority of the time on latest main.

Maybe this step will make it easier to reproduce:

  1. Type :tr in the chat first
  2. Go back to LHN
  3. Go to the chat again, then immediately press on the Composer to focus on it
  4. The suggestions will show with the overlap at around the :next_track_button emoji now

Please see the reproduction video below:
https://github.com/Expensify/App/assets/165644294/bf8ec06b-fcfa-4d89-9dd8-53263722df98

Copy link

melvin-bot bot commented May 21, 2024

@slafortune @c3024 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 removed the Overdue label May 27, 2024
@DylanDylann
Copy link
Contributor

DylanDylann commented May 27, 2024

I can take over this issue as C+, I can reproduce the issue

@c3024
Copy link
Contributor

c3024 commented May 27, 2024

@DylanDylann thanks, unassigning myself.

@c3024 c3024 removed their assignment May 27, 2024
@pecanoro
Copy link
Contributor

I can reproduce it, I think it only happens with really long emoji names when they can't fit the name space.

@DylanDylann
Copy link
Contributor

DylanDylann commented May 28, 2024

@slafortune @pecanoro Could you assign me as C+ here once you have a chance? I'm taking over from @c3024

Copy link

melvin-bot bot commented May 29, 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 May 29, 2024
@JmillsExpensify JmillsExpensify removed their assignment May 29, 2024
@melvin-bot melvin-bot bot removed the Overdue label May 29, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 29, 2024
Copy link

melvin-bot bot commented May 29, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@twisterdotcom twisterdotcom added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 29, 2024
@DylanDylann
Copy link
Contributor

@dominictb There have been many changes since your proposal was created and I see it is outdated in some points. Please help to update your proposal

@melvin-bot melvin-bot bot added the Overdue label Jun 3, 2024
@dominictb
Copy link
Contributor

I'm back from weekends, I'll provide an update tomorrow

@DylanDylann
Copy link
Contributor

@MelvinBot Calm down, the contributor just came back. Let's give him a bit time to update this issue

@melvin-bot melvin-bot bot removed the Overdue label Jun 4, 2024
@dominictb
Copy link
Contributor

The original issue seems not reproducible any more after we change to use FlatList for the BaseAutoCompleteSuggestions from this PR.

I can reproduce it, I think it only happens with really long emoji names when they can't fit the name space.

@pecanoro Could you still reproduce this issue with really long emoji names?

Copy link

melvin-bot bot commented Jun 4, 2024

@slafortune @DylanDylann this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

Copy link

melvin-bot bot commented Jun 5, 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. (First week)

@DylanDylann
Copy link
Contributor

DylanDylann commented Jun 6, 2024

I can't reproduce this issue anymore

@pecanoro Could you check the above comment? If there are no problems, we can close this one

@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@DylanDylann
Copy link
Contributor

@slafortune PLease add re-test label here

@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@slafortune
Copy link
Contributor

not reproducible - 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. Daily KSv2 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
Projects
No open projects
Archived in project
Development

No branches or pull requests

9 participants