Added ability to modify IOU Amount

[sig5]

Details

The NumberPad was missing handling for some features, Added handling for different use cases. We are modifying the selection prop via the selection API and optimistic updates are being done in IOUAmount.js
Note:
Due to the development overheads, the dev App might have some cursor inconsistencies in the dev build. The App works fine in the release builds.
Edit: The components have been restructured such that:

1. iOS uses controlled selection.
2. Rest of the platforms use uncontrolled selection.

Fixed Issues

$ #6154

PR Review Checklist

Contributor (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 tagging the marketing team 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 */
  • Any functional components have the displayName property
  • 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
• 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.

PR Reviewer Checklist

• 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 verified tests pass on all platforms & I tested again 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 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 tagging the marketing team 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 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 */
  • Any functional components have the displayName property
  • 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.

Tests

• Verify that no errors appear in the JS console

QA Steps

1. Go to Send/Receive Money for a chat.
2. Test the following use cases

• Delete a block of text after selecting.
• Delete a single character after entering.
• Select a set of characters and replace it with another character.
• Select one character and replace it with any other character.
• Check if the input allows for invalid values.
• Try copying and pasting valid text in the input.

3. Verify that correct values appear for the operations.

• Verify that no errors appear in the JS console

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Screencast.from.27-01-22.09-36-06.AM.IST.mov

Mobile Web

WhatsApp.Video.2022-01-27.at.9.48.05.AM.mp4

Desktop

out2.mp4

iOS

out3.mp4

Android

WhatsApp.Video.2022-01-27.at.9.29.59.AM.mp4

[sig5]

@rushatgabhane I do not have access to an iOS device rn, So I couldn't test it there.I hope you can verify if it works :D

[rushatgabhane]

@sig5 The cursor position isn't being updated on iOS, can you find a fix for that? android is working tho

[sig5]

Hi folks, sorry for the delay. I have been trying to find out the reason for this erratic behavior on iOS. I will list down the approaches and issues with them down below:

1. The initial solution which involves the standard approach of keeping selection as a state variable and updating state on onSelectionChange Callback seems to work fine on both platforms. But there is a BUG on android. The issue thread mentions it. The copy-paste menu tends to get hidden on Android devices. This solution seems to work fine on iOS though.

2. The approach we took to counter the aforementioned bug is to set the selection on the TextInput using setNativeProps (i.e the selection API).. The approach seemed to work fine on the Android devices, but the iOS devices perform poorly. The selection value of the native element doesn't seem to update as expected. The issue though seems to be internal to react-native as the setNativeProps calls seem to not work only on ios devices for us.

I am trying to add find any more suitable solution if I can. Currently, the two available approaches work exclusively on iOS and Android respectively.

[rushatgabhane]

I am trying to add find any more suitable solution if I can

@sig5 let me know if you have any updates, thanks.

[sig5]

Hi guys,
It seems like the selection API might have some issues here on iOS. The setNativeProps does not play as expected. These issues might also affect our future plans to make TextInputs uncontrolled for better performance. Below are the observations(The behavior is as expected in Android devices).

1. The selection updates work as expected if the text prop of the Input element stays unchanged. Relevant snack : link

2. The unexpected behavior occurs on the TextInput when we start updating the text prop as well as selection prop and selection.start !==selection.end. The selection prop of TextInput doesn't get updated to the expected value. Snack: link

3. When testing I found that the index placements of the API can be unexpected too. The immovable cursor problem can be resolved up to an extent by setting the index at { start: expectedStart-1, end: expectedEnd-1}(which is unexpected and was
   found while trying to debug the code) . But this feels like a workaround.

The issue @rushatgabhane mentioned in the previous comment was faced here too: #6876 (comment).
EDIT:
In my view, We should possibly segregate the logic for iOS using the passing down the props approach and open a tracking issue in the react-native library.

[rushatgabhane]

@sig5 sorry, will get back to you tomorrow.

[rushatgabhane]

@sig5 thanks for your effort, and attaching some snack links. Although, a video for this repo would be preferred next time.

The unexpected behavior occurs on the TextInput when we start updating the text prop as well as selection prop and selection.start !==selection.end. The selection prop of TextInput doesn't get updated to the expected value. Snack: link

Can you help me with a case where this is applicable to the issue or anytime in future?

The selection updates work as expected if the text prop of the Input element stays unchanged.

Do you think direct manipulation of value using setNativeProps will help?

[sig5]

Hi @rushatgabhane ,

@sig5 thanks for your effort, and attaching some snack links. Although, a video would be preferred next time.

I added snack links because this seems like a react native issue and wanted to demonstrate the same. :D will take care of it from now on.

Can you help me with a case where this is applicable to the issue or anytime in the future?

This issue doesn't concern us as of now, but I have mentioned it if we come across it in the future. The behavior of the selection prop is unexpected in the other cases too (https://snack.expo.dev/@sig5/add0b7).And these don't even consider block delete/update yet.

Screencast.from.04-02-22.07-23-13.AM.IST.mov

Screencast.from.04-02-22.07-24-02.AM.IST.mov

Do you think direct manipulation of value using setNativeProps will help?

I have tried to do that. But that doesn't help.

[rushatgabhane]

Okay, I see what you mean.
iOS selection is broken when we set selection using native native method. But it works fine when we pass it as a prop.

We should possibly segregate the logic for iOS using the passing down the props approach and open a tracking issue in the react-native library.

I'd like us to set selection using native props as it's faster but we can't do that for iOS yet.
So let's create a seperate iOS file which uses the selection prop to fix this issue. I hope that makes sense

Could you also link/create a relevant react-native issue?

[sig5]

So let's create a seperate iOS file which uses the selection prop to fix this issue. I hope that makes sense

Yes, that will be ideal.

Could you also link/create a relevant react-native issue?

Sure I will do that.

[rushatgabhane]

@sig5 please merge main from upstream into your branch, and re-test all platforms because there have been many changes to TextInput. Let's also add screenshots for Safari

[rushatgabhane]

@sig5 how's it looking?

[chiragsalian]

Hey there @sig5 👋 , so any update here?
Just wanted to let you know that as per our contributing guidelines, we mention,

Any issue that doesn't receive an update for 1 full week may be considered abandoned and the original contract terminated.

We'll give you this week to provide an update but if there is no update we'll have to consider the PR as abandoned and we won't be able to pay you for your work even if the remaining work is minimal. So anyway, if possible we're hoping you would still continue the task here since the only thing pending at this time is this comment for you to address. You've done good work here so it would definitely be a shame if this was considered abandoned.

[sig5]

Hi @chiragsalian ,
this PR has been particularly difficult to manage because of the textInput refactoring PR's which are being continuously merged over the past few weeks. The changes being done over those PR's remain a tad bit unclear due to lack of detailed plan/comment. These changes lead to breaking changes in the current PR. Every eventual merge leads to new issues while resolving this issue, and leaving the PR owner to analyse the whole set of changes right from the beginning.
I appreciate and totally support the refactors taking place, but since its a bit too coupled with the PR, I was having issues with the PR.
I would still like to work on this PR. I will post an update about potential changes that I might need to do to the TextInput over the couple next days, if that works fine?

[chiragsalian]

Sure thank you for your update. Yes your plan sounds find to me.

As for the concern about merge conflicts arising due to refactors elsewhere, thats a valid concern. Something that could help is to work fast to get close to the end and then block off time by us the reviewers so that we can work fast in that timeframe to review multiple times to get this PR out the door 🙂 The reason i say this is because there are tons of people working on this repo so the more days we wait for an update the more conflicts will arise so it'll be easier to go faster. Let me know if that helps.

[sig5]

That sounds good to me!

[sig5]

I have made a few changes that were an issue in the previous PR and merged main. The current PR tests well for me. The changes are merely modifications to the previous PR as per the modifications in main.

[rushatgabhane]

@sig5 bug: copy pasting amount doesn't work on iOS - Safari.

Screen.Recording.2022-03-26.at.9.27.14.AM.mov

[sig5]

Hi @rushatgabhane, iOS mWeb seems to natively append a space before the pasted text. I have added a replace call using regex for tackling the issue.

[rushatgabhane]

Thanks @sig5, could please add the copy paste thing in QA steps.

Block updates
Single Updates
Invalid Updates (Length/Decimal etc)

And also please overexplain the QA steps so that someone unfamiliar with the issue can also test the PR, for all cases.

[sig5]

Done! @rushatgabhane

[rushatgabhane]

@sig5 kindly add the PR checklist to your post (it didn't exist when your PR was created)
https://github.com/Expensify/App/blob/main/.github/PULL_REQUEST_TEMPLATE.md

[sig5]

Done! @rushatgabhane

[rushatgabhane]

@sig5 the cursor position is wrong when copy pasting in Android.

[rushatgabhane]

@sig5 any updates? how's it looking?

[sig5]

Hi, I have been looking at this issue for a while.

So this caret positioning seems to be off on android and iOS- Safari mWeb. But since the same code works well on all platforms that use index.js . This seems to me like an native issue.

This bug appears in the following cases

• Android - On pasting, cursor stays in same position. (Expected is - cursor goes to end of pasted text)
• mWeb for iOS - On pasting in middle of some amount (expected 123___|789), cursor goes to end of amount. (actual 123___789|)

I have been trying to find the cause but am unable to find the exact cause, so please let me know the next steps.

cc: @rushatgabhane @chiragsalian

[rushatgabhane]

I have been trying to find the cause but am unable to find the exact cause, so please let me know the next steps.

@sig5 cursor position after pasting in compose box is correct on all platforms.
So can you please help us understand why it doesn't work for IOUAmount page?

[rushatgabhane]

@sig5 / @chiragsalian this PR can be closed as it's no longer being worked on.