Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: disable contextmenu on non-secondary pen events or touch #4675

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

dwelle
Copy link
Member

@dwelle dwelle commented Jan 30, 2022

fix #4674

@vercel
Copy link

vercel bot commented Jan 30, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/8Vk82LVYW4vfqSUhW1KXe1KCwP25
✅ Preview: https://excalidraw-git-disablecontextmenuforothertool-375984-excalidraw.vercel.app

@zsviczian
Copy link
Collaborator

This is a good improvement. I tested it on iPad with pencil and touch and on Samsung Note with touch and S-Pen. Works as expected.

Not related to this change... but do you have any idea why the context menu won't on a long press with the apple pencil, but does show with a long press with the S-Pen. This difference is the same for both the current excalidraw.com version and this PR.

Copy link
Collaborator

@zsviczian zsviczian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this?

event.nativeEvent.pointerType === "pen" &&
          event.button !== POINTER_BUTTON.SECONDARY

@dwelle
Copy link
Member Author

dwelle commented Jan 30, 2022

Not related to this change... but do you have any idea why the context menu won't on a long press with the apple pencil, but does show with a long press with the S-Pen. This difference is the same for both the current excalidraw.com version and this PR.

Good catch. AFAIK iOS doesn't support touch-hold to open contextMenu, so we're detecting it manually. We should include pens in that. It also means this PR doesn't handle iOS touch — gonna fix. never mind, the check is downstream so it applies.

@dwelle
Copy link
Member Author

dwelle commented Jan 30, 2022

What is the purpose of this?

event.nativeEvent.pointerType === "pen" &&
          event.button !== POINTER_BUTTON.SECONDARY

Allow opening contextmenu via the pen button that usually is used for opening contextmenus (it usually emits the same event.button type as right-click on a mouse).

I'll add a doc.

@dwelle
Copy link
Member Author

dwelle commented Jan 30, 2022

50a67cc

this fix will require more work because of how it's implemented. I'll remove the commit for now as it actually needs timeout clearing which I'm not doing for pen. I'll open a new PR for this fix later.

@dwelle
Copy link
Member Author

dwelle commented Feb 6, 2022

@zsviczian merging?

@dwelle dwelle merged commit 5007df6 into master Feb 7, 2022
@dwelle dwelle deleted the disable_contextmenu_for_other_tools_on_touch branch February 7, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disable contextmenu on touch for tools other than selection
2 participants