search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$1000] IOS - IOUAmount - When entering/deleting numbers quickly, the cursor doubles #15856

Closed kbecciv closed 6 months ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Issue found when executing PR https://github.com/Expensify/App/pull/15710

Action Performed:

  1. Open the app
  2. Login with any account
  3. Tap on the FAB button -> Send/Request money
  4. Quickly enter/delete numbers

Expected Result:

A second cursor doesn't "flash" after the first digit

Actual Result:

A second cursor appears after the first digit on every keyboard press

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.82.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/224388551-beff33ab-17e9-4ef6-807b-f0d61382124b.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014612000725948717
  • Upwork Job ID: 1634282958953353216
  • Last Price Increase: 2023-03-10
MelvinBot commented 1 year ago

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

trjExpensify commented 1 year ago

It's reproducible, but there seems to be some history on it here: https://github.com/Expensify/App/issues/11146. We seem to have closed that issue due to a freeze on keyboard navigation, but I don't actually think it falls under that umbrella.

Let's open this up. I understand the root cause might be upstream so that's the proposal we're probably looking for. Is that right? CC: @fedirjh @hannojg @thienlnam @Santhosh-Sellavel

MelvinBot commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~014612000725948717

MelvinBot commented 1 year ago

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

MelvinBot commented 1 year ago

