New version of checkbox has issues with multiselect = true + RowSelectionModel.selectActiveRow = true

[SatanEnglish]

On examples like http://6pac.github.io/SlickGrid/examples/example-checkbox-header-row.html

If you turn on Checkbox multiselect and RowSelecitonModel.selectActiveRow

The new version will not allow multiselect to work.

Old version would allow you to select each row by the check box and have multiple selected.
New version does not

     if (lookup[row] !== _selectedRowsLookup[row]) 
    {
          _grid.invalidateRow(row);
          delete _selectedRowsLookup[row];
    }

The above is deleting the row even tho it is the same row.

The issue seems to be one of the commented out rows below.

     function toggleRowSelection(row) {
            if (_selectedRowsLookup[row]) {
                _grid.setSelectedRows($.grep(_grid.getSelectedRows(), function (n) {
                    return n != row
                }));
            } else {
                _grid.setSelectedRows(_grid.getSelectedRows().concat(row));
            }
+            //_grid.setActiveCell(row, getCheckboxColumnCellIndex());
+            //_grid.focus();
        }

The set and then focus seems to be acting like a double click on the checkbox/row

[SatanEnglish]

New version

Old version did

This is without holding CTR or Shift which still do the correct funcitonality

[ghiscoding]

What do you mean by old vs new versions? Do you mean since Frozen feature was introduced?
The answer is... yes it does still works. Here's the proof, see my Aurelia-Slickgrid demo (top portion is single click, bottom portion is multiple selection) and my repo was updated to use latest version that include Frozen feature, the row selection still works as before...

Also if you look at the other example-checkbox-row-select.html it does support multi-select well enough. From what I understood with how to have single versus multiple selection, is to use new Slick.RowSelectionModel({ selectActiveRow: true }) (where true makes single selection, while false will do multiple). I don't think anything changed there, I can still do single or multi whenever I want.

EDIT

I also pulled latest code of slickgrid master, opened the example-checkbox-header-row.html, toggle the boolean flag of selectActiveRow and it works as intended, which is what I wrote in previous paragraph (where true makes single selection, while false will do multiple). So I don't see any problem on my side but I only tried in Chrome. Maybe make sure to clear your cache after changing the flag, javascript stays in the cache, usually doing Ctrl+R and Ctrl+F5 helps remove the cache.

[SatanEnglish]

@ghiscoding
Yep I think it's before the update for the frozen features.

Also it doesn't seem to work how expected which was what i've shown in the gifs.
Original you can have both the selectActiveRow true and multiple set to true and be able to use check boxes to select multiple rows.

PS: Your example is working but it never has worked if you hold control and click.

Here is the piece of code to fix that as I've never uploaded the fix by the looks of it.

Inside the slick.rowselectionmodel.js add the commented section

        function handleKeyDown(e) {
            var activeRow = _grid.getActiveCell();
            if (activeRow && e.shiftKey && !e.ctrlKey && !e.altKey && !e.metaKey && (e.which == 38 || e.which == 40)) {
                var selectedRows = getSelectedRows();
                selectedRows.sort(function (x, y) {
                    return x - y
                });

                if (!selectedRows.length) {
                    selectedRows = [activeRow.row];
                }

                var top = selectedRows[0];
                var bottom = selectedRows[selectedRows.length - 1];
                var active;

                if (e.which == 40) {
                    active = activeRow.row < bottom || top == bottom ? ++bottom : ++top;
                } else {
                    active = activeRow.row < bottom ? --bottom : --top;
                }

+                // Edited to make so that multiSelect via arrows with shift or Ctrl won't happen
+                if (active >= 0 && active < _grid.getDataLength()) {
+                    _grid.scrollRowIntoView(active);
+                    if (!_grid.getOptions().multiSelect) {
+                        top = active;
+                        bottom = active;
+                    }
+                    var tempRanges = rowsToRanges(getRowsRange(top, bottom));
+                    setSelectedRanges(tempRanges);
+                }

                e.preventDefault();
                e.stopPropagation();
            }
        }

[SatanEnglish]

@ghiscoding

Here is my example of how it worked / works with my changes.
MultiSelect = true, selectActiveRow = true

Here you can see select checkboxes allows multi select clicking a row deselects all but selects single.
However holding ctrl and clicking acts like I clicked on the checkbox.

Here is how multiselect = false. selectActiveRow = true worked

Here you can see select checkboxes only one active at a time clicking row deselects all but selects single.
Holding ctrl and clicking acts like I clicked on the checkbox this is only single allowed.

I've not used the second example if I need single select I always just use selectActiveRow = true and turn off checkboxes

[SatanEnglish]

@ghiscoding @6pac
It has changed how the code used to work here.
By all means go and change to the old version and check if you want.

The functionality of these things should now change.
I've given the code here that fixes the issue can you look into if it causes you any issue if we comment out the 2 lines.

I've also given you the fix for if holding control or shift. (slick.rowselectionmodel.js fix)

