Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.54k stars 2.88k forks source link

[HOLD for payment 2023-01-30] [$4000] Mac / Safari - Cursor Pointer changes to I-beam while dragging image or slider knob in edit photo. #13688

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

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 settings > Profile > Edit photo
  2. Upload a photo.
  3. Drag image lider knob

Expected Result:

Cursor should remain in pointer state while dragging.

Actual Result:

Cursor pointer changes to I-beam while dragging image or slider knob.

Workaround:

unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.41-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:

https://user-images.githubusercontent.com/43996225/208452575-e4d9eaab-e2c7-4214-91b8-4b3988a74991.mov

https://user-images.githubusercontent.com/43996225/208452298-e4fee79a-7171-4d93-9252-8e18687a35f3.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ba623ffe6500b581
  • Upwork Job ID: 1604887323754151936
  • Last Price Increase: 2023-01-02
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

thesahindia commented 1 year ago

The issue was reported by @Tushu17 (not me)

Tushu17 commented 1 year ago

The issue was reported by @Tushu17 (not me)

Thank you.

proposal

We need to disable selection when dragging for that can add document.onselectstart = () => false; in useEffect https://github.com/Expensify/App/blob/b0f6785f0f633b062dd08078b8fbb7e0d5868c23/src/components/AvatarCropModal/AvatarCropModal.js#L110-L112

getusha commented 1 year ago

Proposal

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..84e663ee83 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -338,7 +338,13 @@ const AvatarCropModal = (props) => {
                                 <Pressable
                                     style={[styles.mh5, styles.flex1]}
                                     onLayout={initializeSliderContainer}
-                                    onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+                                    onPressIn={e => {
+                                        runOnUI(sliderOnPress)(e.nativeEvent.locationX);
+                                        document.onselectstart = function(){ return false;}
+                                    }}
+                                    onPressOut={()=>{
+                                        document.onselectstart = function(){ return true; }
+                                    }}
                                 >
                                     <Slider sliderValue={translateSlider} onGesture={panSliderGestureEventHandler} />
                                 </Pressable>

Result

https://user-images.githubusercontent.com/75031127/208491862-97824cd6-6187-480c-8132-773cf12fb322.mp4

since it disables the whole selection drag in the entire page, we need to re enable it after the press is lifted.

Without applying onPressOut

https://user-images.githubusercontent.com/75031127/208493889-4bf14471-fe14-4fdc-b464-5d72d91a05b6.mov

Also Noticed that runOnUI(sliderOnPress)(e.nativeEvent.locationX); can sometimes cause the onselectstart not to fire

In this case we can apply the solution separately using onPress

+                                    onPress={() => {
+                                        document.onselectstart = function(){ return false;}
+                                    }}
syedsaroshfarrukhdot commented 1 year ago

Proposal :-

Along with slider we need to disable selection on PanGestureHandler

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901..1250c877c 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -210,6 +210,8 @@ const AvatarCropModal = (props) => {
             // eslint-disable-next-line no-param-reassign
             context.translateSliderX = translateSlider.value;
             isPressableEnabled.value = false;
+
+            document.onselectstart = function(){ return false;}
         },
         onActive: (event, context) => {
             const newSliderValue = clamp(event.translationX + context.translateSliderX, [0, sliderContainerSize]);
@@ -222,9 +224,15 @@ const AvatarCropModal = (props) => {

             const newX = translateX.value * differential;
             const newY = translateY.value * differential;
+
+            document.onselectstart = function(){ return false;}
+            
             updateImageOffset(newX, newY);
         },
-        onEnd: () => isPressableEnabled.value = true,
+        onEnd: () => {
+            document.onselectstart = function(){ return true;}
+            isPressableEnabled.value = true
+        },
     }, [imageContainerSize, clamp, translateX, translateY, translateSlider, scale, sliderContainerSize, isPressableEnabled]);

     // This effect is needed to prevent the incorrect position of
