Allow user to modify IOU amount with on-screen keyboard #10147
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
melvin-bot
bot
requested review from
chiragsalian and
rushatgabhane
and removed request for
a team
|
@eVoloshchak can you please spend some time writing test steps for QA? So that we can discover all possible bugs |
Updated QA steps |
src/pages/iou/steps/IOUAmountPage.js
Outdated
| @@ -59,6 +59,7 @@ class IOUAmountPage extends React.Component { | |||
|
|
|||
| this.state = { | |||
| amount: props.selectedAmount, | |||
| selection: {start: 0, end: 0}, | |||
There was a problem hiding this comment.
nab
| selection: {start: 0, end: 0}, | |
| selection: { | |
| start: 0, | |
| end: 0, | |
| }, |
src/pages/iou/steps/IOUAmountPage.js
Outdated
| @@ -134,19 +135,32 @@ class IOUAmountPage extends React.Component { | |||
| * @param {String} key | |||
| */ | |||
| updateAmountNumberPad(key) { | |||
| // Returns the new selection object based on updated amount | |||
| const getNewSelection = (oldSelection, oldAmount, newAmount) => { | |||
There was a problem hiding this comment.
can you please make this a class method instead?
| @@ -274,6 +273,7 @@ class BaseTextInput extends Component { | |||
| onPressOut={this.props.onPress} | |||
| showSoftInputOnFocus={!this.props.disableKeyboard} | |||
| keyboardType={getSecureEntryKeyboardType(this.props.keyboardType, this.props.secureTextEntry, this.state.passwordHidden)} | |||
| value={this.props.value} | |||
There was a problem hiding this comment.
Hmm, @eVoloshchak did you test on the production build for Android?
My cursor is in the middle and holding backspace deletes all the numbers
screen-20220731-154846.mp4
There was a problem hiding this comment.
Hmm, @eVoloshchak did you test on the production build for Android?
Weird, I remember this behavior being present only on DEV. And it seemed to be the same for 7433. But now i'm getting this bug with about 60% chance.
This is happening because when user holds <, updateAmountNumberPad is called every 100 ms. But due to some kind of event bundling issue (maybe related to setState being async?), textinput's onSelectionChange method is called with selection object being incorrect, probably due to poor performance, since it intencifies in DEV mode. Since we're controlling selection manually in updateAmountNumberPad method, we don't need the selection data from onSelectionChange when user is holding the < button (or using any key of onscreen keyboard)
- We can add a local variable in IOUAmountPage
this.shouldUpdateSelection = true;- Add a new method to
BigNumberPad, which will be called when user starts and stops long pressing
handleLongPress(key) {
// Only handles deleting.
if (key !== '<') {
return;
}
+ this.props.longPressHandlerStateChanged(true);
const timer = setInterval(() => {
this.props.numberPressed(key);
}, 100);
this.setState({timer});
} onPressOut={() => {
clearInterval(this.state.timer);
ControlSelection.unblock();
+ this.props.longPressHandlerStateChanged(false);
}}- And then on IOUAmountPage we can ignore
onSelectionChangeif user is long pressing
<BigNumberPad
numberPressed={this.updateAmountNumberPad}
+ longPressHandlerStateChanged={state => this.shouldUpdateSelection = !state}
/> onSelectionChange={(e) => {
+ if (!this.shouldUpdateSelection) {
+ return;
+ }
this.setState({selection: e.nativeEvent.selection});
}}22-07-31-15-24-23.mp4
| @@ -274,6 +273,7 @@ class BaseTextInput extends Component { | |||
| onPressOut={this.props.onPress} | |||
| showSoftInputOnFocus={!this.props.disableKeyboard} | |||
| keyboardType={getSecureEntryKeyboardType(this.props.keyboardType, this.props.secureTextEntry, this.state.passwordHidden)} | |||
| value={this.props.value} | |||
There was a problem hiding this comment.
Also, use of both state.value and props.value in this component is confusing.
Can we use state.value in all the possible places?
|
Updated PR with all the suggested changes and a fix for issue with holding < buton |
|
Still reviewing, just got hands on a physical iPhone because pasting didn't seem to work on my simulator |
There was a problem hiding this comment.
@eVoloshchak bug - cursor isn't at end when going back to amount page.
- Request / split amount
- Enter any amount and go next
- Go back
Expected: cursor is at end of amount
Actual : cursor is at start of amount
Screen.Recording.2022-08-02.at.9.35.35.AM.mov
Good catch, thank you. selection: {
start: 0,
end: 0,
},But it should depend on the selectedAmount length selection: {
start: props.selectedAmount.length,
end: props.selectedAmount.length,
},Updated the PR |
|
Also found a console error |
|
All done |
There was a problem hiding this comment.
@eVoloshchak thanks for your patience on this one.
@chiragsalian LGTM! 🎉🎉
We still have this bug which is less noticeable on release builds.
And there is no way around it at the moment because we're using controlled text and selection.
We have a gut feeling (no evidence) that enabling RN's fabric architecture should help with it.
PR Reviewer Checklist
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible 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 verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 verified proper code patterns were followed (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.
toggleReportand notonIconClick). - 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 was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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
- 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.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- 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
- 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 */ - Any functional components have the
displayNameproperty - 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
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - 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 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 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
Avataris modified, I verified thatAvataris 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.
- I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.
I think you needed even more patience, so thank you too :) |
|
Sorry for the delay. I totally missed reviewing this. Reviewing now. |
There was a problem hiding this comment.
Is it just me. I feel like something weird is happening here when i'm testing it. In normal testing this works fine. But if i use a keyboard to type the numbers fast on ios the focus is really weird.
For example, i'm typing 12121212 fast on my keyboard but the numbers show up as 22211211, and the input focus is on the wrong spot.
Screen.Recording.2022-08-29.at.12.55.30.PM.mov
P.S i pulled main into your branch to test to make sure its got the latest code. I found the issue only on your branch and when i tested on main there was no issue.
|
@chiragsalian no it's not just you, it is an expected bug that I mentioned in my PR approval #10147 (review) This is because of controlled nature of It is less noticeable on release builds. We are assuming that this issue might be resolved when we enable Fabric architecture as it'll supposedly boost performance by getting rid of JS bridge bottleneck |
|
Oh wow, i totally forgot to click into your comment to see the details of the bug. Cool, thank you for capturing. Code LGTM. |
|
No worries :) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @chiragsalian in version: 1.1.94-0 🚀
|
|
A big change was done on this PR and QA tests do not justify those changes. Was this discussed anywhere before the PR was created? TextInput was migrated to controlled from uncontrolled. Did we take consenses before applying these changes? Could you please share the link for the discussion, I have another issue so it is good to check out? |
Thanks for raising this! Here is what @chiragsalian and I felt was a good way to move forward. #6154 (comment) TextInput was migrated to controlled because I also made an announcement for it on slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1656632630394839?thread_ts=1654878284.799759&cid=C01GTK53T8Q |
|
I verified text inputs on storybook, and a few pages on newDot. @eVoloshchak can you please add a QA step to test all TextInputs on storybook as well? |
|
Thanks for the details. |
|
This PR caused a problem (not a regression) here #16385. The bug is that we are returning prev state here that includes the |
|
@s77rt thanks! that's a good learning
but isn't this a deep copy?? sorry if it's a noob question |
|
@rushatgabhane That's a shallow copy. Shallow copy:objA = {x: 1, nes: {y: 2}};
objB = {...objA};objB is a shallow copy of objA, meaning that the props of objB are independent of objA as long as they (the props) are not objects (bcz objects are referenced by address). Changing prop x of objA have no effect on objB objA.x = 100;
100
objB.x;
1But changing prop nes of objA have an effect on objB: objA.nes.y = 200;
200
objB.nes.y;
200Deep copy:objA = {x: 1, nes: {y: 2}};
objB = _.cloneDeep(objA);objB is a deep copy of objA, meaning that the props of objB are truly independent of objA. Changing prop x of objA have no effect on objB objA.x = 100;
100
objB.x;
1And also changing prop nes of objA have no effect on objB objA.nes.y = 200;
200
objB.nes.y;
2In our case |
|
ohhh thanks for such a detailed explanation |
| @@ -30,6 +30,7 @@ class BaseTextInput extends Component { | |||
| passwordHidden: props.secureTextEntry, | |||
| textInputWidth: 0, | |||
| prefixWidth: 0, | |||
| selection: props.selection, | |||
There was a problem hiding this comment.
@eVoloshchak sorry I know it has been a long time, but what is the purpose of copying the props.selection to an internal state instead of just using it directly the props like selection={this.props.selection} here:
There was a problem hiding this comment.
Check #10147 (comment) So apparently we used state to delay the selection by one cycle to fix a bug in iOS Safari.
We have this PR #16825 that fixed a similar bug on Web. So maybe the original bug is fixed now and we can pass the prop directly.
Curious though, what are you doing 😁 @aldo-expensify
There was a problem hiding this comment.
Check #10147 (comment) So apparently we used state to delay the selection by one cycle to fix a bug in iOS Safari.
Thanks for the explanation :). I think I would have been good to leave a comment so we don't just remove it in the future.
Curious though, what are you doing 😁 @aldo-expensify
Just reviewing this PR: https://github.com/Expensify/App/pull/18579/files and I got curious about that.
Details
This PR allows user to modify IOU amount by moving cursor, selecting any part of the number, pasting any valid number into input.
Fixed Issues
$ #6154
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
4.1. Enter a symbol
4.2. Delete a symbol
4.3. Paste a couple of symbols
4.4. Cut a couple of symbols
4.5. Press and hold
<key on the on-screen keyboardScreenshots
Web
cinnamon-20220728-4.mp4
Note: since there were no changes to the
updateAmountmethod, behavior onWebis unchangedMobile Web
video_2022-07-28_10-01-40.mp4
Desktop
I'm unable to launch the desktop app due to
#8888. It is not present if you downgrade to certain node and npm versions, but since I use MacinCloud, I'm unable to do so. But since there were no changes to theupdateAmountmethod, behavior onDesktopis unchanged.iOS
cinnamon-20220728-1.1.mp4
Note: I'm unable to paste in iOS simulator due to this bug
Android
22-07-28-10-29-19.mp4
Note:
(Both issues should be fixed by upgrading to the new fabric architecture)