Triggered auto assignment to @roryabraham (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

roryabraham commented 1 year ago

I agree with @trjExpensify that this looks like a valid bug report. Let's get some proposals here

narefyev91 commented 1 year ago

Hello, I'm Nicolay from Callstack and I'm interested in taking and analysing this issue to work on a fix for it.

MelvinBot commented 1 year ago

📣 @narefyev91 You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

huzaifa-99 commented 1 year ago

Is this issue open for proposals?

MelvinBot commented 1 year ago

📣 @huzaifa-99! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
huzaifa-99 commented 1 year ago

Contributor details Your Expensify account email: huzaifarasheed900@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0158fa4e8207ebbfab

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

mountiny commented 1 year ago

Is this issue open for proposals?

Nope sorry, taken by @narefyev91 to push this forwards

narefyev91 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Text input cursor jumps from first position to last one when user clicking on custom keyboard.

What is the root cause of that problem?

There are 2 main issues here:

  1. React native issue with selection prop and IOS
  2. Combination of custom keyboard, text input and selection inside IOUAmountPage

Basically what happened - when user click on any number we manually assign selection (also doing some setStates and etc) and pass back it to TextInput. Seems on issues in react native github there are some closely issues to current one - that IOS still have this flickering issue. And it should be ok - because selection and onSelectionChange - are designed to be used for something different - than manually changing cursor position. That's why we see that issue. What we can do here? There are some possible workarounds - but the most correct one - to remove selection prop for IOS only (separating files based on the platform). I also already see the same changes in repo for example here: src/components/RoomNameInput, src/components/Composer

What alternative solutions did you explore?

Alternatives maybe some additional workarounds like:

  1. setting by default last position for cursor - which will not make cursor jumping because we will never get more than 8 numbers in that input 
        selection={{
        start: 8,
        end: 8,
    }}
  2. We can fully remove cursor - because we already have custom keyboard and user will not really expect that something should be focused when they open that screen.
eVoloshchak commented 1 year ago

@narefyev91, thanks for the investigation! You are correct abot selection causing the issue, we've encountered multiple problems because of this. We've started tracking selection on IOUAmountPage in https://github.com/Expensify/App/issues/6154. Currently on native platforms you can't move the cursor, so it seems this behavour was specifically disabled and there is no need to have a cursor at all. If that is the case, we should just remove selection for native platfroms and fully remove cursor, as @narefyev91 has suggested. But i'd like to clarify the expected behavior

@roryabraham

  1. Currently on Android/iOS user is unable to move cursor when etering amount
  2. on mWeb user is able to move the cursor, but the numbers are still added to the end of the string, not to the position of the cursor

Is that the expected behavour? If so, we can fully remove the cursor on iOS/Android/mWeb as per @narefyev91's proposal

trjExpensify commented 1 year ago

Rory can chime in, but that's not expected behaviour. You should been able to tap to move the cursor and add/remove numbers from the cursor position on native.

eVoloshchak commented 1 year ago

@trjExpensify, I took a look at the code and it seems like it's a regression from https://github.com/Expensify/App/pull/15710. I've left a comment there with a potential fix, we might wait for that issue to be resolved before proceeding with this one

narefyev91 commented 1 year ago

@eVoloshchak not really - the issue for IOS is still there - regression from https://github.com/Expensify/App/pull/15710 is not really related to how cursor is jumping. It's jumping because we using selection which is not correct way

eVoloshchak commented 1 year ago

@narefyev91, I meant the issue with not being able to move cursor at all. This issue (with cursor jumping on iOS) is undeed unrelated to https://github.com/Expensify/App/pull/15710

mountiny commented 1 year ago

Correct, the cursor unable to move is being fixed here https://github.com/Expensify/App/pull/16340

narefyev91 commented 1 year ago

yup that's 100% true @eVoloshchak and seems simply removing selection - fix the issue on IOS

https://user-images.githubusercontent.com/28352309/226628137-f9af9f4f-6217-418a-be18-500da8fd5f5f.mov

mountiny commented 1 year ago

@narefyev91 whats the purpose of the selection then, will this work same on Android and both mWeb after removing this prop?

narefyev91 commented 1 year ago

@mountiny selection is need to select text from start to end position - and it should not be used as cursor position to be honest. The fix will be applied only for IOS (the same logic were done in this components already src/components/RoomNameInput, src/components/Composer) - for example Simulator Screen Shot - iPhone 14 Pro - 2023-03-21 at 16 00 27

mountiny commented 1 year ago

Ahahaha, sure 🤦

@trjExpensify what do you think of dropping ability to select on native, I feel like we should keep this and try to find some other way around this

eVoloshchak commented 1 year ago

@narefyev91, nice, it does work for iOS! It is breaking Android, so we would have to apply this for iOS only As far as I can tell we don't lose the ability to select with this fix and there are no obvious cons, is that correct?

We need to try and investigate why this works the way it works. Why does this work for iOS, but not for Android? I remember that there were quite a few issues on IOUAmount page, and controlled selection might be responsible for fixing some of them. Ideally we need a solution for all platforms, or if there is a bug inside react-native, a fix for that.

narefyev91 commented 1 year ago

@eVoloshchak as I see there are some same issues with how ios and selection is working, ex. https://github.com/facebook/react-native/issues/32499 Here also good explanation that selection is really buggy https://github.com/Expensify/App/pull/12632 And the same issue will happened if we comment last line for writing a message. 

        // On native layers we like to have the Text Input not focused so the
        // user can read new chats without the keyboard in the way of the view.
        // On Android, the selection prop is required on the TextInput but this prop has issues on IOS
        // https://github.com/facebook/react-native/issues/29063
        // const propsToPass = _.omit(this.props, 'selection');

https://user-images.githubusercontent.com/28352309/226885278-224f4eff-c3a9-4d04-895a-b0d49c569615.mp4

That's why i think possible solution for now to remove selection for IOS and maybe when RN will be updated or we make migration to fabric - re-visit all inputs and see if we will not need this platform separation any more

trjExpensify commented 1 year ago

@trjExpensify what do you think of dropping ability to select on native, I feel like we should keep this and try to find some other way around this

To answer this @mountiny, I agree. 👍 If we're sure of the root cause, let's bring it out for discussion because this does break cross-platform consistency.

narefyev91 commented 1 year ago

@trjExpensify @eVoloshchak not sure if i get it right - should i add this fix for IOS only - or for now this issue should be on hold until some discussion will be finalised?

eVoloshchak commented 1 year ago

@eVoloshchak as I see there are some same issues with how ios and selection is working, ex.

@narefyev91, yeah, there are a lot of problems with controlled inputs indeed

This might explain why we have our issue only on iOS: https://github.com/facebook/react-native/issues/28865

However, migrating to controlled selection was a prerequisite for enabling Fabric. I'm not sure what the timeline on that is and if we should implement a temporary fix since this should (in theory) be resolved when fabric is enabled

mountiny commented 1 year ago

However, migrating to controlled selection was a prerequisite for enabling Fabric. I'm not sure what the timeline on that is and if we should implement a temporary fix since this should (in theory) be resolved when fabric is enabled

I think we are getting quite close to enable Fabric so maybe we could hold for that if it should help

roryabraham commented 1 year ago

Putting this on HOLD for Fabric then: https://github.com/Expensify/App/issues/8503

roryabraham commented 1 year ago

If this comes off HOLD and these upstream problems with selection still exist, we should set our sights on fixing them upstream

trjExpensify commented 1 year ago

Cool, this on hold. Dropping to weekly for now.

eVoloshchak commented 1 year ago

Not overdue, the issue is on HOLD

trjExpensify commented 1 year ago

No change, Melv.

s77rt commented 1 year ago

@eVoloshchak or anyone. Any context on why we think migrating to Fabric will fix selection issues?

eVoloshchak commented 1 year ago

@s77rt, since this issue is present only on iOS, there is a big chance it's caused by https://github.com/facebook/react-native/issues/28865, which was fixed for Fabric by https://github.com/facebook/react-native/commit/7b4889937ceb0eccdbb62a610b58525c29928be7.

UPD I just noticed the PR you've opened (https://github.com/facebook/react-native/pull/35603), it was merged in v0.72.0-rc.1, I'll check if it resolves the issue today

s77rt commented 1 year ago

@eVoloshchak I fixed the linked issue for non-fabric too by https://github.com/facebook/react-native/commit/112bfeecfac393384de99866569b311dd00a6908. And we are using that fix already since we upgraded our RN recently.

If that was the sole reason to hold, then maybe we should reopen as I think it's unrelated. Or better yet, create a tracking issue, we have so many bugs regarding controlled selection for both iOS and Android.

eVoloshchak commented 1 year ago

I fixed the linked issue for non-fabric too by https://github.com/facebook/react-native/commit/112bfeecfac393384de99866569b311dd00a6908. And we are using that fix already since we upgraded our RN recently.

Thanks for clarifying I also think we should create a tracking issue, there are a lot of similar issues with controlled selection

eVoloshchak commented 1 year ago

This shouldn't be on HOLD for #8503, since it won't resolve the issue

https://github.com/Expensify/App/pull/17687 is moving to using setSelection lib to manipulate the selection directly, which might be the approach to resolve this issue too

s77rt commented 1 year ago

@eVoloshchak I think we should still hold this though but for https://github.com/Expensify/App/pull/17687 just to validate that setSelection is a reliable approach.

trjExpensify commented 1 year ago

@s77rt we've now closed that linked PR. The last comment references moving the discussion to Slack, did that happen?

Let's make sure to keep this issue up-to-date with the latest. When that discussion progresses we can move to align everything with this suggestion:

I also think we should create a tracking issue, there are a lot of similar issues with controlled selection

s77rt commented 1 year ago

@trjExpensify Not yet. I think we are waiting for @hannojg for that.

trjExpensify commented 1 year ago

Okay, @hannojg can perhaps fill us in! :)

hannojg commented 1 year ago

Yes, we are working on a final proposal to fix the following input related bugs:

Expect a solution / proposal by next week 😊

trjExpensify commented 1 year ago

Okay, excellent. Can you make sure to link the slack discussion here when you post? Thanks!

trjExpensify commented 1 year ago

@hannojg, any movement?