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

[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] Chat - Emoji gets inserted in front of entered text #43312

Closed
2 of 6 tasks
lanitochka17 opened this issue Jun 7, 2024 · 19 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 7, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.80-14
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #42523

Action Performed:

  1. Go to https://staging.new.expensify.com/
    and log in
  2. Open any report
  3. Type any text
  4. Open the in-app emoji picker and select any

Expected Result:

The emoji is inserted after the entered text

Actual Result:

The emoji is inserted in front of the entered text

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

Add any screenshot/video evidence

Bug6505452_1717784333732.video_2024-06-07_14-18-39.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01888989e2a7176967
  • Upwork Job ID: 1799815594511102406
  • Last Price Increase: 2024-06-09
Issue OwnerCurrent Issue Owner: @sobitneupane
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jun 7, 2024
Copy link

melvin-bot bot commented Jun 7, 2024

Triggered auto assignment to @blimpich (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Jun 7, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@lanitochka17
Copy link
Author

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

@lanitochka17
Copy link
Author

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

@luacmartins
Copy link
Contributor

luacmartins commented Jun 7, 2024

This bug seems to come from #43255

This was referenced Jun 7, 2024
@luacmartins
Copy link
Contributor

It seems like we're not triggering onSelectionChange to update the selection

This comment was marked as off-topic.

@mountiny mountiny removed the DeployBlocker Indicates it should block deploying the API label Jun 7, 2024
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Jun 9, 2024
Copy link

melvin-bot bot commented Jun 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01888989e2a7176967

@melvin-bot melvin-bot bot changed the title Chat - Emoji gets inserted in front of entered text [$250] Chat - Emoji gets inserted in front of entered text Jun 9, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 9, 2024
Copy link

melvin-bot bot commented Jun 9, 2024

Upwork job price has been updated to $500

@Med-azaza
Copy link

Proposal

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

When a user opens the in-app emoji picker and selects an emoji, the emoji is inserted in front of the entered text instead of after it. This issue is reproducible in the staging environment but not in production.

What is the root cause of that problem?

The root cause appears to be related to the handling of the cursor position and text buffer management in the code responsible for emoji insertion. There may be differences between the staging and production code that affect this functionality.

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

  1. Investigate Code Differences:
  1. Update Emoji Insertion Logic:
  • Ensure the code correctly identifies and handles the cursor position in the text input field.
  • Adjust the logic to insert the emoji at the cursor's current position rather than at the beginning of the text.

@dominictb
Copy link
Contributor

dominictb commented Jun 9, 2024

Alright, it seems coming from my PR here: Expensify/react-native-live-markdown#370, so I would like to take on this issue and fix this

Proposal

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

The emoji is inserted in front of the entered text. This is a regression created by Expensify/react-native-live-markdown#370.

What is the root cause of that problem?

The history is a bit complex. Here's the concise version:

  1. While trying to fix this issue: [HOLD for payment 2024-06-28] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] mWeb - Chat-Pasting text in compose box makes page moves up and down #41137, I encountered this issue raised by C+ [HOLD for payment 2024-06-28] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] mWeb - Chat-Pasting text in compose box makes page moves up and down #41137 (comment), which my suggested solution in [HOLD for payment 2024-06-28] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] mWeb - Chat-Pasting text in compose box makes page moves up and down #41137 (comment) was tackling. My PR was raised in react-native-live-markdown repo, including this change to address that specific issue: https://github.com/Expensify/react-native-live-markdown/pull/357/files#diff-ec487e4a4ccb9c4ffd7ead6ff0a585595aa8d9e9b7d5167d0a2444d70d2001e1R563.

The solution's main idea is to use the contentSelection ref to restore the cursor instead of using blur() and focus() mechanism, which will avoid the main issue #41137, i.e: the keyboard being popping down and up again.

  1. However, this change cause a regression which is described here: [HOLD for payment 2024-06-28] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] mWeb - Chat-Pasting text in compose box makes page moves up and down #41137 (comment) (the original comment is here -raised by @BartoszGrajdek).

The root cause and suggested solution has been identified here: #41137 (comment).

In summary, the contentSelection ref is out-of-date during text change event, so one way to fix is to update the contentSelection value after updating the text, which is the purpose of this PR Expensify/react-native-live-markdown#370

  1. However, fix: update contentSelection ref after text change react-native-live-markdown#370 causes many regression as the fact thatcontentSelection ref is being updated in the handleOnTextChange event will cause the cursor to jump around funny, leading to these 2 regression (which are deploy blockers)

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

It seems to me that react-native-live-markdown and its way of working shouldn't be touched since it's extremely difficult to alter the core flow without causing regression in one way or another, i.e, if I'm trying to figure out the root cause and fix this issue based on the current code, I'll push myself down the black hole. Another factor we need to consider is that this issue is a deploy blocker, so the best course of action will be:

  1. Revert these 2: fix: update contentSelection ref after text change react-native-live-markdown#370, and https://github.com/Expensify/react-native-live-markdown/pull/357/files#diff-ec487e4a4ccb9c4ffd7ead6ff0a585595aa8d9e9b7d5167d0a2444d70d2001e1R563 (for this PR, we only need to revert the specific change that used the contentSelection ref to set the cursor of the new text change in the hook, let's keep the setBaseAndExtent function as it is proved not to cause any issues so far).

  2. By taking 2 reverts above, the issue [HOLD for payment 2024-06-28] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] mWeb - Chat-Pasting text in compose box makes page moves up and down #41137 (comment) will rise again, and this is my proposed solution to resolve this issue without touching the contentSelection ref and causing bunch of regression, and it's well tested on both the main issue and both regression issues mentioned above.

I can open PR immediately for (1), which will unblock the blockers, and for (2) will be handled exclusively in this issue: #41137

What alternative solutions did you explore? (Optional)

N/A

Copy link

melvin-bot bot commented Jun 9, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@mountiny
Copy link
Contributor

mountiny commented Jun 9, 2024

Reverted the offending PR, proper fix will be handled in the original issue, we just need to get the deploy out Expensify/react-native-live-markdown#377

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Jun 9, 2024
@mountiny mountiny removed the DeployBlockerCash This issue or pull request should block deployment label Jun 9, 2024
@mountiny
Copy link
Contributor

mountiny commented Jun 9, 2024

@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 10, 2024
@mountiny
Copy link
Contributor

I think we can close this one out, consolidating the payment here #43332

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 11, 2024
@melvin-bot melvin-bot bot changed the title [$500] Chat - Emoji gets inserted in front of entered text [HOLD for payment 2024-06-18] [$500] Chat - Emoji gets inserted in front of entered text Jun 11, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jun 11, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.81-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-18. 🎊

For reference, here are some details about the assignees on this issue:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 13, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-18] [$500] Chat - Emoji gets inserted in front of entered text [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] Chat - Emoji gets inserted in front of entered text Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-20. 🎊

For reference, here are some details about the assignees on this issue:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants