[$250] [mWeb] - Profile- long pressing checkbox label for timezone gets unfocused on its own after few seconds reported by @daraksha-dk

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the app > Go to Profile
2. Press and hold the checkbox label
3. Focus will be lost on its own and no action will be taken

Expected Result:

User should be able to hold the checkbox label and toggle the checkbox once the focus is released by the user

Actual Result:

Gets unfocused and no action is being taken In Android /iOS will toggle the checkbox

Workaround:

unknown

Platform:

Where is this issue occurring?

• Mobile Web

Version Number: 1.2.26-0 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:

https://user-images.githubusercontent.com/43996225/201196485-0629bd39-7652-4b87-b6ee-270c5fa52687.mp4

https://user-images.githubusercontent.com/43996225/201196586-e435d2e7-31c5-4bdc-bcc0-5d96978cf888.mp4

https://user-images.githubusercontent.com/43996225/201196826-64774762-1e52-46fa-9f6b-90383515e60a.mp4

Expensify/Expensify Issue URL: Issue reported by: @daraksha-dk Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1668092428748689

View all open jobs on GitHub

[melvin-bot[bot]]

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

[davidcardoza]

I was able to reproduce this on mWeb. Sending off for engineering review.

[melvin-bot[bot]]

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

[davidcardoza]

@flodnv I assume this can go out to an external contributor? Let me know and I will get a job post up.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Dima-Kevanishvili]

Proposal

Add onLongPress to checkboxes and currency button.

diff --git a/src/components/CurrencySymbolButton.js b/src/components/CurrencySymbolButton.js
index 85a6ad8e4..a64ee1cec 100644
--- a/src/components/CurrencySymbolButton.js
+++ b/src/components/CurrencySymbolButton.js
@@ -14,7 +14,10 @@ const propTypes = {
 
 function CurrencySymbolButton(props) {
     return (
-        <TouchableOpacity onPress={props.onCurrencyButtonPress}>
+        <TouchableOpacity
+            onPress={props.onCurrencyButtonPress}
+            onLongPress={props.onCurrencyButtonPress}
+        >
             <Text style={styles.iouAmountText}>{props.currencySymbol}</Text>
         </TouchableOpacity>
     );

diff --git a/src/components/Checkbox.js b/src/components/Checkbox.js
index 612220faf..8b7afc00f 100644
--- a/src/components/Checkbox.js
+++ b/src/components/Checkbox.js
@@ -89,6 +89,7 @@ class Checkbox extends React.Component {
             <Pressable
                 disabled={this.props.disabled}
                 onPress={this.firePressHandlerOnClick}
+                onLongPress={this.firePressHandlerOnClick}
                 onMouseDown={this.props.onMouseDown}
                 onFocus={this.onFocus}
                 onBlur={this.onBlur}

diff --git a/src/components/CheckboxWithLabel.js b/src/components/CheckboxWithLabel.js
index 96f85fb25..48d20644f 100644
--- a/src/components/CheckboxWithLabel.js
+++ b/src/components/CheckboxWithLabel.js
@@ -73,16 +73,17 @@ const defaultProps = {
 class CheckboxWithLabel extends React.Component {
     constructor(props) {
         super(props);
-
-        this.isChecked = props.value || props.defaultValue || props.isChecked;
+        this.state = {
+            checked: props.value || props.defaultValue || props.isChecked,
+        };
         this.LabelComponent = props.LabelComponent;
 
         this.toggleCheckbox = this.toggleCheckbox.bind(this);
     }
 
     toggleCheckbox() {
-        this.props.onInputChange(!this.isChecked);
-        this.isChecked = !this.isChecked;
+        this.props.onInputChange(!this.state.checked);
+        this.setState(prevState => ({checked: !prevState.checked}));
     }
 
     render() {
@@ -90,7 +91,7 @@ class CheckboxWithLabel extends React.Component {
             <View style={this.props.style}>
                 <View style={[styles.flexRow, styles.alignItemsCenter]}>
                     <Checkbox
-                        isChecked={this.isChecked}
+                        isChecked={this.state.checked}
                         onPress={this.toggleCheckbox}
                         label={this.props.label}
                         hasError={Boolean(this.props.errorText)}
@@ -99,6 +100,7 @@ class CheckboxWithLabel extends React.Component {
                     <TouchableOpacity
                         focusable={false}
                         onPress={this.toggleCheckbox}
+                        onLongPress={this.toggleCheckbox}
                         style={[
                             styles.ml3,
                             styles.pr2,

Code above also adds state instead of straight up variable, because long pressing added a problem where the checkbox wouldn't switch the first time.

~~I also found a bug that the timezone checkbox doesn't actually persist over a refresh, seems like there's not api calls called when toggling it. Would this be in the scope to fix?~~ - Didn't see the save button ( Sorry )

[mollfpr]

Proposal

RCA Pressable / Touchables onPress function is called if released before entering the long press state (which control by delayLongPress ). We could set the long press like @Dima-Kevanishvili proposal here

Solution I have a different approach to solving this, rather than having onPress and onLongPress calling the same event. We could use onPressOut instead which will be fired when the press is released even on the long press state.

With onPressOut we could combine this with onPressIn to solve the issue when long pressing the checkbox (checkbox label) or the currency button the context menu shows up.

https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/Checkbox.js#L89-L91

             <Pressable
                 disabled={this.props.disabled}
-                onPress={this.firePressHandlerOnClick}
+                onPressIn={ControlSelection.block}
+                onPressOut={this.firePressHandlerOnClick}

https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/CheckboxWithLabel.js#L99-L101

                     <TouchableOpacity
                         focusable={false}
-                        onPress={this.toggleCheckbox}
+                        onPressIn={ControlSelection.block}
+                        onPressOut={() => {
+                            ControlSelection.unblock();
+                            this.toggleCheckbox();
+                        }}

https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/CurrencySymbolButton.js#L17

-        <TouchableOpacity onPress={props.onCurrencyButtonPress}>
+        <TouchableOpacity
+            onPressIn={ControlSelection.block}
+            onPressOut={() => {
+                ControlSelection.unblock();
+                props.onCurrencyButtonPress();
+            }}
+        >

I also agree to use setState instead, because we have dynamic value from isChecked and it changes the UI.

Result

mWeb/Safari

https://user-images.githubusercontent.com/25520267/201465723-3c2284a1-6bc4-4060-a04d-693d0d4ee865.mov

mWeb/Android

https://user-images.githubusercontent.com/25520267/201466081-d46e1745-dfd7-402a-b930-63e415a721ed.mov

iOS

https://user-images.githubusercontent.com/25520267/201465760-7ac3e027-8a1c-43c8-987c-1b775f2d7a76.mov

Android

https://user-images.githubusercontent.com/25520267/201466442-b6c9f20a-7aae-473c-84a7-139c432ef99b.mov

[melvin-bot[bot]]

@flodnv, @davidcardoza, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[flodnv]

@eVoloshchak can you please review the proposals?

[eVoloshchak]

@Dima-Kevanishvili's and @mollfpr's proposals both look great to me, the decision should come down to the expected behavior

@Dima-Kevanishvili's proposal: checkbox is toggled and text is "un-pressed" even if the user keeps pressing down

https://user-images.githubusercontent.com/9059945/201708384-cb3d07c3-1c46-44b2-86f8-83ba8b058ad2.mp4

@mollfpr's proposal: checkbox is toggled only after the user releases the touch, which is, if description is correct, the expected result: User should be able to hold the checkbox label and toggle the checkbox once the focus is released by the user

However, on mWeb android (both chrome and firefox), the behavior is the same as with @Dima-Kevanishvili's proposal, which is checkbox is toggled and text is "un-pressed" even if the user keeps pressing down. mWeb Safari doesn't have this problem @mollfpr, could you check this please?

[mollfpr]

However, on mWeb android (both chrome and firefox), the behavior is the same as with @Dima-Kevanishvili's proposal, which is checkbox is toggled and text is "un-pressed" even if the user keeps pressing down. mWeb Safari doesn't have this problem

Yes, my proposal has kinda the same behavior on Android Chrome and not happening on iOS Safari.

[eVoloshchak]

Yes, my proposal has kinda the same behavior on Android Chrome and not happening on iOS Safari.

That's what I was trying to say, sorry if it was unclear. Is there a way to remove this inconsistency between platforms?

[mollfpr]

I haven't found any solution for that, but I think the cause is the same with this long-press issue on Android web https://github.com/Expensify/App/issues/12324

[davidcardoza]

Job posted - https://www.upwork.com/jobs/~01e96489b139a99840

[Dima-Kevanishvili]

@eVoloshchak I'm assuming that my proposal wasn't accepted, am I correct?

[eVoloshchak]

@eVoloshchak I'm assuming that my proposal wasn't accepted, am I correct?

No, your proposal is absolutely valid, as I said, the decision should come down to the expected behavior, it hasn't been made yet.

@davidcardoza, what are your thoughts on this? We could go with the first proposal (but that would mean we slightly deviate from the expected result) or wait for the second approach to work on all platforms (not sure if possible, but my gut tells me that it is)

[davidcardoza]

My suggestion is to not deviate away from the expected result. Let's go with the second.

[flodnv]

@eVoloshchak based on your investigation and description, I would say let's go with @mollfpr's proposal. One thing I'm unclear on is:

wait for the second approach to work on all platforms (not sure if possible, but my gut tells me that it is)

What does "wait" mean -- wait for what exactly? I think as long as the checkbox gets toggled (which it does?), we're good to go as is...?

[eVoloshchak]

What does "wait" mean -- wait for what exactly? I think as long as the checkbox gets toggled (which it does?), we're good to go as is...?

@flodnv, there is a problem in @mollfpr's proposal, which is present only on mWeb Android

[flodnv]

Right, is this problem fixable @mollfpr ?

[mollfpr]

I’ll try find out about that.

[Emz27]

Proposal

• use react native gesture handler TouchableOpacity as an alternative

https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/CurrencySymbolButton.js#L2

diff --git a/src/components/CurrencySymbolButton.js b/src/components/CurrencySymbolButton.js
index 85a6ad8e4..0a46e0cd1 100644
--- a/src/components/CurrencySymbolButton.js
+++ b/src/components/CurrencySymbolButton.js
@@ -1,4 +1,5 @@
 import React from 'react';
+import {TouchableOpacity as TouchableOpacityGesture} from 'react-native-gesture-handler';
 import {TouchableOpacity} from 'react-native';
 import PropTypes from 'prop-types';
 import Text from './Text';
@@ -14,9 +15,13 @@ const propTypes = {
 
 function CurrencySymbolButton(props) {
     return (
-        <TouchableOpacity onPress={props.onCurrencyButtonPress}>
-            <Text style={styles.iouAmountText}>{props.currencySymbol}</Text>
-        </TouchableOpacity>
+        <TouchableOpacityGesture onPress={props.onCurrencyButtonPress}>
+            {/* gesture touchable to fix long press inconsistency */}
+            <TouchableOpacity pointerEvents="none" activeOpacity={1}>
+                {/* react touchable as child to allow touch hovering on desktop */}
+                <Text style={styles.iouAmountText}>{props.currencySymbol}</Text>
+            </TouchableOpacity>
+        </TouchableOpacityGesture>
     );
 }

https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/CheckboxWithLabel.js#L3

diff --git a/src/components/CheckboxWithLabel.js b/src/components/CheckboxWithLabel.js
index 96f85fb25..5473c78e0 100644
--- a/src/components/CheckboxWithLabel.js
+++ b/src/components/CheckboxWithLabel.js
@@ -1,6 +1,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import {View, TouchableOpacity} from 'react-native';
+import {TouchableOpacity as TouchableOpacityGesture} from 'react-native-gesture-handler';
import styles from '../styles/styles';
import Checkbox from './Checkbox';
import Text from './Text';
@@ -80,9 +81,17 @@ class CheckboxWithLabel extends React.Component {
        this.toggleCheckbox = this.toggleCheckbox.bind(this);
    }

+    shouldComponentUpdate(nextProps) {
+        if (nextProps.isChecked !== this.props.isChecked) {
+            this.isChecked = nextProps.isChecked;
+        } else if (nextProps.value !== this.props.value) {
+            this.isChecked = nextProps.value;
+        }
+        return true;
+    }
+
    toggleCheckbox() {
        this.props.onInputChange(!this.isChecked);
-        this.isChecked = !this.isChecked;
    }

    render() {
@@ -96,7 +105,8 @@ class CheckboxWithLabel extends React.Component {
                        hasError={Boolean(this.props.errorText)}
                        forwardedRef={this.props.forwardedRef}
                    />
-                    <TouchableOpacity
+                    {/* gesture touchable to fix long press inconsistency */}
+                    <TouchableOpacityGesture
                        focusable={false}
                        onPress={this.toggleCheckbox}
                        style={[
@@ -110,13 +120,16 @@ class CheckboxWithLabel extends React.Component {
                            styles.noSelect,
                        ]}
                    >
-                        {this.props.label && (
-                            <Text style={[styles.ml1]}>
-                                {this.props.label}
-                            </Text>
-                        )}
-                        {this.LabelComponent && (<this.LabelComponent />)}
-                    </TouchableOpacity>
+                        {/* react touchable as child to allow touch hovering on desktop */}
+                        <TouchableOpacity pointerEvents="none" activeOpacity={1}>
+                            {this.props.label && (
+                                <Text style={[styles.ml1]}>
+                                    {this.props.label}
+                                </Text>
+                            )}
+                            {this.LabelComponent && (<this.LabelComponent />)}
+                        </TouchableOpacity>
+                    </TouchableOpacityGesture>
                </View>
                <FormHelpMessage message={this.props.errorText} />
            </View>

I can make a standard opacity button for this to make it less ugly. I hope you consider my workaround

I also fix some potential bug : this.isChecked = !this.isChecked; - this does not cause a render and will still depend on the parent for triggering the render. I made a handler for it to have a one source of truth and prevent hard to replicate bug

already tested in Android, IOS and macos

[Emz27]

working branch : https://github.com/Emz27/App/tree/bugfix/12650

[flodnv]

That feels a bit overkill to me but I don't know a whole lot... what do you think @eVoloshchak ?

[eVoloshchak]

I feel like this can cause a lot of regressions, this is technically a workaround, agree with @flodnv I think we should try to resolve the root cause of the issue, why is this happening only on mWeb Android, is there a chance we could fix this in react-native-web?

[Emz27]

The main solution here is to just nest 2 touchable and disable the inner one, the other diff is to fix issue on rendering the check status. Not sure about the regression because its supposed to be the same as the React native touchable. The component has been reused a just few times so I think it is easy to test

[mollfpr]

Updated proposal

The problem that occurred on Android Chrome is because the context menu is canceling the long press. The solution for that problem is to add an event listener for the element and prevent the context menu to appear with preventDefault and stopPropagation.

I also incline to update the Checkbox, CheckboxWithLabel, and CurrencySymbolButton to the platform-specific component because the context menu issues are only on the web.

diff --git a/src/components/Checkbox.js b/src/components/Checkbox.js
index 612220faf..654e6dc6b 100644
--- a/src/components/Checkbox.js
+++ b/src/components/Checkbox.js
@@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
 import styles from '../styles/styles';
 import stylePropTypes from '../styles/stylePropTypes';
 import Icon from './Icon';
+import ControlSelection from '../libs/ControlSelection';
 import * as Expensicons from './Icon/Expensicons';

 const propTypes = {
@@ -58,6 +59,26 @@ class Checkbox extends React.Component {
         this.firePressHandlerOnClick = this.firePressHandlerOnClick.bind(this);
     }

+    componentDidMount() {
+        if (!this.pressableRef.addEventListener) {
+            return;
+        }
+        this.pressableRef.addEventListener('contextmenu', (event) => {
+            event.preventDefault();
+            event.stopPropagation();
+        });
+    }
+
+    componentWillUnmount() {
+        if (!this.pressableRef.removeEventListener) {
+            return;
+        }
+        this.pressableRef.removeEventListener('contextmenu', (event) => {
+            event.preventDefault();
+            event.stopPropagation();
+        });
+    }
+
     onFocus() {
         this.setState({isFocused: true});
     }
@@ -88,11 +109,18 @@ class Checkbox extends React.Component {
         return (
             <Pressable
                 disabled={this.props.disabled}
-                onPress={this.firePressHandlerOnClick}
+                onPressIn={ControlSelection.block}
+                onPressOut={(event) => {
+                    ControlSelection.unblock();
+                    this.firePressHandlerOnClick(event);
+                }}
                 onMouseDown={this.props.onMouseDown}
                 onFocus={this.onFocus}
                 onBlur={this.onBlur}
-                ref={this.props.forwardedRef}
+                ref={(el) => {
+                    this.pressableRef = el;
+                    this.props.forwardedRef(el);
+                }}
                 style={this.props.style}
                 onKeyDown={this.handleSpaceKey}
                 accessibilityRole="checkbox"
diff --git a/src/components/CheckboxWithLabel.js b/src/components/CheckboxWithLabel.js
index 96f85fb25..246d8c2a8 100644
--- a/src/components/CheckboxWithLabel.js
+++ b/src/components/CheckboxWithLabel.js
@@ -5,6 +5,7 @@ import styles from '../styles/styles';
 import Checkbox from './Checkbox';
 import Text from './Text';
 import FormHelpMessage from './FormHelpMessage';
+import ControlSelection from '../libs/ControlSelection';

 const requiredPropsCheck = (props) => {
     if (!props.label && !props.LabelComponent) {
@@ -74,15 +75,42 @@ class CheckboxWithLabel extends React.Component {
     constructor(props) {
         super(props);

-        this.isChecked = props.value || props.defaultValue || props.isChecked;
         this.LabelComponent = props.LabelComponent;

         this.toggleCheckbox = this.toggleCheckbox.bind(this);
+
+        this.state = {
+            isChecked: props.value || props.defaultValue || props.isChecked,
+        };
+    }
+
+    componentDidMount() {
+        if (!this.touchableRef.addEventListener) {
+            return;
+        }
+        this.touchableRef.addEventListener('contextmenu', (event) => {
+            event.preventDefault();
+            event.stopPropagation();
+        });
+    }
+
+    componentWillUnmount() {
+        if (!this.touchableRef.removeEventListener) {
+            return;
+        }
+        this.touchableRef.removeEventListener('contextmenu', (event) => {
+            event.preventDefault();
+            event.stopPropagation();
+        });
     }

     toggleCheckbox() {
-        this.props.onInputChange(!this.isChecked);
-        this.isChecked = !this.isChecked;
+        this.setState((prevState) => {
+            this.props.onInputChange(!prevState.isChecked);
+            return {
+                isChecked: !prevState.isChecked,
+            };
+        });
     }

     render() {
@@ -90,15 +118,20 @@ class CheckboxWithLabel extends React.Component {
             <View style={this.props.style}>
                 <View style={[styles.flexRow, styles.alignItemsCenter]}>
                     <Checkbox
-                        isChecked={this.isChecked}
+                        isChecked={this.state.isChecked}
                         onPress={this.toggleCheckbox}
                         label={this.props.label}
                         hasError={Boolean(this.props.errorText)}
                         forwardedRef={this.props.forwardedRef}
                     />
                     <TouchableOpacity
+                        ref={el => this.touchableRef = el}
                         focusable={false}
-                        onPress={this.toggleCheckbox}
+                        onPressIn={ControlSelection.block}
+                        onPressOut={() => {
+                            ControlSelection.unblock();
+                            this.toggleCheckbox();
+                        }}
                         style={[
                             styles.ml3,
                             styles.pr2,
diff --git a/src/components/CurrencySymbolButton.js b/src/components/CurrencySymbolButton.js
index 85a6ad8e4..d963aad0b 100644
--- a/src/components/CurrencySymbolButton.js
+++ b/src/components/CurrencySymbolButton.js
@@ -1,8 +1,9 @@
-import React from 'react';
+import React, {Component} from 'react';
 import {TouchableOpacity} from 'react-native';
 import PropTypes from 'prop-types';
 import Text from './Text';
 import styles from '../styles/styles';
+import ControlSelection from '../libs/ControlSelection';

 const propTypes = {
     /** Currency symbol of selected currency */
@@ -12,15 +13,43 @@ const propTypes = {
     onCurrencyButtonPress: PropTypes.func.isRequired,
 };

-function CurrencySymbolButton(props) {
-    return (
-        <TouchableOpacity onPress={props.onCurrencyButtonPress}>
-            <Text style={styles.iouAmountText}>{props.currencySymbol}</Text>
-        </TouchableOpacity>
-    );
+class CurrencySymbolButton extends Component {
+    componentDidMount() {
+        if (!this.touchableRef.addEventListener) {
+            return;
+        }
+        this.touchableRef.addEventListener('contextmenu', (event) => {
+            event.preventDefault();
+            event.stopPropagation();
+        });
+    }
+
+    componentWillUnmount() {
+        if (!this.touchableRef.removeEventListener) {
+            return;
+        }
+        this.touchableRef.removeEventListener('contextmenu', (event) => {
+            event.preventDefault();
+            event.stopPropagation();
+        });
+    }
+
+    render() {
+        return (
+            <TouchableOpacity
+                ref={el => this.touchableRef = el}
+                onPressIn={ControlSelection.block}
+                onPressOut={(event) => {
+                    ControlSelection.unblock();
+                    this.props.onCurrencyButtonPress(event);
+                }}
+            >
+                <Text style={styles.iouAmountText}>{this.props.currencySymbol}</Text>
+            </TouchableOpacity>
+        );
+    }
 }

 CurrencySymbolButton.propTypes = propTypes;
-CurrencySymbolButton.displayName = 'CurrencySymbolButton';

 export default CurrencySymbolButton;

iOS Safari

https://user-images.githubusercontent.com/25520267/203065367-74ceeeab-7694-4b73-b1f8-c8dbbcf32aff.mov

Android Chrome

https://user-images.githubusercontent.com/25520267/203065388-fc191db3-3770-4252-9e33-e0c99a390c66.mov

iOS

https://user-images.githubusercontent.com/25520267/203065406-f00b30cd-ee80-4d49-9a1a-da68254df942.mov

Android

https://user-images.githubusercontent.com/25520267/203065427-a58c5229-6904-4f50-b8c1-14886823f637.mov

[flodnv]

We discussed this internally with Contributor+s. The more discussion we are getting on this issue, the more this looks like something that needs to be fixed with Touchable in react-native-web. This issue is indeed a bit weird/inconsistent, but also not a real action users want to take. Additionally, Touchable is being deprecated as part of the Fabric/React 18 upgrade. As such, we are closing this issue as not a bug and won't fix.