Keyboard is visible for a brief moment on request/send money page after getting back from currency search page

[narefyev91]

Details

Implement new event handler for navigation which will close keyboard before user will go out from the screen

Fixed Issues

$ #15085
$ #15085 (comment)

Tests

1. Open the app in android
2. Open any report
3. Click on plus
4. Click on Request money/ send money/ split bill
5. Click on currency
6. On currency search page, when keyboard is displayed for search, click on 'X' to close and get back to previous page
7. Note for QA: On mWeb, this behaviour might still be slugging, its same as in production

• Verify that no errors appear in the JS console

Offline tests

This feature doesn't change or is impacted by offline mode.

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 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 correct English and 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 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-02-16.at.12.22.18.mov

Mobile Web - Chrome

device-2023-02-16-113949.mp4

Mobile Web - Safari

Screen.Recording.2023-02-16.at.11.58.19.mov

Desktop

Screen.Recording.2023-02-16.at.12.24.49.mov

iOS

Screen.Recording.2023-02-16.at.12.20.10.mov

Android

device-2023-02-16-113435.mp4

[MelvinBot]

@mountiny @0xmiroslav 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]

[mountiny]

This is looking good to me! I have noticed there is a slight delay for the numpad to go down on iOS Safari not sure if that is on main as well.

@0xmiroslav there was also an alternative proposal to add this listener to the ScreenWrapper so this handles all the pages. From the linked issue:

We can solve issue in 2 ways - globally and locally.
For global one - we can add subscription to event beforeremove inside ScreenWrapper class and add in compose withKeyboardState HOC - which will guaranty that keyboard will be closed on any screen which was wrapped in ScreenWrapper
For local one - doing the same steps - but only under IOUCurrencySelection class

Curious what do you think of it @0xmiroslav? I am just worried we might not be able to catch some issues this might cause on some other pages, but it might be better to move this level up, what do you think?

[narefyev91]

This is looking good to me! I have noticed there is a slight delay for the numpad to go down on iOS Safari not sure if that is on main as well.

@0xmiroslav there was also an alternative proposal to add this listener to the ScreenWrapper so this handles all the pages. From the linked issue:

We can solve issue in 2 ways - globally and locally.
For global one - we can add subscription to event beforeremove inside ScreenWrapper class and add in compose withKeyboardState HOC - which will guaranty that keyboard will be closed on any screen which was wrapped in ScreenWrapper
For local one - doing the same steps - but only under IOUCurrencySelection class

Curious what do you think of it @0xmiroslav? I am just worried we might not be able to catch some issues this might cause on some other pages, but it might be better to move this level up, what do you think?

Yup for safari on mobile it's the same on main branch. Happened because keyboard is coming from device, and when it's closing, web receive this info with delay and re-build UI. As a workaround it can be fixed using setTimeout - but that's may cause some issues on slow devices

[mountiny]

Thanks for the context 🙇 I think we dont have to worry about this then!

[mountiny]

@0xmiroslav I have asked @narefyev91 to move the logic up but if you have any concerns, feel free to raise them.

Would be great to test some other screens which this might influence it

[0xmiros]

reviewing today

[0xmiros]

First of all, this issue is duplicated with #13449 which is on hold for navigation reboot.
But I believe this issue will still remain even after navigation reboot unless new navigation doesn't handle dismiss keyboard automatically when screen changes.

I have some feedback about current code changes:

Regarding general solution:

• When enter new page which doesn't require auto-focus, keyboard may still be visible for a brief moment because current page is not unmounted and beforeRemove callback is not called.
  Here's alternative solution: [HOLD #11768] [$1000] Android - Split Bill - Keyboard is not dismissed for a brief moment after changing currency #13449 (comment) which always dismiss keyboard when any navigation state changes. But this still doesn't solve my concerns below:

Regarding possible regression:

• I tried to find any case of auto-focusing on previous page (already mounted) after closing new page but no case found yet in our app.
• This change also might affect the case when 2 pages both require focus on mount. So when open 2nd page, it may lose auto-focus while opening it from 1st page with keyboard open. But no case found yet in our app.

My suggestion:
I am still more inclined towards @narefyev91's solution than changes in NavigationRoot for performance and optimization - less events triggered.
As long as no cases of possible regression found, I think we are good to go with current PR.
If we encounter the case of new screen which doesn't need keyboard dismiss while closing, we may introduce new prop like shouldDismissKeyboardBeforeClose in ScreenWrapper with default value to true.

@0xmiroslav there was also an alternative proposal to add this listener to the ScreenWrapper so this handles all the pages.

I like this general solution and this is implemented already in current PR.
If we too care about regression, we can introduce new prop as suggested above and set default value to false and set true only on this page.

