[$1000] Web - Workspace - An error is visible when go back from Connect manually

[kbecciv]

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

---

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Log in with any account
3. Go to Settings -> Workspaces -> Connect bank account -> Connect manually
4. Click "Back" NOT entering any data

Expected Result:

There should NOT be any error

Actual Result:

An error is visible

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.31.5

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5835453_Recording__3072.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016000f297c530dd31
• Upwork Job ID: 1596215030529708032
• Last Price Increase: 2022-12-08

[melvin-bot]

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[hungvu193]

Proposal
When clicking back button, the input loses focus and trigger the error validation.

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Line 77 in 39b2f99

onBackButtonPress={() => BankAccounts.setBankAccountSubStep(null)}

Solution
Add preventDefault when click back will do the trick.

-  onBackButtonPress={() => BankAccounts.setBankAccountSubStep(null)} 
+ onBackButtonPress={(e) => { BankAccounts.setBankAccountSubStep(null); e.preventDefault(); }}

We can also add onLongPress event too incase we don't want the input to loose focus when long click.
It will also happen if we click other button like: Get Assistance, Close. We can also do the same.
Result:

Screen.Recording.2022-11-25.at.17.54.13.mov

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016000f297c530dd31

[melvin-bot]

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

[melvin-bot]

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

[thesahindia]

This behaviour is present at every page that is using Form component. If we wanna fix this then we should be fixing this for all the pages.

[allroundexperts]

### Proposal

Problem

We're not preventing default in the HeaderWithBackButton component.

Solution

Following is a generic solution that works across all pages.

diff --git a/src/components/HeaderWithCloseButton.js b/src/components/HeaderWithCloseButton.js
index 27ab5dbbe..3a2a94336 100755
--- a/src/components/HeaderWithCloseButton.js
+++ b/src/components/HeaderWithCloseButton.js
@@ -202,7 +202,10 @@ class HeaderWithCloseButton extends Component {
                         && (
                         <Tooltip text={this.props.translate('common.close')}>
                             <Pressable
-                                onPress={this.props.onCloseButtonPress}
+                                onPress={(e) => {
+                                    e.preventDefault();
+                                    this.props.onCloseButtonPress(e);
+                                }}
                                 style={[styles.touchableButtonImage, styles.mr0]}
                                 accessibilityRole="button"
                                 accessibilityLabel={this.props.translate('common.close')}

[allroundexperts]

Updated Proposal

Problem

The issue is that clicking on the x button triggers the onBlur handler of the Form input element. Since onBlur has a greater priority than onClick handler, onBlur gets called first. As such, adding an e.preventDefault in the onClick handler is of no use.

Fix

We need to not fire validation in the onBlur handler that is attached to the Form component given:

1. Events related target is clickable having a tab index of less than 1.
   (This logic can be easily changed ie tab index < 0 or related target having a specific id / class. IMO this will suffice though)

Thus the following changes:

diff --git a/src/components/Form.js b/src/components/Form.js
index 4734381bc..423a9ec81 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -217,7 +217,10 @@ class Form extends React.Component {
                 ref: node => this.inputRefs[inputID] = node,
                 value: this.state.inputValues[inputID],
                 errorText: this.state.errors[inputID] || fieldErrorMessage,
-                onBlur: () => {
+                onBlur: (e) => {
+                    if (e.relatedTarget && 'tabIndex' in e.relatedTarget && e.relatedTarget.tabIndex < 1) {
+                        return e.preventDefault();
+                    }
                     this.setTouchedInput(inputID);
                     this.validate(this.state.inputValues);
                 },

Result

Screen.Recording.2022-11-26.at.2.44.51.AM.mov

[Delgee]

Proposal

We encountered a similar issue on #12715 and fixed it by calling preventDefault on onMouseDown event. We can do a similar thing on back button. Since this issue is only occurring on back button. No need to change Form component I think.

[allroundexperts]

@Delgee onMouseDown has a greater precedence than the onBlur event. We need to preventDefault in a handler that has a greater precedence than the onBlur event. As such, your solution works as well. But it's not generic. We'll have to do it on every Form with Back button. What I've purposed is generic and will work for all the forms.

[Delgee]

@allroundexperts I think validating input on focus loss is correct behavior. And we should be able to validate input even if the focus is lost because of a click outside of Form element. So we should fix this issue on every page this issue happening instead of removing validation when clicked outside Form element. Also using e.relatedTarget.tabIndex is a bad idea tabIndex is just working on the specific cases it is not related to Form element at all.

[allroundexperts]

@Delgee We are validating the input if the focus is lost because of click outside the Form. It's just for specific elements (like href), that when clicked, we don't want to validate. Also, TabIndex is just a single way to do it. As mentioned in proposal, we can use id / class as well to pinpoint this further.

Also, If we change this on every page, I'm sure this will come up again when new pages get added.

[Delgee]

@allroundexperts Yes, we just don't want to validate in specific cases, not general cases. So we should fix this issue in those specific cases.

[fedirjh]

Proposal

Problem

As mentioned before onBlur is fired before onClick , hence the error shows when we click back button

Solution

We can handle mouse event for the form and only validate when mouse is inside the form , this will prevent any validation when we click outside the Form

diff --git a/src/components/Form.js b/src/components/Form.js
index 4734381bc9..6a47329fa9 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -75,6 +75,7 @@ class Form extends React.Component {
         this.state = {
             errors: {},
             inputValues: {},
+            focused: false,
         };
 
         this.inputRefs = {};
@@ -83,6 +84,7 @@ class Form extends React.Component {
         this.setTouchedInput = this.setTouchedInput.bind(this);
         this.validate = this.validate.bind(this);
         this.submit = this.submit.bind(this);
+        this.toggleFocus = this.toggleFocus.bind(this);
     }
 
     /**
@@ -128,6 +130,10 @@ class Form extends React.Component {
         this.props.onSubmit(this.state.inputValues);
     }
 
+    toggleFocus() {
+        this.setState(prev => ({focused: !prev.focused}));
+    }
+
     /**
      * @param {Object} values - An object containing the value of each inputID, e.g. {inputID1: value1, inputID2: value2}
      * @returns {Object} - An object containing the errors for each inputID, e.g. {inputID1: error1, inputID2: error2}
@@ -218,6 +224,9 @@ class Form extends React.Component {
                 value: this.state.inputValues[inputID],
                 errorText: this.state.errors[inputID] || fieldErrorMessage,
                 onBlur: () => {
+                    if (!this.state.focused) {
+                        return;
+                    }
                     this.setTouchedInput(inputID);
                     this.validate(this.state.inputValues);
                 },
@@ -249,6 +258,8 @@ class Form extends React.Component {
                     style={[styles.w100, styles.flex1]}
                     contentContainerStyle={styles.flexGrow1}
                     keyboardShouldPersistTaps="handled"
+                    onMouseEnter={this.toggleFocus}
+                    onMouseLeave={this.toggleFocus}
                 >
                     <View style={[this.props.style]}>
                         {this.childrenWrapperWithProps(this.props.children)}

[thesahindia]

@flodnv, can you please confirm if we wanna fix this issue. Here's a thread about keeping the validation only at the submit button but we haven't decided anything. Let's discuss there.

[flodnv]

Thank you for the discussion everyone. For now let's hold on that discussion, see you there!

[JmillsExpensify]

Commented in the linked thread. I think we should close this issue. Taking it off hold in any case because the discussion there is wrapping up.