Fix cursor position when adding emoji #12632

AndreasBBS · 2022-11-10T03:06:49Z

Details

Changed the way the cursor is placed after an emoji is written through the "two dot mechanism" :emoji: or a string with an :emoji: is pasted. For example:

Let's say we're pasting the string :wave: friend in the string Hello nice to meet you between the word Hello and nice: Hello |nice to meet you (representing the cursor with the char |):

Before the fix, it results in Hello 👋 friend nice to meet you|
After the fix, it results in Hello 👋 friend |nice to meet you

Let's say we're writing the emoji :wave: in the string Hello nice to meet you between the word Hello and nice: Hello |nice to meet you:

Before the fix, it would result in Hello 👋nice to meet you|
After the fix, it results in hello 👋|nice to meet you

Fixed Issues

$ #12325
PROPOSAL: https://github.com/Expensify/App/issues/12325#issuecomment-1299518471

Tests

Case 1:

Start with a string Hello nice to meet you
Write the emoji :wave: in the string Hello nice to meet you between the word Hello and nice
Verify the resulting string has the emoji converted and the cursor is placed after the emoji

Case 2:

Start with a string Hello nice to meet you
Paste the string :wave: friend in the string Hello nice to meet you between the word Hello and nice
Verify the resulting string has the emoji converted and the cursor is placed after the word friend

Verify that no errors appear in the JS console

QA Steps

Case 1:

Start with a string Hello nice to meet you
Write the emoji :wave: in the string Hello nice to meet you between the word Hello and nice
Verify the resulting string has the emoji converted and the cursor is placed after the emoji

Case 2:

Start with a string Hello nice to meet you
Paste the string :wave: friend in the string Hello nice to meet you between the word Hello and nice
Verify the resulting string has the emoji converted and the cursor is placed after the word friend

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

bug12325_review_web.webm

macos_chrome.mov

macos_safari.mov

Mobile Web - Chrome

bug12325_review_android_chrome.1.webm

Mobile Web - Safari

Desktop

electron.mov

iOS

Android

bug12325_review_android.webm

github-actions · 2022-11-10T03:07:06Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

melvin-bot · 2022-11-10T03:07:21Z

@dangrous Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

AndreasBBS · 2022-11-10T03:14:59Z

I'm adding this message here to provide a bit of context. I did not test for the platforms: Mobile Web - Safari and iOS because my currently available work computer is not a Mac. I can get access to a friend's phone to test the Mobile Web - Safari probably still this week. I'll also try to get my hands on a Mac to be able to use Xcode to run the iOS version of the App and test the changes. This said, if someone would be kind enough to run the tests for the iOS version for me I'd be very grateful.

