Draggable: No cloning in connectToSortable and ensure correct position

Draggables now forcefully recalculate their position when dragged out
of a sortable. Sortables now override draggable position when a
draggable is dragged into it. Lastly, no longer remove sortable helper
when dragging a draggable out, which allows us to not use a clone.

Fixes #7734
Fixes #8809
Closes jquerygh-1322

Author: mikesherov

tests/unit/draggable/draggable.html

@@ -46,6 +46,31 @@
 		#draggable3, #draggable4 {
 			z-index: 100;
 		}
+		#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;
+		}
+		.sortable li {
+			height: 100px;
+			padding: 0;
+			margin: 0;
+			border: 0;
+			list-style: none;
+			display: inline-block;
+		}
 	</style>
 
 	<script src="../../../external/qunit/qunit.js"></script>
@@ -60,7 +85,8 @@
 			"ui/mouse.js",
 			"ui/resizable.js",
 			"ui/draggable.js",
-			"ui/droppable.js"
+			"ui/droppable.js",
+			"ui/sortable.js"
 		]
 	});
 	</script>
@@ -90,6 +116,18 @@
 	</div>
 	<div style="width: 1px; height: 1000px;"></div>
 	<div style="position: absolute; width: 1px; height: 2000px;"></div>
+	<ul id="sortable" class="sortable">
+		    <li>Item 0</li>
+		    <li id="draggableSortable">Item 1</li>
+		    <li>Item 2</li>
+		    <li>Item 3</li>
+		  </ul>
+		  <ul id="sortable2" class="sortable">
+		    <li id="draggableSortableClone">Item 0</li>
+		    <li>Item 1</li>
+		    <li>Item 2</li>
+		    <li>Item 3</li>
+		  </ul>
 </div>
 
 </body>

tests/unit/draggable/draggable_options.js

@@ -252,6 +252,72 @@ test( "{ connectToSortable: selector }, default", function() {
 });
 */
 
