Bug/12325

[AndreasBBS]

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

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

Tests

Case 1:

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

Case 2:

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

• [x] Verify that no errors appear in the JS console

QA Steps

Case 1:

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

Case 2:

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

• [x] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• [ ] I included screenshots or videos for tests on all platforms
• [ ] I ran the tests on all platforms & verified they passed on:
  • [ ] iOS / native
  • [ ] Android / native
  • [ ] iOS / Safari
  • [x] Android / Chrome
  • [ ] MacOS / Chrome
  • [ ] MacOS / Desktop
  • [x] Desktop / Chrome
  • [x] Desktop / Firefox
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product was added in all src/languages/* files
  • [x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] If a new component is created I verified that:
  • [x] A similar component doesn't exist in the codebase
  • [x] All props are defined accurately and each prop has a /** comment above it */
  • [x] The file is named correctly
  • [x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [x] The only data being stored in the state is data necessary for rendering and nothing else
  • [x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [x] All JSX used for rendering exists in the render method
  • [x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

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

• [ ] I have verified the author checklist is complete (all boxes are checked off).
• [ ] I verified the correct issue is linked in the ### Fixed Issues section above
• [ ] I verified testing steps are clear and they cover the changes made in this PR
  • [ ] I verified the steps for local testing are in the Tests section
  • [ ] I verified the steps for Staging and/or Production testing are in the QA steps section
  • [ ] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [ ] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• [ ] I checked that screenshots or videos are included for tests on all platforms
• [ ] I included screenshots or videos for tests on all platforms
• [ ] I verified tests pass on all platforms & I tested again on:
  • [ ] iOS / native
  • [ ] Android / native
  • [ ] iOS / Safari
  • [ ] Android / Chrome
  • [ ] MacOS / Chrome
  • [ ] MacOS / Desktop
• [ ] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• [ ] I verified proper code patterns were followed (see Reviewing the code)
  • [ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • [ ] I verified that comments were added to code that is not self explanatory
  • [ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [ ] I verified any copy / text shown in the product was added in all src/languages/* files
  • [ ] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [ ] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [ ] I verified that this PR follows the guidelines as stated in the Review Guidelines
• [ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [ ] If a new component is created I verified that:
  • [ ] A similar component doesn't exist in the codebase
  • [ ] All props are defined accurately and each prop has a /** comment above it */
  • [ ] The file is named correctly
  • [ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [ ] The only data being stored in the state is data necessary for rendering and nothing else
  • [ ] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [ ] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [ ] All JSX used for rendering exists in the render method
  • [ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [ ] If a new CSS style is added I verified that:
  • [ ] A similar style doesn't already exist
  • [ ] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• [ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [ ] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

bug12325_review_web.webm

Mobile Web - Chrome

bug12325_review_android_chrome(1).webm

Mobile Web - Safari

Desktop

iOS

Android

bug12325_review_android.webm

[github-actions[bot]]

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

[melvin-bot[bot]]

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

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]

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

[dangrous]

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

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

[AndreasBBS]

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:

• https://github.com/facebook/react-native/issues/30503 which has remained unsolved since 2020 despite previous attempts
• https://github.com/facebook/react-native/issues/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 https://github.com/Expensify/App/issues/12298 and that lead me to having to open yet another issue in React's repo itself https://github.com/facebook/react/issues/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:

• https://github.com/Expensify/App/issues/12298
• https://github.com/Expensify/App/issues/12523
• https://github.com/Expensify/App/issues/12521
• etc.

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]

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)

[ctkochan22]

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]

@AndreasBBS feel free to ask for help :)

[AndreasBBS]

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

[ctkochan22]

Looks good! Thank you for fixing your commits!

[melvin-bot[bot]]

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

:hand: 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]

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

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

[OSBotify]

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

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

[jjcoffee]

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. https://github.com/Expensify/App/blob/81882002eee55586afc0b79c4a903f496c91f1e5/src/pages/home/report/ReportActionCompose.js#L386-L400

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.