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

[HOLD for payment 2023-01-25] [$1000] Bank account : The bottom section of Personal Information blinks when opening and closing DatePicker #14084

Closed
1 task done
kavimuru opened this issue Jan 6, 2023 · 33 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jan 6, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Actions Performed:

  1. Login with any account
  2. Open Settings => Workspace => Connect Bank Account => Connect Manually => Enter valid information.
  3. Tap on any TextInput then tap on DatePicker, tap outside to dismiss DatePicker.
  4. Notice that the bottom section of the page is blinking after Date Picker closes.

Expected Result:

The Date Picker should be closed without any blinking.

Actual Result:

The bottom section of the page is blinking after Date Picker closes.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • iOS / native

Version Number: v1.2.50-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

RPReplay_Final1672983151.MP4
QXZT6339.1.MP4

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1672983560696649

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010fdafccebe9cfd56
  • Upwork Job ID: 1612956886638092288
  • Last Price Increase: 2023-01-10
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 6, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 10, 2023
@twisterdotcom
Copy link
Contributor

twisterdotcom commented Jan 10, 2023

My first few of these, so taking the template from StackOverflowTeams: https://stackoverflowteams.com/c/expensify/questions/14418

  • The "bug" is actually a bug.
  • The bug is not a duplicate report of an existing GH
  • The bug is reproducible, following the reproduction steps.
  • The GH template is filled out as fully as possible
  • The GH was created by an Expensify employee or a QA tester.
  • The bug is an /App issue

Yeah, this happens for me too on v1.2.52.3. ted@expensify.com:

RPReplay_Final1673393070.MP4

Going to add External now.

@melvin-bot melvin-bot bot removed the Overdue label Jan 10, 2023
@twisterdotcom twisterdotcom added the External Added to denote the issue can be worked on by a contributor label Jan 10, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 10, 2023
@melvin-bot melvin-bot bot changed the title Bank account : The bottom section of Personal Information blinks when opening and closing DatePicker [$1000] Bank account : The bottom section of Personal Information blinks when opening and closing DatePicker Jan 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

Job added to Upwork: https://www.upwork.com/jobs/~010fdafccebe9cfd56

@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 10, 2023

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

@hungvu193
Copy link
Contributor

hungvu193 commented Jan 11, 2023

Proposal
The root cause of this issue is that when we opened the DatePicker, we tried to call Keyboard.dismiss() immediately and then show the modal.
If we tried to remove Keyboard.dismiss() you won't see the bottom section blink.
Solution
We need to wait for the Keyboard 's totally dismissed then we will show the modal. Here's the diff for IOS picker, we can do it similar with Android.

diff --git a/src/components/DatePicker/index.ios.js b/src/components/DatePicker/index.ios.js
index d0945e060..f456fbc95 100644
--- a/src/components/DatePicker/index.ios.js
+++ b/src/components/DatePicker/index.ios.js
@@ -30,16 +30,31 @@ class DatePicker extends React.Component {
         this.reset = this.reset.bind(this);
         this.selectDate = this.selectDate.bind(this);
         this.updateLocalDate = this.updateLocalDate.bind(this);
+        this.delayOpenDatePicker = this.delayOpenDatePicker.bind(this);
+        this.timer = null;
+    }
+
+    componentWillUnmount() {
+        clearTimeout(this.timer);
+    }
+
+    delayOpenDatePicker = () => {
+        this.timer = setTimeout(() => {
+            this.setState({isPickerVisible: true});
+        }, 200);
     }
 
     /**
      * @param {Event} event
      */
     showPicker(event) {
+        if (this.timer) {
+            clearTimeout(this.timer);
+        }
         Keyboard.dismiss();
         this.initialValue = this.state.selectedDate;
-        this.setState({isPickerVisible: true});
         event.preventDefault();
+        this.delayOpenDatePicker();
     }
 
     /**

Reuslt:

Screen.Recording.2023-01-11.at.11.43.26.mov

cc @Gonals since you already worked with this kind of DatePicker issue.

@dinhhien2006
Copy link

Should we use InteractionManager instead? @hungvu193

@hungvu193
Copy link
Contributor

@dinhhien2006 I already tried with InteractionManager but seems It doesn't work so I need to use timeout.

@priyeshshah11
Copy link
Contributor

Proposal

This issue is related to the general iOS issue about how it works with modals & keyboard opening & closing after each other

Solution

We should use a keyboardDidHide listener & then open the picker only after the keyboard has been closed

diff --git a/src/components/DatePicker/index.ios.js b/src/components/DatePicker/index.ios.js
index d0945e060..48dc00262 100644
--- a/src/components/DatePicker/index.ios.js
+++ b/src/components/DatePicker/index.ios.js
@@ -36,9 +36,12 @@ class DatePicker extends React.Component {
      * @param {Event} event
      */
     showPicker(event) {
+        const listener = Keyboard.addListener('keyboardDidHide', () => {
+            this.setState({isPickerVisible: true});
+            listener.remove();
+        });
         Keyboard.dismiss();
         this.initialValue = this.state.selectedDate;
-        this.setState({isPickerVisible: true});
         event.preventDefault();
     }