[mountiny]

@0xmiroslav thank you very much!

If we encounter the case of new screen which doesn't need keyboard dismiss while closing, we may introduce new prop like shouldDismissKeyboardBeforeClose in ScreenWrapper with default value to true.

I think in near future we could have form flow where there could be multiple pages in row which will also have autofocus in it, so I think we might want to get ready for that.

Then the props would be useful I imagine.

@narefyev91 Could you try to implement the shouldDismissKeyboardBeforeClose props @0xmiroslav mentioned?

[mountiny]

@narefyev91 there is also merge conflict

[narefyev91]

@mountiny @0xmiroslav Yup i get you guys - btw we could not really worried about navigation between screen - navigation event handler will unmount keyboard before moving to next screen and in case based on your paradigm and controlling directly focus event in didMount such issues will not be happened
As an example same screen with currency and adding money - we have autofocus on mount - after choose currency we go back - keyboard dismissed, and input focused

Screen.Recording.2023-02-22.at.15.58.19.mov

[0xmiros]

navigation event handler will unmount keyboard before moving to next screen

yes, I had this concern below but I confirmed that keyboard dismisses instantly when navigate to new screen, unlike when navigate to previous screen which is already mounted:

Actually, this is android issue.

On iOS, keyboard dismisses instantly always - either navigating to previous or next

[narefyev91]

navigation event handler will unmount keyboard before moving to next screen

yes, I had this concern below but I confirmed that keyboard dismisses instantly when navigate to new screen, unlike when navigate to previous screen which is already mounted:

Actually, this is android issue.

On iOS, keyboard dismisses instantly always - either navigating to previous or next

yup - that's totally makes sense - @0xmiroslav @mountiny added additional prop to handle event handlers - let me know - if that works!

[mountiny]

Thanks everyone!

@0xmiroslav will you be able to go through the PR review checklist?

[mountiny]

Codewise looks good to me!

[narefyev91]

@mountiny @0xmiroslav pushed description for prop

[0xmiros]

msafari.mp4

The issue is still reproducible in mWeb as mentioned before.
Do we leave this for now since mWeb keyboard is out of our control?
I think we should fix mWeb issue here as well.

[narefyev91]

msafari.mp4

The issue is still reproducible in mWeb as mentioned before.

Do we leave this for now since mWeb keyboard is out of our control?

I think we should fix mWeb issue here as well.

That will be pretty tricky to find a way to fix that on mWeb. We may not block this current fix for android and I will start investigating mWeb issue - it happened in different places in the app. And to find global fix will be very hard - I will do a workaround inside currency screens, and will keep posted what I will find

[mountiny]

@0xmiroslav I think we have confirmed though that behaviour is same on main so I am not sure if we should hold on that. We cannot fix it easily.

[mountiny]

@narefyev91 More conflicts, can you please resolve those?

[0xmiros]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section 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 Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • 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 checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• 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. toggleReport and not onIconClick).
  • 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
  • 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 correct English and 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 verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• 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 */
  • 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 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.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mp4

Desktop

desktop.mov

iOS

ios.mp4

Android

android.mov

[0xmiros]

This is android fix only.
Other platforms just keep same behavior as production.
Known that mWeb issues are difficult to fix, we agreed they are considered as separate issue.

[mountiny]

One NAB

[mountiny]

Thanks!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	753.960 ms → 777.791 ms (+23.831 ms, +3.2%)
App start runJsBundle	208.969 ms → 213.281 ms (+4.313 ms, +2.1%)
App start regularAppStart	0.016 ms → 0.016 ms (+0.001 ms, +3.8%)
Open Search Page TTI	588.343 ms → 587.812 ms (-0.531 ms, ±0.0%)
App start nativeLaunch	21.344 ms → 20.036 ms (-1.308 ms, -6.1%)

Show details

