Skip to main content

Search and Top Navigation

#9185 closed bug (fixed)

Opened March 26, 2013 02:45PM UTC

Closed April 02, 2013 02:43PM UTC

Last modified April 17, 2013 07:10PM UTC

Sortable: Placeholder breaks table-layout: fixed

Reported by: chri33s Owned by: chri33s
Priority: minor Milestone: 1.10.3
Component: ui.sortable Version: 1.10.2
Keywords: Cc:
Blocked by: Blocking:
Description

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'> </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

Attachments (0)
Change History (20)

Changed March 26, 2013 02:49PM UTC by scottgonzalez comment:1

owner: → chri33s
status: newpending

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?

Changed March 26, 2013 02:55PM UTC by scottgonzalez comment:2

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?

Changed March 26, 2013 03:59PM UTC by chri33s comment:3

status: pendingnew

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

Changed March 26, 2013 04:05PM UTC by scottgonzalez comment:4

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

Changed March 26, 2013 04:06PM UTC by scottgonzalez comment:5

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.

Changed March 26, 2013 04:16PM UTC by chri33s comment:6

_comment0: > 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 all 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 a event to recalculate this number1364315215371333
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

Changed March 26, 2013 04:37PM UTC by chri33s comment:7

_comment0: 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></tr> within placeholder <tr> and it wasn't giving any problems \ Someone must have recently added this colspan="99" for some unknown reason..1364315863969286
_comment1: 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..1364315899708236

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> </td>" );
} else if ( nodeName === "img" ) {
	element.attr( "src", that.currentItem.attr( "src" ) );
}

In older jQuery UI versions it wasn'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..

Changed March 26, 2013 04:44PM UTC by scottgonzalez comment:8

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

Changed March 26, 2013 06:29PM UTC by mikesherov comment:9

So... this is fixed?

Changed March 26, 2013 06:34PM UTC by scottgonzalez comment:10

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.

Changed March 26, 2013 10:33PM UTC by chri33s comment:11

_comment0: 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); \ }); \ \ // 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='" + 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 , what do you think ?1364337309550328
_comment1: 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 , what do you think ?1364337970013924
_comment2: 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 ?1364338762192465

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 + "'> </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 ?

Changed March 29, 2013 02:00PM UTC by scottgonzalez comment:12

status: newpending

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).

Changed March 29, 2013 05:54PM UTC by chri33s comment:13

status: pendingnew

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

Changed March 29, 2013 06:43PM UTC by mikesherov comment:14

status: newpending

jsfiddle can accept any javascript file: just include

<script src=""></script>

and you should be good to go.

Changed March 29, 2013 06:49PM UTC by chri33s comment:15

_comment0: oh didn't know :) \ \ here: \ \ http://jsfiddle.net/5SCnJ/1364583122161841
status: pendingnew

oh didn't know :)

here:

http://jsfiddle.net/5SCnJ/

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

Changed March 29, 2013 06:52PM UTC by scottgonzalez comment:16

status: newopen
summary: Sortable table td colspan=99Sortable: Placeholder breaks table-layout: fixed
type: featurebug

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

Changed April 02, 2013 02:43PM UTC by Scott González comment:17

resolution: → fixed
status: openclosed

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

Changeset: 09b3533910e887377fc87126608db1ded06f38f6

Changed April 02, 2013 02:43PM UTC by scottgonzalez comment:18

milestone: none1.11.0

Changed April 17, 2013 07:10PM UTC by Scott Gonz��lez comment:19

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

Changed April 17, 2013 07:10PM UTC by scottgonzalez comment:20

milestone: 1.11.01.10.3