perf: fix (comment) input performance

[hannojg]

Details

I was investigating a performance (and crash) issue when typing a comment for a report.
I was able to reproduce the issue on iOS, where I noticed that the JS thread completely dies when you are typing (typing super slow seems fine, but typing a bit faster caused a massive slowdown):

video_before.mp4

Note: When typing super fast, the UI thread gets impacted as well, however, this is expected behaviour and has nothing to do with this app's setup/code.

In addition to that I also get some warnings:

After:

After fixing the issue the JS thread is almost not impacted at all. It's only impacted when starting typing. I also want to improve performance for that as well, however, that's for another PR:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-10-08.at.17.56.34.mp4

Notice how even when typing super fast the JS thread doesn't die (the UI thread does but that's expected).

The main fix was to remove the text input from the state of the ReportActionCompose. On each keystroke, it re-rendered ~3 times. Now it doesn't re-render at all when just typing (it does when starting and stopping typing, but that's for other PRs to improve).

Removal of the space after emojis:

When I started working on this I realized an inconsistency in behaviour when adding emojis between the composer on the report screen and the composer when editing a sent message/action.
I made the behaviour equal, and when adding emojis there is no additional space added. This was discussed and decided on slack: https://expensify.slack.com/archives/C035J5C9FAP/p1665560521827199

Fixed Issues

$ #8349
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

• Verify that no errors appear in the JS console

Testing adding emojis:

1. Open a report
2. Type some text
3. Add emojis using the emoji picker to the text
4. Try to add emojis in between the words as well
5. Try to add an emoji with a part of the sentence selected
6. Make sure everything behaves as expected
7. Try the same when editing a sent message

When adding emojis make sure that no additional whitespace gets added. This behavior has changes as discussed (see link above)

Testing android selection menu

(Only needed on android)

1. Open a report
2. Type some text
3. Select some text
4. Make sure that the selection menu with copy / paste / cut is showing up
5. Make sure that you can change the selection (extend / shorten)

The following is a list of regression tests to run against this PR to make sure everything is working as expected.

Regression tests:

Test plan

Helper list to confirm you ran every test

• Selection is not reset after selecting an emoji
  • Android
  • iOS
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Broken emojis are rendered when picked from the picker in a series
  • Android
  • iOS
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Cursor position is broken
  • Android
  • iOS
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Adding emoji via EmojiPicker in replacement for multiple lines creates a delay before the composer height is calculated.
  • Android
  • iOS
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Selected Emoji is placed in the wrong position.
  • Android
  • iOS
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Pasting image does not send the attached text message.
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• There is a delay before Composer expands the height on the web.
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Cursor position is not focused after picking a emoji in Composer.
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Content flashing while pasting.
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Edit Message: Copy-pasting is slow.
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Cursor jumping or moving positions after picking emoji.
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop
• Compose text is not cleared after sending the message
  • mWeb
  • Safari
  • Chrome
  • Firefox
  • Desktop

All platforms, make sure for web to run on every browser (Safari, Chrome, Firefox, Edge):

Selection is not reset after selecting an emoji

1. Add multiple message on the composer.
2. selected multiple lines.
3. Open the emoji picker and choose an emoji.
4. selection remained.

screen-2022-10-12_01.10.50.mp4

Broken emojis are rendered when picked from the picker in a series

1. Add a bunch of text lines in the composer.
2. Move the cursor to some middle position in the para.
3. Now pick an emoji from the emoji picker.
4. Try to pick the same emoji a few more times from the picker.

screen-2022-10-12_01.35.38.mp4

Cursor position is broken

1. Steps

1. Add a bunch of lines to the composer on web.
2. Place your cursor somewhere in the middle.
3. Go to any other page and copy some strings. (e.g. Copy step 2 in clipboard)
4. Paste in the composer.
5. Cursor is not at the end of pasted string.

screen-2022-10-12_01.46.21.mp4

2. Steps

Steps.

1. Open the app on the web.
2. Copy long text from some where (only text no markup).
3. Paste it on the composer.
4. Cursor is at the start of the pasted text.

Expected: it should be at the end of the pasted text.

Adding emoji via EmojiPicker in replacement for multiple lines creates a delay before the composer height is calculated.

1. Add multi-line message in composer.
2. Selects the multi-line text.
3. Add an emoji from the picker.
4. Observe that there is a delay before the composer height is adjusted correctly.

screen-2022-10-19_01.19.07.mp4

Selected Emoji is placed in the wrong position.

1. open the app.
2. Add three lines of message in composer.
3. Select some text from the middle line.
4. Open Emoji Picker and select an emoji.
5. Notice emoji is placed correctly and the selected text is gone.
6. Now reopen the EmojiPicker.
7. Select another emoji.
8. Notice that new emojis are added to the end of the line (not the previous cursor position).
9. Repeat from 4 to 6 to observe the same pattern.

---

[Web]

Pasting image does not send the attached text message.

1. Type some message in the composer.
2. Go to image and copy the image.
3. Back to the app and focus after the typed text in the composer.
4. Press CTRL+V to paste the image.
5. Click send on the opened attachment modal.
6. Observe the image is sent but the text remained in the composer and Send button is disabled.

Expected: Pressing send on the attachment modal should first send the text and then the image. (See staging for correct behaviour).

There is a delay before Composer expands the height on the web.

1. Open the app on the web.
2. type text quickly.
3. Use Shift+Enter to enter a couple of lines.

screen-2022-11-16_19.39.59.mp4

Cursor position is not focused after picking a emoji in Composer.

1. Paste the following content in composer.

ASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DAS

