[$1000] VBA - Cannot select the checkbox when a valid field is being focused entering bank account manually

[kavimuru]

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. Open Settings => Workspace => Choose any workspaces => Connect Bank Account => Connect Manually.
2. Notice that Routing Number is being focused and no error is showed, click on check box
3. Again focus on the Routing Number and click on check box

Expected Result:

Checkbox should be selected and focused at the first click

Actual Result:

Checkbox is not selected when a valid field is focused for the first time

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.34-1
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.1030.mp4

Screen.Recording.2022-11-30.at.21.39.59.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a77f27abe4f6fc46
• Upwork Job ID: 1603384341212991488
• Last Price Increase: 2022-12-15

[melvin-bot]

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

[allroundexperts]

Proposal

There is no easy fix to it because onBlur on the text field gets called first (which in turn causes validate, which changes the layout). Since the layout is changed after onBlur, the click event never gets fired.

Solutions

We can:
a. On the checkbox, use onMouseDown instead of onClick. This will ensure that the click event fires before the layout gets a chance to change. However, onMouseDown is different from onClick in the sense that it gets triggered on all type of clicks. We can sort of make up for this by checking the button type in the event that gets passed to onMouseDown.

diff --git a/src/components/CheckboxWithLabel.js b/src/components/CheckboxWithLabel.js
index 96f85fb25..e75d81dd6 100644
--- a/src/components/CheckboxWithLabel.js
+++ b/src/components/CheckboxWithLabel.js
@@ -91,14 +91,14 @@ class CheckboxWithLabel extends React.Component {
                 <View style={[styles.flexRow, styles.alignItemsCenter]}>
                     <Checkbox
                         isChecked={this.isChecked}
-                        onPress={this.toggleCheckbox}
+                        onMouseDown={this.toggleCheckbox}
                         label={this.props.label}
                         hasError={Boolean(this.props.errorText)}
                         forwardedRef={this.props.forwardedRef}
                     />
                     <TouchableOpacity
                         focusable={false}
-                        onPress={this.toggleCheckbox}
+                        onMouseDown={this.toggleCheckbox}
                         style={[
                             styles.ml3,
                             styles.pr2,

b. Add a condition inside the onBlur method of the field such that if the role of relatedTarget is a checkbox, then skip calling the validate method. ie

diff --git a/src/components/Form.js b/src/components/Form.js
index 34199bd4f..e84d95521 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -217,9 +217,11 @@ 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) => {
                     this.setTouchedInput(inputID);
-                    this.validate(this.state.inputValues);
+                    if (e.nativeEvent.relatedTarget.role !== 'checkbox') {
+                        this.validate(this.state.inputValues);
+                    }
                 },
                 onInputChange: (value, key) => {
                     const inputKey = key || inputID;

Let me know which approach makes more sense and I'll implement that one.

[marcaaron]

I am casting a vote for "do nothing". The blur happens first and causes the layout shift. We shouldn't try to interfere with that. Our forms are design to get you to address errors and resolve them so I think this behavior would be more than tolerated (and probably unusual for anyone to actually do).

[adeel0202]

This could be a duplicate of #12715

[tienifr]

Proposal

Problem

Event execution order: mousedown -> blur -> click, so the blur happens cause layout change, then click event targets to wrong component.

Solution

we can implement the behavior as ios/android is to keep focusing when click checkbox

In

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Line 114 in 820e876

<CheckboxWithLabel

<CheckboxWithLabel
                        style={styles.mt4}
                        inputID="acceptedTerms"
+                       onMouseDown={(event)=>{
+                           event.preventDefault();
+                           event.stopPropagation();
+                        }}
...

In

App/src/components/CheckboxWithLabel.js

Line 92 in 820e876

<Checkbox

                    <Checkbox
                        isChecked={this.isChecked}
                        onPress={this.toggleCheckbox}
+                       onMouseDown={this.props.onMouseDown}
                        label={this.props.label}
                        hasError={Boolean(this.props.errorText)}
                        forwardedRef={this.props.forwardedRef}
                    />
                    <TouchableOpacity
                        focusable={false}
                        onPress={this.toggleCheckbox}
+                       onMouseDown={this.props.onMouseDown}
...

Screen.Recording.2022-12-01.at.17.03.01.mp4

[trjExpensify]

This could be a duplicate of #12715

I don't think it's a dupe, but similar. The fix for the ToS link is on staging and this issue with the checkbox still remains.

I am casting a vote for "do nothing". The blur happens first and causes the layout shift. We shouldn't try to interfere with that. Our forms are design to get you to address errors and resolve them so I think this behavior would be more than tolerated (and probably unusual for anyone to actually do).

I tend to agree @marcaaron, though the fact it works fine on native but not Web is an inconsistency. Not sure if that wholly sways the opinion here, but if it's simple enough to remove that inconsistency, it could be worth it.

[marcaaron]

I'm a little concerned about the proposals we've got. The mouse down solution is missing some explanation for why a "mouse down" gets fired before a "blur" but a "click" happens after. And adding checks to the blur handler to "cancel the blur" really goes against out current design. On blur the error should be shown. I don't think we really need to make exceptions to that and I think users will have a high tolerance for a small issue like this.

[trjExpensify]

Yeah, I agree on blur the error should be shown. On native, we don't blur if you interact with the checkbox on the form. Here's a vid of iOS for reference:

RPReplay_Final1669913690.MP4

[marcaaron]

😞

[melvin-bot]

@trjExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

Coming back here. Why doesn't mobile work the same as web on this form, could we look at what explanation there is for why it isn't consistent? I think our options boil down to:

1. Live with the inconsistency between web and mobile (i.e do nothing)
2. Fix web to not lose focus on the field when clicking the checkbox (i.e like on mobile)
3. Fix mobile to lose focus on clicking the checkbox (i.e like on web)

[JmillsExpensify]

Huh, those are good questions. Maybe we bring this to Slack to align on the behavior?

[trjExpensify]

Yah mon, posted here.

[trjExpensify]

Discussion going on in the thread.

[trjExpensify]

The latest of where we're at on the expected behaviour from the thread is:

The correct behavior for mWeb and mobile would be:

1. Field gets blurred (you interacted with another field so the one that was focused should get blurred now)
2. Checkbox is checked (a blur event shouldn’t necessarily block the checkbox from getting pressed)

I have an outstanding question for @marcaaron re: platform consistency, but I believe this should be implemented consistency across web, desktop, mobile and mWeb.

I'm going to add the External label in the meantime to get this one moving.

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a77f27abe4f6fc46

[trjExpensify]

Still held, but we have someone picking up the form validation project at large now. Woop!

[trjExpensify]

Still the same here, Melv.

[muttmuure]

Not overdue

[trjExpensify]

Still held.

[muttmuure]

Held

[trjExpensify]

👋 The tracking issue this was held on has been completed now. I just re-tested this issue and the checkbox is checked on first click now as expected. @muttmuure, I've removed the hold from this issue title, dropped it back to daily, and you can proceed to process the $250 payment to @hungvu193 for this initial bug report.

[trjExpensify]

Taking care of this to clear it from WAQ dashboard. @hungvu193 sent you a contract here.

[hungvu193]

@trjExpensify Thank you, I've just applied

[trjExpensify]

Settled!

[muttmuure]

Thanks Tom!