@mollfpr

@tienifr
Copy link
Contributor

tienifr commented Jan 11, 2023

Proposal

Problem

In

showPicker(event) {
Keyboard.dismiss();
this.initialValue = this.state.selectedDate;
this.setState({isPickerVisible: true});
event.preventDefault();
}

The action Keyboard.dismiss() and this.setState({isPickerVisible: true}) are executed asynchronous. Besides, Ios will dismiss the keyboard when popover is opened and open again when it's closed

Solution

In

showPicker(event) {
Keyboard.dismiss();
this.initialValue = this.state.selectedDate;
this.setState({isPickerVisible: true});
event.preventDefault();
}

If keyboard is opening => I'll dismiss keyboard and open popover in keyboardDidHide listener. Otherwise I just open popover

diff --git a/src/components/DatePicker/index.ios.js b/src/components/DatePicker/index.ios.js
index d0945e0607..04378da86a 100644
--- a/src/components/DatePicker/index.ios.js
+++ b/src/components/DatePicker/index.ios.js
@@ -3,7 +3,7 @@ import React from 'react';
 import {Button, View, Keyboard} from 'react-native';
 import RNDatePicker from '@react-native-community/datetimepicker';
 import moment from 'moment';
-import _ from 'underscore';
+import _, { compose } from 'underscore';
 import TextInput from '../TextInput';
 import withLocalize, {withLocalizePropTypes} from '../withLocalize';
 import Popover from '../Popover';
@@ -11,6 +11,7 @@ import CONST from '../../CONST';
 import styles from '../../styles/styles';
 import themeColors from '../../styles/themes/default';
 import {propTypes, defaultProps} from './datepickerPropTypes';
