Sortable: Setting table row placeholder height to be same as sorted row

Fixes #13662
Closes gh-1578

Author: kevincupp

tests/unit/sortable/options.js

@@ -255,15 +255,55 @@ test("{ dropOnEmpty: true }, default", function() {
 test("{ dropOnEmpty: false }", function() {
 	ok(false, "missing test - untested code is broken code.");
 });
+*/
 
-test("{ forcePlaceholderSize: false }, default", function() {
-	ok(false, "missing test - untested code is broken code.");
-});
+QUnit.test( "{ forcePlaceholderSize: false } table rows", function( assert ) {
+	assert.expect( 1 );
 
-test("{ forcePlaceholderSize: true }", function() {
-	ok(false, "missing test - untested code is broken code.");
-});
+	var element = $( "#sortable-table2 tbody" );
+
+	element.sortable( {
+		placeholder: "test",
+		forcePlaceholderSize: false,
+		start: function( event, ui ) {
+			assert.notEqual( ui.placeholder.height(), ui.item.height(),
+				"placeholder is same height as item" );
+		}
+	} );
+
+	// This row has a non-standard height
+	$( "tr", element ).eq( 0 ).simulate( "drag", {
+		dy: 1
+	} );
+} );
 
+QUnit.test( "{ forcePlaceholderSize: true } table rows", function( assert ) {
+	assert.expect( 2 );
+
+	// Table should have the placeholder's height set the same as the row we're dragging
+	var element = $( "#sortable-table2 tbody" );
+
+	element.sortable( {
+		placeholder: "test",
+		forcePlaceholderSize: true,
+		start: function( event, ui ) {
+			assert.equal( ui.placeholder.height(), ui.item.height(),
+				"placeholder is same height as item" );
+		}
+	} );
+
+	// First row has a non-standard height
+	$( "tr", element ).eq( 0 ).simulate( "drag", {
+		dy: 1
+	} );
+
+	// Second row's height is normal
+	$( "tr", element ).eq( 1 ).simulate( "drag", {
+		dy: 1
+	} );
+} );
+
+/*
 test("{ forceHelperSize: false }, default", function() {
 	ok(false, "missing test - untested code is broken code.");
 });

tests/unit/sortable/sortable.html

@@ -22,7 +22,8 @@
 		border-width: 0;
 		height:19px;
 	}
-	#sortable-table {
+	#sortable-table,
+	#sortable-table2 {
 		width: 100%;
 	}
 	</style>
@@ -105,6 +106,24 @@
 	</tbody>
 </table>
 
+<!-- This table has rows of varying height -->
+<table id="sortable-table2">
+	<tbody>
+		<tr>
+			<td>1<br>1</td>
+			<td>1</td>
+		</tr>
+		<tr>
+			<td>2</td>
+			<td>2</td>
+		</tr>
+		<tr>
+			<td>3</td>
+			<td>3</td>
+		</tr>
+	</tbody>
+</table>
+
 <div id="sortable-images">
 	<img src="../../images/jqueryui_32x32.png" alt="">
 	<img src="../../images/jqueryui_32x32.png" alt="">

ui/widgets/sortable.js

@@ -875,20 +875,20 @@ return $.widget( "ui.sortable", $.ui.mouse, {
 
 	_createPlaceholder: function( that ) {
 		that = that || this;
-		var className,
+		var className, nodeName,
 			o = that.options;
 
 		if ( !o.placeholder || o.placeholder.constructor === String ) {
 			className = o.placeholder;
+			nodeName = that.currentItem[ 0 ].nodeName.toLowerCase();
 			o.placeholder = {
 				element: function() {
 
-					var nodeName = that.currentItem[ 0 ].nodeName.toLowerCase(),
-						element = $( "<" + nodeName + ">", that.document[ 0 ] );
+					var element = $( "<" + nodeName + ">", that.document[ 0 ] );
 
-						that._addClass( element, "ui-sortable-placeholder",
-								className || that.currentItem[ 0 ].className )
-							._removeClass( element, "ui-sortable-helper" );
+					that._addClass( element, "ui-sortable-placeholder",
+							className || that.currentItem[ 0 ].className )
+						._removeClass( element, "ui-sortable-helper" );
 
 					if ( nodeName === "tbody" ) {
 						that._createTrPlaceholder(
@@ -917,9 +917,15 @@ return $.widget( "ui.sortable", $.ui.mouse, {
 						return;
 					}
 
-					//If the element doesn't have a actual height by itself (without styles coming
-					// from a stylesheet), it receives the inline height from the dragged item
-					if ( !p.height() ) {
+					// If the element doesn't have a actual height or width by itself (without
+					// styles coming from a stylesheet), it receives the inline height and width
+					// from the dragged item. Or, if it's a tbody or tr, it's going to have a height
+					// anyway since we're populating them with <td>s above, but they're unlikely to
+					// be the correct height on their own if the row heights are dynamic, so we'll
+					// always assign the height of the dragged item given forcePlaceholderSize
+					// is true.
+					if ( !p.height() || ( o.forcePlaceholderSize &&
+							( nodeName === "tbody" || nodeName === "tr" ) ) ) {
 						p.height(
 							that.currentItem.innerHeight() -
 							parseInt( that.currentItem.css( "paddingTop" ) || 0, 10 ) -