Opened 11 years ago

Closed 7 years ago

Last modified 7 years ago

#7971 closed defect (fixed)

Tree: fast cursor movement can break Drag and Drop

Reported by: boumel Owned by: bill
Priority: high Milestone: 1.8
Component: Dijit Version: 1.2.0
Keywords: Tree Drag Drop Cc: Eugene Lazutkin
Blocked By: Blocking:

Description

Error description:

After i accidentally deleted some treenodes i researched this issue. To do so i put a lot of console.log in the source. Tested with ff3 and firebug:

this dragging: false
new current
dndSource onMouseMove: 0 x:88 y:92
dndSource onMouseMove: 0 x:88 y:92
dndSource onMouseOver: 0 x:79 y:88
dndSource onMouseOver: this.current==n
dndSource onMouseMove: 0 x:79 y:88
...
dndSource onMouseOver: 0 x:73 y:86
dndSource onMouseOver: current==n
dndSource onMouseMove: 0 x:73 y:86

moving the cursor on a node, the dndsource.current is set to that node

dndSource onMouseDown: 0 x:63 y:84
dndSelector onMouseDown: 0 x:63 y:84

pressing the button, the node from dndsource.current is selected in dndSelector as source

dndSource onMouseOver: 0 x:50 y:84
dndSource onMouseOver: this dragging: false
dndSource onMouseOver: n is null
dndSource onMouseOver: this.current is null

moving the cursor out of the node, dndsource.current is set to null

dndSource onMouseMove: 0 x:50 y:84
dndSource onMouseMove: selectedNodes 1
Manager startDrag: 0
dndSource onDndStart
Manager overSource: 0
dndSource onDndStart: set dragging true

the mousemove-event starts the dnd -> dndManager triggers dnd/start -> dndSource.onDndStart calls dndManager.overSource -> overSource sets canDrop to true!!!

--- more mouse movement ---
dndSource onMouseUp: 0 x:2 y:86
Manager onMouseUp 0 x:2 y:86
dndSource onDndDrop: 1
dndSource onDndDrop: set dragging false
dndSource onDndDrop: target is null
dndSource onDndDrop: newparentitem undefined
tree model: remove item

releasing the button triggers the drop event: dndManager triggers dnd/drop ( canDrop still is true) -> dndSource.onDndDrop reads dndSource.current (is still null) as target node. with no target newParentItem is null, so model.pasteItem with null as newParentItem is called -> the node is removed.

Reason:

The problem is that the mouse-move event is triggered after the mouse-over event. So mouse-over doesn't call checkItemAcceptance because no dnd has been started. And because canDrop is set to true by overSource you can drop it without a target.

So there should be a checkItemAcceptance call in onDndStart or at least canDrop should be set to false by default.

Reproduce:

It works also with the example page http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/tree/test_Tree_DnD.html but it's a bit tricky to reproduce, you have to move the cursor out of the node very fast and push the button in the nick of time before leaving the node. Or you move the cursor very fast over the node and push the button in the moment passing the node.

Attachments (2)

dijit.tree.dndSource.7971.fix.patch (402 bytes) - added by btbngr 7 years ago.
Simple fix, addressing Tree DND
dijit.tree.dndSource.7971.fix2.patch (342 bytes) - added by btbngr 7 years ago.
Whoops. Randomly setting this.current=null after already testing for this.current==null, it's late.…

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by bill

Milestone: tbd1.5

comment:2 Changed 11 years ago by bill

Cc: Eugene Lazutkin added

Hi bourmel, thanks for the detailed report.

My question is, why does onDnDStart() call

dojo.dnd.manager().overSource(this);

at all? It seems unnecessary, since it's also called from onOverEvent().

And a second question, if you just remove that code from dndSource's onDndStart(), does it fix the problem?

Your report above is very detailed but it seems to boil down to the fact that, by the time the drag starts, the mouse is already away from the entire Tree, right? I can imagine that happening w/fast mouse movements, as onmousemove isn't guaranteed to report every 1px change in the mouse position.

CC'ing Eugene since the base DnD code also seems to have this issue.

comment:3 Changed 10 years ago by sowelie

I have noticed this very same problem. Drag and drop on the tree is also way too touchy. If you click and move ever so slightly it will trigger DnD. And usually, because of the quick movement, this throws an error (I've seen it in FF and IE). I personally think this issue is worthy of a 1.4 fix...

comment:4 Changed 10 years ago by bill

Hmm, the DnD triggering is controlled by dragThreshold, which for some reason by default is set to 0:

// dragThreshold: Integer
//		Number of pixels mouse moves before it's considered the start of a drag operation
dragThreshold: 0,

Not sure why the default is 0, although dojo.dnd.Source also defaults to 0:

delay: 0, // pixels

Seems like they should both be around 3 or 5.

comment:5 Changed 9 years ago by Adam Peller

Milestone: 1.51.6

comment:6 Changed 8 years ago by bill

Milestone: 1.6future

Changed 7 years ago by btbngr

Simple fix, addressing Tree DND

Changed 7 years ago by btbngr

Whoops. Randomly setting this.current=null after already testing for this.current==null, it's late....

comment:7 Changed 7 years ago by btbngr

The easiest way I've found to trigger this bug is as follows:

Move the mouse over the LAST node in a tree, specifically around the bottom pixel. Then drag downwards, fast enough that the mouse will skip pixels to arrive over the tree view, but not over any active node (a tall tree with few items will help here). If the bug is triggered- you will see that the dnd avatar is green, indicating that there is a valid drop target, despite the dndSource.current being null. Releasing the mouse will incorrectly trigger a drop, which can in pathological cases cause the avatar to just get stuck on the page.

The above patch can be applied to dojo 1.7.2, and appears to fix this problem, seeking feedback from dojo committers.

A workaround I'm currently using in my own software while still enabling me to use the CDN:

        var src = new declare(dndSource, {
            _onDragMouse: function(e) {
                if(this.current == null) {
                    DNDManager.manager().canDrop(false);
                    return null;
                }
                this.inherited(arguments);
            }
            /** Your stuff here **/
        });

Where DNDManager is required from dojo/dnd/Manager.

I'm new to dojo, and I thought this bug was just my code, hopefully this or a better fix can be ready for 2.0.

Hope this helps.

Last edited 7 years ago by btbngr (previous) (diff)

comment:8 Changed 7 years ago by bill

Milestone: future1.8
Owner: set to bill
Status: newassigned

Hi btbngr - Thanks! I was able to reproduce based on your description. As you suggested, it occurs when the first and only mousemove event occurs over the Tree but not over any TreeNode.

I'll check in a fix, basically what you listed above although implemented slightly differently.

comment:9 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [28025]:

Handle literal edge case where a there's only one mousemove event before the drop operation, and that mousemove event placed the cursor at the edge of the Tree, above the Tree but not above any TreeNodes. Fixes #7971 !strict.

comment:10 Changed 7 years ago by bill

#13253 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.