Name	Duration
App start TTI	Baseline Mean: 753.960 ms Stdev: 33.689 ms (4.5%) Runs: 703.2665399999823 709.4908000000287 711.1373550000135 714.6233949998859 714.6758570000529 718.3330310001038 718.8257289999165 719.517770000035 724.0549290000927 725.071413999889 733.9494789999444 735.496875999961 737.1093079999555 741.56146300002 743.5936120001134 745.7456370000727 750.4784880001098 755.584065000061 759.3107769999187 768.2630270000082 770.1647459999658 773.6271480000578 776.5743110000622 777.1126109999605 784.3947119999211 786.1743050001096 790.8024100000039 798.0110110000242 803.0364890000783 804.8846710000653 806.2262450000271 825.6112669999711 Current Mean: 777.791 ms Stdev: 25.610 ms (3.3%) Runs: 728.0463350000791 730.1864859999623 740.1290219998918 749.3595000000205 751.0747789999004 753.9480119999498 754.1981289999094 755.2223779999185 759.6248010001145 761.2062099999748 765.2328810000326 766.3687120000832 769.8781220000237 771.1138269999065 777.901162999915 781.8924789999146 783.2111919999588 783.2248780000955 784.0300310000312 785.1942920000292 787.9465310000814 788.3838969999924 788.5535869998857 791.7920079999603 792.8474880000576 794.11518600001 797.2358399999794 809.2068179999478 811.0400600000285 816.8581970001105 828.8238399999682 831.4628860000521
App start runJsBundle	Baseline Mean: 208.969 ms Stdev: 18.692 ms (8.9%) Runs: 180 180 181 187 189 192 194 195 195 199 201 201 201 204 205 205 206 207 208 209 210 215 219 219 226 227 228 236 236 237 239 256 Current Mean: 213.281 ms Stdev: 15.780 ms (7.4%) Runs: 188 188 194 197 198 198 198 201 201 201 203 205 207 208 208 210 210 212 213 215 219 220 222 225 230 231 232 232 234 234 239 252
App start regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.001 ms (9.2%) Runs: 0.013264999957755208 0.013794000027701259 0.013835000107064843 0.013916000025346875 0.013916000025346875 0.014201000100001693 0.0147299999371171 0.014933000085875392 0.014973999932408333 0.015014000004157424 0.015137000009417534 0.015340000158175826 0.015340999932959676 0.01534100016579032 0.015421000076457858 0.015421999851241708 0.015461999922990799 0.015584999928250909 0.015625 0.015868999995291233 0.01603199983946979 0.016032000072300434 0.016276000067591667 0.0165200000628829 0.01700900006107986 0.01700900006107986 0.01745599997229874 0.017536999890580773 0.01782199996523559 0.018431999953463674 0.018757999874651432 0.018920999951660633 Current Mean: 0.016 ms Stdev: 0.001 ms (7.5%) Runs: 0.013752999948337674 0.014364000177010894 0.014769999776035547 0.015137000009417534 0.015178000088781118 0.01521800016053021 0.015705999918282032 0.01570700015872717 0.015746999997645617 0.015787999844178557 0.015828999923542142 0.015829000156372786 0.015868999995291233 0.01590899983420968 0.01599099999293685 0.016234999988228083 0.016234999988228083 0.016398000065237284 0.016682999907061458 0.016723999986425042 0.016723999986425042 0.017008000053465366 0.017008000053465366 0.017374999821186066 0.017497000051662326 0.017497000051662326 0.017536999890580773 0.017985000042244792 0.018026000121608377 0.01887999987229705 0.019245999865233898
Open Search Page TTI	Baseline Mean: 588.343 ms Stdev: 24.903 ms (4.2%) Runs: 547.5251060000155 551.6941329999827 554.6466069999151 555.6758220000193 555.9037679999601 559.2507740000729 563.8554689998273 569.0792640000582 570.586425999878 573.1809489999432 578.260377000086 578.3629160001874 578.711262999801 579.211262999801 583.5353600000963 584.5789389999118 589.6064460000489 589.7912600000855 594.3929850000422 596.394164999947 600.6610520000104 601.7063400000334 602.1288659998681 604.810792000033 605.3865159999114 607.2934170002118 610.5263269999996 613.8739420000929 618.9374589999206 625.2628589998931 629.7889809999615 652.3441170000006 Current Mean: 587.812 ms Stdev: 22.603 ms (3.8%) Runs: 553.8742680000141 554.8979499998968 556.0372719999868 558.5593669998925 558.7449950000737 563.4373379999306 563.5915939998813 565.2818609999958 572.1735839999747 572.2093919999897 573.2014160000253 573.7689209999517 577.807209999999 584.2091069999151 586.5618900000118 586.860514999833 587.5869549999479 588.9106040000916 589.1621910000686 590.7361649998929 591.6494550001808 595.7113040001132 597.0550140000414 601.7285970000084 604.1263829998206 604.6323649999686 608.4319670000114 611.1047370000742 613.4480389999226 617.7712399999145 627.6210530002136 630.1944580001291 636.7042639998253
App start nativeLaunch	Baseline Mean: 21.344 ms Stdev: 3.027 ms (14.2%) Runs: 17 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 21 21 21 21 22 23 23 24 24 25 25 25 26 27 27 27 Current Mean: 20.036 ms Stdev: 1.085 ms (5.4%) Runs: 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 21 22 22 22 22

[OSBotify]

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.77-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.77-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