Let me know if you still don't have enough information to understand the issue here and I can give step by step what to click and do to get the functionality of this plugin back to what it use to do.

Thanks

[ghiscoding]

I didn't even know we could use the Ctrl key with this plugin lol

I'll have to re-read everything another day, I'm just too tired now and I don't know what was the old way vs new way since I didn't know we could certain keyboard keys in combo with this plugin

[ghiscoding]

@SatanEnglish can this be close since PR #343 ?
I think you said that it was going to include fixes for this issue, correct?

[SatanEnglish]

@ghiscoding
Just checked you seem to have removed the focus which fixes the Crtl key double click part of issue.

However may pay to do the fix to the slick.rowselectionmodel.js plugin mentioned above so that the items you gave me link to will work.

comment link

[SatanEnglish]

@ghiscoding
Did you end up adding the stuff to the slick.rowselectionmodel.js?
Then we can close this one

[ghiscoding]

I didn't yet, I was focusing on the other PRs first. You could create a PR if you want.

[ghiscoding]

@SatanEnglish
I'm giving a try at the code change you mentioned here and the first thing I see is that

The first thing I see is that, only a small piece of code is different compare to the diff that you provided, so from what I can see, only this piece is different with latest code

        function handleKeyDown(e) {
            var activeRow = _grid.getActiveCell();
            if (activeRow && e.shiftKey && !e.ctrlKey && !e.altKey && !e.metaKey && (e.which == 38 || e.which == 40)) {
                // .... some code.... removed for brevity of this post

                if (e.which == 40) {
                    active = activeRow.row < bottom || top == bottom ? ++bottom : ++top;
                } else {
                    active = activeRow.row < bottom ? --bottom : --top;
                }

+                // Edited to make so that multiSelect via arrows with shift or Ctrl won't happen
                if (active >= 0 && active < _grid.getDataLength()) {
                    _grid.scrollRowIntoView(active);
+                    if (!_grid.getOptions().multiSelect) {
+                        top = active;
+                        bottom = active;
+                    }
                    var tempRanges = rowsToRanges(getRowsRange(top, bottom));
                    setSelectedRanges(tempRanges);
                }

                e.preventDefault();
                e.stopPropagation();
            }
        }

However even if I add that piece of code, it doesn't seem to change anything on my side. That is, if I enable both flags like so { selectActiveRow: true, multiSelect: true } { selectActiveRow: true (plugin option) and multiSelect: true } (grid option) and I try to choose multiple rows from the 1st column, it doesn't work, however if I use the keyboard Ctrl & select multiple rows, that works. What is the expected behavior when selectActiveRow: true? You seem to say that it should allow multiple rows to be selected when on 1st column, but is it really suppose to permit that? selectActiveRow to me seems more like this explanation "only select the active row (only 1)"

Here's an animated gif of that test I did, the last step I'm doing is with the Ctrl key, that is the only time I can select multiple rows.

Also note, you don't seem to have latest code since this line (118 for me) if (e.which == 40) { was replaced by if (e.which == Slick.keyCode.DOWN) { and that was February of last year. Lastly, by looking at the history, that file never got touched or affected by the frozen feature, you can see that by looking at the commit history

There's actually another bug, which you said earlier I think, if we use it like this {selectActiveRow: true, multiSelect: false} and use Ctrl key, it still allows user to select multiple rows even though multiSelect is false. I tried with and without your code change and no difference. OMG I just realized that multiSelect is on the root Grid Options while only selectActiveRow is in the plugin options, so that is not a bug then

[ghiscoding]

@6pac
If we look at @SatanEnglish post here, what would you expect the behavior to be when selectActiveRow: true (plugin option) and multiSelect: true (grid option)? @SatanEnglish says that when both flags are True, it should permit user to select multiple rows by clicking on 1st column, but to me selectActiveRow: true seems more like a single row selection, at all time and even if user select from 1st column.

[6pac]

My first instinct is to agree that selectActiveRow is single row selection at all times.

However, looking at the linked sample, this is a very useful feature, but it probably should be called toggleSelectOnActiveRow, since it toggles on or off, which I don't think it does with multiselect: false.

So we either view this as an overloaded flag that has slightly different meaning depending on the value of multiselect, and make sure it is well documented as such, or create a new option as above for use only with multiselect on.

[ghiscoding]

@6pac
Thanks I agree, I would rather keep selectActiveRow as a single row selection at all time and come up with another flag to allow multi-select on 1st column but single row selection (active row) on any other column. I wasn't sure about your new flag name toggleSelectOnActiveRow but after reflection it does make sense, it's to provide selectActiveRow on any columns but toggleSelect on 1st column, I guess it make sense... and yes this plugin is totally missing documentation, we should add it the same as we did with other plugins on top of the plugin file.

@SatanEnglish
Would you be ok to keep selectActiveRow as a single row selection at all time but add a new flag name toggleSelectOnActiveRow to do what you described in the first animated gif of your previous post here?