2. Now place the cursor after the first three emojies.
3. Scroll the composer content below so that cursor is no more visible but composer remains focused.
4. open emoji Picker.
5. select an emoji.
6. Notice that cursor position is not revealed.

screen-2023-01-05_00.03.18.mp4

Content flashing while pasting

1. Paste the following content in composer.

ASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DAS

2. Now use Cmd + A to select the whole content in the composer.
3. Paste the same copied content to replace.
4. Notice that there is a flashing of content. (Not present on staging).

screen-2023-01-05_00.11.41.mp4

Edit Message: Copy-pasting is slow.

1. Edit an old send message.
2. Copy following to clipboard.

ASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DASASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DAS

3. Replace the content on the edit message input with the copied content.
4. Select everything on the edit composer.
5. Paste via shortcuts.
6. Notice that it takes a few secs to replace.

screen-2023-01-09_16.19.55.mp4

Cursor jumping or moving positions after picking emoji.

1. Edit a message or draft a long message on the composer. (Use the content from the previous report).
2. Place the cursor either before the center or after the center line.
3. Now pick an emoji.
4. Notice that the cursor does not remain in the same position even though the previous cursor position was in the view.

screen-2023-01-09_16.26.09.mp4

Compose text is not cleared after sending the message

bug1-web.mov

QA Steps

Same as in testing section above.

Testing adding emojis:

1. Open a report
2. Type some text
3. Add emojis using the emoji picker to the text
4. Try to add emojis in between the words as well
5. Try to add an emoji with a part of the sentence selected
6. Make sure everything behaves as expected
7. Try the same when editing a sent message

Testing android selection menu

(Only needed on android)

1. Open a report
2. Type some text
3. Select some text
4. Make sure that the selection menu with copy/paste/cut is showing up
5. Make sure that you can change the selection (extend/shorten)

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 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
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• 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)
• I followed proper code patterns (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 followed the guidelines as stated in the Review Guidelines
• 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)
• 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 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

Screen.Recording.2022-10-14.at.3.08.26.PM.mov

Mobile Web - Chrome

android_chrome.mp4

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-10-14.at.15.10.37.mp4

Desktop

Screen.Recording.2022-10-14.at.3.18.25.PM.mov

iOS

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-10-14.at.15.01.11.mp4

Android

SVID_20221014_150501_1.mp4

[roryabraham]

This isn't something that was introduced by this PR, but I find it sort of alarming how great the difference between Composer/index.js and Composer/index.native.js is. At minimum, they should have all the same PropTypes.

[trjExpensify]

Any update here, @hannojg?

[hannojg]

Yeah, this PR aims do to two things:

• Fix this bug on Android ([Ready for payment via newdot][$16000] Android - Copy / Paste / Cut menu is not displayed when selecting text in compose box - Reported by: @parasharrajat #8349)
• improve the performance of the report screen when entering a chat message

For the first point I went with a different approach an created a fix for within react native itself.
The PR can be found here:

• fix(android): selection menu missing controlled textinput react-native#50

Once this PR is merged I will revisit this PR and make it only about the performance improvement :)

[trjExpensify]

Okay, cool. Shall we update the holds then to reference the PR we're waiting on: Expensify/react-native#50?

[hannojg]

Shall we update the holds then to reference the PR we're waiting on

Definitely!

(However there will be the need for a follow up PR where we update the react native version in this repository, however I can only create this PR once Expensify/react-native#50 got merged 😅 )

[roryabraham]

@hannojg I know that this PR has been open for a while, but I think it makes sense to put it on HOLD for #16263

[hannojg]

Oh yeah, agree @roryabraham

[hannojg]

Also as an update, I am not sure if the problem is really fixable within react native's code base.
After evaluating this carefully it seems to me that:

If you want to keep the selection menu working on android while manually controlling the selection of a TextInput you have to use an imperative approach (e.g. calling textInputRef.setSelection instead of using the selection prop)

Which this PR is about.

Will still wait for meta's final response if they agree on this.

[trjExpensify]

I'm a bit behind on this PR. What are the next steps?

Will still wait for meta's final response if they agree on this.

Did we get anywhere here?

[hannojg]

We found a native fix. I am double-checking today If this PR is still needed 😊

[parasharrajat]

Awesome @hannojg

[trjExpensify]

We found a native fix. I am double-checking today If this PR is still needed 😊

Dope! What's the verdict?

[hannojg]

@trjExpensify So I did another performance check, on my weakest device it shows that there is room for improvement.

However, I'd like to redo the PR. As the original PR also aimed to fix the selection bug on android, i used for example the imperative API to update the selection of the input, which introduced a lot of regressions.
As this bug is handled in another PR, i don't think this approach is needed anymore.
I just want to try to improve the performance by moving the state away from the screen into a dedicated sub component.

If I understand you correctly, you like to keep this original PR right?

[trjExpensify]

If I understand you correctly, you like to keep this original PR right?

Not necessarily man, it has just been open for a long time in this limbo draft and I see from time-to-time other issues on hold for it / referenced as potentially fixing a related problem. So I don't have a strong opinion on proceeding with this PR (just that we do something with it haha), Closing it and starting over elsewhere for the performance considerations is probably better.

[hannojg]

Closing it and starting over elsewhere for the performance considerations is probably better.

@trjExpensify Alright awesome, let's do that please. Also we should create a performance issue first and start from there.

[parasharrajat]

OK. That works for me. But we close this, we need to create a PR to update the fork version on the NewDot repo as well. That PR can be linked to solving the original issue #8349.

Meanwhile, the upstream branch is still not merged Expensify/react-native#55.

[parasharrajat]

Let's close this PR.