Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Droppable event ordering issue #968

Closed
wants to merge 3 commits into from

4 participants

@wesleycho

Fixes bug #9258. Previously, when two droppable elements were nested and an item was dropped on the child (with "greedy" disabled), the order in which the callbacks were called was determined only by the order in which they were bound. This PR changes it so that deeper droppable elements always have their callbacks called first.

@scottgonzalez

Thanks. We'll need you to sign our CLA before we can review this pull request.

@mikesherov
Collaborator

@wesleycho, thanks again for contributing! now that you've signed our CLA, can you clean up your patch to conform to our style guide: http://contribute.jquery.org/style-guide/js/ I know a lot of the surrounding code doesn't conform, but all new code should.

Also, we'll need a unit test with this pull request proving this fixes the bug. Can you add one

@wesleycho

I wrote up unit tests and made changes to conform to the style guide.

@mikesherov
Collaborator

@scottgonzalez, this looks right to me. Can you verify?

@scottgonzalez scottgonzalez commented on the diff
tests/unit/droppable/droppable_events.js
@@ -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" );
@scottgonzalez Owner

Why not just store the references as we create the elements? They don't even need IDs then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
tests/unit/droppable/droppable_events.js
@@ -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 });
@scottgonzalez Owner

Always use double quotes for strings. Might as well just chain these into the creation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
tests/unit/droppable/droppable_events.js
@@ -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 });
+
+ droppable3.droppable({ drop: function() { droppable3.addClass( "dropped" ) } });
+ droppable4.droppable({ drop: function() { droppable4.addClass( "dropped" ) } });
@scottgonzalez Owner

Can chain these as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
tests/unit/droppable/droppable_events.js
((8 lines not shown))
+ $( "<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 });
+
+ droppable3.droppable({ drop: function() { droppable3.addClass( "dropped" ) } });
+ droppable4.droppable({ drop: function() { droppable4.addClass( "dropped" ) } });
+
+ draggable2.draggable();
+ draggable2.css({ position: 'absolute', width: 1, height: 1, left: 0, top: 0 });
+
+ draggable2.simulate( "drag", { dx: 1, dy: 1 } );
@scottgonzalez Owner

And all of these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.droppable.js
@@ -42,7 +42,8 @@ $.widget("ui.droppable", {
_create: function() {
var o = this.options,
- accept = o.accept;
+ accept = o.accept,
+ tgt = this.element;
@scottgonzalez Owner

Use target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.droppable.js
@@ -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
@scottgonzalez Owner

Space after //.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.droppable.js
@@ -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
+ this.depth = 0;
+ while ( tgt.parent().size() >= 1 ) {
+ ++this.depth;
+ tgt = tgt.parent();
@scottgonzalez Owner

Why not just use the length for the depth and grab the last element? This loop isn't necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez scottgonzalez commented on the diff
ui/jquery.ui.droppable.js
@@ -275,6 +282,16 @@ $.ui.ddmanager = {
}
},
+ addDroppable: function ( scope, droppable ) {
+ $.ui.ddmanager.droppables[ scope ] = $.ui.ddmanager.droppables[ scope ] || [];
+ // TODO: Binary search would be faster
@scottgonzalez Owner

Blank line above comments.

@scottgonzalez Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scottgonzalez

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
41 tests/unit/droppable/droppable_events.js
@@ -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" );
@scottgonzalez Owner

Why not just store the references as we create the elements? They don't even need IDs then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ droppable3.css({ position: 'absolute', width: 100, height: 100, left: 0, top: 0 });
+ droppable4.css({ position: 'absolute', width: 50, height: 50, left: 0, top: 0 });
@scottgonzalez Owner

Always use double quotes for strings. Might as well just chain these into the creation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ droppable3.droppable({ drop: function() { droppable3.addClass( "dropped" ) } });
+ droppable4.droppable({ drop: function() { droppable4.addClass( "dropped" ) } });
@scottgonzalez Owner

Can chain these as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ draggable2.draggable();
+ draggable2.css({ position: 'absolute', width: 1, height: 1, left: 0, top: 0 });
+
+ draggable2.simulate( "drag", { dx: 1, dy: 1 } );
@scottgonzalez Owner

And all of these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ 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 );
View
23 ui/jquery.ui.droppable.js
@@ -42,7 +42,8 @@ $.widget("ui.droppable", {
_create: function() {
var o = this.options,
- accept = o.accept;
+ accept = o.accept,
+ tgt = this.element;
@scottgonzalez Owner

Use target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
@scottgonzalez Owner

Space after //.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ this.depth = 0;
+ while ( tgt.parent().size() >= 1 ) {
+ ++this.depth;
+ tgt = tgt.parent();
@scottgonzalez Owner

Why not just use the length for the depth and grab the last element? This loop isn't necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
// 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
@scottgonzalez Owner

Blank line above comments.

@scottgonzalez Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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;
Something went wrong with that request. Please try again.