@@ -338,7 +346,13 @@ const AvatarCropModal = (props) => {
                                 <Pressable
                                     style={[styles.mh5, styles.flex1]}
                                     onLayout={initializeSliderContainer}
-                                    onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+                                    onPressIn={e => {
+                                        runOnUI(sliderOnPress)(e.nativeEvent.locationX)
+                                        document.onselectstart = function(){ return false;}
+                                    }}
+                                    onPressOut={() => {
+                                        document.onselectstart = function(){ return true;}
+                                    }}
                                 >
                                     <Slider sliderValue={translateSlider} onGesture={panSliderGestureEventHandler} />
                                 </Pressable>
aneequeahmad commented 1 year ago

PROPOSAL

We just need to add the following line of code in index.html. It didn't compromise any other functionality and it will fix all the I-beam cursor issues on drag. We are using -webkit-user-select: text !important; which is for inputs and that is working fine too.

Code changes will be here after the line#25 https://github.com/Expensify/App/blob/3e050bd5de58f594dc07a0b2e9f2720b3f0a9bd2/web/index.html#L23-L25

  * {
        -webkit-user-select: none !important
     }

Please see this link for more info.

After the fix

https://user-images.githubusercontent.com/36359331/208511498-20c8023c-12b7-4b3f-a7ac-0bc3e8a2bf58.mov

jatinsonijs commented 1 year ago

Proposal

It's happening due to safari default behaviour - that's way safari handle dragging. It's not just related to Expensify. I have tested several website all are showing I-beam cursor on drawing so we have to handle it manually or by css.

If we directly use document in AvatarCropModal it will throw an error on mobile bcz document not available on mobile. it should apply only for web. If we apply global css it will block other area also like

With this css not able to select text in chat also.

I saw in code we have a utility function to handle user select active - inactive (ControlSelection)

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..4531fa598d 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -302,6 +303,14 @@ const AvatarCropModal = (props) => {
         updateImageOffset(newX, newY);
     };

+    useEffect(() => {
+        if (props.isVisible) {
+            ControlSelection.block();
+        } else { ControlSelection.unblock(); }
+
+        return () => ControlSelection.unblock();
+    }, [props.isVisible]);
+
     return (
         <Modal
             onClose={props.onClose}

I have tested it with apply on gesture handlers onStart, onEnd and onPressIn, onPressOut but sometime its not working.

We can disable selection when crop modal visible and active again when modal hide. drawback with this solution is it will not allow any text selection until modal open. During modal open i have checked there is not any text which should be selectable. So I think we can use this solution.

https://user-images.githubusercontent.com/114683042/208636539-c864e00f-29c9-4d86-b67b-3116e5e8ac4c.mov

s77rt commented 1 year ago

@jatinsonijs The approach of using ControlSelection seems the best so far, I was working on that too and got the same conclusions as you:

I have tested it with apply on gesture handlers onStart, onEnd and onPressIn, onPressOut but sometime its not working.

However, your proposal is still not that good as it will disable the selection on the entire screen as long as the crop modal is visible.

The selection should be disabled only when dragging.

jatinsonijs commented 1 year ago

yes @s77rt, I understood. I have found other solution

Proposal 2

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..611ec46395 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -316,7 +317,16 @@ const AvatarCropModal = (props) => {
                 onCloseButtonPress={props.onClose}
             />
             <Text style={[styles.mh5]}>{props.translate('avatarCropModal.description')}</Text>
-            <GestureHandlerRootView onLayout={initializeImageContainer} style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}>
+            <GestureHandlerRootView
+                onMouseDown={() => {
+                    ControlSelection.block();
+                }}
+                onMouseUp={() => {
+                    ControlSelection.unblock();
+                }}
+                onLayout={initializeImageContainer}
+                style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}
+            >

                 {/* To avoid layout shift we should hide this component until the image container & image is initialized */}
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />
s77rt commented 1 year ago

@jatinsonijs The code looks much better now, can you just double check if this works all the time (onStart and onEnd on PanGestureHandler didn't work well) Although the use of onMouseDown/onMouseUp looks promising

jatinsonijs commented 1 year ago

Yes I have tested it working fine

s77rt commented 1 year ago

@jatinsonijs I'm not a C+ but you have my vote.

puneetlath commented 1 year ago

@aimane-chnaif thoughts on these proposals?

aimane-chnaif commented 1 year ago

@aimane-chnaif thoughts on these proposals?

reviewing today

aimane-chnaif commented 1 year ago

In @jatinsonijs 's proposal 1: I think Edit photo, Drag, zoom, and rotate ... texts should still be selectable.

In @jatinsonijs 's proposal 2: When mouse up outside of modal, text is not selectable until mouse down again.

And I gave all other proposals πŸ‘Ž directly because they cause regressions on native apps or text selection.

So no acceptable solutions so far. Still looking for better proposals.

syedsaroshfarrukhdot commented 1 year ago

Proposal :-

We could fix this issue by setting selection to unblock when mouse up in the model.

diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 323df19c7..cc506586c 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -10,6 +10,7 @@ import {propTypes as modalPropTypes, defaultProps as modalDefaultProps} from './
 import * as Modal from '../../libs/actions/Modal';
 import getModalStyles from '../../styles/getModalStyles';
 import variables from '../../styles/variables';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     ...modalPropTypes,
@@ -86,7 +87,6 @@ class BaseModal extends PureComponent {
                     }
                     this.props.onClose();
                 }}

                 // Note: Escape key on web/desktop will trigger onBackButtonPress callback
                 // eslint-disable-next-line react/jsx-props-no-multi-spaces
                 onBackButtonPress={this.props.onClose}
