Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

fix(dnd): ignore non-file drag'n'drop events #1819

Merged
merged 4 commits into from
Nov 8, 2017
Merged

fix(dnd): ignore non-file drag'n'drop events #1819

merged 4 commits into from
Nov 8, 2017

Conversation

Novex
Copy link
Contributor

@Novex Novex commented Apr 14, 2017

Brief description of the changes

Since we only deal with files, it makes sense to ignore all events non-file related (eg. dragging
plaintext). This commit fixes a few things that have changed in the browsers which subtly break
the current checks.

  • The contains function on dt.files has been removed from the spec and will always return
    undefined. Except for IE, which hasn't implemented the change.

    • Chrome and Firefox have replaced it with includes, which we now use
    • We've left a contains check in there for IE as a last resort
    • Remove the comment about it being Firefox only, since it also works in Chrome now
    • More info re: removal at: https://github.com/tc39/Array.prototype.includes#status
  • The dt.files property always seems to be an empty array for non-drop events. Empty arrays are
    truthy, and so this will always satisfy the isValidFileDrag check before it can validate that
    the types array includes files

    • It will now only be truthy if the files array actually contains entries
  • There is a drop handler which binds to the document and always prevents all default drop
    behaviour from occurring, including things like dropping text into textfields

    • It will now only prevent default behaviour for file drops, which has the handy side-effect
      of preventing the page from navigating to the dropped file if the user misses the dropzone.

Fixes #1588.

What browsers and operating systems have you tested these changes on?

Chrome 57, Firefox 52 and IE11 on Windows 8.1

Have you written unit tests? If not, explain why.

Yes!

Though I had to use mock objects because we can't edit the fields we need to on the
DragEvent/DataTransfer. Suggestions for improvements very welcome :)

Since we only deal with files, it makes sense to ignore all events non-file related (eg. dragging
plaintext). This commit fixes a few things that have changed in the browsers which subtly break
the current checks.

* The `contains` function on `dt.files` has been removed from the spec and will always return
  undefined. Except for IE, which hasn't implemented the change.
  * Chrome and Firefox have replaced it with `includes`, which we now use
  * We've left a `contains` check in there for IE as a last resort
  * Remove the comment about it being Firefox only, since it also works in Chrome now
  * More info re: removal at: https://github.com/tc39/Array.prototype.includes#status

* The dt.files property always seems to be an empty array for non-drop events. Empty arrays are
  truthy, and so this will always satisfy the `isValidFileDrag` check before it can validate that
  the types array includes files
  * It will now only be truthy if the files array actually contains entries

* There is a drop handler which binds to the document and always prevents all default drop
  behaviour from occurring, including things like dropping text into textfields
  * It will now only prevent default behaviour for file drops, which has the handy side-effect
    of preventing the page from navigating to the dropped file if the user misses the dropzone.

Fixes #1588.
@ramusus
Copy link

ramusus commented Oct 17, 2017

I'm really interested to get this PR merged into any upcoming release. Is there any chance to get it there?

@ramusus
Copy link

ramusus commented Oct 17, 2017

Great work @Novex! I investigate this issue myself and came to the same conclusions - the problem in isValidFileDrag. It's outdated and doesn't let Fineuploader to work together with drag-and-drop libraries, for example react-dnd.

BTW, I see the Travis crashed because of just one trailing whitespace.. It shouldn't be difficult to fix.

@rnicholus
Copy link
Member

As luck would have it - i may need this fix for one of my projects too! So I'll be looking closer at this very soon. I independently verified a similar approach, and I too am trying to integrate Fine Uploader on a page w/ react-dnd.

@rnicholus rnicholus added this to the 5.15.2 milestone Nov 8, 2017
Copy link
Member

@rnicholus rnicholus left a comment

Choose a reason for hiding this comment

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

LGTM - going to start testing this out now. If all looks good, I'll release as v5.15.2.

@rnicholus
Copy link
Member

rnicholus commented Nov 8, 2017

All moderns browsers check out except for Edge 41 (which still triggers Fine Uploader's DnD behavior when dragging non-files). I'll look closer tomorrow in hopes that Edge can be easily covered too.

@rnicholus
Copy link
Member

False alarm - caching issue in Edge caused the above problem. Looks like we're clear to merge and release as v5.15.2. I'll do that very soon.

@rnicholus rnicholus merged commit a3eaa77 into FineUploader:develop Nov 8, 2017
@ramusus
Copy link

ramusus commented Nov 8, 2017

@rnicholus For some reason v.5.15.2 doesn't contain fixes from this PR.
Could you, please, check what version uploaded to npm with label 5.15.2?

@rnicholus
Copy link
Member

Just released 5.15.3 which should contain the fix. All of my other projects lack a develop branch, and I overlooked the fact that Fine Uploader has a develop branch in addition to master, which caused some confusion for me when publishing. I removed develop today to prevent this from happening in the future since develop isn't really serving a purpose anymore.

@ramusus
Copy link

ramusus commented Nov 15, 2017

@rnicholus thanks, it's working!

@Novex
Copy link
Contributor Author

Novex commented Nov 19, 2017

Hooray! Thank you for merging this 🎉

@Novex Novex deleted the ignore-non-file-drag-and-drops branch November 19, 2017 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants