Permalink
Browse files

Again fix for Chrom width cell problems.

  • Loading branch information...
tonytomov committed May 17, 2012
1 parent 8d7eff0 commit 3988795bc8d1ac43e349af4eec42f1032f82bb1f
Showing with 3 additions and 3 deletions.
  1. +3 −3 js/grid.base.js
View
@@ -182,9 +182,9 @@ $.extend($.jgrid,{
}
},
cellWidth : function () {
- var testcell = $("<div class='ui-jqgrid'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td style='width:5px;'></td></tr></table></div>").find("td").width(),
- ret = ( testcell != 5);
- testcell = null;
+ var testcell = $('body').append("<div class='ui-jqgrid' id='jq_testcell' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td style='width:5px;'></td></tr></table></div>").find("td").width(),
+ ret = ( testcell != 5 );
+ $("#jq_testcell").remove();
return ret;
},
ajaxOptions: {},

6 comments on commit 3988795

@OlegKi

This comment has been minimized.

Show comment
Hide comment
@OlegKi

OlegKi May 17, 2012

Contributor

OK! Now testcell is 1 on the Chrome 18 and cellWidth returns false. Fine!

Contributor

OlegKi replied May 17, 2012

OK! Now testcell is 1 on the Chrome 18 and cellWidth returns false. Fine!

@justinethier

This comment has been minimized.

Show comment
Hide comment
@justinethier

justinethier May 17, 2012

Contributor

This fix does not work on pages that contain other td elements. One workaround is to apply an ID to the table cell:

var testcell = $('body')
.append("<div class='ui-jqgrid' id='jq_testcell' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td id='jq_testcell_td' style='width:5px;'></td></tr></table></div>")
.find("#jq_testcell_td")
.width(),
Contributor

justinethier replied May 17, 2012

This fix does not work on pages that contain other td elements. One workaround is to apply an ID to the table cell:

var testcell = $('body')
.append("<div class='ui-jqgrid' id='jq_testcell' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td id='jq_testcell_td' style='width:5px;'></td></tr></table></div>")
.find("#jq_testcell_td")
.width(),
@OlegKi

This comment has been minimized.

Show comment
Hide comment
@OlegKi

OlegKi May 17, 2012

Contributor

You are right Justin! It's very good, that you found the bug, but I think that the problem can be fixed without introducing additional cell id.
The function $('body').append() return the $('body') as the result of the expression. So the problem exist like you as mention because .find("td").width() will be interpreted as $('body').find("td").width() or $('body').find("td")eq(0).width().
I think one can fix the problem by usage appendTo instead of append:

var testcell = $("<div class='ui-jqgrid' id='jq_testcell' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td style='width:5px;'></td></tr></table></div>")
               .appendTo("body").find("td").width(),
    ret = ( testcell != 5 );
$("#jq_testcell").remove();

In the case .find("td") will be executed of the div 'jq_testcell', it will be found one <td> and all will work correctly.

Event more better in my opinion will be to save the div to variable and use it in the following .remove. In the case one will not use searching by id and the part id='jq_testcell' will be not needed for the div at all (no ids for the test div):

cellWidth : function () {
    var $testDiv = $("<div class='ui-jqgrid' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td style='width:5px;'></td></tr></table></div>"),
        testcell = $testDiv.appendTo("body").find("td").width();
    $testDiv.remove();
    return testcell !== 5;
}

Best regards
Oleg

Contributor

OlegKi replied May 17, 2012

You are right Justin! It's very good, that you found the bug, but I think that the problem can be fixed without introducing additional cell id.
The function $('body').append() return the $('body') as the result of the expression. So the problem exist like you as mention because .find("td").width() will be interpreted as $('body').find("td").width() or $('body').find("td")eq(0).width().
I think one can fix the problem by usage appendTo instead of append:

var testcell = $("<div class='ui-jqgrid' id='jq_testcell' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td style='width:5px;'></td></tr></table></div>")
               .appendTo("body").find("td").width(),
    ret = ( testcell != 5 );
$("#jq_testcell").remove();

In the case .find("td") will be executed of the div 'jq_testcell', it will be found one <td> and all will work correctly.

Event more better in my opinion will be to save the div to variable and use it in the following .remove. In the case one will not use searching by id and the part id='jq_testcell' will be not needed for the div at all (no ids for the test div):

cellWidth : function () {
    var $testDiv = $("<div class='ui-jqgrid' style='left:10000px'><table class='ui-jqgrid-btable' style='width:5px;'><tr class='jqgrow'><td style='width:5px;'></td></tr></table></div>"),
        testcell = $testDiv.appendTo("body").find("td").width();
    $testDiv.remove();
    return testcell !== 5;
}

Best regards
Oleg

@justinethier

This comment has been minimized.

Show comment
Hide comment
@justinethier

justinethier May 17, 2012

Contributor

Thanks for the update, Oleg! I agree that in this case DOM ID's should be avoided if possible. Your latest fix works great in all browsers I tested including IE, FF, Chrome 12, Chrome 19, and Safari.

Going forward, this version should be merged back into jqGrid.

Contributor

justinethier replied May 17, 2012

Thanks for the update, Oleg! I agree that in this case DOM ID's should be avoided if possible. Your latest fix works great in all browsers I tested including IE, FF, Chrome 12, Chrome 19, and Safari.

Going forward, this version should be merged back into jqGrid.

@OlegKi

This comment has been minimized.

Show comment
Hide comment
@OlegKi

OlegKi May 17, 2012

Contributor

Thanks you too for finding the original bug! I think that iterations is the best way to find the most effective solution.

Contributor

OlegKi replied May 17, 2012

Thanks you too for finding the original bug! I think that iterations is the best way to find the most effective solution.

@tonytomov

This comment has been minimized.

Show comment
Hide comment
@tonytomov

tonytomov May 18, 2012

Owner

Hello,
Thank you both. More eyes better jqGrid.
Kind Regards

Owner

tonytomov replied May 18, 2012

Hello,
Thank you both. More eyes better jqGrid.
Kind Regards

Please sign in to comment.