Remove stopPropagation from drag&drop event handlers

[enumag]

I was trying to highlight the dropzone when a file is dragged but before he hovers the dropzone itself. Basically to show him where he should drag the files to.

I tried to solve it using this aproach.

It didn't work correctly because simple-ajax-uploader stopped propagation of the dragover event. After I removed it everything works fine.

Why was this event handler added? I presume there was some reason behind it.

[enumag]

Ok so the preventDefault actually needs to be called. I'm not sure why you added stopPropagation as well though - that's what causing my problems. When I remove the stopPropagation calles everything works fine.

[enumag]

In my use case the stopPropagation needs to be removed only from dragover and drop. I removed it from dragenter just to be consistent.

[LPology]

@enumag What browsers have you tested that in? stopPropagation was added according to Microsoft documentation:

https://msdn.microsoft.com/library/hh673539(v=vs.85).aspx

From their example:

function init() 
{
  // Set the drop-event handlers.
  var dropArea = document.getElementById("dropspot");
  dropArea.addEventListener("drop", dropHandler, false);
  dropArea.addEventListener("dragover", doNothing, false);
}

// Prevents the event from continuing so our handlers can process the event.
function doNothing(event)
{
  event.stopPropagation();
  event.preventDefault();
}

This example shows the same thing:

https://msdn.microsoft.com/en-us/library/hh580307(v=vs.85).aspx

[enumag]

I've tested in Firefox, Chrome and IE11.

Oh so you just copied it without knowing why? Well preventDefault stops the browser from doing its default operation - in this case opening the dragged file in the current tab - we need to do that for sure. StopPropagation halts bubbling of the event to make sure no other scripts will interfere with your own handler - I believe we don't actually need that.

[LPology]

@enumag

First of all, the SO link you posted is a bad solution if you're trying to add a CSS class to a drop zone. That's because it uses dragover. The dragover event fires every 350ms, and it fires while dragging over the target. [0]

What you want instead is an event that fires once at the moment the drag enters the target. That would be the dragenter event.

Using stopPropagation on dragover is correct because it prevents dragover from being triggered on any parent elements. The problem is that your solution is bad. Making the code worse in order to accommodate a bad solution is the very definition of an anti-pattern.

So, to answer your question, no, I did not just copy that without knowing why.

Btw, the plugin's dragClass option is probably what you're looking for.

[0] https://www.w3.org/TR/2011/WD-html5-20110113/dnd.html

[enumag]

@LPology

I'm aware but dragenter doesn't really work in this case. Trust me, I've tried to get it working without dragover. It can't be done. The solution is bad but it's the only solution possible.

Letting the dragover event bubble isn't really a problem. JS constantly bubbles a lot of mouse-related events. Stopping propagation of one doesn't really make any difference. It doesn't make your code worse.

And no dragClass option is not what I'm looking for because I need to add the class before the cursor enters the dropzone - to show the user where the dropzone is. I want it to be invisible unless the user is trying to drag some files. That is why I bind the events to document and not the dropzone element and why dragenter doesn't work correctly and dragover has to be used.

[LPology]

I misread your first comment. You're right, you can only use dragover to do that.

However, I disagree that letting dragover bubble couldn't be a problem. You're right about mouse events, but there is a difference. It is difficult to predict the intention of a user based on a mouse event. But when a user drags a file to upload, we can know they have one specific intention - to upload the file.

I submit that removing stopPropagation does make the code worse because we could no longer ensure that the behavior of the application matches that expectation.

On balance, the value of accommodating this specific use case doesn't justify losing that. For that reason, I am opposed to merging this change. I would be happy to consider any evidence that my opinion is wrong.

[enumag]

I do agree with yout that with the stopPropagation removed you can no longer be so sure about the behaviour. However the behavior could only break because of this by some other script listening to the same event and doing something bad. I believe that the probability of that happening is not very high because the drag events are not something commonly used (unlike normal mouse events). For this reason I believe that allowing such listeners for features like this outweights the disadvantages.

For now I managed to fix it by hacking your library without actually changing it. I still would prefer if this was merged though. Here is my code for anyone who might need it:

(function (ss) {

    // Fixes issue with dropzone highlighting.
    // @link https://github.com/LPology/Simple-Ajax-Uploader/pull/144

    var original = ss.SimpleUpload.prototype.addDropZone;

    ss.SimpleUpload.prototype.addDropZone = function (elem) {
        var self = this;

        original.call(self, elem);

        elem.ondragover = function(e) {
            e.preventDefault();
            return false;
        };

        elem.ondrop = function(e) {
            e.preventDefault();

            ss.removeClass(this, self._opts.dragClass);

            if (!self._dragFileCheck(e)) {
                return;
            }

            if (false !== self._addFiles(e.dataTransfer.files)) {
                self._cycleQueue();
            }
        };
    };

})(ss);

Thanks for your time.