Bug Fixes for _fnCalculateColumnWidths

awelch

This bug fix is for tables using the style table-layout: fixed. There is a problem when trying to set discreet column widths if scrollX is true. If a column has content wider than the specified width, the content's width will override the set width. You can see an example here: live.datatables.net/rumomuze/10/edit. As you can see in the example the bottom table which uses a workaround has the columns sized appropriately but columns 2 and 3 in the top table are wider than they have been set. The workaround applied to the bottom table is problematic as it involves the use of scrollXInner which has been deprecated and according to this doc and @allan's comment on this thread should not be used. Not to mention calculation of the appropriate width before initialization of the table can be messy and reduces the dynamic capabilities of the table. While developing a better fix for this I came across a couple related bugs in the _fnCalculateColumnWidths function.

Bug Fix 1

The first bug does not affect the functionality of the main bug I am addressing and I actually have not noticed a manifestation of any problems from this bug in practice (I haven't explicitly looked into it and I think I may have addressed it with some custom css a while back) but the code does not look right. In the section where tmpTable is created and modified there is the following block of code:

// When scrolling (X or Y) we want to set the width of the table as 
// appropriate. However, when not scrolling leave the table width as it
// is. This results in slightly different, but I think correct behaviour
if ( scrollX && scrollXInner ) {
    tmpTable.width( scrollXInner );
}
else if ( scrollX ) {
    tmpTable.css( 'width', 'auto' );
    tmpTable.removeAttr('width');
    
    // If there is no width attribute or style, then allow the table to
    // collapse
    if ( tmpTable.width() < tableContainer.clientWidth && tableWidthAttr ) {
        tmpTable.width( tableContainer.clientWidth );
    }
}
else if ( scrollY ) {
    tmpTable.width( tableContainer.clientWidth );
}
else if ( tableWidthAttr ) {
    tmpTable.width( tableWidthAttr );
}

The if statement that allows a scrollX enabled table to collapse does not seem to match the comment above it. It seems like it should be the following:

// If there is no width attribute or style, then allow the table to
// collapse
if (tmpTable.width() < tableContainer.clientWidth && !tableWidthAttr && !styleWidth) {
    tmpTable.width(tableContainer.clientWidth);
}

I may be misinterpreting the goal of this block of code. If by collapse it is meant that the table should be allowed to be smaller than the tableContainer and only set to the size of the container if there is a width attribute or style then alternatively I would think the code should read:

// If there is no width attribute or style, then allow the table to
// collapse
if (tmpTable.width() < tableContainer.clientWidth && (tableWidthAttr || styleWidth)) {
    tmpTable.width(tableContainer.clientWidth);
}

This, however, necessitates the use of the css width: 100% on a table when scrollX is set to true or you end up with the undesirable formatting shown here (when the page is wide enough): live.datatables.net/rumomuze/11/edit?output. I'm having a hard time wrapping my head around when it would be useful to let a table collapse smaller than the container when using scrollX. It seems to be contrary to the purpose of horizontal scrolling. If someone could provide an example that would be much appreciated and I can tailor the solution accordingly.

Bug Fix 2

The second bug fix is to the setting of the userInputs variable. This fix is necessary to fix the main issue I have described here and may fix additional issues I haven't tested for. It involves the section of code that converts any user input sizes into pixel sizes:

/* Convert any user input sizes into pixel sizes */
for ( i=0 ; i<visibleColumns.length ; i++ ) {
    column = columns[ visibleColumns[i] ];
    
    if ( column.sWidth !== null ) {
        column.sWidth = _fnConvertToWidth( column.sWidthOrig, tableContainer );
    
        userInputs = true;
    }
}

In this block of code if column.sWidth already has a value it is assigned the value of column.sWidthOrig. This seems redundant and since _fnCalculateColumnWidths always sets sWidth at some point this has the consequence that userInputs is always set to true after the first time this function is called even if there were no actual user inputs to the initialization. I believe this if statement is meant to check that column.sWidthOrig is not null:

if ( column.sWidthOrig !== null ) {
    column.sWidth = _fnConvertToWidth( column.sWidthOrig, tableContainer );
    
    userInputs = true;
}

Main Bug Fix

The final change that directly influences the behavior I initially described involves the same block of code as the first bug. If scrollX is set to true and scrollXInner is not set tmpTable has the css width: auto applied. This is problematic as the table will then ignore the specified size on any column that is not wide enough to fit the biggest content in said column. The fix I am using (along with the corrected code to set userInputs in Bug Fix 2) is to only set the width to auto if there is no user specified column sizes as follows:

else if (scrollX) {
    if (!userInputs) {
        tmpTable.css('width', 'auto');
        tmpTable.removeAttr('width');
    }

    // If there is no width attribute or style, then allow the table to
    // collapse
    if (tmpTable.width() < tableContainer.clientWidth && !tableWidthAttr && !styleWidth) {
        tmpTable.width(tableContainer.clientWidth);
    }
}

allan

This is excellent - thank you! I need to give myself a little head room to think about this a bit more and make sure that there aren't any knock on effects, so it might be a little while before I can get this integrated (probably in v2) but that looks great. I've added it to my bug tracker for v2 so I don't loose it.

Regards,
Allan

awelch

After some more thorough testing of this solution I have determined that there needs to be a few adjustments. In the posted solution there is a problem when mixing columns with width definitions and columns without width definitions. Any columns that do not have a width defined will be set to a width of 0px rather than having their widths calculated based on the widest cell in the column. You can see an example of this bug (using a custom datatables.js script with the changes mentioned above) here: live.datatables.net/tuyazaso/1/edit. Notice the first column's width is 0 since it does not have a width set at initialization. In order to get around this there needs to be a change to the Main Bug Fix section. The proposed change should be ignored, that section should read:

else if (scrollX) {
    tmpTable.css('width', 'auto');
    tmpTable.removeAttr('width');
 
    // If there is no width attribute or style, then allow the table to
    // collapse
    if (tmpTable.width() < tableContainer.clientWidth && !tableWidthAttr && !styleWidth) {
        tmpTable.width(tableContainer.clientWidth);
    }
}

Instead, the change should be made to the block where the widest node is retrieved. That section should be changed to:

// Find the widest cell for each column and put it into the table
if (oSettings.aoData.length) {
    for (i = 0; i < visibleColumns.length; i++) {
        columnIdx = visibleColumns[i];
        column = columns[columnIdx];

        if (!column.sWidthOrig) {
            $(_fnGetWidestNode(oSettings, columnIdx))
                .clone(false)
                .append(column.sContentPadding)
                .appendTo(tr);
        }                    
    }
}

This will only populate the column with the widest cell if it doesn't already have a width set.

With this change, tmpTable will always have its width set to auto when using scrollX. This means that this change will not allow a column to have its width set to a size smaller than the text in the header. You can see an example of this here: live.datatables.net/tuyazaso/2/edit. Notice the second column is set to 50px but is rendered wider so that all of the header text shows. This was not an adequate solution for me so I made another small change to account for this. In the block just above the aforementioned section I made the following change:

// Apply custom sizing to the cloned header
headerCells = _fnGetUniqueThs(oSettings, tmpTable.find('thead')[0]);

var hiddenWidth = 0;
for (i = 0; i < visibleColumns.length; i++) {
    column = columns[visibleColumns[i]];
    if (/^0*\.?0*%$/.test(column.sWidthOrig)) {
        var header = $(column.nTh);
        hiddenWidth += header.outerWidth() - header.width();
    }
    headerCells[i].style.width = column.sWidthOrig !== null && column.sWidthOrig !== '' ?
        _fnStringToCss(column.sWidthOrig) :
        '';

    // For scrollX we need to force the column width otherwise the
    // browser will collapse it. If this width is smaller than the
    // width the column requires, then it will have no effect
    if (column.sWidthOrig && scrollX) {
        $(headerCells[i]).html('').append($('<div/>').css({
            width: column.sWidthOrig,
            margin: 0,
            padding: 0,
            border: 0,
            height: 1
        }));
    }
}

Note the line $(headerCells[i]).html(''). This will clear all content from the header, when column.sWidthOrig has a value and scrollX is used, so the columns width will be exactly what it is set to.

allan

Amazing - thank you for the update.

Allan

awelch

I came across a bug in my revised solution above. In the block of code where the widest node is retrieved I forgot to add empty nodes in the case that sWidthOrig is set. The code should actually read:

// Find the widest cell for each column and put it into the table
if (oSettings.aoData.length) {
    for (i = 0; i < visibleColumns.length; i++) {
        columnIdx = visibleColumns[i];
        column = columns[columnIdx];
 
        if (!column.sWidthOrig) {
            $(_fnGetWidestNode(oSettings, columnIdx))
                .clone(false)
                .append(column.sContentPadding)
                .appendTo(tr);
        }
        else {
            $("<td/>")
                .append(column.sContentPadding)
                .appendTo(tr);
        }                
    }
}

awelch

I have another addendum to this solution. I almost never use DataTables without the scrollY setting but I recently have and realized there is another issue that must be addressed. The following section of code is unreachable in the current implementation since the userInputs variable is not correctly defined (it will always be true, see Bug Fix 2 above):

/* If the number of columns in the DOM equals the number that we have to
* process in DataTables, then we can use the offsets that are created by
* the web- browser. No custom sizes can be set in order for this to happen,
* nor scrolling used
*/
if (ie67 || !userInputs && !scrollX && !scrollY &&
    columnCount == _fnVisbleColumns(oSettings) &&
    columnCount == headerCells.length
) {
    for (i = 0; i < columnCount; i++) {
        var colIdx = _fnVisibleToColumnIndex(oSettings, i);

        if (colIdx !== null) {
            columns[colIdx].sWidth = _fnStringToCss(headerCells.eq(i).width());
        }
    }
}

This problem with this section of code is that it will never change the column widths after the initial call. headerCells.eq(i).width() is used to set the width of the columns. Once the widths are set on the header cells this will keep pulling the same value rather than letting the browser calculate the value and returning that. The solution is to remove all css widths from the header cells using headerCells.css("width", "");:

/* If the number of columns in the DOM equals the number that we have to
* process in DataTables, then we can use the offsets that are created by
* the web- browser. No custom sizes can be set in order for this to happen,
* nor scrolling used
*/
if (ie67 || !userInputs && !scrollX && !scrollY &&
    columnCount == _fnVisbleColumns(oSettings) &&
    columnCount == headerCells.length
) {
    //Remove header cell width settings so the browser can recalculate
    headerCells.css("width", "");
    for (i = 0; i < columnCount; i++) {
        var colIdx = _fnVisibleToColumnIndex(oSettings, i);

        if (colIdx !== null) {
            columns[colIdx].sWidth = _fnStringToCss(headerCells.eq(i).width());
        }
    }
}

colin

Excellent, thanks for sharing.