From 1cc380778b99c87acc82f363f6376d11d4d3759a Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Sun, 24 Aug 2014 15:06:51 -0400 Subject: [PATCH 1/8] Draggable: Ensure correct widget removes helper in connectToSortable --- ui/draggable.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/ui/draggable.js b/ui/draggable.js index 997bba107b1..d290ea539b8 100644 --- a/ui/draggable.js +++ b/ui/draggable.js @@ -729,13 +729,16 @@ $.ui.plugin.add("draggable", "connectToSortable", { item: inst.element }); + inst.cancelHelperRemoval = false; + $.each(inst.sortables, function() { if (this.instance.isOver) { this.instance.isOver = 0; - inst.cancelHelperRemoval = true; //Don't remove the helper in the draggable instance - this.instance.cancelHelperRemoval = false; //Remove it in the sortable instance (so sortable plugins like revert still work) + // Allow this sortable to handle removing the helper + inst.cancelHelperRemoval = true; + this.instance.cancelHelperRemoval = false; //The sortable revert is supported, and we have to set a temporary dropped variable on the draggable to support revert: "valid/invalid" if (this.shouldRevert) { @@ -755,7 +758,11 @@ $.ui.plugin.add("draggable", "connectToSortable", { left: "" }); } else { - this.instance.cancelHelperRemoval = false; //Remove the helper in the sortable instance + // Prevent this Sortable from removing the helper. + // However, don't set the draggable to remove the helper + // either as another connected Sortable may yet handle the removal. + this.instance.cancelHelperRemoval = true; + this.instance._trigger("deactivate", event, uiSortable); } From 27ed20715f4b3e256f5279825ac551bbfcdfbe81 Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Sun, 24 Aug 2014 21:08:02 -0400 Subject: [PATCH 2/8] Draggable: Ensure css is always restored after connectToSortable drag Fixes #9675 --- ui/draggable.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/ui/draggable.js b/ui/draggable.js index d290ea539b8..ba8eca42b35 100644 --- a/ui/draggable.js +++ b/ui/draggable.js @@ -744,19 +744,20 @@ $.ui.plugin.add("draggable", "connectToSortable", { if (this.shouldRevert) { this.instance.options.revert = this.shouldRevert; } + // Use _storedCSS To restore properties in the sortable, + // as this also handles revert (#9675) since the draggable + // may have modified them in unexpected ways (#8809) + this.instance._storedCSS = { + position: this.instance.placeholder.css( "position" ), + top: this.instance.placeholder.css( "top" ), + left: this.instance.placeholder.css( "left" ) + }; //Trigger the stop of the sortable this.instance._mouseStop(event); this.instance.options.helper = this.instance.options._helper; - // restore properties in the sortable, since the draggable may have - // modified them in unexpected ways (#8809) - this.instance.currentItem.css({ - position: this.instance.placeholder.css( "position" ), - top: "", - left: "" - }); } else { // Prevent this Sortable from removing the helper. // However, don't set the draggable to remove the helper From e8c99b9abf7ca9368668ee5886e469d31ea33c09 Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Sun, 24 Aug 2014 21:10:08 -0400 Subject: [PATCH 3/8] Draggable: Ensure sortable revert still works after draggable is removed Fixes #9481 --- ui/draggable.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/ui/draggable.js b/ui/draggable.js index ba8eca42b35..87176150e9f 100644 --- a/ui/draggable.js +++ b/ui/draggable.js @@ -713,8 +713,7 @@ $.ui.plugin.add("draggable", "connectToSortable", { var sortable = $( this ).sortable( "instance" ); if (sortable && !sortable.options.disabled) { inst.sortables.push({ - instance: sortable, - shouldRevert: sortable.options.revert + instance: sortable }); sortable.refreshPositions(); // Call the sortable's refreshPositions at drag start to refresh the containerCache since the sortable container cache is used in drag and needs to be up to date (this will ensure it's initialised as well as being kept in step with any changes that might have happened on the page). sortable._trigger("activate", event, uiSortable); @@ -740,10 +739,6 @@ $.ui.plugin.add("draggable", "connectToSortable", { inst.cancelHelperRemoval = true; this.instance.cancelHelperRemoval = false; - //The sortable revert is supported, and we have to set a temporary dropped variable on the draggable to support revert: "valid/invalid" - if (this.shouldRevert) { - this.instance.options.revert = this.shouldRevert; - } // Use _storedCSS To restore properties in the sortable, // as this also handles revert (#9675) since the draggable // may have modified them in unexpected ways (#8809) @@ -859,12 +854,18 @@ $.ui.plugin.add("draggable", "connectToSortable", { sortable.isOver = 0; sortable.cancelHelperRemoval = true; - // Prevent reverting on this forced stop + // Calling sortable's mouseStop would trigger a revert, + // so revert must be temporarily false until after mouseStop is called. + sortable.options._revert = sortable.options.revert; sortable.options.revert = false; sortable._trigger( "out", event, sortable._uiHash( sortable ) ); sortable._mouseStop( event, true ); + + // restore sortable behaviors that were modfied + // when the draggable entered the sortable area (#9481) + sortable.options.revert = sortable.options._revert; sortable.options.helper = sortable.options._helper; if ( sortable.placeholder ) { From 368fc8395b60b0f1fcfd0ceebe697709052ab44d Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Sun, 24 Aug 2014 21:12:16 -0400 Subject: [PATCH 4/8] Draggable: Clarify comments and whitespace in connectToSortable --- ui/draggable.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ui/draggable.js b/ui/draggable.js index 87176150e9f..7ad5d0c3765 100644 --- a/ui/draggable.js +++ b/ui/draggable.js @@ -732,7 +732,6 @@ $.ui.plugin.add("draggable", "connectToSortable", { $.each(inst.sortables, function() { if (this.instance.isOver) { - this.instance.isOver = 0; // Allow this sortable to handle removing the helper @@ -751,6 +750,8 @@ $.ui.plugin.add("draggable", "connectToSortable", { //Trigger the stop of the sortable this.instance._mouseStop(event); + // Once drag has ended, the sortable should return to using + // its original helper, not the shared helper from draggable this.instance.options.helper = this.instance.options._helper; } else { @@ -812,7 +813,7 @@ $.ui.plugin.add("draggable", "connectToSortable", { return ui.helper[ 0 ]; }; - // Fire the start event of the sortable with our passed browser event, + // Fire the start events of the sortable with our passed browser event, // and our own helper (so it doesn't create a new one) event.target = sortable.currentItem[ 0 ]; sortable._mouseCapture( event, true ); @@ -829,7 +830,8 @@ $.ui.plugin.add("draggable", "connectToSortable", { draggable._trigger( "toSortable", event ); - // draggable revert needs this variable + // Inform draggable that the helper is in a valid drop zone, + // used solely in the revert option to handle "valid/invalid". draggable.dropped = sortable.element; // hack so receive/update callbacks work (mostly) @@ -860,7 +862,6 @@ $.ui.plugin.add("draggable", "connectToSortable", { sortable.options.revert = false; sortable._trigger( "out", event, sortable._uiHash( sortable ) ); - sortable._mouseStop( event, true ); // restore sortable behaviors that were modfied @@ -879,7 +880,7 @@ $.ui.plugin.add("draggable", "connectToSortable", { draggable._trigger( "fromSortable", event ); - // draggable revert needs that + // Inform draggable that the helper is no longer in a valid drop zone draggable.dropped = false; } } From a611dd8971a5fada1ca9e661ad1944b401192f0d Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Mon, 25 Aug 2014 08:21:15 -0400 Subject: [PATCH 5/8] Draggable: Refresh sortables when draggable is added or removed Since a sortable grows or shrinks when a draggable element is added to it, refresh the cached positions of sortables whenever an element is added or removed from the sortable. Refs #9675 --- ui/draggable.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ui/draggable.js b/ui/draggable.js index 7ad5d0c3765..4e81ac3ebe9 100644 --- a/ui/draggable.js +++ b/ui/draggable.js @@ -834,6 +834,12 @@ $.ui.plugin.add("draggable", "connectToSortable", { // used solely in the revert option to handle "valid/invalid". draggable.dropped = sortable.element; + // Need to refreshPositions of all sortables in the case that + // adding to one sortable changes the location of the other sortables (#9675) + $.each( draggable.sortables, function() { + this.refreshPositions(); + }); + // hack so receive/update callbacks work (mostly) draggable.currentItem = draggable.element; sortable.fromOutside = draggable; @@ -882,6 +888,12 @@ $.ui.plugin.add("draggable", "connectToSortable", { // Inform draggable that the helper is no longer in a valid drop zone draggable.dropped = false; + + // Need to refreshPositions of all sortables just in case removing + // from one sortable changes the location of other sortables (#9675) + $.each( draggable.sortables, function() { + this.refreshPositions(); + }); } } }); From aada9d5ae7bfac64ff013931e26a73fd0284c921 Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Mon, 25 Aug 2014 08:49:30 -0400 Subject: [PATCH 6/8] Draggable: Whitespace and naming cleanup of connectToSortable --- ui/draggable.js | 85 ++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 44 deletions(-) diff --git a/ui/draggable.js b/ui/draggable.js index 4e81ac3ebe9..b59955add55 100644 --- a/ui/draggable.js +++ b/ui/draggable.js @@ -703,74 +703,72 @@ $.widget("ui.draggable", $.ui.mouse, { }); -$.ui.plugin.add("draggable", "connectToSortable", { - start: function( event, ui, inst ) { +$.ui.plugin.add( "draggable", "connectToSortable", { + start: function( event, ui, draggable ) { + var uiSortable = $.extend( {}, ui, { + item: draggable.element + }); - var o = inst.options, - uiSortable = $.extend({}, ui, { item: inst.element }); - inst.sortables = []; - $(o.connectToSortable).each(function() { + draggable.sortables = []; + $( draggable.options.connectToSortable ).each(function() { var sortable = $( this ).sortable( "instance" ); - if (sortable && !sortable.options.disabled) { - inst.sortables.push({ - instance: sortable - }); - sortable.refreshPositions(); // Call the sortable's refreshPositions at drag start to refresh the containerCache since the sortable container cache is used in drag and needs to be up to date (this will ensure it's initialised as well as being kept in step with any changes that might have happened on the page). + + if ( sortable && !sortable.options.disabled ) { + draggable.sortables.push( sortable ); + + // refreshPositions is called at drag start to refresh the containerCache + // which is used in drag. This ensures it's initialized and synchronized + // with any changes that might have happened on the page since initialization. + sortable.refreshPositions(); sortable._trigger("activate", event, uiSortable); } }); - }, - stop: function( event, ui, inst ) { - - //If we are still over the sortable, we fake the stop event of the sortable, but also remove helper + stop: function( event, ui, draggable ) { var uiSortable = $.extend( {}, ui, { - item: inst.element + item: draggable.element }); - inst.cancelHelperRemoval = false; + draggable.cancelHelperRemoval = false; - $.each(inst.sortables, function() { - if (this.instance.isOver) { - this.instance.isOver = 0; + $.each( draggable.sortables, function() { + var sortable = this; + + if ( sortable.isOver ) { + sortable.isOver = 0; // Allow this sortable to handle removing the helper - inst.cancelHelperRemoval = true; - this.instance.cancelHelperRemoval = false; + draggable.cancelHelperRemoval = true; + sortable.cancelHelperRemoval = false; // Use _storedCSS To restore properties in the sortable, // as this also handles revert (#9675) since the draggable // may have modified them in unexpected ways (#8809) - this.instance._storedCSS = { - position: this.instance.placeholder.css( "position" ), - top: this.instance.placeholder.css( "top" ), - left: this.instance.placeholder.css( "left" ) + sortable._storedCSS = { + position: sortable.placeholder.css( "position" ), + top: sortable.placeholder.css( "top" ), + left: sortable.placeholder.css( "left" ) }; - //Trigger the stop of the sortable - this.instance._mouseStop(event); + sortable._mouseStop(event); // Once drag has ended, the sortable should return to using // its original helper, not the shared helper from draggable - this.instance.options.helper = this.instance.options._helper; - + sortable.options.helper = sortable.options._helper; } else { // Prevent this Sortable from removing the helper. // However, don't set the draggable to remove the helper // either as another connected Sortable may yet handle the removal. - this.instance.cancelHelperRemoval = true; + sortable.cancelHelperRemoval = true; - this.instance._trigger("deactivate", event, uiSortable); + sortable._trigger( "deactivate", event, uiSortable ); } - }); - }, drag: function( event, ui, draggable ) { $.each( draggable.sortables, function() { var innermostIntersecting = false, - thisSortable = this, - sortable = this.instance; + sortable = this; // Copy over variables that sortable's _intersectsWith uses sortable.positionAbs = draggable.positionAbs; @@ -782,15 +780,16 @@ $.ui.plugin.add("draggable", "connectToSortable", { $.each( draggable.sortables, function() { // Copy over variables that sortable's _intersectsWith uses - this.instance.positionAbs = draggable.positionAbs; - this.instance.helperProportions = draggable.helperProportions; - this.instance.offset.click = draggable.offset.click; + this.positionAbs = draggable.positionAbs; + this.helperProportions = draggable.helperProportions; + this.offset.click = draggable.offset.click; - if ( this !== thisSortable && - this.instance._intersectsWith( this.instance.containerCache ) && - $.contains( sortable.element[ 0 ], this.instance.element[ 0 ] ) ) { + if ( this !== sortable && + this._intersectsWith( this.containerCache ) && + $.contains( sortable.element[ 0 ], this.element[ 0 ] ) ) { innermostIntersecting = false; } + return innermostIntersecting; }); } @@ -799,7 +798,6 @@ $.ui.plugin.add("draggable", "connectToSortable", { // If it intersects, we use a little isOver variable and set it once, // so that the move-in stuff gets fired only once. if ( !sortable.isOver ) { - sortable.isOver = 1; sortable.currentItem = ui.helper @@ -852,7 +850,6 @@ $.ui.plugin.add("draggable", "connectToSortable", { // element has now become. (#8809) ui.position = sortable.position; } - } else { // If it doesn't intersect with the sortable, and it intersected before, // we fake the drag stop of the sortable, but make sure it doesn't remove From 52a1de5caadd9dd0665d4bf092f6061d9d3a3a8e Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Mon, 25 Aug 2014 10:28:39 -0400 Subject: [PATCH 7/8] Sortable: cancelHelperRemoval only considers helper, not placeholder Refs #9675 --- tests/unit/draggable/draggable_options.js | 2 +- ui/sortable.js | 22 ++++++---------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/tests/unit/draggable/draggable_options.js b/tests/unit/draggable/draggable_options.js index 46b80608cb1..6c4a593961c 100644 --- a/tests/unit/draggable/draggable_options.js +++ b/tests/unit/draggable/draggable_options.js @@ -305,7 +305,7 @@ test( "connectToSortable, dragging clone into sortable", function() { sortable = $( "#sortable" ).sortable(), offsetSortable = sortable.offset(); - $( sortable ).one( "sortbeforestop", function( event, ui ) { + $( sortable ).one( "sort", function( event, ui ) { // http://bugs.jqueryui.com/ticket/8809 // Position issue when connected to sortable deepEqual( ui.helper.offset(), offsetSortable, "sortable offset is correct" ); diff --git a/ui/sortable.js b/ui/sortable.js index da4d1311712..0207c86206d 100644 --- a/ui/sortable.js +++ b/ui/sortable.js @@ -1253,18 +1253,6 @@ return $.widget("ui.sortable", $.ui.mouse, { } this.dragging = false; - if(this.cancelHelperRemoval) { - if(!noPropagation) { - this._trigger("beforeStop", event, this._uiHash()); - for (i=0; i < delayedTriggers.length; i++) { - delayedTriggers[i].call(this, event); - } //Trigger all delayed events - this._trigger("stop", event, this._uiHash()); - } - - this.fromOutside = false; - return false; - } if(!noPropagation) { this._trigger("beforeStop", event, this._uiHash()); @@ -1273,10 +1261,12 @@ return $.widget("ui.sortable", $.ui.mouse, { //$(this.placeholder[0]).remove(); would have been the jQuery way - unfortunately, it unbinds ALL events from the original node! this.placeholder[0].parentNode.removeChild(this.placeholder[0]); - if(this.helper[0] !== this.currentItem[0]) { - this.helper.remove(); + if ( !this.cancelHelperRemoval ) { + if ( this.helper[ 0 ] !== this.currentItem[ 0 ] ) { + this.helper.remove(); + } + this.helper = null; } - this.helper = null; if(!noPropagation) { for (i=0; i < delayedTriggers.length; i++) { @@ -1286,7 +1276,7 @@ return $.widget("ui.sortable", $.ui.mouse, { } this.fromOutside = false; - return true; + return !this.cancelHelperRemoval; }, From bfb65076e4727d4d7fc88561b0304853accb925c Mon Sep 17 00:00:00 2001 From: Mike Sherov Date: Tue, 26 Aug 2014 08:50:50 -0400 Subject: [PATCH 8/8] Draggable: Add battery of tests to cover connectToSortable Refs #9481 Refs #9675 Closes gh-1323 --- tests/unit/draggable/draggable.html | 16 +-- tests/unit/draggable/draggable_options.js | 119 +++++++++++++++++++--- 2 files changed, 107 insertions(+), 28 deletions(-) diff --git a/tests/unit/draggable/draggable.html b/tests/unit/draggable/draggable.html index 3b4db9921e5..eb4985adc37 100644 --- a/tests/unit/draggable/draggable.html +++ b/tests/unit/draggable/draggable.html @@ -46,19 +46,11 @@ #draggable3, #draggable4 { z-index: 100; } - #sortable { + .sortable { position: relative; top: 8000px; left: 10px; - } - #sortable2 { - position: relative; - top: 9000px; - left: 10px; - } - .sortable { width: 300px; - height: 100px; padding: 0; margin: 0; border: 0; @@ -117,13 +109,13 @@
    -
  • Item 0
  • -
  • Item 1
  • +
  • Item 0
  • +
  • Item 1
  • Item 2
  • Item 3
    -
  • Item 0
  • +
  • Item 0
  • Item 1
  • Item 2
  • Item 3
  • diff --git a/tests/unit/draggable/draggable_options.js b/tests/unit/draggable/draggable_options.js index 6c4a593961c..e54ae1df46d 100644 --- a/tests/unit/draggable/draggable_options.js +++ b/tests/unit/draggable/draggable_options.js @@ -244,23 +244,15 @@ test( "cancelement, default, switching after initialization", function() { TestHelpers.draggable.shouldNotDrag( element, "cancel: input, input dragged", input ); }); -/* -test( "{ connectToSortable: selector }, default", function() { - expect( 1 ); - - ok(false, "missing test - untested code is broken code" ); -}); -*/ - test( "connectToSortable, dragging out of a sortable", function() { - expect( 3 ); + expect( 4 ); var sortItem, dragHelper, element = $( "#draggableSortable" ).draggable({ scroll: false, connectToSortable: "#sortable" }), - sortable = $( "#sortable" ).sortable(), + sortable = $( "#sortable" ).sortable({ revert: 100 }), dx = 50, dy = 50, offsetBefore = element.offset(), @@ -286,6 +278,10 @@ test( "connectToSortable, dragging out of a sortable", function() { // HTML IDs are removed when dragging to a Sortable equal( sortItem[ 0 ], dragHelper[ 0 ], "both have the same helper" ); equal( sortItem.attr( "id" ), dragHelper.attr( "id" ), "both have the same id" ); + + // http://bugs.jqueryui.com/ticket/9481 + // connectToSortable causes sortable revert to fail on second attempt + equal( sortable.sortable( "option", "revert" ), 100, "sortable revert behavior is preserved" ); }); element.simulate( "drag", { @@ -294,21 +290,31 @@ test( "connectToSortable, dragging out of a sortable", function() { }); }); -test( "connectToSortable, dragging clone into sortable", function() { - expect( 1 ); +asyncTest( "connectToSortable, dragging clone into sortable", function() { + expect( 3 ); - var element = $( "#draggableSortableClone" ).draggable({ + var offsetPlaceholder, + element = $( "#draggableSortableClone" ).draggable({ scroll: false, connectToSortable: "#sortable", helper: "clone" }), - sortable = $( "#sortable" ).sortable(), + sortable = $( "#sortable" ).sortable({ revert: 100 }), offsetSortable = sortable.offset(); $( sortable ).one( "sort", function( event, ui ) { + offsetPlaceholder = ui.placeholder.offset(); // http://bugs.jqueryui.com/ticket/8809 // Position issue when connected to sortable deepEqual( ui.helper.offset(), offsetSortable, "sortable offset is correct" ); + notDeepEqual( ui.helper.offset(), offsetPlaceholder, "offset not equal to placeholder" ); + }); + + $( sortable ).one( "sortstop", function( event, ui ) { + // http://bugs.jqueryui.com/ticket/9675 + // Animation issue with revert and connectToSortable + deepEqual( ui.item.offset(), offsetPlaceholder, "offset eventually equals placeholder" ); + start(); }); element.simulate( "drag", { @@ -318,6 +324,87 @@ test( "connectToSortable, dragging clone into sortable", function() { }); }); +test( "connectToSortable, dragging multiple elements in and out of sortable", function() { + expect( 1 ); + + var element = $( "#draggableSortableClone" ).draggable({ + scroll: false, + connectToSortable: "#sortable", + helper: "clone" + }), + element2 = $( "#draggableSortable" ).draggable({ + scroll: false, + connectToSortable: "#sortable" + }), + sortable = $( "#sortable" ).sortable({ revert: false }), + sortableOffset = sortable.offset(); + + // Move element into sortable + element.simulate( "drag", { + x: sortableOffset.left + 1, + y: sortableOffset.top + 1, + moves: 10 + }); + + // Move element in sortable out + element2.simulate( "drag", { + dx: 200, + dy: 200, + moves: 10 + }); + + // http://bugs.jqueryui.com/ticket/9675 + // Animation issue with revert and connectToSortable + sortable.one( "sortstop", function( event, ui ) { + ok( !$.contains( document, ui.placeholder[ 0 ] ), "placeholder was removed" ); + }); + + // Move the clone of the first element back out + $( "#sortable .sortable2Item" ).simulate( "drag", { + dx: 200, + dy: 200, + moves: 10 + }); +}); + +test( "connectToSortable, dragging through one sortable to a second", function() { + expect( 2 ); + + var overCount = 0, + element = $( "#draggableSortable" ).draggable({ + scroll: false, + connectToSortable: ".sortable" + }), + delta = 200, + sortable = $( "#sortable" ).sortable({ revert: false }), + sortable2 = $( "#sortable2" ).sortable({ revert: false }), + sortable2Offset = sortable2.offset(), + dragParams = { + x: sortable2Offset.left + 25, + y: sortable2Offset.top + sortable.outerHeight() + delta, + moves: 10 + }; + + $( sortable ).one( "sortover", function() { + overCount++; + sortable2.css( "top", "+=" + delta ); + }); + + $( sortable2 ).on( "sortupdate", function() { + ok( true, "second sortable is updated" ); + }); + + $( sortable2 ).one( "sortover", function() { + overCount++; + }); + + $( sortable2 ).one( "sortstop", function() { + equal( overCount, 2, "went over both sortables" ); + }); + + element.simulate( "drag", dragParams ); +}); + test( "{ containment: Element }", function() { expect( 1 ); @@ -456,8 +543,8 @@ test( "containment, element cant be pulled out of container", function() { }) .draggable({ containment: "parent" }) .simulate( "drag", { - dx: 200, - dy: 200 + dx: 500, + dy: 500 }); offsetBefore = element.offset();