+test( "connectToSortable, dragging out of a sortable", function() {
+	expect( 3 );
+
+	var sortItem, dragHelper,
+		element = $( "#draggableSortable" ).draggable({
+			scroll: false,
+			connectToSortable: "#sortable"
+		}),
+		sortable = $( "#sortable" ).sortable(),
+		dx = 50,
+		dy = 50,
+		offsetBefore = element.offset(),
+		offsetExpected = {
+			left: offsetBefore.left + dx,
+			top: offsetBefore.top + dy
+		};
+
+	$( sortable ).one( "sortstart", function( event, ui ) {
+		sortItem = ui.item;
+	});
+
+	$( element ).one( "drag", function( event, ui ) {
+		dragHelper = ui.helper;
+	});
+
+	$( element ).one( "dragstop", function( event, ui ) {
+		// http://bugs.jqueryui.com/ticket/8809
+		// Position issue when connected to sortable
+		deepEqual( ui.helper.offset(), offsetExpected, "draggable offset is correct" );
+
+		// http://bugs.jqueryui.com/ticket/7734
+		// 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" );
+	});
+
+	element.simulate( "drag", {
+		dx: dx,
+		dy: dy
+	});
+});
+
+test( "connectToSortable, dragging clone into sortable", function() {
+	expect( 1 );
+
+	var element = $( "#draggableSortableClone" ).draggable({
+			scroll: false,
+			connectToSortable: "#sortable",
+			helper: "clone"
+		}),
+		sortable = $( "#sortable" ).sortable(),
+		offsetSortable = sortable.offset();
+
+	$( sortable ).one( "sortbeforestop", function( event, ui ) {
+		// http://bugs.jqueryui.com/ticket/8809
+		// Position issue when connected to sortable
+		deepEqual( ui.helper.offset(), offsetSortable, "sortable offset is correct" );
+	});
+
+	element.simulate( "drag", {
+		x: offsetSortable.left + 1,
+		y: offsetSortable.top + 1,
+		moves: 1
+	});
+});
+
 test( "{ containment: Element }", function() {
 	expect( 1 );
 

ui/draggable.js

@@ -177,23 +177,8 @@ $.widget("ui.draggable", $.ui.mouse, {
 		this.offsetParentCssPosition = this.offsetParent.css( "position" );
 
 		//The element's absolute position on the page minus margins
-		this.offset = this.positionAbs = this.element.offset();
-		this.offset = {
-			top: this.offset.top - this.margins.top,
-			left: this.offset.left - this.margins.left
-		};
-
-		//Reset scroll cache
-		this.offset.scroll = false;
-
-		$.extend(this.offset, {
-			click: { //Where the click happened, relative to the element
-				left: event.pageX - this.offset.left,
-				top: event.pageY - this.offset.top
-			},
-			parent: this._getParentOffset(),
-			relative: this._getRelativeOffset() //This is a relative to absolute position minus the actual position calculation - only used for relative positioned helper
-		});
+		this.positionAbs = this.element.offset();
+		this._refreshOffsets( event );
 
 		//Generate the original position
 		this.originalPosition = this.position = this._generatePosition( event, false );
@@ -234,6 +219,21 @@ $.widget("ui.draggable", $.ui.mouse, {
 		return true;
 	},
 
+	_refreshOffsets: function( event ) {
+		this.offset = {
+			top: this.positionAbs.top - this.margins.top,
+			left: this.positionAbs.left - this.margins.left,
+			scroll: false,
+			parent: this._getParentOffset(),
+			relative: this._getRelativeOffset()
+		};
+
+		this.offset.click = {
+			left: event.pageX - this.offset.left,
+			top: event.pageY - this.offset.top
+		};
+	},
+
 	_mouseDrag: function(event, noPropagation) {
 		// reset any necessary cached properties (see #5009)
 		if ( this.offsetParentCssPosition === "fixed" ) {
@@ -736,11 +736,13 @@ $.ui.plugin.add("draggable", "connectToSortable", {
 
 				this.instance.options.helper = this.instance.options._helper;
 
-				//If the helper has been the original item, restore properties in the sortable
-				if (inst.options.helper === "original") {
-					this.instance.currentItem.css({ top: "auto", left: "auto" });
-				}
-
+				// 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 {
 				this.instance.cancelHelperRemoval = false; //Remove the helper in the sortable instance
 				this.instance._trigger("deactivate", event, uiSortable);
@@ -750,8 +752,6 @@ $.ui.plugin.add("draggable", "connectToSortable", {
 
 	},
 	drag: function( event, ui, draggable ) {
-		var dragElement = this;
-
 		$.each( draggable.sortables, function() {
 			var innermostIntersecting = false,
 				thisSortable = this,
@@ -787,9 +787,7 @@ $.ui.plugin.add("draggable", "connectToSortable", {
 
 					sortable.isOver = 1;
 
-					sortable.currentItem = $( dragElement )
-						.clone()
-						.removeAttr( "id" )
+					sortable.currentItem = ui.helper
 						.appendTo( sortable.element )
 						.data( "ui-sortable-item", true );
 
@@ -827,6 +825,10 @@ $.ui.plugin.add("draggable", "connectToSortable", {
 
 				if ( sortable.currentItem ) {
 					sortable._mouseDrag( event );
+					// Copy the sortable's position because the draggable's can potentially reflect
+					// a relative position, while sortable is always absolute, which the dragged
+					// element has now become. (#8809)
+					ui.position = sortable.position;
 				}
 
 			} else {
@@ -846,11 +848,15 @@ $.ui.plugin.add("draggable", "connectToSortable", {
 					sortable._mouseStop( event, true );
 					sortable.options.helper = sortable.options._helper;
 
-					sortable.currentItem.remove();
 					if ( sortable.placeholder ) {
 						sortable.placeholder.remove();
 					}
 
+					// Recalculate the draggable's offset considering the sortable
+					// may have modified them in unexpected ways (#8809)
+					draggable._refreshOffsets( event );
+					ui.position = draggable._generatePosition( event, true );
+
 					draggable._trigger( "fromSortable", event );
 
 					// draggable revert needs that