fix: Allow right mouse click on thought.

[Krishna2323]

Details

• Browser's default context menu wasn't opened when right clicking on a thought.

Fixed Issue

• Allow right mouse click on thought #2253

RCA

• Currently we only prevent default when the condition below is matched.
  

  em/src/hooks/useLongPress.ts

  Lines 112 to 117 in 31a13f0

  	if (!isTouch || ('pointerType' in e.nativeEvent && e.nativeEvent.pointerType === 'touch')) {
  	e.preventDefault()
  	e.stopPropagation()
  	selection.clear()
  	dispatch(editing({ value: false }))
  	}

Changes

• Instead of always calling e.preventDefault(), we can call it when lock is true, lock will be true when long press event occurs.

Screenshots/Videos

Android: mWeb Chrome

android_chrome.mp4

iOS: mWeb Safari

ios_safari.mp4

Chrome / Safari

desktop_app.mp4

[raineorshine]

Thanks for your contribution.

Based on the logic, it looks the selection still gets cleared on right click on desktop (untested). Can you confirm? We don't want to clear the selection on desktop.

In fact, we don't want to use the long press behavior at all on right mouse click. It seem strange to me that there is no code that actually checks the mouse button.

[Krishna2323]

Based on the logic, it looks the selection still gets cleared on right click on desktop (untested). Can you confirm? We don't want to clear the selection on desktop.

You mean text selection by selection, right? If so, then I don't think that's the case, the text selection isn't cleared on right click, please see the video below. Do you mean the selection gets cleared when we right click on the drag dots?

Monosnap.screencast.2024-08-14.16-23-43.mp4

In fact, we don't want to use the long press behavior at all on right mouse click. It seem strange to me that there is no code that actually checks the mouse button.

We can check for right clicks using the button property on the mouseDown event. We can early return if the button value is 2.

[raineorshine]

Yes, the thought may have some text selected. The code reads as if the selection is cleared on right-mouse click, but maybe that can't happen. I'll pull your branch down and test it myself.

[Krishna2323]

Ahh, in that case the selection is removed. If the text is selected and we right click on the drag dot then the selection gets cleared. Let me check how can we prevent it.

[raineorshine]

Exactly. Thanks :)

[Krishna2323]

@raineorshine, I was able to persist the text selection on right click on the draggable dot but I think there is no real benefit of that because the context menu which is opened on right clicking on draggable dot does not contain text selection options.

Monosnap.screencast.2024-08-15.20-21-24.mp4

We have few options:

• We can disable context menu for the draggable dot.
• Create our custom context menu.

I can spend more time on this but I don't think there is a possible solution for this, I tried to trigger context menu for a node but that won't work (reference).

[raineorshine]

Thanks. I'll have to test it myself to confirm the desired behavior. I’m traveling this week, but will review as soon as I return to work.

[raineorshine]

Okay, I've tested it out on desktop and here's what I'm seeing:

✓ Right-clicking on selected text opens the context menu.
✗ Right-clicking on the bullet clears the browser selection.
✗ Right-clicking on the bullet initiates a drag-and-drop.

No need for a custom context menu, and I don't care whether the context menu works or not on the bullet as long as the above two points are fixed. Thanks!

[raineorshine]

Why piggyback on lock instead of just checking for right-mouse click? If we're trying to change the behavior only under the condition that the right-mouse button is clicked, it seems like we should test for that directly, unless I'm missing something. Tying it to lock tightly couples the right-mouse click behavior to the unrelated lock behavior which was intended to prevent a long press from more than one component at a time.

[Krishna2323]

Sorry, I was busy somewhere else. Thanks for the review. I will update the code and provide more details very soon, probably today. I believe we won't need lock now, as I'm planning to prevent the context menu on the bullet. That's the simplest and most reliable solution, in my opinion.

[Krishna2323]

Hi, sorry for the long delay. I pushed the changes 3 days ago but wasn't able to test on native devices, so I didn't ping you for testing. I'm still facing issues when installing the dependencies. Thankfully, I recorded the result after the last commit for desktop. Despite re-cloning the repo, I'm still encountering many issues while installing the dependencies. Please let me know if you have any ideas on how to fix this.

Result after the last commit:

Monosnap.screencast.2024-08-25.03-37-47.mp4

[raineorshine]

Try rebasing on main. The packageManager was added and page-lifecycle was fixed in 10b9d7c.

[Krishna2323]

Thanks, that worked. Could you please test the changes when you have some time?

[raineorshine]

Removing e.preventDefault impacts mobile. This PR should only affect desktop.

[raineorshine]

It turns out that we mainly just needed to remove the !isTouch || condition in order to avoid disabling the context menu on desktop. This obviates the need to save and restore the selection.

See a2ba727

Thank you anyway!