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: Scope drag and drop events to Excalidraw container to prevent overriding the host drag and drop events #4445

Merged

Conversation

zsviczian
Copy link
Collaborator

Excalidraw global event listeners to disable EVENT.DRAG_OVER and EVENT.DROP interfere with host apps functionality. Some of the drag&drop functionality stops working in Obsidian once Excalidraw was started.

See linked Obsidian-Excalidraw issue:
zsviczian/obsidian-excalidraw-plugin#304

I decided to use handleKeyboardGlobally to control whether to add the global drag event listeners because I believe that the need for global event handling for KEYDOWN and DRAG_OVER and DROP will go hand in hand.

It may be appropriate to rename handleKeyboardGlobally to handleEventsGlobally or similar. If this change is pushed all the way to the API, this will be a breaking change to the API. An alternative is to add a separate property for this or to explain in the documentation for handleKeyboardGlobally, that the property also controls onDragOver and onDrop event handlers.

@vercel
Copy link

vercel bot commented Dec 20, 2021

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/AJ7rPaFFuy9ufKCt5Y6NDvUeTFH2
✅ Preview: https://excalidraw-git-fork-zsviczian-fix-ondragover-c8a15a-excalidraw.vercel.app

Comment on lines 990 to 993
if (this.props.handleKeyboardGlobally) {
window.addEventListener(EVENT.DRAG_OVER, this.disableEvent, false);
window.addEventListener(EVENT.DROP, this.disableEvent, false);
}
Copy link
Member

Choose a reason for hiding this comment

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

We might just want to scope it down to excalidraw-container unless we want to support globally optionally ?
cc @dwelle

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean like this?

this.excalidrawContainerValue.container?.addEventListener(EVENT.DRAG_OVER, this.disableEvent, false);
this.excalidrawContainerValue.container?.addEventListener(EVENT.DROP, this.disableEvent, false);

Copy link
Member

Choose a reason for hiding this comment

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

Yup, this.excalidrawContainerRef.current?. in this case.

@zsviczian
Copy link
Collaborator Author

Hi @dwelle, is there still anything holding you back from pushing this fix to production?

@ad1992
Copy link
Member

ad1992 commented Dec 22, 2021

can you also add this in changelog since this is related to package ?

Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>
@ad1992 ad1992 changed the title fix: global onDragOver and onDrop event listners interfere with host app fix: Scope drag and drop events to Excalidraw container to prevent overriding the host drag and drop events Dec 22, 2021
@ad1992
Copy link
Member

ad1992 commented Dec 22, 2021

Merging, thanks @zsviczian 🎉

@ad1992 ad1992 merged commit c76784b into excalidraw:master Dec 22, 2021
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.

None yet

3 participants