Fixed moving cursor to end problem except IOUAmountPage

[tugbadogan]

@johnmlee101

Details

• Added forceMoveCursorToEnd prop to TextInputFocusable component.
• Set this prop in IOUAmountPage to keep the cursor at the end all the time.

Fixed Issues

Fixes #1691

Tests

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Screen.Recording.2021-03-23.mov

[roryabraham]

LGTM 👍

[roryabraham]

Tested on desktop too, seemed to solve the issues I'm seeing on master

[tgolen]

Hold off on merging this for a second. I'm not sure I agree with the solution here and I want to dig into it a little bit.

[tgolen]

OK, here is my thinking.

The <TextInputFocusable> component is using the defaultValue prop to update the value of the <TextInput> and I think this is wrong. The purpose of defaultValue is to set the initial value only, and after that, since this is a controlled component, then value should be the prop that is updated. This should be refactored IMO.

This refactoring would make the component much more traditional, but won't solve the problem itself. I actually am not entirely sure what the original problem was. The commit this was introduced was in 65fe5a9 and I would like to get @NikkiWines context about the reason that updateSelection() was created (which was eventually renamed to moveCursorToEnd).

I'm almost thinking that there should be two separate TextInputFocusable components so we aren't jamming too much functionality into a big monolith component.

[tgolen]

Sorry, I meant to ping @tugbadogan about that previous PR. Could you please see my previous comment and see if you can provide more context around the need for updateSelection()?

[tugbadogan]

Sorry, I meant to ping @tugbadogan about that previous PR. Could you please see my previous comment and see if you can provide more context around the need for updateSelection()?

Hi @tgolen I added this function to solve an issue where we validate and update text input value from IOUAmountPage. If we don't move cursor to end on component update, cursor stays at the beginning of text input as seen in this video. What do you recommend here as a next step?

Screen.Recording.2021-03-23.at.23.52.18.mov

[tgolen]

Whoa, that's weird. What causes that? Is it constantly losing and regaining focus after each key stroke?

[tugbadogan]

Whoa, that's weird. What causes that? Is it constantly losing and regaining focus after each key stroke?

In order to validate the user input and set TextInput value manually with the validated amount, I've added event.preventDefault(); to onKeyPress function callback of TextInputFocusable here.

https://github.com/Expensify/Expensify.cash/blob/9b282f10de193d9a4115bc30f51e0dfdb4133050/src/pages/iou/steps/IOUAmountPage.js#L90-L92

I think this part prevents TextInputFocusable to move the cursor to end itself when user presses a key. I'm updating TextInput value by passing defaultValue prop with updated amount value and I had to move cursor to end on component update using the new defaultValue prop.

[tgolen]

Thanks for more context! I'm digging into it now and I'm starting to see several missteps that lead down this path to an outcome that is less than ideal.

1. updateAmount() is modifying state that is only used by the props of IOUAmountPage.js so all that logic should just be moved to IOUAmountPage.js (unless I'm missing something). I see that this.state.amount is used for the title, but there is no reason that will ever change once the user is past step 0, so it doesn't need to be set from updateAmount() and could be set from onStepComplete. The typical pattern that should be followed is: a component containing a form should be entirely self-contained. It has an onSubmit prop that is called once the form is filled out completely, and all the form data is passed to that callback. The parent component is then in charge of handling the onSubmit() and dealing with the data appropriately (eg. sending it to the API or whatever).
2. The validation method should be changed with the textInput (and have this logic live in IOUAmountPage.js). Rather than stripping characters that aren't allowed, and setting the value again, use onKeyPress() to restrict characters from being typed in the first place. Essentially, only trigger e.preventDefault() when the key pressed is a non-numeric key (excluding backspace and period), or when there are more than two numbers after a decimal.
3. I also think the functionality for auto-width based on the value of the input should be moved to its own separate component like TextInputGrow or something to help clean up IOUAmountPage.js even more.

I think that with the validation properly refactored, and coupled with the refactoring of defaultValue that I mentioned in my previous comment it will fix the problems and put everything into a more traditional state which will avoid needing to add hacks that are there to restore the behavior that should be there all along (the cursor remaining at the end of the input).

[tgolen]

For now, I would recommend simply commenting out the event.preventDefault() while all this is refactored properly. The reason I say that is because this IOU functionality isn't live yet, but it's affecting core functionality in the rest of the app so it should be fixed quickly, and then this code can be refactored properly without affecting the rest of the app.

[tugbadogan]

For now, I would recommend simply commenting out the event.preventDefault() while all this is refactored properly. The reason I say that is because this IOU functionality isn't live yet, but it's affecting core functionality in the rest of the app so it should be fixed quickly, and then this code can be refactored properly without affecting the rest of the app.

I removed move cursor to end functionality from TextInputFocusable. Thanks for explanation above. I will refactor the code as you recommended.

[tgolen]

Thank you so much!

I wanted to try out the refactoring myself to see if it did indeed work, and it looks like it does work pretty well. You can see just a really rough idea of it here: https://github.com/Expensify/Expensify.cash/compare/tgolen-fix-indent?expand=1

[tgolen]

I'm gonna approve this so we can merge it and fix the regression in the main report compose field

[isagoico]

Anything that should be tested on our side on this PR? CC @tgolen @roryabraham @thienlnam

[Julesssss]

Hi @isabelastisser. This should verify that the issue was solved:

• type out a message in the compose box
• move cursor back to an earlier character/space in the text
• type something then stop
• The cursor should stay where you stopped

(previously, the cursor would jump back to the last input character)

[isabelastisser]

Hi @Julesssss, you meant to tag @isagoico :)

[Julesssss]

🤦 not again, sorry!

[isagoico]

Ahahaha It actually happens a lot 🤣 We're testing this atm