+import withKeyboardState from '../withKeyboardState';
 
 const datepickerPropTypes = {
     ...propTypes,
@@ -36,9 +37,17 @@ class DatePicker extends React.Component {
      * @param {Event} event
      */
     showPicker(event) {
-        Keyboard.dismiss();
+        if(this.props.isKeyboardShown){
+            Keyboard.dismiss();
+            const listener = Keyboard.addListener('keyboardDidHide', () => {
+                this.setState({isPickerVisible: true});
+                listener.remove();
+            });
+        }else{
+            this.setState({isPickerVisible: true});
+        }      
         this.initialValue = this.state.selectedDate;
-        this.setState({isPickerVisible: true});
         event.preventDefault();
     }
 
@@ -134,7 +143,10 @@ DatePicker.defaultProps = defaultProps;
  * locale. Otherwise the spinner would be present in the system locale and it would be weird if it happens
  * that the modal buttons are in one locale (app) while the (spinner) month names are another (system)
  */
-export default withLocalize(React.forwardRef((props, ref) => (
-    /* eslint-disable-next-line react/jsx-props-no-spreading */
-    <DatePicker {...props} innerRef={ref} />
-)));
+export default compose(
+    withLocalize,
+    withKeyboardState,
+    )(React.forwardRef((props, ref) => (
+        /* eslint-disable-next-line react/jsx-props-no-spreading */
+        <DatePicker {...props} innerRef={ref} />
+    )));

Result

Screen.Recording.2023-01-11.at.16.07.43.mp4

@mollfpr
Copy link
Contributor

mollfpr commented Jan 12, 2023

Reviewing now!

@s77rt
Copy link
Contributor

s77rt commented Jan 12, 2023

@tienifr Just a small note, move the listener to be above the keyboard.dismiss() as @priyeshshah11 did just to avoid race conditions.

@mollfpr
Copy link
Contributor

mollfpr commented Jan 12, 2023

@hungvu193 I still see the blinks in your proposal and we avoid using setTimeout as a workaround.

Screen.Recording.2023-01-12.at.21.46.48.mov

@mollfpr
Copy link
Contributor

mollfpr commented Jan 12, 2023

@priyeshshah11 While your proposal fixing the issue, after the date picker close I can't open again the date picker.

Screen.Recording.2023-01-12.at.21.50.08.mov

@priyeshshah11
Copy link
Contributor

priyeshshah11 commented Jan 12, 2023

@priyeshshah11 While your proposal fixing the issue, after the date picker close I can't open again the date picker.

Screen.Recording.2023-01-12.at.21.50.08.mov

I think the problem in my solution is that it only works when the keyboard is open as I forgot about the case when the keyboard is closed, which I think should be solved in @tienifr's proposal but we should setup the listener before calling keyboard.dismiss()

diff --git a/src/components/DatePicker/index.ios.js b/src/components/DatePicker/index.ios.js
index d0945e0607..04378da86a 100644
--- a/src/components/DatePicker/index.ios.js
+++ b/src/components/DatePicker/index.ios.js
@@ -3,7 +3,7 @@ import React from 'react';
 import {Button, View, Keyboard} from 'react-native';
 import RNDatePicker from '@react-native-community/datetimepicker';
 import moment from 'moment';
-import _ from 'underscore';
+import _, { compose } from 'underscore';
 import TextInput from '../TextInput';
 import withLocalize, {withLocalizePropTypes} from '../withLocalize';
 import Popover from '../Popover';
@@ -11,6 +11,7 @@ import CONST from '../../CONST';
 import styles from '../../styles/styles';
 import themeColors from '../../styles/themes/default';
 import {propTypes, defaultProps} from './datepickerPropTypes';
+import withKeyboardState from '../withKeyboardState';
 
 const datepickerPropTypes = {
     ...propTypes,
@@ -36,9 +37,17 @@ class DatePicker extends React.Component {
      * @param {Event} event
      */
     showPicker(event) {
-        Keyboard.dismiss();
+        if (this.props.isKeyboardShown) {
+            const listener = Keyboard.addListener('keyboardDidHide', () => {
+                this.setState({isPickerVisible: true});
+                listener.remove();
+            });
+            Keyboard.dismiss();
+        } else {
+            this.setState({isPickerVisible: true});
+        }      
          this.initialValue = this.state.selectedDate;
-         this.setState({isPickerVisible: true});
          event.preventDefault();
     }
 
@@ -134,7 +143,10 @@ DatePicker.defaultProps = defaultProps;
  * locale. Otherwise the spinner would be present in the system locale and it would be weird if it happens
  * that the modal buttons are in one locale (app) while the (spinner) month names are another (system)
  */
-export default withLocalize(React.forwardRef((props, ref) => (
-    /* eslint-disable-next-line react/jsx-props-no-spreading */
-    <DatePicker {...props} innerRef={ref} />
-)));
+export default compose(
+    withLocalize,
+    withKeyboardState,
+    )(React.forwardRef((props, ref) => (
+        /* eslint-disable-next-line react/jsx-props-no-spreading */
+        <DatePicker {...props} innerRef={ref} />
+    )));

@tienifr
Copy link
Contributor

tienifr commented Jan 12, 2023

@s77rt Thanks for pointing that out.
In theory I think it can't cause the race condition since the event is batched at the end of the event loop.
Ref: https://reactnative.dev/docs/performance#:~:text=For%20most%20React,all%20goes%20well)

Yeah we can put the logic open modal before dismissing keyboard just to make sure everything works well
This is just the minor issue

     showPicker(event) {
-        Keyboard.dismiss();
+        if(this.props.isKeyboardShown){
+            const listener = Keyboard.addListener('keyboardDidHide', () => {
+                this.setState({isPickerVisible: true});
+                listener.remove();
+            });
+            Keyboard.dismiss();
+        }else{
+            this.setState({isPickerVisible: true});
+        }      
         this.initialValue = this.state.selectedDate;
-        this.setState({isPickerVisible: true});
         event.preventDefault();
     }

@mollfpr
Copy link
Contributor

mollfpr commented Jan 12, 2023

Okay, I think @tienifr proposal's working well.

With update here and also replacing the lodash compose with our compose libs function.

cc @jasperhuangg @twisterdotcom

🎀 👀 🎀 C+ reviewed!

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jan 12, 2023

Hey @mollfpr I agree that @tienifr's proposal is ideal. Assigning!

Thanks to everyone else for your proposals!

  • @hungvu193 We always try to avoid setting time outs when solving these types of interaction issues. Something to keep in mind moving forward!
  • @priyeshshah11 In the future let's make sure to test that all parts of the page still function as expected after the fix for the buggy behavior has been applied.

Thanks again :)

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 12, 2023

📣 @tienifr You have been assigned to this job by @jasperhuangg!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@priyeshshah11
Copy link
Contributor

  • @priyeshshah11 In the future let's make sure to test that all parts of the page still function as expected after the fix for the buggy behavior has been applied.

Thanks again :)

Agreed, I normally would but I had some setup issues which was preventing me from testing thoroughly but will be 100% sure next time.

@jasperhuangg Do I deserve any partial reward as the accepted proposal kind of uses the same idea from my proposal & even that proposal wasn't complete like mine? just asking 😅

@tienifr
Copy link
Contributor

tienifr commented Jan 16, 2023

@jasperhuangg Thanks, I've applied to the job, the PR is already created

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jan 18, 2023
@melvin-bot melvin-bot bot changed the title [$1000] Bank account : The bottom section of Personal Information blinks when opening and closing DatePicker [HOLD for payment 2023-01-25] [$1000] Bank account : The bottom section of Personal Information blinks when opening and closing DatePicker Jan 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 18, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.55-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-01-25. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Jan 18, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@mallenexpensify mallenexpensify added Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Jan 20, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 23, 2023
@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jan 23, 2023

The PR that introduced the bug has been identified. Link to the PR:

I wouldn't say that any specific PR introduced this bug. If anything, it's pretty difficult to look out for this specific bug and I wouldn't blame anyone for not catching it before merging their PR. It's a very minor visual glitch that doesn't affect behavior anywhere.

Therefore, I'm checking off the 2 todos involving any follow-up on a PR. With that being said, I'll still start a discussion in #expensify-bugs about minor visual glitches like this

@melvin-bot melvin-bot bot removed the Overdue label Jan 23, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Jan 25, 2023

A note on this the PR is introduced regression #14417 and has been resolved with #14427 PR

cc @jasperhuangg @tienifr @twisterdotcom

@twisterdotcom
Copy link
Contributor

@mollfpr and @hungvu193 - could you both please apply on the UpWork job for your payments please: https://www.upwork.com/jobs/~010fdafccebe9cfd56

@twisterdotcom
Copy link
Contributor

@tienifr offer sent for you to accept.

@hungvu193
Copy link
Contributor

@twisterdotcom Thanks, I've applied!

@mollfpr
Copy link
Contributor

mollfpr commented Jan 25, 2023

@twisterdotcom Applied, just for a reminder mine will reduce the cause of the regression.

@twisterdotcom
Copy link
Contributor

Payments sent for @mollfpr and @hungvu193.

@tienifr
Copy link
Contributor

tienifr commented Jan 25, 2023

@twisterdotcom I’ve accepted the offer, thanks!

@twisterdotcom
Copy link
Contributor

Nice.
Assigned Jan 12: #14084 (comment)
Merged Jan 14: #14270 (comment)

Paid with bonus.

@situchan
Copy link
Contributor

I think this caused another regression of #14529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests