Sortable: Placeholder breaks table-layout: fixed

[chri33s]

Helo,

I found a problem with sortable plugin, and to be exact with .ui-sortable-placeholder element.

When you have a table with sortable rows (TR) and you try to drag a row it shrinks all other rows in a table. I found out that the issue lies with colspan="99" property that is applied to TD within .ui-sortable-placeholder

It makes the table wider. So for example if you have 6 columns (TD) within a row they appear as a part of 99 which means you have 93 columns of empty space.

I also found out that it only applies to tables with "table-layout: fixed;" when you have "table-layout: auto;" all seems to render fine and browser forgets about extra empty columns.

Hope you understand...

Here's a part of code that applies colspan="99":

element: function() {

	var nodeName = that.currentItem[0].nodeName.toLowerCase(),
		element = $( that.document[0].createElement( nodeName ) )
			.addClass(className || that.currentItem[0].className+" ui-sortable-placeholder")
			.removeClass("ui-sortable-helper");

	if ( nodeName === "tr" ) {
		// Use a high colspan to force the td to expand the full
		// width of the table (browsers are smart enough to
		// handle this properly)
		element.append( "<td colspan='99'>&#160;</td>" );
	} else if ( nodeName === "img" ) {
		element.attr( "src", that.currentItem.attr( "src" ) );
	}

	if ( !className ) {
		element.css( "visibility", "hidden" );
	}

	return element;
},

I would really appreciate if you could fix this, I'm using "table-layout: fixed;" alot and I need to have a solution for this issue.

I propose that the plugin would calculate number od TD elements within row and apply colspan="" with exact numer instead of just 99.

Best Regards

[Scott González]

Do you have a proposal for how to make this work with table-layout: fixed? Should we walk the row and add up all the column counts?

[Scott González]

I should clarify, you suggested we should "calculate number od TD elements within row". I assume we need to account for colspan within the columns. What happens if the rows have different column counts? I guess we can copy the current row down to the td level by generating a td with a colspan that matches each td in the existing row. Do you see any issues with that?

[chri33s]

If I understand you correctly you want to take column count from row above current .ui-sortable-placeholder ? For example we have:

<tr>
<td>First</td>
<td>Second</td>
</tr>
<tr>
<td>First</td>
<td>Second</td>
<td>Third</td>

so if .ui-sortable-placeholder is after first row it would have colspan="2" and if after second colspan="3" ?

If that's what you mean I think it would solve the issue

To be honest I can't realy imagine why people would want different numer of columns per row Table are always rendered with maximum number of columns

So I think if we would calculate maxium numer od columns from all rows it would be even better solution

[Scott González]

Rows are not always rendered with the same number of tds. Imagine the first row had colspan=2 on one of those td elements.

[Scott González]

So I think if we would calculate maxium numer od columns from all rows it would be even better solution

The performance of this degrades linearly with the number of rows.

[chri33s]

Rows are not always rendered with the same number of tds. Imagine the first row had colspan=2 on one of those td elements.

oh you're right, so it would have to take into account eventual colspan's from all columns...

I guess if it would calculate number of columns from the row before .ui-sortable-placeholder it would be ok for most cases

or... it could calcalate maximum number of columns once, not each time when .ui-sortable-placeholder is created but just once when plugin starts and the applied this previously calculated value to -placeholder

but then again.. when someone adds a column dynamically with JS it would have to have an event to recalculate this number

[chri33s]

I just tried removing colspan="99" from code, and this solution (with just one column on placeholder) also seems to be working fine

if ( nodeName === "tr" ) {
	// Use a high colspan to force the td to expand the full
	// width of the table (browsers are smart enough to
	// handle this properly)
	element.append( "<td>&#160;</td>" );
} else if ( nodeName === "img" ) {
	element.attr( "src", that.currentItem.attr( "src" ) );
}

In older jQuery UI versions it wasnt't even creating <td> element within placeholder <tr> and it wasn't giving any problems Someone must have recently added this colspan="99" for some unknown reason..

[Scott González]

It was me; it wasn't working fine and it's not an unknown reason: ​https://github.com/jquery/jquery-ui/commit/bd47bd4ace3789d9eb302b0dce6f6e042d08a7f1

[mikesherov]

So... this is fixed?

[Scott González]

No, this is broken with table-layout: fixed. At least, that's the claim. I haven't checked yet and there was no test case provided.

[chri33s]

check this out, I wrote a fix

element: function() {

        var nodeName = that.currentItem[0].nodeName.toLowerCase(),
                element = $( that.document[0].createElement( nodeName ) )
                        .addClass(className || that.currentItem[0].className+" ui-sortable-placeholder")
                        .removeClass("ui-sortable-helper");
                        
        if ( nodeName === "tr" ) {
                
                // calculate colspan
                var colspan = 0;
                $("td", that.currentItem[0]).each(function() {
                        colspan += ($(this).attr('colspan') ? parseInt($(this).attr('colspan')) : 1);
                });

                element.append( "<td colspan='" + colspan + "'>&#160;</td>" );
        } else if ( nodeName === "img" ) {
                element.attr( "src", that.currentItem.attr( "src" ) );
        }

        if ( !className ) {
                element.css( "visibility", "hidden" );
        }

        return element;
},

it solves everything (at least in my case)

what do you think ?

[Scott González]

I think we still don't have a test case (visual or unit). I'm not seeing any difference between table-layout: fixed and table-layout: auto in my simple test. Can you provide an example showing the difference on jsbin or jsFiddle?

On a side note, please do not paste large blocks of code into the ticket (as mentioned in the red box).

[chri33s]

no problem but jsFiddle uses UI v1.9.2 so it's not good testing place

I wrote simple example that demostrates this issue: ​http://74.220.219.73/~naszedzi/upload/sortable/test.html

and here's screenshot how it looks when you grab handle (tested on Firefox 19.0.2) ​http://74.220.219.73/~naszedzi/upload/sortable/test-screen.png

[mikesherov]

jsfiddle can accept any javascript file: just include

<script src=""></script>

and you should be good to go.

[chri33s]

oh didn't know :)

here:

​http://jsfiddle.net/5SCnJ/

(btw. I've tested on Chromium 25 and it's got same issue)

[Scott González]

Thanks. Reduced: ​http://jsfiddle.net/5SCnJ/1/

[Scott González]

Sortable: Copy the cell structure when sorting a table row. Fixes #9185 - Sortable: Placeholder breaks table-layout: fixed.

Changeset: 09b3533910e887377fc87126608db1ded06f38f6

[Scott Gonz��lez]

Sortable: Copy the cell structure when sorting a table row. Fixes #9185 - Sortable: Placeholder breaks table-layout: fixed. (cherry picked from commit 09b3533910e887377fc87126608db1ded06f38f6)

Changeset: 9711c54c6d3d7ecffa9bfccc205522be1f4aa148