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

[kavimuru]

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

---

Action Performed:-

1. 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?

• Web - Mac Safari

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:

Screen.Recording.2022-12-17.at.1.22.46.AM.mov

Recording.29.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]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[thesahindia]

The issue was reported by @Tushu17 (not me)

[Tushu17]

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

App/src/components/AvatarCropModal/AvatarCropModal.js

Lines 110 to 112 in b0f6785

	useEffect(() => {
	if (!props.imageUri) {
	return;

[getusha]

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

Screen.Recording.2022-12-19.at.10.08.23.AM.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

• Selecting text don't work on the whole page

Screen.Recording.2022-12-19.at.10.15.49.AM.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]

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]

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

App/web/index.html

Lines 23 to 25 in 3e050bd

	#root > div > div {
	height: 100% !important;
	}

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

Please see this link for more info.

After the fix

Screen.Recording.2022-12-20.at.1.08.34.AM.mov

[s77rt]

@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.

[s77rt]

@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]

Yes I have tested it working fine

[s77rt]

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

[puneetlath]

@aimane-chnaif thoughts on these proposals?

[aimane-chnaif]

@aimane-chnaif thoughts on these proposals?

reviewing today

[aimane-chnaif]

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.

[ntdiary]

Oh, no worries. I like the latter simply because this's exactly the scenario it's used in. 😂

As I explained before:

    // During selection, use an I-beam regardless of the content beneath the cursor.
    // If a drag may be starting or we're capturing mouse events for a particular node, don't treat this as a selection.
    if (m_mousePressed
        && mouseDownMayStartSelect()
#if ENABLE(DRAG_SUPPORT)
        && !m_mouseDownMayStartDrag
#endif
        && m_frame.selection().isCaretOrRange()
        && !m_capturingMouseEventsElement)
        return iBeam;

onselectstart => false haves mouseDownMayStartSelect being false.
setPointerCapture haves !m_capturingMouseEventsElement being false.

They can all break the conditions here, the difference is only in their meaning.

The former means disabling the select feature, it does have better compatibility.

The latter is more suitable for dragging scenes, but it's implemented since safari 13.

This is useful in scenarios like a custom slider control (e.g. similar to the [HTML] control). Pointer capture can be set on the slider thumb element, allowing the user to slide the control back and forth even if the pointer slides off of the thumb. ¹

Footnotes

1. https://w3c.github.io/pointerevents/#pointer-capture ↩

[JmillsExpensify]

@puneetlath Do you mind providing your take based @aimane-chnaif's recent review and the more recent commentary?

[puneetlath]

Sorry for the delay! I reached out to software mansion to get their perspective. Will make a decision within the next day or two depending on their response.

[puneetlath]

SWM said they’d get back to me tomorrow about how long they think their experimental web implementation will stay “experimental”. Based on that, we can decide the path forward.

[jatinsonijs]

@puneetlath It will come up with other changes as well, which will impact various part of app. As react-native-gesture handler used on other area as well for example in react-navigation, Scrollview etc... I think when we required same thing on another part then we should do these library changes.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[puneetlath]

Ok thanks for your patience everyone. Let's go ahead with @jatinsonijs proposal.

[melvin-bot]

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

[puneetlath]

@jatinsonijs can you please:

1. apply to the Upwork job: https://www.upwork.com/jobs/~01ba623ffe6500b581
2. let us know when you expect to have a PR up and ready for review?

Thanks!

[jatinsonijs]

Thanks @puneetlath applied, PR is ready for review
cc @aimane-chnaif

[melvin-bot]

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

• Prevent slider and image drag i-beam cursor on safari #14381

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

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

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

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

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

[melvin-bot]

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

• [@aimane-chnaif / @puneetlath] The PR that introduced the bug has been identified. Link to the PR: there isn't really a PR that caused this. This is just a quirk of Safari and we've improved our cross-platform testing to include Safari.
• [@aimane-chnaif / @puneetlath] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: there isn't really a PR that caused this. This is just a quirk of Safari and we've improved our cross-platform testing to include Safari.
• [@aimane-chnaif / @puneetlath] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: we have updated out checklist to include Safari, so I think we're good here
• [@puneetlath] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:We decided against a regression test here, in favor of general guidelines for this sort of thing.

[puneetlath]

Paid @jatinsonijs and @aimane-chnaif. Just waiting for @thesahindia to accept offer for the reporting bonus. Then we're done.

[thesahindia]

It was reported by @Tushu17 (thread)

[puneetlath]

Ah cool. Thanks for correcting that! @Tushu17 can you please apply to this Upwork job: https://www.upwork.com/jobs/~01ba623ffe6500b581

[Tushu17]

@puneetlath Okay applied. Thank you.

[puneetlath]

All paid. Thanks for your contribution everyone!