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

Make alt drag work for drop sources that can take either copy or move drag operations #675

Merged
merged 7 commits into from
Mar 14, 2017

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Mar 1, 2017

Browsers treat users holding down the alt-key before or during a drag operation as a special case where if the dropEffect is move, the drop event will not fire. This PR makes react-dnd default the dropEffect to copy when the alt-key is pressed, which makes it so that by default, drop events still fire with or without holding alt, and passes its dropEffect to dropResult.

This addresses:

An easy way to see how this works is to pull down this branch and run react-dnd, then check the examples. All of the examples now work out of the box with users holding down alt, while the specific drop-effect example still works as before (no behavior changes there, because it sets a specific dropEffect, which overrides the default one).

Also, we are using a build of react-dnd-htm5-backend with this PR in place for our drag-and-drop implementation in Brandcast. If anyone wants to see it working in a real app, feel free to email me at [email protected] and I will send you a signup invite link (currently a closed beta, but will be opened up to everybody in the next two weeks).

@darthtrevino
Copy link
Member

@acusti This is looking pretty good; would you be willing to add a case in the docs for using alt when drag-dropping?

@t1mmen
Copy link

t1mmen commented Mar 6, 2017

@acusti Please contact me when this gets merged/released to claim you bounty, ref react-dnd/react-dnd-html5-backend#23

Thanks for taking the time to review this @darthtrevino!

@darthtrevino darthtrevino self-requested a review March 6, 2017 19:28
Copy link
Member

@darthtrevino darthtrevino left a comment

Choose a reason for hiding this comment

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

Minor comment

@@ -397,6 +397,8 @@ export default class HTML5Backend {
return;
}

this.altKeyPressed = e.altKey;
Copy link
Member

Choose a reason for hiding this comment

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

We're setting this.altKeyPressed twice, is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure. This instance is for the dragenter handler, while the other is for dragover. From looking at http://stackoverflow.com/questions/32676890/want-to-know-the-difference-of-dragover-and-dragenter-events-of-html5-drag-and-d and https://social.msdn.microsoft.com/Forums/windows/en-US/be07bcde-6c99-4d68-824e-8ad93e820fe1/dragenter-vs-dragover?forum=winforms (ignoring the “Answer” and instead seeing the first reply under “All Replies”), it seems possible that dragenter could get invoked without dragover being called (on immediately entering a drop target), hence why I decided to just set the boolean in both handlers (in case the user dropped before a dragover event could be called).

@acusti
Copy link
Contributor Author

acusti commented Mar 8, 2017

Thanks for reviewing, @darthtrevino! I’d be happy to add docs or similar for this behavior. Were you imagining a mention of the behavior and use cases in the docs, or a new example illustrating copy vs move in the examples/ folder?

@darthtrevino
Copy link
Member

A new example would be helpful for me, that way I can start the doc server and verify the work you've done

@acusti
Copy link
Contributor Author

acusti commented Mar 14, 2017

@darthtrevino Checking back in on this. Regarding #675 (comment), do you think my response makes sense for setting this.altKeyPressed both on dragenter and dragover? If you’d prefer, I’d be happy to update it to only set on dragover.

Also, have you had a chance to try the example I added? I hope it makes sense. Let me know if there’s anything else you’d like to see here. Thanks!

@darthtrevino
Copy link
Member

Thanks for the ping, @acusti, we'll get this one worked out today

@darthtrevino
Copy link
Member

darthtrevino commented Mar 14, 2017

@acusti The docs look good on my end; can you add this.altKeyPressed = false, to the constructor, then I'll merge this in

@darthtrevino darthtrevino merged commit 1573fe9 into react-dnd:master Mar 14, 2017
@alvinhui
Copy link
Contributor

This is an important feature, when will it be released?

@darthtrevino
Copy link
Member

It was released today, v2.3.0

@cdauth
Copy link

cdauth commented Nov 10, 2020

It would be really nice if this supported other modifier keys as well. Using Alt seems to be Apple-specific behaviour. On Linux, Alt+Drag is typically not even possible because it moves the current window around.

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.

5 participants