@@ -116,6 +116,7 @@ class BaseModal extends PureComponent {
                 animationInTiming={this.props.animationInTiming}
                 animationOutTiming={this.props.animationOutTiming}
                 statusBarTranslucent={this.props.statusBarTranslucent}
+                onMouseUp={this.props.onMouseUp}
             >
                 <SafeAreaInsetsContext.Consumer>
                     {(insets) => {

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901..48efe0b5f 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -237,6 +238,7 @@ const AvatarCropModal = (props) => {
         );
     }, [sliderContainerSize]);

+      
     // Rotates the image by changing the rotation value by 90 degrees
     // and updating the position so the image remains in the same place after rotation
     const rotateImage = useCallback(() => {
@@ -309,6 +311,8 @@ const AvatarCropModal = (props) => {
             type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
             onModalHide={resetState}
             statusBarTranslucent={false}
+            onMouseUp={() => ControlSelection.unblock()}
+            
         >
             {props.isSmallScreenWidth && <HeaderGap />}
             <HeaderWithCloseButton
@@ -316,7 +320,16 @@ const AvatarCropModal = (props) => {
                 onCloseButtonPress={props.onClose}
             />
             <Text style={[styles.mh5]}>{props.translate('avatarCropModal.description')}</Text>
-            <GestureHandlerRootView onLayout={initializeImageContainer} style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}>
+            <GestureHandlerRootView 
+                 onLayout={initializeImageContainer} 
+                 style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}
+                 onMouseDown={() => {
+                    ControlSelection.block();
+                 }}
+                 onMouseUp={() => {
+                    ControlSelection.unblock();
+                 }}
+            >

                 {/* To avoid layout shift we should hide this component until the image container & image is initialized */}
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />

Or We could also unblock control selection in onEnd of panSliderGestureEventHandler and panGestureEventHandler

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901..5b4005f8e 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection'

 const propTypes = {
     /** Link to image for cropping */
@@ -197,6 +198,10 @@ const AvatarCropModal = (props) => {

             updateImageOffset(newX, newY);
         },
+        onEnd: () => {
+            ControlSelection.unblock()
+        },
+
     }, [imageContainerSize, updateImageOffset, translateX, translateY]);

     /**
@@ -224,7 +229,10 @@ const AvatarCropModal = (props) => {
             const newY = translateY.value * differential;
             updateImageOffset(newX, newY);
         },
-        onEnd: () => isPressableEnabled.value = true,
+        onEnd: () => {
+            isPressableEnabled.value = true
+            ControlSelection.unblock()
+        },
     }, [imageContainerSize, clamp, translateX, translateY, translateSlider, scale, sliderContainerSize, isPressableEnabled]);

     // This effect is needed to prevent the incorrect position of

After Fix :-

https://user-images.githubusercontent.com/81307212/209021759-825ae7f0-fba6-448a-9142-7f33e2270a51.mov

jatinsonijs commented 1 year ago

Proposal If we can add new prop in modal If parent component change anonymous function may case render in modal as it will break PureComponent comparison. And we also not need to pass onMouseUp on GestureHandlerRootView as parent (Modal) already have it with same functionality.

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..6f5df518d9 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -92,6 +93,9 @@ const AvatarCropModal = (props) => {

     // Changes the modal state values to initial
     const resetState = useCallback(() => {
+        // Unblock selection on modal close incase modal
+        // onMouseUp not fired due to forcefully navigate back
+        ControlSelection.unblock();
         originalImageWidth.value = CONST.AVATAR_CROP_MODAL.INITIAL_SIZE;
         originalImageHeight.value = CONST.AVATAR_CROP_MODAL.INITIAL_SIZE;
         translateY.value = 0;
@@ -309,6 +313,7 @@ const AvatarCropModal = (props) => {
             type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
             onModalHide={resetState}
             statusBarTranslucent={false}
+            onMouseUp={ControlSelection.unblock}
         >
             {props.isSmallScreenWidth && <HeaderGap />}
             <HeaderWithCloseButton
@@ -316,7 +321,11 @@ const AvatarCropModal = (props) => {
                 onCloseButtonPress={props.onClose}
             />
             <Text style={[styles.mh5]}>{props.translate('avatarCropModal.description')}</Text>
-            <GestureHandlerRootView onLayout={initializeImageContainer} style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}>
+            <GestureHandlerRootView
+                onMouseDown={ControlSelection.block}
+                onLayout={initializeImageContainer}
+                style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}
+            >

                 {/* To avoid layout shift we should hide this component until the image container & image is initialized */}
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />
diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 323df19c77..f2b078eebf 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -116,6 +116,7 @@ class BaseModal extends PureComponent {
                 animationInTiming={this.props.animationInTiming}
                 animationOutTiming={this.props.animationOutTiming}
                 statusBarTranslucent={this.props.statusBarTranslucent}
+                onMouseUp={this.props.onMouseUp}
             >
                 <SafeAreaInsetsContext.Consumer>
                     {(insets) => {
diff --git a/src/components/Modal/modalPropTypes.js b/src/components/Modal/modalPropTypes.js
index 8f09588e37..8917dbb431 100644
--- a/src/components/Modal/modalPropTypes.js
+++ b/src/components/Modal/modalPropTypes.js
@@ -61,6 +61,9 @@ const propTypes = {
     /** Whether the modal should go under the system statusbar */
     statusBarTranslucent: PropTypes.bool,

+    /** Callback method fired when the mouseup inside the modal */
+    onMouseUp: PropTypes.func,
+
     ...windowDimensionsPropTypes,
 };
jatinsonijs commented 1 year ago

Combined with above Proposal handled edge cases => If drag and release mouse outside of window on chrome not showing I-Beam above text. => If "Drag, zoom, and rotate your image..." already selected drag image showing I-Beam in Safari. => Shifted onMouseDown to very specific area Slider and ImageCrop only.

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..d6afe6270a 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -197,6 +198,9 @@ const AvatarCropModal = (props) => {

             updateImageOffset(newX, newY);
         },
+        onEnd: () => {
+            ControlSelection.unblock();
+        },
     }, [imageContainerSize, updateImageOffset, translateX, translateY]);

     /**
@@ -224,7 +228,10 @@ const AvatarCropModal = (props) => {
             const newY = translateY.value * differential;
             updateImageOffset(newX, newY);
         },
-        onEnd: () => isPressableEnabled.value = true,
+        onEnd: () => {
+            isPressableEnabled.value = true;
+            ControlSelection.unblock();
+        },
     }, [imageContainerSize, clamp, translateX, translateY, translateSlider, scale, sliderContainerSize, isPressableEnabled]);

     // This effect is needed to prevent the incorrect position of
@@ -309,6 +316,7 @@ const AvatarCropModal = (props) => {
             type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
             onModalHide={resetState}
             statusBarTranslucent={false}
+            onMouseUp={ControlSelection.unblock}
         >
             {props.isSmallScreenWidth && <HeaderGap />}
             <HeaderWithCloseButton
@@ -322,23 +330,26 @@ const AvatarCropModal = (props) => {
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />
                     : (
                         <>
-                            <ImageCropView
-                                imageUri={props.imageUri}
-                                containerSize={imageContainerSize}
-                                panGestureEventHandler={panGestureEventHandler}
-                                originalImageHeight={originalImageHeight}
-                                originalImageWidth={originalImageWidth}
-                                scale={scale}
-                                translateY={translateY}
-                                translateX={translateX}
-                                rotation={rotation}
-                            />
+                            <Pressable onMouseDown={ControlSelection.block}>
+                                <ImageCropView
+                                    imageUri={props.imageUri}
+                                    containerSize={imageContainerSize}
+                                    panGestureEventHandler={panGestureEventHandler}
+                                    originalImageHeight={originalImageHeight}
+                                    originalImageWidth={originalImageWidth}
+                                    scale={scale}
+                                    translateY={translateY}
+                                    translateX={translateX}
+                                    rotation={rotation}
+                                />
+                            </Pressable>
                             <View style={[styles.mt5, styles.justifyContentBetween, styles.alignItemsCenter, styles.flexRow, StyleUtils.getWidthStyle(imageContainerSize)]}>
                                 <Icon src={Expensicons.Zoom} fill={themeColors.icons} />
                                 <Pressable
                                     style={[styles.mh5, styles.flex1]}
                                     onLayout={initializeSliderContainer}
                                     onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+                                    onMouseDown={ControlSelection.block}
                                 >
                                     <Slider sliderValue={translateSlider} onGesture={panSliderGestureEventHandler} />
                                 </Pressable>
syedsaroshfarrukhdot commented 1 year ago

Updated Proposal :-

I noticed an issue on native Platforms as highlighted in this Comment by @aimane-chnaif which was causing regression on native platform.

We were using ControlSelection.unblock() directly which was throwing an exception on native platforms so to call it asynchronously we need to wrap it in runOnJS in onEnd of panSliderGestureEventHandler and panGestureEventHandler to make sure that we are calling callback from js thread.

Error Before Directly Using ControlSelection.unblock() in onEnd of panSliderGestureEventHandler and panGestureEventHandler on NativePlatforms :-

WhatsApp Image 2022-12-22 at 1 08 34 PM

This proposal cover all edge cases to fix issue on all platforms.

  1. Selected Text Drag And Drop On Avatar Modal Shows I-Beam. (Fixed)
  2. While Mouse Down Move Out Of Modal Text Is Not Again Selectable. (Fixed)

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901..ae28e5141 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -8,6 +8,7 @@ import {
 import {GestureHandlerRootView} from 'react-native-gesture-handler';
 import {
     runOnUI,
+    runOnJS,
     interpolate,
     useAnimatedGestureHandler,
     useSharedValue,
@@ -30,6 +31,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection'

 const propTypes = {
     /** Link to image for cropping */
@@ -178,12 +180,25 @@ const AvatarCropModal = (props) => {
         return ((newSliderValue / containerSize) * (MAX_SCALE - MIN_SCALE)) + MIN_SCALE;
     });

     /**
      * Calculates new x & y image translate value on image panning
      * and updates image's offset.
      */
     const panGestureEventHandler = useAnimatedGestureHandler({
         onStart: (_, context) => {
             // we have to assign translate values to a context
             // since that is required for proper work of turbo modules.
             // eslint-disable-next-line no-param-reassign
@@ -197,6 +212,8 @@ const AvatarCropModal = (props) => {

             updateImageOffset(newX, newY);
         },
+        onEnd : () => runOnJS(ControlSelection.unblock)(),
+       
     }, [imageContainerSize, updateImageOffset, translateX, translateY]);

     /**

  // Changes the modal state values to initial
    const resetState = useCallback(() => {
+      ControlSelection.unblock()
        originalImageWidth.value = CONST.AVATAR_CROP_MODAL.INITIAL_SIZE;
        originalImageHeight.value = CONST.AVATAR_CROP_MODAL.INITIAL_SIZE;
        translateY.value = 0;
        translateX.value = 0;
        scale.value = CONST.AVATAR_CROP_MODAL.MIN_SCALE;
        rotation.value = 0;
        translateSlider.value = 0;
        setImageContainerSize(CONST.AVATAR_CROP_MODAL.INITIAL_SIZE);
        setSliderContainerSize(CONST.AVATAR_CROP_MODAL.INITIAL_SIZE);
        setIsImageContainerInitialized(false);
        setIsImageInitialized(false);
    }, []);
@@ -224,7 +241,11 @@ const AvatarCropModal = (props) => {
             const newY = translateY.value * differential;
             updateImageOffset(newX, newY);
         },
-        onEnd: () => isPressableEnabled.value = true,
+        onEnd: () => {
+            isPressableEnabled.value = true
+            runOnJS(ControlSelection.unblock)()
+        },
     }, [imageContainerSize, clamp, translateX, translateY, translateSlider, scale, sliderContainerSize, isPressableEnable
d]);

     // This effect is needed to prevent the incorrect position of
@@ -302,6 +323,8 @@ const AvatarCropModal = (props) => {
         updateImageOffset(newX, newY);
     };

+    
+
     return (
         <Modal
             onClose={props.onClose}
@@ -316,12 +339,17 @@ const AvatarCropModal = (props) => {
                 onCloseButtonPress={props.onClose}
             />
             <Text style={[styles.mh5]}>{props.translate('avatarCropModal.description')}</Text>
-            <GestureHandlerRootView onLayout={initializeImageContainer} style={[styles.alignSelfStretch, styles.m5, style
s.flex1, styles.alignItemsCenter]}>
+            <GestureHandlerRootView 
+                onLayout={initializeImageContainer} 
+                style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}
+                onMouseDown={() => ControlSelection.block()}
+             >

                 {/* To avoid layout shift we should hide this component until the image container & image is initialized 
*/}
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} s
tyle={[styles.flex1]} size="large" />
                     : (
                         <>
+                        <Pressable onMouseDown={() => ControlSelection.block()}>
                             <ImageCropView
                                 imageUri={props.imageUri}
                                 containerSize={imageContainerSize}
@@ -333,12 +361,14 @@ const AvatarCropModal = (props) => {
                                 translateX={translateX}
                                 rotation={rotation}
                             />
+                        </Pressable>
                             <View style={[styles.mt5, styles.justifyContentBetween, styles.alignItemsCenter, styles.flexR
ow, StyleUtils.getWidthStyle(imageContainerSize)]}>
                                 <Icon src={Expensicons.Zoom} fill={themeColors.icons} />
                                 <Pressable
                                     style={[styles.mh5, styles.flex1]}
                                     onLayout={initializeSliderContainer}
                                     onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+                                    onMouseDown={() => ControlSelection.block()}
                                 >
                                     <Slider sliderValue={translateSlider} onGesture={panSliderGestureEventHandler} />
                                 </Pressable>

Chrome:-

https://user-images.githubusercontent.com/81307212/209084663-7c7a2880-12f3-4d3f-b99e-7a40ac72c675.mov

Safari :-

https://user-images.githubusercontent.com/81307212/209085044-2a872575-30ee-424a-8c20-c35b20a1bc89.mov

IOS :-

https://user-images.githubusercontent.com/81307212/209088594-8d2747a8-7d6e-4a11-a918-01858ebf254f.mov

Android :-

https://user-images.githubusercontent.com/81307212/209088673-5623307a-4b4e-4fc6-9c96-6a12aefe4914.mp4

jatinsonijs commented 1 year ago

@syedsaroshfarrukhdot interesting, my bad I have missed it but you're still missing one edge case. and re-render optimization.

My Final Merged Proposal => If drag and release mouse outside of window on chrome not showing I-Beam above text. => If "Drag, zoom, and rotate your image..." already selected drag image showing I-Beam in Safari. => Shifted onMouseDown to very specific area Slider and ImageCrop only.

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..f2eb7069d1 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -12,6 +12,7 @@ import {
     useAnimatedGestureHandler,
     useSharedValue,
     useWorkletCallback,
+    runOnJS,
 } from 'react-native-reanimated';
 import CONST from '../../CONST';
 import compose from '../../libs/compose';
@@ -30,6 +31,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -92,6 +94,9 @@ const AvatarCropModal = (props) => {

     // Changes the modal state values to initial
     const resetState = useCallback(() => {
+        // Unblock selection on modal close incase modal
+        // onMouseUp not fired due to forcefully navigate back
+        ControlSelection.unblock();
         originalImageWidth.value = CONST.AVATAR_CROP_MODAL.INITIAL_SIZE;
         originalImageHeight.value = CONST.AVATAR_CROP_MODAL.INITIAL_SIZE;
         translateY.value = 0;
@@ -197,6 +202,7 @@ const AvatarCropModal = (props) => {

             updateImageOffset(newX, newY);
         },
+        onEnd: () => runOnJS(ControlSelection.unblock)(),
     }, [imageContainerSize, updateImageOffset, translateX, translateY]);

     /**
@@ -224,7 +230,10 @@ const AvatarCropModal = (props) => {
             const newY = translateY.value * differential;
             updateImageOffset(newX, newY);
         },
-        onEnd: () => isPressableEnabled.value = true,
+        onEnd: () => {
+            isPressableEnabled.value = true;
+            runOnJS(ControlSelection.unblock)();
+        },
     }, [imageContainerSize, clamp, translateX, translateY, translateSlider, scale, sliderContainerSize, isPressableEnabled]);

     // This effect is needed to prevent the incorrect position of
@@ -309,6 +318,7 @@ const AvatarCropModal = (props) => {
             type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
             onModalHide={resetState}
             statusBarTranslucent={false}
+            onMouseUp={ControlSelection.unblock}
         >
             {props.isSmallScreenWidth && <HeaderGap />}
             <HeaderWithCloseButton
@@ -322,23 +332,26 @@ const AvatarCropModal = (props) => {
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />
                     : (
                         <>
-                            <ImageCropView
-                                imageUri={props.imageUri}
-                                containerSize={imageContainerSize}
-                                panGestureEventHandler={panGestureEventHandler}
-                                originalImageHeight={originalImageHeight}
-                                originalImageWidth={originalImageWidth}
-                                scale={scale}
-                                translateY={translateY}
-                                translateX={translateX}
-                                rotation={rotation}
-                            />
+                            <Pressable onMouseDown={ControlSelection.block}>
+                                <ImageCropView
+                                    imageUri={props.imageUri}
+                                    containerSize={imageContainerSize}
+                                    panGestureEventHandler={panGestureEventHandler}
+                                    originalImageHeight={originalImageHeight}
+                                    originalImageWidth={originalImageWidth}
+                                    scale={scale}
+                                    translateY={translateY}
+                                    translateX={translateX}
+                                    rotation={rotation}
+                                />
+                            </Pressable>
                             <View style={[styles.mt5, styles.justifyContentBetween, styles.alignItemsCenter, styles.flexRow, StyleUtils.getWidthStyle(imageContainerSize)]}>
                                 <Icon src={Expensicons.Zoom} fill={themeColors.icons} />
                                 <Pressable
                                     style={[styles.mh5, styles.flex1]}
                                     onLayout={initializeSliderContainer}
                                     onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+                                    onMouseDown={ControlSelection.block}
                                 >
                                     <Slider sliderValue={translateSlider} onGesture={panSliderGestureEventHandler} />
                                 </Pressable>
diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 323df19c77..f2b078eebf 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -116,6 +116,7 @@ class BaseModal extends PureComponent {
                 animationInTiming={this.props.animationInTiming}
                 animationOutTiming={this.props.animationOutTiming}
                 statusBarTranslucent={this.props.statusBarTranslucent}
+                onMouseUp={this.props.onMouseUp}
             >
                 <SafeAreaInsetsContext.Consumer>
                     {(insets) => {
diff --git a/src/components/Modal/modalPropTypes.js b/src/components/Modal/modalPropTypes.js
index 8f09588e37..8917dbb431 100644
--- a/src/components/Modal/modalPropTypes.js
+++ b/src/components/Modal/modalPropTypes.js
@@ -61,6 +61,9 @@ const propTypes = {
     /** Whether the modal should go under the system statusbar */
     statusBarTranslucent: PropTypes.bool,

+    /** Callback method fired when the mouseup inside the modal */
+    onMouseUp: PropTypes.func,
+
     ...windowDimensionsPropTypes,
 };
Android https://user-images.githubusercontent.com/114683042/209100833-5304955d-0725-4bd4-8a5c-f50d773b6ec4.mov
iOS https://user-images.githubusercontent.com/114683042/209101098-36052215-dc62-467c-9a46-20d12497de64.mov
Web (Safari) https://user-images.githubusercontent.com/114683042/209101475-47616ccc-8b57-4944-b602-736b17d4cc22.mov
Web (Chrome) https://user-images.githubusercontent.com/114683042/209103193-847c1155-2ac8-49c5-bc02-a4f31a18d7c8.mov
Desktop https://user-images.githubusercontent.com/114683042/209102458-c1066c64-2a6a-45f2-a5da-4f1cf6636cf8.mov
syedsaroshfarrukhdot commented 1 year ago

@jatinsonijs Haha yes, I missed it I was still working to find more edge cases if there were any just found that force close edge case and edited myProposal and saw that you have taken care of it already.

May be team would prefer my proposal as I found the regression cause and fix on Native Platforms just missed that edge case.

And I believe we don't need onMouseUp prop anymore in the BaseModel as we are already removing unblocking selection from on onEnd of panSliderGestureEventHandler and panGestureEventHandler after blocking it.

jatinsonijs commented 1 year ago

Yes @syedsaroshfarrukhdot, we have tried to improve each other proposal πŸ™‚, but this one easily catchable during testing as it occurring directly. So surely it's cannot miss in final version. I thought in this https://github.com/Expensify/App/issues/13688#issuecomment-1362190411 He is talking about document use in native.

syedsaroshfarrukhdot commented 1 year ago

Yes @syedsaroshfarrukhdot, we have tried to improve each other proposal πŸ™‚, but this one easily catchable during testing as it occurring directly. So surely it's cannot miss in final version. I thought in this #13688 (comment) He is talking about document use in native.

I agree with you that was my bad to miss it in final version initially. I would be okay with team decision here πŸ™‚.

jatinsonijs commented 1 year ago

And I believe we don't need onMouseUp prop anymore in the model as we are already removing unblocking selection from on onEnd of panSliderGestureEventHandler and panGestureEventHandler

It's still needed in some edge case creating issue with slider. But if we are going to add mouseDown on GestureHandlerRootView its needed in many case.

s77rt commented 1 year ago

I think having too many ControlSelection.block / ControlSelection.unlock in different places is over-engineered and not worth it.

How other websites handle this behaviour? Afterall this is a bug on Safari

bernhardoj commented 1 year ago

Proposal

Why don't we combine the use of onMouseDown and onPressOut? No need to unblock at onModalHide and at onEnd of the gesture. onPressOut will get called on both cases (at least from what I tested πŸ˜„).

+<Pressable
+    onMouseDown={ControlSelection.block}
+    onPressOut={ControlSelection.unblock}
+>
    <ImageCropView
        imageUri={props.imageUri}
        containerSize={imageContainerSize}
        panGestureEventHandler={panGestureEventHandler}
        originalImageHeight={originalImageHeight}
        originalImageWidth={originalImageWidth}
        scale={scale}
        translateY={translateY}
        translateX={translateX}
        rotation={rotation}
    />
+</Pressable>
<Pressable
    style={[styles.mh5, styles.flex1]}
    onLayout={initializeSliderContainer}
    onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+   onMouseDown={ControlSelection.block}
+   onPressOut={ControlSelection.unblock}
>
    <Slider .../>
</Pressable>
getusha commented 1 year ago

@bernhardoj that's what i am trying to say in my proposal but using native js. we can also useonPress

jatinsonijs commented 1 year ago

Why don't we combine the use of onMouseDown and onPressOut? No need to unblock at onModalHide and at onEnd of the gesture. onPressOut will get called on both cases (at least from what I tested πŸ˜„).

There are still some edge cases where onPressOut not working, I have tried it already. I also like neat and clean solution. But not working in all cases πŸ™‚

jatinsonijs commented 1 year ago

Example for if you're on Laptop - with trackpad just tap (don't press) on slider bar. Now onPressOut will not call as Press gesture not started due to quick release but mousedown called in this case not able to select text anywhere in app. So onModalHide its exist handle if any failure occur at least text selection should work on another area.

jatinsonijs commented 1 year ago

Proposal: We can combine onMouseDown, onMouseUp, onPressOut

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..89e2ba327c 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -30,6 +30,7 @@ import Slider from './Slider';
 import cropOrRotateImage from '../../libs/cropOrRotateImage';
 import HeaderGap from '../HeaderGap';
 import * as StyleUtils from '../../styles/StyleUtils';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping */
@@ -322,23 +323,32 @@ const AvatarCropModal = (props) => {
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />
                     : (
                         <>
-                            <ImageCropView
-                                imageUri={props.imageUri}
-                                containerSize={imageContainerSize}
-                                panGestureEventHandler={panGestureEventHandler}
-                                originalImageHeight={originalImageHeight}
-                                originalImageWidth={originalImageWidth}
-                                scale={scale}
-                                translateY={translateY}
-                                translateX={translateX}
-                                rotation={rotation}
-                            />
+                            <Pressable
+                                onMouseDown={ControlSelection.block}
+                                onMouseUp={ControlSelection.unblock}
+                                onPressOut={ControlSelection.unblock}
+                            >
+                                <ImageCropView
+                                    imageUri={props.imageUri}
+                                    containerSize={imageContainerSize}
+                                    panGestureEventHandler={panGestureEventHandler}
+                                    originalImageHeight={originalImageHeight}
+                                    originalImageWidth={originalImageWidth}
+                                    scale={scale}
+                                    translateY={translateY}
+                                    translateX={translateX}
+                                    rotation={rotation}
+                                />
+                            </Pressable>
                             <View style={[styles.mt5, styles.justifyContentBetween, styles.alignItemsCenter, styles.flexRow, StyleUtils.getWidthStyle(imageContainerSize)]}>
                                 <Icon src={Expensicons.Zoom} fill={themeColors.icons} />
                                 <Pressable
                                     style={[styles.mh5, styles.flex1]}
                                     onLayout={initializeSliderContainer}
                                     onPressIn={e => runOnUI(sliderOnPress)(e.nativeEvent.locationX)}
+                                    onMouseDown={ControlSelection.block}
+                                    onMouseUp={ControlSelection.unblock}
+                                    onPressOut={ControlSelection.unblock}
                                 >
                                     <Slider sliderValue={translateSlider} onGesture={panSliderGestureEventHandler} />
                                 </Pressable>

I have tested Its working fine most of the cases. Optionally we can unblock on onModalClose for safety if in any case its not working which we are not able to create now.

bernhardoj commented 1 year ago

Yeah, just tested the trackpad tap and it does not work. I guess combine it more with onMouseUp solves all known cases.

fedirjh commented 1 year ago

Proposal

@aimane-chnaif we can disable selection only for the specific Slider component

diff --git a/src/components/AvatarCropModal/Slider.js b/src/components/AvatarCropModal/Slider.js
index 11df31e67b..96a72f18f3 100644
--- a/src/components/AvatarCropModal/Slider.js
+++ b/src/components/AvatarCropModal/Slider.js
@@ -1,5 +1,6 @@
 import PropTypes from 'prop-types';
-import React from 'react';
+import React, {useEffect, useRef} from 'react';
+import _ from 'underscore';
 import {View} from 'react-native';
 import {PanGestureHandler} from 'react-native-gesture-handler';
 import Animated, {useAnimatedStyle} from 'react-native-reanimated';
@@ -21,6 +22,15 @@ const defaultProps = {

 // This component can't be written using class since reanimated API uses hooks.
 const Slider = (props) => {
+    const sliderRef = useRef(null);
+
+    useEffect(() => {
+        if (_.isNull(sliderRef.current)) {
+            return;
+        }
+        sliderRef.current.onselectstart = () => false;
+    }, [sliderRef]);
+
     // A reanimated memoized style, which tracks
     // a translateX shared value and updates the slider position.
     const rSliderStyle = useAnimatedStyle(() => ({
@@ -28,7 +38,7 @@ const Slider = (props) => {
     }));

     return (
-        <View style={styles.sliderBar}>
+        <View style={styles.sliderBar} ref={sliderRef}>
             <PanGestureHandler onGestureEvent={props.onGesture}>
                 <Animated.View style={[styles.sliderKnob, rSliderStyle]} />
             </PanGestureHandler>

Result

https://user-images.githubusercontent.com/36869046/209172756-1975e0b0-e4ed-493f-a6a1-c43af49c10eb.mov

jatinsonijs commented 1 year ago

Great @fedirjh we can do same for ImageCropView I think

jatinsonijs commented 1 year ago

I have tested its working Basically we are setting onselectstart with return false in ref which canceled the selection whenever user will try to select (drag). Why this event not exist in React so that we can use directly like (onMouseUp) without ref. Reason is it's still not supported by all platform as per discussion here:

https://github.com/facebook/react/issues/16521 https://developer.mozilla.org/en-US/docs/Web/API/Node/selectstart_event

But in our issue we only need it for Mac safari which is supported

diff --git a/src/components/AvatarCropModal/ImageCropView.js b/src/components/AvatarCropModal/ImageCropView.js
index a61cc7d20a..e21303284d 100644
--- a/src/components/AvatarCropModal/ImageCropView.js
+++ b/src/components/AvatarCropModal/ImageCropView.js
@@ -1,5 +1,6 @@
 import PropTypes from 'prop-types';
-import React from 'react';
+import React, {useEffect, useRef} from 'react';
+import _ from 'underscore';
 import {View} from 'react-native';
 import {PanGestureHandler} from 'react-native-gesture-handler';
 import Animated, {interpolate, useAnimatedStyle} from 'react-native-reanimated';
@@ -45,6 +46,14 @@ const defaultProps = {
 };

 const ImageCropView = (props) => {
+    const viewRef = useRef(null);
+    useEffect(() => {
+        if (_.isNull(viewRef.current)) {
+            return;
+        }
+        viewRef.current.onselectstart = () => false;
+    }, [viewRef]);
+
     const containerStyle = StyleUtils.getWidthAndHeightStyle(props.containerSize, props.containerSize);

     // A reanimated memoized style, which updates when the image's size or scale changes.
@@ -65,7 +74,7 @@ const ImageCropView = (props) => {

     return (
         <PanGestureHandler onGestureEvent={props.panGestureEventHandler}>
-            <Animated.View style={[containerStyle, styles.imageCropContainer]}>
+            <Animated.View ref={viewRef} style={[containerStyle, styles.imageCropContainer]}>
                 <Animated.Image style={[imageStyle, styles.h100, styles.w100]} source={{uri: props.imageUri}} resizeMode="contain" />
                 <View style={[containerStyle, styles.l0, styles.b0, styles.pAbsolute]}>
                     <Icon src={Expensicons.ImageCropMask} width={props.containerSize} height={props.containerSize} />
diff --git a/src/components/AvatarCropModal/Slider.js b/src/components/AvatarCropModal/Slider.js
index 11df31e67b..4d7af03c74 100644
--- a/src/components/AvatarCropModal/Slider.js
+++ b/src/components/AvatarCropModal/Slider.js
@@ -1,5 +1,6 @@
 import PropTypes from 'prop-types';
-import React from 'react';
+import React, {useEffect, useRef} from 'react';
+import _ from 'underscore';
 import {View} from 'react-native';
 import {PanGestureHandler} from 'react-native-gesture-handler';
 import Animated, {useAnimatedStyle} from 'react-native-reanimated';
@@ -21,6 +22,15 @@ const defaultProps = {

 // This component can't be written using class since reanimated API uses hooks.
 const Slider = (props) => {
+    const viewRef = useRef(null);
+
+    useEffect(() => {
+        if (_.isNull(viewRef.current)) {
+            return;
+        }
+        viewRef.current.onselectstart = () => false;
+    }, [viewRef]);
+
     // A reanimated memoized style, which tracks
     // a translateX shared value and updates the slider position.
     const rSliderStyle = useAnimatedStyle(() => ({
@@ -28,7 +38,7 @@ const Slider = (props) => {
     }));

     return (
-        <View style={styles.sliderBar}>
+        <View style={styles.sliderBar} ref={viewRef}>
             <PanGestureHandler onGestureEvent={props.onGesture}>
                 <Animated.View style={[styles.sliderKnob, rSliderStyle]} />
             </PanGestureHandler>
fedirjh commented 1 year ago

Great @fedirjh we can do same for ImageCropView I think

Ah yes , I haven't noticed that ImageCropView have the same issue , we can apply same solution there too

fedirjh commented 1 year ago

Since we have same solution for both component , we can create custom hook to disable selection

src/components/AvatarCropModal/useSelectionDisable.js

import {useEffect, useRef} from 'react';
import _ from 'underscore';

// custom hook to disable selection on component
const useSelectionDisable = () => {
    const ref = useRef(null);

    useEffect(() => {
        if (_.isNull(ref.current)) {
            return;
        }
        ref.current.onselectstart = () => false;
    }, [ref]);

    return ref;
};

useSelectionDisable.displayName = 'useSelectionDisable';
export default useSelectionDisable;

We can use then the custom hook useSelectionDisable on both component

ImageCropView.js

diff --git a/src/components/AvatarCropModal/ImageCropView.js b/src/components/AvatarCropModal/ImageCropView.js
index a61cc7d20a..99d223c021 100644
--- a/src/components/AvatarCropModal/ImageCropView.js
+++ b/src/components/AvatarCropModal/ImageCropView.js
@@ -8,6 +8,7 @@ import Icon from '../Icon';
 import * as Expensicons from '../Icon/Expensicons';
 import * as StyleUtils from '../../styles/StyleUtils';
 import gestureHandlerPropTypes from './gestureHandlerPropTypes';
+import useSelectionDisable from './useSelectionDisable';

 const propTypes = {
     /** Link to image for cropping   */
@@ -46,6 +47,7 @@ const defaultProps = {

 const ImageCropView = (props) => {
     const containerStyle = StyleUtils.getWidthAndHeightStyle(props.containerSize, props.containerSize);
+    const ref = useSelectionDisable();

     // A reanimated memoized style, which updates when the image's size or scale changes.
     const imageStyle = useAnimatedStyle(() => {
@@ -65,7 +67,7 @@ const ImageCropView = (props) => {

     return (
         <PanGestureHandler onGestureEvent={props.panGestureEventHandler}>
-            <Animated.View style={[containerStyle, styles.imageCropContainer]}>
+            <Animated.View style={[containerStyle, styles.imageCropContainer]} ref={ref}>

Slider.js

diff --git a/src/components/AvatarCropModal/Slider.js b/src/components/AvatarCropModal/Slider.js
index 11df31e67b..5e23149624 100644
--- a/src/components/AvatarCropModal/Slider.js
+++ b/src/components/AvatarCropModal/Slider.js
@@ -5,6 +5,7 @@ import {PanGestureHandler} from 'react-native-gesture-handler';
 import Animated, {useAnimatedStyle} from 'react-native-reanimated';
 import styles from '../../styles/styles';
 import gestureHandlerPropTypes from './gestureHandlerPropTypes';
+import useSelectionDisable from './useSelectionDisable';

 const propTypes = {
     /** React-native-reanimated lib handler which executes when the user is panning slider */
@@ -21,6 +22,8 @@ const defaultProps = {

 // This component can't be written using class since reanimated API uses hooks.
 const Slider = (props) => {
+    const ref = useSelectionDisable();
+
     // A reanimated memoized style, which tracks
     // a translateX shared value and updates the slider position.
     const rSliderStyle = useAnimatedStyle(() => ({
@@ -28,7 +31,7 @@ const Slider = (props) => {
     }));

     return (
-        <View style={styles.sliderBar}>
+        <View style={styles.sliderBar} ref={ref}>
jatinsonijs commented 1 year ago

Proposal to follow DRY

We already have ControlSelection utility we can extend it. It will not do anything on native (no ref storage etc...) can use in class component also

diff --git a/src/components/AvatarCropModal/ImageCropView.js b/src/components/AvatarCropModal/ImageCropView.js
index a61cc7d20a..80efa50c8d 100644
--- a/src/components/AvatarCropModal/ImageCropView.js
+++ b/src/components/AvatarCropModal/ImageCropView.js
@@ -8,6 +8,7 @@ import Icon from '../Icon';
 import * as Expensicons from '../Icon/Expensicons';
 import * as StyleUtils from '../../styles/StyleUtils';
 import gestureHandlerPropTypes from './gestureHandlerPropTypes';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** Link to image for cropping   */
@@ -65,7 +66,7 @@ const ImageCropView = (props) => {

     return (
         <PanGestureHandler onGestureEvent={props.panGestureEventHandler}>
-            <Animated.View style={[containerStyle, styles.imageCropContainer]}>
+            <Animated.View ref={ControlSelection.blockElement} style={[containerStyle, styles.imageCropContainer]}>
                 <Animated.Image style={[imageStyle, styles.h100, styles.w100]} source={{uri: props.imageUri}} resizeMode="contain" />
                 <View style={[containerStyle, styles.l0, styles.b0, styles.pAbsolute]}>
                     <Icon src={Expensicons.ImageCropMask} width={props.containerSize} height={props.containerSize} />
diff --git a/src/components/AvatarCropModal/Slider.js b/src/components/AvatarCropModal/Slider.js
index 11df31e67b..0636bd682a 100644
--- a/src/components/AvatarCropModal/Slider.js
+++ b/src/components/AvatarCropModal/Slider.js
@@ -5,6 +5,7 @@ import {PanGestureHandler} from 'react-native-gesture-handler';
 import Animated, {useAnimatedStyle} from 'react-native-reanimated';
 import styles from '../../styles/styles';
 import gestureHandlerPropTypes from './gestureHandlerPropTypes';
+import ControlSelection from '../../libs/ControlSelection';

 const propTypes = {
     /** React-native-reanimated lib handler which executes when the user is panning slider */
@@ -28,7 +29,7 @@ const Slider = (props) => {
     }));

     return (
-        <View style={styles.sliderBar}>
+        <View style={styles.sliderBar} ref={ControlSelection.blockElement}>
             <PanGestureHandler onGestureEvent={props.onGesture}>
                 <Animated.View style={[styles.sliderKnob, rSliderStyle]} />
             </PanGestureHandler>
diff --git a/src/libs/ControlSelection/index.js b/src/libs/ControlSelection/index.js
index 8a1795b292..31cd2707be 100644
--- a/src/libs/ControlSelection/index.js
+++ b/src/libs/ControlSelection/index.js
@@ -1,3 +1,5 @@
+import _ from 'underscore';
+
 /**
  * Block selection on the whole app
  *
@@ -14,7 +16,29 @@ function unblock() {
     document.body.classList.remove('disable-select');
 }

+/*
+ * Block selection only on particular element
+ */
+function blockElement(ref) {
+    if (_.isNull(ref)) { return; }
+
+    // eslint-disable-next-line no-param-reassign
+    ref.onselectstart = () => false;
+}
+
+/*
+ * Unblock selection only on particular element
+ */
+function unblockElement(ref) {
+    if (_.isNull(ref)) { return; }
+
+    // eslint-disable-next-line no-param-reassign
+    ref.onselectstart = () => true;
+}
+
 export default {
     block,
     unblock,
+    blockElement,
+    unblockElement,
 };
diff --git a/src/libs/ControlSelection/index.native.js b/src/libs/ControlSelection/index.native.js
index 8fe6c7d30b..ea91f4bbb1 100644
--- a/src/libs/ControlSelection/index.native.js
+++ b/src/libs/ControlSelection/index.native.js
@@ -1,7 +1,11 @@
 function block() {}
 function unblock() {}
+function blockElement() {}
+function unblockElement() {}

 export default {
     block,
     unblock,
+    blockElement,
+    unblockElement,
 };
jatinsonijs commented 1 year ago

Proposal

We can block selection at single place - We have to wrap ImageCropper and slider both in View and we can use ref on it

diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index a42da9901c..f14d20a62a 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -13,6 +13,7 @@ import {
     useSharedValue,
     useWorkletCallback,
 } from 'react-native-reanimated';
+import underscore from 'underscore';
 import CONST from '../../CONST';
 import compose from '../../libs/compose';
 import styles from '../../styles/styles';
@@ -302,6 +303,12 @@ const AvatarCropModal = (props) => {
         updateImageOffset(newX, newY);
     };

+    const blockSelection = useCallback((ref) => {
+        if (underscore.isNull(ref)) { return; }
+        // eslint-disable-next-line no-param-reassign
+        ref.onselectstart = () => false;
+    }, []);
+
     return (
         <Modal
             onClose={props.onClose}
@@ -316,7 +323,8 @@ const AvatarCropModal = (props) => {
                 onCloseButtonPress={props.onClose}
             />
             <Text style={[styles.mh5]}>{props.translate('avatarCropModal.description')}</Text>
-            <GestureHandlerRootView onLayout={initializeImageContainer} style={[styles.alignSelfStretch, styles.m5, styles.flex1, styles.alignItemsCenter]}>
+            <GestureHandlerRootView onLayout={initializeImageContainer} style={[styles.alignSelfStretch, styles.m5, styles.flex1]}>
+                <View style={[styles.alignSelfStretch, styles.flex1, styles.alignItemsCenter]} ref={blockSelection}>

                 {/* To avoid layout shift we should hide this component until the image container & image is initialized */}
                 {(!isImageInitialized || !isImageContainerInitialized) ? <ActivityIndicator color={themeColors.spinner} style={[styles.flex1]} size="large" />
@@ -353,6 +361,7 @@ const AvatarCropModal = (props) => {
                             </View>
                         </>
                     )}
+                </View>
             </GestureHandlerRootView>
             <Button
                 success
s77rt commented 1 year ago

Proposal

We can fix this upstream on react-native-gesture-handler I have raised a PR

Once the PR is merged, we can fix this issue by simply passing disableSelection to <PanGestureHandler />s

https://user-images.githubusercontent.com/16493223/209702540-843d67d0-a2b0-43b1-a8f9-58177f68c4c8.mp4

jatinsonijs commented 1 year ago

I don't think it's issue of react-native-gesture-handler, we may consider it as feature request to that library. But if we can fix it by just wrap one view why we should wait for them - Just sharing my thought. Issue with library is they are not forwarding ref of GestureHandlerRootView if they forward ref we not have to use another view for ref.

s77rt commented 1 year ago

@jatinsonijs Indeed, it's not an issue on react-native-gesture-handler but it's also not an issue on Expensify/App. We can agree that Safari itself is causing the issue, we are just writing custom code to cover Safari issues.

It's about where to write the code, if we write it on App, then we will have to write it in multiple places, right? But if we write it on react-native-gesture-handler that would be the only place.

jatinsonijs commented 1 year ago

@s77rt currently its only required in one place if needed multiple places we have ControlSelection utility just pass ref={ControlSelection.blockElement} (my previous proposal) and for react-native-gesture-handler we also need to pass that new prop if they approved your proposal

s77rt commented 1 year ago

@jatinsonijs Yes, you are correct. We would still need to pass the new prop. Yet if an issue can be fixed upstream I think it's better to fix it upstream

melvin-bot[bot] commented 1 year ago

@puneetlath, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

aimane-chnaif commented 1 year ago

How other websites handle this behaviour? Afterall this is a bug on Safari

Can anyone take example website with the same issue on Safari? I don't find any.

jatinsonijs commented 1 year ago

@aimane-chnaif You can try https://www.flipkart.com/search?q=iPhone&otracker=search&otracker1=search&marketplace=FLIPKART&as-show=on&as=off&as-pos=1&as-type=HISTORY&p%5B%5D=facets.price_range.from%3D20000&p%5B%5D=facets.price_range.to%3DMax

Try to drag price slider - located left side