IOU - digit appears cutoff for a moment when entering a digit before decimal point

[narefyev91]

Details

When trying to add new digit before dot, app cuts off the digit for few seconds on android chrome

Fixed Issues

$ #17801
$ #17801 (comment)

Tests

1. Open the app on android chrome
2. Click on plus
3. Click on request money/send money
4. Write any amount of digits, add dot and now get the cursor before dot
5. Insert any digit (2 and 5 has most amount of cut and lag) and observe that digit is cut off for few seconds and dot is also not visible for that moment

• Verify that no errors appear in the JS console

Offline tests

Same as above

QA Steps

Same as above

• Verify that no errors appear in the JS console

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 / Chrome
  • iOS / native
  • iOS / 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 approved by marketing by adding the Waiting for Copy label for a copy review 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 */
  • 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 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(themeColors.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 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

android-web.mp4

Mobile Web - Safari

ios-web.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

[melvin-bot]

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

[melvin-bot]

@aldo-expensify @parasharrajat One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[ArekChr]

Overall LGTM 👌

[parasharrajat]

Is there a reason to keep this draft?

[narefyev91]

Is there a reason to keep this draft?

Based on Callstack workflow - we need to get local approve -> pass QA approve -> and after it - i can make PR as ready for review

[narefyev91]

Is there a reason to keep this draft?

Our QA is on vacation now - in that case - PR is ready for review

[parasharrajat]

Will review this today.

[parasharrajat]

BUG: WEB: Cursor jumps to the end.

Steps:

1. Type any long number on the amount input.
2. Put the cursor in the middle somewhere.
3. type a digit.

BUG: WEB: Cursor jumps to the end when a digit is deleted from the middle.

Steps: same as above.

[narefyev91]

BUG: WEB: Cursor jumps to the end.

Steps:

1. Type any long number on the amount input.
2. Put the cursor in the middle somewhere.
3. type a digit.

BUG: WEB: Cursor jumps to the end when a digit is deleted from the middle.

Steps: same as above.

Hey @parasharrajat is it related to mobile web or web itself?

[parasharrajat]

It's desktop web browser.

[narefyev91]

It's desktop web browser.

Fixed - please check again

[parasharrajat]

Ok, it seems we are just preventing the new logic on desktop browsers to solve the bug.

[parasharrajat]

Can we try to find a way to fix this bug's root cause? Or Can you share more details about your analysis on this bug?

[aldo-expensify]

As @parasharrajat posted, I agree it would be good to investigate this further before just disabling it for desktop. If we don't understand the cause, we may be creating a bug.

[narefyev91]

Can we try to find a way to fix this bug's root cause? Or Can you share more details about your analysis on this bug?

Hey sure - the root case of the bug - calculation width for text input should be dynamic (as it is now) but new width should be applied before final value comes inside input.
The sequence is this - click on numpad -> send value to BaseTextInput as prop -> in BaseTextInput put value in state -> Show value in TextInput -> fire onLayout for hidden field -> put in state new width -> apply new width to TextInput
Some times android hangs during multiple rendering (the issue not happened all the time). It also happened to me inside samsung browser - which indicates that it's not really a browser issue, but issue with how new styles applies during re-renders for android web (IOS web does not have those issues).
Not sure that all changes should be done for all other platforms - because there are no issue with them, and i think we should only apply this for platform specific.

Other case why we should extract only for android web - because for desktop and web - we use local state inside BaseTextInput - and not using it - will cause issue with jumping cursor - why it's happened? Because when we set state locally it will immediately applies to text input and text input does not have any maxLength limitation (everything controlling outside). You can see on screenshot - that in console log - i was trying to add 2 between 1 and 3 (position 2).

In render TextInput will get this 2 and cursor will stay on the same position -> next re-render - we get value from props (when value will be formatted) - and cursor position will stay the same - but only final value changed and applies (because local value is not equal value from props). And when we not use local state - input will get every time formatted value from props - and when you try to insert new number - text input will receive same value (state value will be the same as prop value) - and text input will automatically move cursor (as described in this issue #16385)
In any case - we should not expect that android will handle this for us - we should use correct workflow for dynamic width - value should be applied after new width ready to be used.
@aldo-expensify @parasharrajat

[strepanier03]

@aldo-expensify @parasharrajat - Friendly bump on @narefyev91 comments above.

[parasharrajat]

Will be checking in in one hour.

[aldo-expensify]

Same, I spent some time on Friday by the end of the day trying to understand the problem and changes. I'll get back to it today.

[aldo-expensify]

Sorry, I didn't get to it today, I'll try tomorrow again

[parasharrajat]

Definitely need help from @aldo-expensify for the next steps.

[narefyev91]

@parasharrajat what you described here: #18579 (comment) only happens when there is no room for more digits, right?

If that is the problem, by disabling what we did here in desktop, I can see the same bug in Android web.

@aldo-expensify is it possible to see this on any video? because currently i was not able to reproduce on real device

8mb.video-y1D-6vlZXTys.mp4

[strepanier03]

@aldo-expensify - Little bump on #18579 (comment) so we can keep this moving forward please.

[aldo-expensify]

@aldo-expensify is it possible to see this on any video? because currently i was not able to reproduce on real device

I'll try again and get a recording if I can replicate

[aldo-expensify]

@parasharrajat can you spend some time investigating this and possible side effects of the solution? I think your thoughts can be useful here. I don't think I have anything in my environment that allows me to debug this better than you.

[aldo-expensify]

@aldo-expensify is it possible to see this on any video? because currently i was not able to reproduce on real device

I'll try again and get a recording if I can replicate

@narefyev91 ahh I noticed that this is reproducible in android mWeb if you use you computer's keyboard... but if you use the keyboard in the view it doesn't happen. I think we can ignore this report then for mWeb because it doesn't happen in a real use case.

[aldo-expensify]

To summarize...

• The PR is delaying setting the input's value until after the width has been calculated with the <Text> that is out of view
• This causes a small bug when you use a keyboard that is not the <BigNumberPad number pad we show in mobile
• To fix the bug, we disabled what this PR is doing for desktop size application (see this commit)

This last point feels like workaround and it would be good to decide if we are fine with it, and see if there is a better solution for it. It also feels like a workaround because the digit "cutoff" issue probably exist in desktop too, but it just happens too fast and we don't see it.

Do we know if a keyboard that is not <BigNumberPad can ever show up in these views in small screens? I'm guessing that the answer is no.

[aldo-expensify]

Asked here for advice on how to proceed: https://expensify.slack.com/archives/C01GTK53T8Q/p1685477825309709

We will accept the workaround, but we should add a comment above shouldWaitWidthCalculation={BrowserUtils.isMobile()} saying why we disabled this for desktop and mention the cursor jumping bug.

[aldo-expensify]

@parasharrajat can you test and review this again 🙏 ?

[parasharrajat]

I will do that asap. Overall, I was asking about the logic on this PR. Took some time off for health reasons.

[aldo-expensify]

I will do that asap.

thanks

Overall, I was asking about the logic on this PR.

Ok!, I'm fine with the delay mechanism implemented, the main push back for me was the workaround of disabling it when not in mobile.

Took some time off for health reasons.

I hope you area feeling better!

[narefyev91]

To summarize...

• The PR is delaying setting the input's value until after the width has been calculated with the <Text> that is out of view
• This causes a small bug when you use a keyboard that is not the <BigNumberPad number pad we show in mobile
• To fix the bug, we disabled what this PR is doing for desktop size application (see this commit)

This last point feels like workaround and it would be good to decide if we are fine with it, and see if there is a better solution for it. It also feels like a workaround because the digit "cutoff" issue probably exist in desktop too, but it just happens too fast and we don't see it.

Do we know if a keyboard that is not <BigNumberPad can ever show up in these views in small screens? I'm guessing that the answer is no.

Yup keyboard will never be shown for small screens for sure.

[narefyev91]

@aldo-expensify @parasharrajat hey guys - what will be the next steps here?

[parasharrajat]

I will test it in a few hours. Wasn't working till today.

[narefyev91]

@parasharrajat @aldo-expensify do you think guys this we should move this ticket on hold - because seems this one #19961 and #16696 will fully change the logic of current component and potentially will fix current issue with cut of?

[hannojg]

Hey, I just quickly tested the changes from #16696 PR, and confirm the effect is less visible:

Screen.Recording.2023-06-06.at.14.58.11.mov

However, the issue is still there. If you go frame by frame through the video you can see that for one frame the state is still there.
That was something I intended to fix in #19961 by rewriting the autoGrow logic. (However, up until now I wasn't aware of this PR 👀 )

[narefyev91]

Hey, I just quickly tested the changes from #16696 PR, and confirm the effect is less visible:

Screen.Recording.2023-06-06.at.14.58.11.mov
However, the issue is still there. If you go frame by frame through the video you can see that for one frame the state is still there. That was something I intended to fix in #19961 by rewriting the autoGrow logic. (However, up until now I wasn't aware of this PR 👀 )

Interesting that this issue mostly reproduced on real device - on emulator i was not able to see such massive delay between expanding width and inserting new character. I think it will make cense to recap when your PR will be merged

[strepanier03]

@parasharrajat and @aldo-expensify - Bump on the comments here and here.

After looking it over, can you share the next steps that should be done here? Do you think we should move forward and finish up this PR? If so, how close are we to merging?

[narefyev91]

@strepanier03 Hey! I think we are in a process of re-writing the whole area - this is the first one PR #20186, which i hope will be good to go soon, and this one margelo#10 - will also fix all issues which we faced with auto growing - in my view - we should put this onhold - and waiting when all pr's will be finalised and merged (also we need a new version of react native merged to Expensify Expensify/react-native#55). Hope that makes cense

[parasharrajat]

Thanks @narefyev91 for the update. I agree, we should redo this PR.

Let's hold this PR and I will watch the other ones.

[parasharrajat]

@strepanier03 We are doing a major refactor of the main component in #20186 which will make the changes of this PR obsolete. The timeline to finish this #17801 might be close to 15 days. There will be 2 PRs to complete this.

1. First, Main refactor [Fix #16696] Rewrite BaseTextInput to function component #20186.
2. Second, [WIP] Fix/auto grow textinput margelo/expensify-app-fork#10.
3. Update the RN version which is needed for a few issues so might be done parallelly. We are in the process of releasing a new version.

[parasharrajat]

We can move forward here.

[parasharrajat]

What is the latest on this? Do we still need margelo#10?

[hannojg]

Yeah the PR would still be needed + probably a fix on the native side (or a good other solution that doesn't run into the native bug).
This is currently on the back burner, while we work on high priority performance tasks.

[hannojg]

We could also think about opening this to external contributors, however, as the underlying issue is one in react-native I'd be afraid of proposals that are rather esoteric, trying to work around the bug instead of really trying to fix the underlying problem.

[parasharrajat]

Agree. Happy to wait here.

[aldo-expensify]

Agree too, lets hold