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 comment:1
owner: | → chri33s |
---|---|
status: | new → pending |
Changed March 26, 2013 02:55PM UTC by 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 comment:3
status: | pending → new |
---|
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 comment:4
Rows are not always rendered with the same number of td
s. Imagine the first row had colspan=2
on one of those td
elements.
Changed March 26, 2013 04:06PM UTC by 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 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 number → 1364315215371333 |
---|
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 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> </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> </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 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 comment:9
So... this is fixed?
Changed March 26, 2013 06:34PM UTC by 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 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 + "'> </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 + "'> </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 + "'> </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 comment:12
status: | new → pending |
---|
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 comment:13
status: | pending → new |
---|
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 comment:14
status: | new → pending |
---|
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 comment:15
_comment0: | oh didn't know :) \ \ here: \ \ http://jsfiddle.net/5SCnJ/ → 1364583122161841 |
---|---|
status: | pending → new |
oh didn't know :)
here:
(btw. I've tested on Chromium 25 and it's got same issue)
Changed March 29, 2013 06:52PM UTC by comment:16
status: | new → open |
---|---|
summary: | Sortable table td colspan=99 → Sortable: Placeholder breaks table-layout: fixed |
type: | feature → bug |
Thanks. Reduced: http://jsfiddle.net/5SCnJ/1/
Changed April 02, 2013 02:43PM UTC by comment:17
resolution: | → fixed |
---|---|
status: | open → closed |
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 comment:18
milestone: | none → 1.11.0 |
---|
Changed April 17, 2013 07:10PM UTC by 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 comment:20
milestone: | 1.11.0 → 1.10.3 |
---|
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?