App: [HOLD for payment 2022-09-07] [$2000] IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
- Navigate to a conversation
- Click on attachment button
- Click on request money
- On the amount, enter 1550
- Try to modify the amount
Expected Result:
User should be able to modify the amount by either using the < back caret from the end of the number, or highlighting any grouping of numbers and deleting them with a single keypress using the < caret.
Actual Result:
User is unable to tap to highlight the numbers on the BNP
Workaround:
Use the back caret < to delete/modify from the last number.
Expected Solution:
- Fix the issue on all platforms by using the
selectionandvalueprop of TextInput. - Don’t use setNativeProps as it won’t be supported in Fabric
- Don’t use any workaround like setTimeout, setImmediate, requestAnimationFrame etc.
Drawbacks of the expected solution:
- On Android, Copy / Paste menu will not work. But it’ll eventually be fixed by https://github.com/Expensify/App/issues/8349
- From past experience, I’m assuming poor performance (at least on the dev build) when
- typing quickly
- or moving the cursor around
I believe that upgrading to the new Fabric architecture will fix both the drawbacks (anecdotal evidence). We already have a ticket for the upgrade over here - #8503
Platform:
Where is this issue occurring?
- iOS
- Android
Version Number: 1.1.12-0
Reproducible in staging?: Yes Reproducible in production?: Yes Logs: https://stackoverflow.com/c/expensify/questions/4856 Upwork: https://www.upwork.com/jobs/~017b2406f1c8e307db
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @Santhosh-Sellavel Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1635545417275400
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 159 (119 by maintainers)
Commits related to this issue
- Merge branch 'main' into fix-#6154 — committed to sig5/App by sig5 2 years ago
- Merge branch 'fix-#6154' into testing-6154 — committed to sig5/App by sig5 2 years ago
- Merge branch 'fix-#6154' into testing-6154 — committed to sig5/App by sig5 2 years ago
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
I’ve tried running on a real device, cannot reproduce this bug.
https://user-images.githubusercontent.com/9059945/180599821-a205ed7e-29e4-4899-add7-44f395d565e4.mp4
In https://github.com/Expensify/App/pull/7433 selection logic for
mWeb(and everything exceptiOSfor that matter) was handled viasetNativeProps, and since we use controlled selection for all platforms, this bug doesn’t appearNo problem. This is a rare edge case, I agree
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
@trjExpensify yep, and it just got fixed in
main. Back to reviewingunder review
@eVoloshchak
setNativePropswon’t be supported in the new fabric architecture https://github.com/Expensify/App/issues/6154#issuecomment-1171764558. So let’s not use it 😃P.S. I really appreciate that you tested and pointed out the bugs in all platforms
Awaiting more proposals
:nevermind: @chiragsalian @trjExpensify got a reply from a RN maintainer that
setNativePropswill not be supported in the new Fabric architecture.Let’s throw the expected solution out of the window. This is great news! It saved us so much time and there is only one possible solution now! i.e to put everything in
state.Perfect, that looks much better to me. Thanks for the update @rushatgabhane. @trjExpensify feel free to update the OP.
This is the day that I dreaded the most 😂 But sure, im fine if you assign it to me.
@Santhosh-Sellavel this will be funny if it works, maybe increasing the width by 2:
layout.width + 2fixes the mWeb bug? (The first one - “on Android end of selection isn’t visible sometimes”)https://github.com/Expensify/App/blob/db7a1b751d9a34cac51dffd54e18907d8ecb8dd9/src/components/TextInput/BaseTextInput.js#L307
@eVoloshchak The proposal looks good as it is mostly based on the existing PR.
Read our Philosophy point 5 we don’t encourage using
Platform.OS. So please post an updated proposal.cc: @chiragsalian
Thanks @trjExpensify , really appreciate it!
@chiragsalian assigned you to this issue and the PR since I’m going on sabbatical for 2 months. Thanks!
Done! Thanks 😄
Got it, that makes sense. @trjExpensify let’s go forward with hiring @sig5 for this job. Thanks!