Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

HtmlTable.selectNone fails with empty table #1081

Closed
jfly opened this Issue · 2 comments

2 participants

@jfly

HtmlTable.selectNone is failing when my table is empty. This appears to be due to the fact that HtmlTable.selectRange doesn't do very good bounds checking. It bounds properly bounds endRow, but doesn't deal with startRow < 0. Here's the relevant section of code:

selectRange: function(startRow, endRow, _deselect){
    if (!this.options.allowMultiSelect && !_deselect) return;
    var method = _deselect ? 'deselectRow' : 'selectRow',
        rows = Array.clone(this.body.rows);

    if (typeOf(startRow) == 'element') startRow = rows.indexOf(startRow);
    if (typeOf(endRow) == 'element') endRow = rows.indexOf(endRow);
    endRow = endRow < rows.length - 1 ? endRow : rows.length - 1;

    if (endRow < startRow){
        var tmp = startRow;
        startRow = endRow;
        endRow = tmp;
    }

    for (var i = startRow; i <= endRow; i++){
        if (this.options.selectHiddenRows || rows[i].isDisplayed()) this[method](rows[i], true);
    }

    return this;
},

Here's my suggested fix:

selectRange: function(startRow, endRow, _deselect){
    if (!this.options.allowMultiSelect && !_deselect) return;
    var method = _deselect ? 'deselectRow' : 'selectRow',
        rows = Array.clone(this.body.rows);

    if (typeOf(startRow) == 'element') startRow = rows.indexOf(startRow);
    if (typeOf(endRow) == 'element') endRow = rows.indexOf(endRow);

    if (endRow < startRow){
        var tmp = startRow;
        startRow = endRow;
        endRow = tmp;
    }

    var rowAfterEndRow = Math.min(endRow + 1, rows.length);
    startRow = Math.max(startRow, 0);

    for (var i = startRow; i < rowAfterEndRow; i++){
        if (this.options.selectHiddenRows || rows[i].isDisplayed()) this[method](rows[i], true);
    }

    return this;
},
@jfly

I dug into this a bit more, and it appears that there are already tests in the Specs directory which tickle this problem.

Without my change:

should select all and select none
TypeError: Cannot call method 'isDisplayed' of undefined

Whereas with my change, this test passes.

(I ran this test by simply opening up .../mootools-more/Tests/Specs/Runner/runner.html?preset=more-all&spec=HtmlTable.Select%20should%20select%20all%20and%20select%20none. in my browser)

In fact, running Tests/Specs/Runner/runner.html without my change gives me

549 specs, 968 assertions, 19 failures in 7.241sFinished at Fri Dec 16 2011 03:43:59 GMT-0800 (PST)

and with my change:

549 specs, 974 assertions, 17 failures in 7.247sFinished at Fri Dec 16 2011 03:42:48 GMT-0800 (PST)
@jfly jfly referenced this issue from a commit in jfly/mootools-more
@jfly jfly Fix for #1081. b04ef52
@jfly jfly referenced this issue
Open

Fix for #1081 #1082

@anutron anutron referenced this issue from a commit in anutron/mootools-more
@anutron anutron Fixes #1081 - calling .empty() an already empty HtmlTable instance 57ce9e6
@anutron anutron closed this in f55c257
@jfly

I've updated my pull request to fix the bug you pointed out. I believe it is better to fix the selectNone() method than to hack around it in the empty() method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.