Regarding the Desktop version, I tried spinning up the electron build on my local computer (I'm running Ubuntu 22.04) but I did not have much success. If someone could help me get the desktop environment setup it would be awesome. It's my first time contributing for this App and I only started getting into the code last week so I'm still getting my grip with setting up the proper dev environment.

AndreasBBS · 2022-11-10T03:17:05Z

I have read the CLA Document and I hereby sign the CLA

dangrous · 2022-11-11T17:48:20Z

@ctkochan22 and @rushatgabhane I think this didn't get properly linked to the issue so I added you as reviewers, removing myself. Let me know if you want extra eyes on this though!

ctkochan22 · 2022-11-11T18:21:51Z

Looks good. @rushatgabhane Let me know when you can test this 🙇

AndreasBBS · 2022-11-11T18:49:46Z

Android Bug

I'm making this comment to address the Android example:

bug12325_review_android.webm

As you can see in the video, the desired behavior is not achieved in this case. This is because of a preexisting bug. Here are videos from the App without my commit showing the same behavior happening:

bug12325_review_android_previous_bug4.webm

bug12325_review_android_previous_bug3.webm

bug12325_review_android_previous_bug2.webm

bug12325_review_android_previous_bug1.webm

bug12325_review_android_previous_bug.webm

Initially when I tested in the Android app I was surprised to see this bug because of the small change I made to the code and me not being aware of the preexisting bug. Only when I undid my commit and tested the behavior again is it that I found out it was preexisting.

Discussion

What I want to discuss now in this comment is a broader decision that's regarding Controlled Selection vs Uncontrolled Selection.

When I was working on this bug and I was finished implementing my solution I wanted to check if the bug was still present on the default branch. Out of habit I checkout out the master branch. I tested for the bug in there and it was fixed. I was confused and went to take a look at the code and noticed that a lot was different. It took me a while to realize the default branch is now the main branch and that master was old code. Now the interesting thing I noticed was that in the masterbranch the selection prop was not being used and things were working fine. The uncontrolled selection methodology (meaning leaving selection control to react-native to handle) was being used in the past, and for some reason there was a decision to switch to a controlled selection methodology.

The problem

Turns out that if you go to react-native repository and you search in issues for onSelectionChange you'll see that there's a lot of problems with this handler. The most relevant issues in my opinion are:

TextInput issues on Android after updating to version 0.63.0 facebook/react-native#30503 which has remained unsolved since 2020 despite previous attempts
TextInput controlled selection broken on both ios and android. facebook/react-native#29063 which has also remained unsolved since 2020 and I believe is the reason the bug you see in Android is happening

There's a lot of problems regarding the onSelectionChange in react-native, causing different behaviors in the browser, ios, android and desktop environments. As well as onSelect with React itself between different browsers, as I discovered as I was tackling this bug #12298 and that lead me to having to open yet another issue in React's repo itself facebook/react#25643

In fact, as I was looking into the bug seeing if I could fix it (because even though it was preexisting, I don't feel good presenting a solution that does not behave as intended) I found out that the onSelectionChange handler is very erratic. With the same input, sometimes if fires the event 3 times, sometimes 5 times. Sometimes it puts the cursor 3 chars away, other times 7, other times leaves it in the place I set it, other times it takes it to the end of the string. Setting state every time onSelectionChange fires is causing multiple re-renders and that's why in the footage you see the cursor "flashing around" (you can only tell it's flashing because that's an emulator running a live dev server, in the built app the re-renders happen to fast to notice the "flashing" but the cursor still ends up in completely erratic locations)

I also suspect that a lot of bugs that are happening lately are due to this erratic behavior of onSelectionChange, for example:

Opinion

I'm sure there was a really good reason to transition from the default Uncontrolled Selection strategy to the Controlled Selection strategy but there's a real problem with onSelectionChange events that needs to be addressed. I've worked previously with react-native in other projects and although most of the times the default Uncontrolled Selection strategy is the one pursued there's been a handful of cases where it was decided to use Controlled Selection. In the cases of using Controlled Selection it was decided to avoid, every time it was possible, using the onSelectionChange due to its erratic nature and I believe this is a philosophy that really positively impacted the development.
Using the selection prop to set the selection values from state and then using onSelectionChange to set state of the selection is a halfway solution between uncontrolled and controlled selection strategies and in my opinion will lead to problems. I think that if the controlled selection strategy is chosen, then you really have to control the selection values and not rely on the onSelectionChange event handler to do half the job, especially with how buggy it is right now. Even this bug I'm solving should not be a bug, the event that's fired and captured by onSelectionChange should hold the correct end and start values, but it does not. And in the case of Android, even though I set the selection to the correct values, it's almost always overwritten by the erratic nature of this handler / event triggering.
I know it would be a significant refactor to do to try to make everything work without the usage of the onSelectionChange event handler but at the end of the day, in my opinion, it's something that leads to less bugs and real control over the selection prop.

rushatgabhane

@AndreasBBS

please run npx eslint . and fix the lint errors
Expensify requires that you can test on all platforms - https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#make-sure-you-can-test-on-all-platforms
suggestion: change issue title to something more meaningful like Fix cursor position when adding emoji

rushatgabhane · 2022-11-13T07:30:00Z

I'm sure there was a really good reason to transition from the default Uncontrolled Selection strategy to the Controlled Selection strategy but there's a real problem with onSelectionChange events that needs to be addressed

@AndreasBBS Thanks for your detailed investigation!!
Migrating to controlled selection was a prerequisite for enabling Fabric - https://reactnative.dev/docs/next/new-architecture-library-intro#migrating-off-setnativeprops

I know it would be a significant refactor to do to try to make everything work without the usage of the onSelectionChange event handler but at the end of the day, in my opinion, it's something that leads to less bugs and real control over the selection prop

Let's address this in a seperate issue because it's unrelated to the current PR/issue.
You should report it as a bug on slack after this PR has been merged. (You'll be eligible for a $250 reporting bonus)

rushatgabhane

@AndreasBBS Fantastic work!

@ctkochan22 LGTM! 🎉

Web

Screen.Recording.2022-11-16.at.12.23.57.AM.mov

mWeb

Screen.Recording.2022-11-16.at.12.27.53.AM.mov

iOS

Screen.Recording.2022-11-16.at.12.32.10.AM.mov

Android

Screen.Recording.2022-11-16.at.12.30.05.AM.mov

Desktop

Screen.Recording.2022-11-16.at.12.30.53.AM.mov

PR Reviewer Checklist

ctkochan22

great work!

ctkochan22 · 2022-11-16T06:33:40Z

Oh man your commits are not verified -- https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#begin-coding-your-solution-in-a-pull-request

rushatgabhane · 2022-11-16T09:22:07Z

@AndreasBBS feel free to ask for help :)

AndreasBBS · 2022-11-16T17:10:17Z

@rushatgabhane Thanks a million for testing on all platforms. I don't have a mac and I've been trying to make ends meet with what I can do in Ubuntu. I added my own videos for macos safari, chrome and electron (I managed to run catalina in virtualbox). But I'm having trouble installing XCode on the VM. I think I should be able to have the tests on iOS by the end of the week. I meanwhile fixed the linting errors and changed the title. Let me know if there's something else on my side that's missing.

EDIT: I'm currently signing the commits.
EDIT2: Commits are currently signed. Sorry for that 🙏 git was not reading the config from the .gitconfig file for some reason. Had to add through git config ...

- In ReportActionItemMessageEdit.js change updateDraft's setState - In ReportActionCompose.js change updateComment's setState - In both changes, verify if the input has been manipulated. If yes, mantain cursor position from previous state

ctkochan22 · 2022-11-16T20:19:02Z

Looks good! Thank you for fixing your commits!

melvin-bot · 2022-11-16T20:19:21Z

@ctkochan22 looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-11-16T20:24:34Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-11-17T08:24:39Z

🚀 Deployed to staging by @ctkochan22 in version: 1.2.29-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-11-22T17:04:57Z

🚀 Deployed to production by @AndrewGable in version: 1.2.29-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

aimane-chnaif · 2023-07-31T00:16:22Z

src/pages/home/report/ReportActionCompose.js

+                value: newComment,
+            };
+            if (comment !== newComment) {
+                const remainder = prevState.value.slice(prevState.selection.end).length;


There was hidden bug in this logic. #17275 (comment) for more details about the root cause.

jjcoffee · 2023-10-27T09:05:50Z

Just flagging that this PR caused a bug where the cursor position would freeze after pasting emojis. The root cause is the reliance on setState in updateComment when setting the selection.

App/src/pages/home/report/ReportActionCompose.js

Lines 386 to 400 in 8188200

    
           this.setState((prevState) => { 
        
               let newState = { 
        
                   isCommentEmpty: !!newComment.match(/^(\s|`)*$/), 
        
                   value: newComment 
        
               } 
        
               if(comment !== newComment) { 
        
                   const remainder = prevState.value.slice(prevState.selection.end).length 
        
                   newState.selection =  { 
        
                       start: newComment.length - remainder, 
        
                       end: newComment.length - remainder 
        
                   } 
        
               } 
        
               return newState

The issue is that due to the async nature of setState, we can't guarantee that it will actually return the most recent prevState and so by the time the cursor's position is calculated we could be using an outdated prevState value. More details here.

AndreasBBS requested a review from a team as a code owner November 10, 2022 03:06

melvin-bot bot requested review from dangrous and removed request for a team November 10, 2022 03:07

dangrous requested review from rushatgabhane and ctkochan22 and removed request for dangrous November 11, 2022 17:47

rushatgabhane suggested changes Nov 13, 2022

View reviewed changes

AndreasBBS changed the title ~~Bug/12325~~ Fix cursor position when adding emoji Nov 14, 2022

rushatgabhane previously approved these changes Nov 15, 2022

View reviewed changes

ctkochan22 previously approved these changes Nov 16, 2022

View reviewed changes

lschurr mentioned this pull request Nov 16, 2022

[HOLD react-native 25643] [$1000] IOU - Correct digit is not getting deleted in IOU request reported by @daraksha-dk #12298

Closed

AndreasBBS dismissed stale reviews from ctkochan22 and rushatgabhane via bea9e61 November 16, 2022 17:37

AndreasBBS force-pushed the bug/12325 branch from 7e83d17 to bea9e61 Compare November 16, 2022 17:37

AndreasBBS added 2 commits November 16, 2022 17:48

Fix Expensify#12325

8188200

- In ReportActionItemMessageEdit.js change updateDraft's setState - In ReportActionCompose.js change updateComment's setState - In both changes, verify if the input has been manipulated. If yes, mantain cursor position from previous state

Fix formatting. npx eslint no errors

0a9087c

AndreasBBS force-pushed the bug/12325 branch from bea9e61 to 0a9087c Compare November 16, 2022 17:48

ctkochan22 approved these changes Nov 16, 2022

View reviewed changes

ctkochan22 merged commit e79e484 into Expensify:main Nov 16, 2022

melvin-bot bot added the Emergency label Nov 16, 2022

OSBotify mentioned this pull request Nov 17, 2022

Deploy Checklist: New Expensify 2022-11-17 #12812

Closed

35 tasks

kbecciv mentioned this pull request Nov 18, 2022

[HOLD setSelection refactor] [$2000] Android - Cursor is placed in wrong position after write/paste the emoji between the word Hello and nice - Reported by: @AndreasBBS #12854

Closed

AndreasBBS deleted the bug/12325 branch November 18, 2022 19:31

Ollyws mentioned this pull request Dec 1, 2022

[hold for app #12854] [$2000] Shows incorrect emoji while combining the manual and emoji picker selected emoji within the text #13066

Closed

narefyev91 mentioned this pull request Mar 22, 2023

[$1000] IOS - IOUAmount - When entering/deleting numbers quickly, the cursor doubles #15856

Closed

6 tasks

aimane-chnaif reviewed Jul 31, 2023

View reviewed changes

aimane-chnaif mentioned this pull request Jul 31, 2023

[HOLD for payment 2023-07-26] [$1000] Cursor jumps to previous word while correcting incorrect emoji #17275

Closed

6 tasks

jjcoffee mentioned this pull request Oct 27, 2023

[Awaiting Checklist] [$500] Cursor freeze on paste emoji in edit message #29128

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cursor position when adding emoji #12632

Fix cursor position when adding emoji #12632

AndreasBBS commented Nov 10, 2022 •

edited by mountiny

Loading

github-actions bot commented Nov 10, 2022 •

edited

Loading

melvin-bot bot commented Nov 10, 2022

AndreasBBS commented Nov 10, 2022

AndreasBBS commented Nov 10, 2022

dangrous commented Nov 11, 2022

ctkochan22 commented Nov 11, 2022

AndreasBBS commented Nov 11, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Nov 13, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

ctkochan22 left a comment

ctkochan22 commented Nov 16, 2022

rushatgabhane commented Nov 16, 2022

AndreasBBS commented Nov 16, 2022 •

edited

Loading

ctkochan22 commented Nov 16, 2022

melvin-bot bot commented Nov 16, 2022

OSBotify commented Nov 16, 2022

OSBotify commented Nov 17, 2022

OSBotify commented Nov 22, 2022

aimane-chnaif Jul 31, 2023

jjcoffee commented Oct 27, 2023

Fix cursor position when adding emoji #12632

Fix cursor position when adding emoji #12632

Conversation

AndreasBBS commented Nov 10, 2022 • edited by mountiny Loading

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

github-actions bot commented Nov 10, 2022 • edited Loading

melvin-bot bot commented Nov 10, 2022

AndreasBBS commented Nov 10, 2022

AndreasBBS commented Nov 10, 2022

dangrous commented Nov 11, 2022

ctkochan22 commented Nov 11, 2022

AndreasBBS commented Nov 11, 2022 • edited Loading

Android Bug

Discussion

The problem

Opinion

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane commented Nov 13, 2022 • edited Loading

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

ctkochan22 left a comment

Choose a reason for hiding this comment

ctkochan22 commented Nov 16, 2022

rushatgabhane commented Nov 16, 2022

AndreasBBS commented Nov 16, 2022 • edited Loading

ctkochan22 commented Nov 16, 2022

melvin-bot bot commented Nov 16, 2022

OSBotify commented Nov 16, 2022

OSBotify commented Nov 17, 2022

OSBotify commented Nov 22, 2022

aimane-chnaif Jul 31, 2023

Choose a reason for hiding this comment

jjcoffee commented Oct 27, 2023

AndreasBBS commented Nov 10, 2022 •

edited by mountiny

Loading

github-actions bot commented Nov 10, 2022 •

edited

Loading

AndreasBBS commented Nov 11, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Nov 13, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

AndreasBBS commented Nov 16, 2022 •

edited

Loading