The row selection is interrupted randomly

[pettmatt]

I tried implementing row selection using SlickCellRangeSelector and SlickRowSelectionModel. After testing for a while I noticed that my own implementation as well as the Example 17 - Grid 2 included unwanted behavior. While pressing shift or control and selecting rows, the row selection seemingly breaks randomly and deselects everything but the last row clicked.

Gif for clarification, how selection breaks while pressing control and selecting rows

Is this behavior a known issue?

[ghiscoding]

this seems like a bug, however it seems to happen only on certain columns, for example it doesn't replicate on Title/Duration but it replicates on %/Start. I rarely use this feature and I would suggest you to troubleshoot it yourself, the code is in the file show below

[pettmatt]

I would argue that it doesn't happen only on specific column(s), I tested selecting by clicking on different columns like Title and Duration and the deselection appears to happen on every column, but because the deselection happens randomly it may appear being a problem only on some columns. Sometimes it appears within 10 clicks, sometimes it takes almost 50 or more. But yeah, seems like a bug.

Thanks for pointing out how to troubleshoot the problem.

[ghiscoding]

oops sorry I pointed to the wrong one for troubleshooting, in your case it's the SlickRowSelectionModel (not the SlickCellSelection), so the correct event is this one

slickgrid-universal/packages/common/src/extensions/slickRowSelectionModel.ts

Lines 168 to 188 in a2799f2

	protected handleClick(e: SlickEventData): boolean | void {
	const cell = this._grid.getCellFromEvent(e);
	if (!cell || !this._grid.canCellBeActive(cell.row, cell.cell)) {
	return false;
	}
	if (!this.gridOptions.multiSelect || (
	!e.ctrlKey && !e.shiftKey && !e.metaKey)) {
	return false;
	}
	let selection = this.rangesToRows(this._ranges);
	const idx = selection.indexOf(cell.row);
	if (idx === -1 && (e.ctrlKey || e.metaKey)) {
	selection.push(cell.row);
	this._grid.setActiveCell(cell.row, cell.cell);
	} else if (idx !== -1 && (e.ctrlKey || e.metaKey)) {
	selection = selection.filter((o: number) => o !== cell.row);
	this._grid.setActiveCell(cell.row, cell.cell);
	} else if (selection.length && e.shiftKey) {

[ghiscoding]

I troubleshooted a bit and found that it happens because in some occasion you move the mouse slightly and that causes a drag event, and when a drag event is called then that becomes a new cell range event (triggered by SlickCellRangeSelector.handleDragTo and since you drag within the same cell then the new SlickRange becomes a single row which in turn cancels all other rows that you previously selected... kinda of a long story but that's the problem. So to test that out, you can select a few rows then on the last row, just keep holding the mouse key but move a few pixels and then release the mouse click then you'll see the problem.

So maybe a way to fix this would be to ignore drag when Ctrl/Shift keys are pressed, though I'm not sure if that would cause other regressions though. Currently if you use those keys and you start selecting multiple rows by using mouse drag, then it works, so if we start ignoring drag with these keys then that exercise would no longer work but that might be acceptable (or add a new grid option flag in case we need to keep that behavior!?).

[capacic]

I tried this and I found that there is no bug in the handleClick code. The problem is that handleClick() is not called on all clicks.

This is my code:

const multiSelect = { // options for slickgrid
	multiSelect: true,
	enableCheckboxSelector: true,
	enableRowSelection: true,
	rowSelectionOptions: {
		selectActiveRow: true
	},
	enableCellNavigation: true
}


const cellRangeSelectorOption = {
	selectionCss: {
		border: 'none'
	},
	autoScroll: true
}
cellRangeSelector = new SlickCellRangeSelector(cellRangeSelectorOption)
rowSelectionModel = new SlickRowSelectionModel({
	cellRangeSelector: cellRangeSelector,
	selectActiveRow: true,
	dragToSelect: true,
	autoScrollWhenDrag: true
})
grid.slick.setSelectionModel(rowSelectionModel) // todo: SlickCellSelectionModel
grid.slick.onSelectedRowsChanged.subscribe((event, arg) => {
	console.debug(`🚀 ~ slickgrid: rowClicked ~ row:`, arg.changedSelectedRows[0])
})

[ghiscoding]

like I said, it's caused by the mouse drag, read my previous comment, that is the root cause of the problem. The cell click is not being called when the mouse drag event occurred .

The mouse drag is actually a custom class named Draggable, if I modify this code

slickgrid-universal/packages/common/src/core/slickInteractions.ts

Lines 93 to 96 in a2799f2

	function userMoved(event: MouseEvent | TouchEvent) {
	const targetEvent: MouseEvent | Touch = (event as TouchEvent)?.touches?.[0] ?? event;
	deltaX = targetEvent.clientX - startX;
	deltaY = targetEvent.clientY - startY;

with this

  function userMoved(event: MouseEvent | TouchEvent | KeyboardEvent) {
    const targetEvent: MouseEvent | Touch = (event as TouchEvent)?.touches?.[0] ?? event;
    if ((event as KeyboardEvent).ctrlKey || (event as KeyboardEvent).shiftKey) {
      return; // skip drag event when Ctrl/Shift keys are pressed
    }

then the issue disappears but I'm not sure this is a good solution, it might bring other regressions. Maybe I could provide a grid option something like this maybe preventDragFromKeys: ['ctrlKey', 'shiftKey'] perhaps just ctrlKey is enough though

[capacic]

We were writing at the same time, that's why I did not see your message before I posted mine.

Changing sources is not a good option.

That solution is not very good. Now when I drag the selection bigger with shiftKey down then the selection does not grow as it does without shiftKey down.

I tied grid.slick.onDragInit.subscribe() but it does not fire in this case. Can I control or replace the Draggable class or userMoved function with my version somehow after importing Slickgrid?

I was able to do replace SlickRowSelectionModel.prototype.handleKeyDown = myHandleKeyDown but I don't know how to do it with Draggable.

[capacic]

I changed userMoved the way you said but now it does not work correctly when shiftKey is down, it does not extend the selection like it does when shifKey is not down. The old behavior was correct when dragging and keeping shiftKey down.

I would like to replace the default code with my code to do some testing. I'm not using Typescript but plain Javascript and I'm doing import dynamically only in one place.

[ghiscoding]

like I mentioned above we could restrict it to only Ctrl key if you prefer and I can update the PR with just Ctrl key. Also in the PR, the user (you) will be able to change the keys to something else if it's problematic.

I still don't quite understand what you're doing with the Shift key when dragging

[capacic]

Your solution seems to solve the missing click -problem. But while it solves one problem it creates another problem.

Normally shift is not down when dragging but if the user keeps it down when dragging it should not change the result. Your solution does.

There are also other cases where I would like to test and change dragging behavior with different combinations of keys. This is the reason I would like to do some testing with my code.

[ghiscoding]

I already mentioned that I can limit the prevent drag to the Ctrl key only (and Meta key for Mac) if you want, so that you can still keep using Shift the same as before. By preventing Ctrl key this will still solve the original issue. I would suggest that you take a look at the new PR #1538 to understand how it works. I'm adding a new grid option preventDragFromKeys to make it configurable, so if you wish to remove all keys then you would configure it to gridOptions = { preventDragFromKeys: [] } and that would remove all key restrictions (but that would bring back the issue). As per your comment, I have changed the default to be preventDragFromKeys: ['ctrlKey', 'metaKey'] and since it's configurable, it should make everyone happy. I'm most probably going to merge the PR tomorrow since I'm pretty sure this is the correct (and only) way to address the problem.

I've merged the PR #1538, it will be available in the next release