fix: composer not cleared when sending message

[hannojg]

Details

(Slack thread)

This PR fixes an issue where the composer's text input wouldn't show the value the user expects. The following use case now works:

• Spam the 'a' key, typing 'aaaaaa...'
• Press send
• Spam the 'b' key, typing 'bbbbbb...'
• Expect that the composer shows first 'aaaaaa', then clears it, then it shows 'bbbbbb'

In react native the text input sends the whole text thats currently in the text input in the onChange/onChangeText event callbacks. However, we maintain the "source of truth" for our text input in JavaScript in our value react state variable.
So it can happen that we set our JS value to "" to clear the input, but the native side is ignoring that event (as the JS thread is lagging behind in events), and sends us a new update with the current native input (which would be "aaaaaabbbbbb" by then).

With the approach in this PR, we only apply the difference from the native side to our JS value. E.g. if the user pressed another "a", instead of setting the whole native text to our js value we just append the a that was added.

This way the native side might still ignore our clear event (setting the value to ""), but when the user presses another character like "b" it will be appended to our empty value "" + "b", and when the native side then processes this update, it shows correctly "b" instead of "aaaaaab".

For this to work we need to receive the range in which the changes to the new active text have been made. This requires changes in react-native and @expensify/react-native-live-markdown. Two patches have been added for which we have PRs here:

• feat: include information about text range changes in onChange facebook/react-native#45248
• feat: add count/before/start info to onChange event react-native-live-markdown#412

This PR bumps rn-live-markdown to include the changes necessary for this PR. Before merging this PR make sure to merge this one first:

• Bump react-native-live-markdown #44958

TODO:

• Investigate potential re-render loop: (Shouldn't be part of this PR, this PR didn't;t introduce it)
  • Enter some text
  • Reload the app
  • Remove the draft text
  • At this point I get a "Maximum call depth warning" - not sure if introduced by this PR, but there might be something wrong
• Bug: Write a message with a mention, send the message, press on the mention, go back, the text will reappear in the composer
• Make changes to react-native-web
• Test on web
  • Test writing a comment in one tab, check that the other tab updates
• Fix web bugs: before, count, start are often incorrect for the different types of events
• Fix web bug where multi-character glyphs are not deleted correctly
• Fix Todos / create issues for it / revert some of the earlier code cleanups that just bloat the changes of this PR
• Raise discussion in RN (link)
• Make PR in RN (link)
• Make a PR in @expensify/react-native-live-markdown

Fixed Issues

$#37896
$#44185
PROPOSAL:

Tests

• Check that the original bug is fixed, reproduction:
  • Spam the 'a' key, typing 'aaaaaa...'
  • Press send
  • Spam the 'b' key, typing 'bbbbbb...'
  • Expect that the composer shows first 'aaaaaa', then clears it, then it shows 'bbbbbb'
  • NOTE: In development mode there is still a severe lag on the JS thread, so it might be that the events are not happening in sync with the user action. It's not a goal of this PR to fix this performance issue (will create a new issue for that; the issue is not present in release mode interestingly)
• Check everything in regards to the composer:
  • add text
  • delete text
  • paste text
  • select text and replace it with pasting
  • insert emojis
  • try mentioning someone
  • use the :colon: format for adding emojis
  • check the composer is synchronised properly between tabs
  • All markdown commands work
  • Any other uses cases you can think of with the composer

Offline tests

n/a

QA Steps

Same as testing steps

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 the expected offline behavior in the Offline steps 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • 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 is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• 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(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• 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.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

android_native.webm

Android: mWeb Chrome

Screen.Recording.2024-07-02.at.17.14.16.mov

iOS: Native

ios-native.mov

iOS: mWeb Safari

Screen.Recording.2024-07-02.at.17.15.43.mov

MacOS: Chrome / Safari

Screen.Recording.2024-07-02.at.17.11.44.mov

MacOS: Desktop

Screen.Recording.2024-07-02.at.17.18.58.mov

[hannojg]

Screen.Recording.2024-07-05.at.13.21.53.mov

Currently investigating one bug where on web when selecting the whole text, deleting it, and then typing the full text reappears with the new text prefixed

[hannojg]

Okay, pushed a fix. Next bug I am looking into is when pasting the content is currently doubled:

Screen.Recording.2024-07-05.at.13.35.39.mov

[hannojg]

Hm, might be a regression that has been introduced here:

• Unable pasting photos and styled text react-native-live-markdown#369 (comment)

[hannojg]

Okay I just reverted the changes from the PR in the patch - it's working now. I think the pasting issue is a bug that needs to be handled separately once we upgrade live-markdown >= 0.1.92

[Skalakid]

Hi @hannojg, we worked recently on fixing double pasting issue and we found a fix for it. I will create a PR that with the changes and live-markdown bump to the latest version. Is it okay for you?

[hannojg]

Hi @hannojg, we worked recently on fixing double pasting issue and we found a fix for it. I will create a PR that with the changes and live-markdown bump to the latest version. Is it okay for you?

Sure! 😊

[Skalakid]

@hannojg Here is the PR with the fix

[hannojg]

TODO: The rn-live-markdown PR has just been merged. I need to update to version 0.1.103 and remove the patch

[hannojg]

Added a PR here to fix one issue on the web:

• fix(web): onChange event start negative react-native-live-markdown#433

can you maybe have a look @Skalakid ?

[hannojg]

Found another bug in this PR on web:

• write aaaaa
• write bbbb
• press ctrl + z, text should be aaaaabbb
• write c
• Expected: aaaaabbbc
• Actual: aaaaabbbcb

[hannojg]

Opened this PR to address the bug:

• fix(web): handle text input metrics correctly on history undo events react-native-live-markdown#434

[hannojg]

I implemented a simpler solution over here:

• Composer: add clear command that bypasses the event count #45613

[hannojg]

(we can reopen this PR in case the other PR wouldn't work for any reason)