Select api "select()" not working correctly with select style "os"

petr24

Hey,

I think I discovered a weird bug in the pretty much flawless select extension.

Link to JsFiddle

*** Reproduce use case to cause bug ***
1. Select any row
2. Then press shift key and select another row to get range of selected rows
3. Everything at this point works the way it should, and the range is working as expected.
4. Click "Unselect rows" button to clear selection.
5. In the "enter row number" input, enter any row number, preferably one you will be able to see if it's selected.
6. This row has now been selected programmatically via "select()" vs user click select.
7. Go to step number 2 and try to the select a range of contacts. The range of selected rows is no longer from the programmatically selected row to row you clicked. It's some other range.

PS I tried triggering a row select via "click()" and "trigger('click')" to try and see that could be a work around, but that doesn't work either.

Not sure whats causing this. Any help would be appreciated on this. Thanks!

petr24

So after doing some debugging. I found the issue...

There is a function called "rowColumnRange" the last param it takes is called 'last' which is the last row selected, but for whatever reason calling row().select() does not assign that to last.

So when the function is called and a row has been selected via API the 'last' param is empty, and that's what causes it to mess up.

I fixed it where if the 'last' var is empty, assign the value of the current selected row. This is not a good fix, but it does the job as a first go.

Here is the full function, with my line added at the beginning

function rowColumnRange( dt, type, idx, last )
{
    
    //Added line to check if last is null, if it is
    //need to assign the current select contact as the starting point
    last = last || dt[type]({selected: true}).indexes()[0];
    
    // Add a range of rows from the last selected row to this one
    var indexes = dt[type+'s']( { search: 'applied' } ).indexes();
    var idx1 = $.inArray( last, indexes );
    var idx2 = $.inArray( idx, indexes );

    if ( ! dt[type+'s']( { selected: true } ).any() && idx1 === -1 ) {
        // select from top to here - slightly odd, but both Windows and Mac OS
        // do this
        indexes.splice( $.inArray( idx, indexes )+1, indexes.length );
    }
    else {
        // reverse so we can shift click 'up' as well as down
        if ( idx1 > idx2 ) {
            var tmp = idx2;
            idx2 = idx1;
            idx1 = tmp;
        }

        indexes.splice( idx2+1, indexes.length );
        indexes.splice( 0, idx1 );
    }

    if ( ! dt[type]( idx, { selected: true } ).any() ) {
        // Select range
        dt[type+'s']( indexes ).select();
    }
    else {
        // Deselect range - need to keep the clicked on row selected
        indexes.splice( $.inArray( idx, indexes ), 1 );
        dt[type+'s']( indexes ).deselect();
    }
}

allan

Thanks for this - good one!

The fix I'm tempted to put in is to make it that a shift click will only select a range if it has been preceded by a click. An API select would clear the "last" selected row and mean you have to effectively start again with the range.

It would need to allow a click, shift+click, shift+click to keep adding to the range as well, so it couldn't just reset on shift-click.

What do you think?

Allan

petr24

Hey Allan,

Didn't see that you replied. Sorry for the delay.

In my use case I don't think that fix would solve the outgoing problem.

I use state saving, so when a user comes back to the table and a row is selected where they have left off, if for whatever reason they would try and do the shift select without selecting another row, this would prevent that.

Another edge case I see this not working is on new rows and deleted rows. When a user in our app deletes a row, we select the next row for them. And if they create a new row, we select the newly created row. In both these cases this would prevent them from working with shift select unless they physically click the row.

I also did some searching and found this library, where it replicates OS X finder select behavior. And that library use a class to determine that last selected row. So it's pretty easy just to add the class name on page init along with highlighting the row.

petr24

I tweaked my fix, It's a hard coded fix which sucks, but it solves the problem for me in a solid way. And I am open to suggestions and maybe this might trigger an idea for someone on how to properly solve it.

So basically to get things to work, to my understanding, DT uses "_select_lastCell" internally to store an object of the last selected data.

The first part of the fix comes from the "select()" function

Any time a row is selected via the api, I store that row index as the last
selected row, The hard code comes in storing which column (doesn't matter for me) but I store the column that's sortable in our table. And column visible, which in our case again is always visible and no way for a user to hide it. And finally store the row index also which is dynamic.

apiRegisterPlural( 'rows().select()', 'row().select()', function ( select ) {
    var api = this;

    if ( select === false ) {
        return this.deselect();
    }

    this.iterator( 'row', function ( ctx, idx ) {
        clear( ctx );
        
        //Added by Petr
        if (!ctx._select_lastCell) {
            ctx._select_lastCell = {
                column: 1,
                columnVisible: 1,
                row: idx
            }
        } else {
            ctx._select_lastCell.row = idx
        }
        
        ctx.aoData[ idx ]._select_selected = true;
        $( ctx.aoData[ idx ].nTr ).addClass( ctx._select.className );
    } );

    this.iterator( 'table', function ( ctx, i ) {
        eventTrigger( api, 'select', [ 'row', api[i] ], true );
    } );

    return this;
} );

And then in the columnRange function
Instead of and relying on the 'last' parameter I used "_select_lastCell"

function rowColumnRange( dt, type, idx, last )
{
    
    //Added by Petr
    var ctx = dt.settings()[0];
    last = ctx._select_lastCell && ctx._select_lastCell.row;
    
    // Add a range of rows from the last selected row to this one
    var indexes = dt[type+'s']( { search: 'applied' } ).indexes();
    var idx1 = $.inArray( last, indexes );
    var idx2 = $.inArray( idx, indexes );

    if ( ! dt[type+'s']( { selected: true } ).any() && idx1 === -1 ) {
        // select from top to here - slightly odd, but both Windows and Mac OS
        // do this
        indexes.splice( $.inArray( idx, indexes )+1, indexes.length );
    }
    else {
        // reverse so we can shift click 'up' as well as down
        if ( idx1 > idx2 ) {
            var tmp = idx2;
            idx2 = idx1;
            idx1 = tmp;
        }

        indexes.splice( idx2+1, indexes.length );
        indexes.splice( 0, idx1 );
    }

    if ( ! dt[type]( idx, { selected: true } ).any() ) {
        // Select range
        dt[type+'s']( indexes ).select();
    }
    else {
        // Deselect range - need to keep the clicked on row selected
        indexes.splice( $.inArray( idx, indexes ), 1 );
        dt[type+'s']( indexes ).deselect();
    }
}

allan

Very nice - thank you for sharing your solution with me!

Your two use cases above are ones which I hadn't even considered to be honest! I'll take a look through the library you linked to and also your fix and see how best to include this for the next version of Select.

Allan

Drujik

Hi Allan,
Are you going to include this fix?

Thanks

allan

I can't quite remember where I was at with this. I've made a note to make sure that this conversation is updated before the next release of Select with whatever the state of play is.

Thanks for the prompt.

Allan

allan

I've been thinking more about this, and I'm actually going to leave it as it is at the moment. I feel that the automatic range selection should be triggered by mouse interaction only - the fact that API methods have been used shouldn't effect the mouse interaction. Equally by that measure, it wouldn't be appropriate to clear the last clicked cell when an API method is used, since the API should have no effect on mouse selection.

I can be swayed - but for the moment, I feel that the current interaction is the correct one.

Allan