Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. #11146

Closed
kavimuru opened this issue Sep 20, 2022 · 27 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kavimuru
Copy link

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:

  1. Open the App
  2. Login with any account
  3. Click on the FAB button and select request money
  4. Enter an amount

Expected Result:

The cursor must remain on the last digit when adding digits

Actual Result:

The cursor moves to the first digit while adding other digits.

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS ( only in iOS 16)

Version Number: v1.2.3-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5740196_cursor_1909.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2022

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Sep 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2022

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

@melvin-bot melvin-bot bot changed the title iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [$250] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Sep 20, 2022
@laurenreidexpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added the Overdue label Sep 26, 2022
@thienlnam thienlnam added Weekly KSv2 and removed Daily KSv2 labels Sep 26, 2022
@melvin-bot melvin-bot bot removed the Overdue label Sep 26, 2022
@laurenreidexpensify laurenreidexpensify changed the title [$250] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [$500] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Sep 28, 2022
@laurenreidexpensify laurenreidexpensify changed the title [$500] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Sep 30, 2022
@laurenreidexpensify laurenreidexpensify changed the title [$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [$500] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Sep 30, 2022
@laurenreidexpensify laurenreidexpensify changed the title [$500] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Oct 4, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 5, 2022
@thienlnam
Copy link
Contributor

Pending proposals

@melvin-bot melvin-bot bot removed the Overdue label Oct 6, 2022
@Uros787
Copy link
Contributor

Uros787 commented Oct 10, 2022

Proposal

At first, I thought the problem was with the heavy checking we are doing on every click.
BUT, it turns out it's something new introduced in react native
facebook/react-native#34695

The solution is to remove the selection prop from [IOUAmountPage.js] or just comment it out until react native resolves its issue(

<TextInputWithCurrencySymbol
formattedAmount={formattedAmount}
onChangeAmount={this.updateAmount}
onCurrencyButtonPress={this.navigateToCurrencySelectionPage}
placeholder={this.props.numberFormat(0)}
preferredLocale={this.props.preferredLocale}
ref={el => this.textInput = el}
selectedCurrencyCode={this.props.iou.selectedCurrencyCode || CONST.CURRENCY.USD}
selection={this.state.selection}
onSelectionChange={(e) => {
if (!this.shouldUpdateSelection) {
return;
}
this.setState({selection: e.nativeEvent.selection});
}}
/>
)

<TextInputWithCurrencySymbol
  formattedAmount={formattedAmount}
  onChangeAmount={this.updateAmount}
  onCurrencyButtonPress={this.navigateToCurrencySelectionPage}
  placeholder={this.props.numberFormat(0)}
  preferredLocale={this.props.preferredLocale}
  ref={(el) => (this.textInput = el)}
  selectedCurrencyCode={
    this.props.iou.selectedCurrencyCode || CONST.CURRENCY.USD
  }
  // selection={this.state.selection}
  onSelectionChange={(e) => {
    if (!this.shouldUpdateSelection) {
      return;
    }
    this.setState({ selection: e.nativeEvent.selection });
  }}
/>;

I've tested the behavior on android and ios and there is no more flickering.

IOS:

194812047-69ff15d4-115b-4921-8ab2-e06ec1f42508.mov

ANDROID:

194812135-35115418-58f6-4827-8344-566e7b619f40.mov

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 10, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@Uros787,
Thanks, but this is a temporary workaround, not a solution.

If you could solve the linked issue facebook/react-native#34695 by submitting a PR then that's a solution.

cc: @thienlnam

@Santhosh-Sellavel
Copy link
Collaborator

@thienlnam Please add your thoughts too.

@Uros787
Copy link
Contributor

Uros787 commented Oct 10, 2022

Hi @Santhosh-Sellavel , if the prop selection was used somewhere then it would be okay to say that its a workaround
I was checking ios android, web and mweb and selection didn't really have any impact anywhere
Would be great if someone could explain why this selection was added

@Santhosh-Sellavel
Copy link
Collaborator

@eVoloshchak Since you added it, why is selection prop used here, what happens if remove it

@thienlnam
Copy link
Contributor

@Uros787 We've actually been trying to move off of setNativeProps to upgrade to fabric.
Additional context here: #11053. But we can't just remove the selection prop

It actually seems like the bug we're running into here seems to be related to changes we're working on here: #11053 so I am going to put this issue on hold

@thienlnam thienlnam changed the title [$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [HOLD][$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Oct 11, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 13, 2022
@thienlnam thienlnam added Monthly KSv2 and removed Daily KSv2 labels Oct 13, 2022
@melvin-bot melvin-bot bot removed the Overdue label Oct 13, 2022
@thienlnam
Copy link
Contributor

Putting a monthly on this as it's on hold

@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@Santhosh-Sellavel
Copy link
Collaborator

on Hold!

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2022
@thienlnam
Copy link
Contributor

Seems like we've closed a lot of related issues - is this bug still occuring @kavimuru?

@kavimuru
Copy link
Author

@thienlnam Will check with team and update here.

@kavimuru
Copy link
Author

@thienlnam Issue is still able to reproduce.

repro.1411.cursor.mp4

@thienlnam
Copy link
Contributor

Thank you @kavimuru, going to remove the hold on this issue so we can get some proposals

@thienlnam thienlnam changed the title [HOLD][$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. [$1000] iOS 16 - IOU - Request money - The cursor jumps to the first digit for a moment while adding other digits. Nov 14, 2022
@thienlnam thienlnam added Weekly KSv2 and removed Monthly KSv2 labels Nov 14, 2022
@thienlnam
Copy link
Contributor

Issue is back open for proposals!

@laurenreidexpensify
Copy link
Contributor

Bumped in Slack here

@hannojg
Copy link
Contributor

hannojg commented Nov 15, 2022

Would we be open for a proposal where we remove the selection prop? I think I can make it work without the selection prop. Or do we want a upstream RN fix?

@fedirjh
Copy link
Contributor

fedirjh commented Nov 15, 2022

Proposal

Problem

There is an extra state update to selection that's triggered onSelectionChange even when this.state.selection === e.nativeEvent.selection . onChangeAmount (which update state of selection) is fired before onSelectionChange which also will update the same state when user is adding digits

Solution

We have to prevent extra state updates if values are equal

onSelectionChange={(e) => {
if (!this.shouldUpdateSelection) {
return;
}
this.setState({selection: e.nativeEvent.selection});
}}

-       if (!this.shouldUpdateSelection) {
+       if (!this.shouldUpdateSelection || _.isEqual(e.nativeEvent.selection, this.state.selection)) {
            return;
        }

Before

Simulator.Screen.Recording.-.iPhone.13.-.2022-11-15.at.19.04.38.mp4

After

Simulator.Screen.Recording.-.iPhone.13.-.2022-11-15.at.19.12.56.mp4

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 15, 2022
@rushatgabhane
Copy link
Member

Would we be open for a proposal where we remove the selection prop? I think I can make it work without the selection prop.

@hannojg sounds like a great idea to me

@laurenreidexpensify
Copy link
Contributor

As per this update, am closing this issue out.
2022-11-16_15-25-51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

9 participants