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
Droppable event ordering issue #968
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,8 +35,6 @@ test( "droppable destruction/recreation on drop event", function() { | |
ok( !droppable2.hasClass( "active" ), "subsequent droppable no longer active" ); | ||
}); | ||
|
||
|
||
|
||
// todo: comment the following in when ready to actually test | ||
/* | ||
test("activate", function() { | ||
|
@@ -60,4 +58,43 @@ test("drop", function() { | |
}); | ||
*/ | ||
|
||
test( "drop event callback order", function() { | ||
expect( 4 ); | ||
|
||
$( "<div id='droppable3'></div>" ).appendTo( "body" ); | ||
$( "<div id='droppable4'></div>" ).appendTo( $( "#droppable3" ) ); | ||
$( "<div id='draggable2'></div>" ).appendTo( "body" ); | ||
var droppable3 = $( "#droppable3" ), | ||
droppable4 = $( "#droppable4" ), | ||
draggable2 = $( "#draggable2" ); | ||
|
||
droppable3.css({ position: 'absolute', width: 100, height: 100, left: 0, top: 0 }); | ||
droppable4.css({ position: 'absolute', width: 50, height: 50, left: 0, top: 0 }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always use double quotes for strings. Might as well just chain these into the creation. |
||
|
||
droppable3.droppable({ drop: function() { droppable3.addClass( "dropped" ) } }); | ||
droppable4.droppable({ drop: function() { droppable4.addClass( "dropped" ) } }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can chain these as well. |
||
|
||
draggable2.draggable(); | ||
draggable2.css({ position: 'absolute', width: 1, height: 1, left: 0, top: 0 }); | ||
|
||
draggable2.simulate( "drag", { dx: 1, dy: 1 } ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And all of these. |
||
|
||
ok( droppable3.hasClass( "dropped" ), "parent droppable receives event" ); | ||
ok( droppable4.hasClass( "dropped" ), "child droppable receives event" ); | ||
|
||
droppable3.removeClass( "dropped" ); | ||
droppable4.removeClass( "dropped" ); | ||
|
||
droppable4.droppable({ greedy: true }); | ||
|
||
draggable2.simulate( "drag", { dx: 1, dy: 1 } ); | ||
|
||
ok( !droppable3.hasClass( "dropped" ), "parent droppable does not receive event" ); | ||
ok( droppable4.hasClass( "dropped" ), "child droppable receives event" ); | ||
|
||
droppable3.remove(); | ||
droppable4.remove(); | ||
draggable2.remove(); | ||
}); | ||
|
||
})( jQuery ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,8 @@ $.widget("ui.droppable", { | |
_create: function() { | ||
|
||
var o = this.options, | ||
accept = o.accept; | ||
accept = o.accept, | ||
tgt = this.element; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
|
||
this.isover = false; | ||
this.isout = true; | ||
|
@@ -54,9 +55,15 @@ $.widget("ui.droppable", { | |
//Store the droppable's proportions | ||
this.proportions = { width: this.element[0].offsetWidth, height: this.element[0].offsetHeight }; | ||
|
||
//Store the droppable's depth in the DOM tree | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space after |
||
this.depth = 0; | ||
while ( tgt.parent().size() >= 1 ) { | ||
++this.depth; | ||
tgt = tgt.parent(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just use the length for the depth and grab the last element? This loop isn't necessary. |
||
} | ||
|
||
// Add the reference and positions to the manager | ||
$.ui.ddmanager.droppables[o.scope] = $.ui.ddmanager.droppables[o.scope] || []; | ||
$.ui.ddmanager.droppables[o.scope].push(this); | ||
$.ui.ddmanager.addDroppable( o.scope, this ); | ||
|
||
(o.addClasses && this.element.addClass("ui-droppable")); | ||
|
||
|
@@ -275,6 +282,16 @@ $.ui.ddmanager = { | |
} | ||
|
||
}, | ||
addDroppable: function ( scope, droppable ) { | ||
$.ui.ddmanager.droppables[ scope ] = $.ui.ddmanager.droppables[ scope ] || []; | ||
// TODO: Binary search would be faster | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blank line above comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, just drop this. If it ever becomes a problem, someone will file a bug and we can deal with it then. Otherwise, this todo will just sit forever. |
||
for ( var idx = 0; idx < $.ui.ddmanager.droppables[ scope ].length; ++idx ) { | ||
if ( $.ui.ddmanager.droppables[ scope ][ idx ].depth <= droppable.depth ) { | ||
break; | ||
} | ||
} | ||
$.ui.ddmanager.droppables[ scope ].splice( idx, 0, droppable ); | ||
}, | ||
drop: function(draggable, event) { | ||
|
||
var dropped = false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just store the references as we create the elements? They don't even need IDs then.