search

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.12k stars 2.62k forks source link

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

Closed lanitochka17 closed 4 weeks ago

lanitochka17 commented 1 month 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: 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 https://github.com/Expensify/App/issues/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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/aa9188a5-1907-4cf4-83a4-511a353fa15e

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
melvin-bot[bot] commented 1 month ago

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

github-actions[bot] commented 1 month ago

:wave: 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 commented 1 month ago

@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 commented 1 month ago

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

luacmartins commented 1 month ago

This bug seems to come from https://github.com/Expensify/App/pull/43255

luacmartins commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

⚠️ 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.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $500

Med-azaza commented 1 month ago

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:
    • Compare the relevant code sections between staging and production environments to pinpoint differences.
    • Specifically, analyze the code in PR #42523 related to text input and emoji picker functionality.
  2. 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 commented 1 month ago

Alright, it seems coming from my PR here: https://github.com/Expensify/react-native-live-markdown/pull/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 https://github.com/Expensify/react-native-live-markdown/pull/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: https://github.com/Expensify/App/issues/41137, I encountered this issue raised by C+ https://github.com/Expensify/App/issues/41137#issuecomment-2122076927, which my suggested solution in https://github.com/Expensify/App/issues/41137#issuecomment-2127866960 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: https://github.com/Expensify/App/issues/41137#issuecomment-2149334870 (the original comment is here -raised by @BartoszGrajdek).

The root cause and suggested solution has been identified here: https://github.com/Expensify/App/issues/41137#issuecomment-2153777091.

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 https://github.com/Expensify/react-native-live-markdown/pull/370

  1. However, https://github.com/Expensify/react-native-live-markdown/pull/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: https://github.com/Expensify/react-native-live-markdown/pull/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 https://github.com/Expensify/App/issues/41137#issuecomment-2122076927 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: https://github.com/Expensify/App/issues/41137

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 month ago

⚠️ 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 commented 1 month ago

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

mountiny commented 4 weeks ago

fixed in the -17 version https://github.com/Expensify/App/assets/36083550/3e715225-ed27-4c0e-8284-db96b68d5699

mountiny commented 4 weeks ago

I think we can close this one out, consolidating the payment here https://github.com/Expensify/App/issues/43332

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

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

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

melvin-bot[bot] commented 3 weeks ago

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

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

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