fix: cell selection range with key combos were incorrect

package.json

@@ -85,7 +85,7 @@
     "rimraf": "^5.0.5",
     "rxjs": "^7.8.1",
     "servor": "^4.0.2",
-    "slickgrid": "^4.1.5",
+    "slickgrid": "^4.1.6",
     "sortablejs": "^1.15.1",
     "ts-jest": "^29.1.1",
     "ts-node": "^10.9.1",

packages/common/package.json

@@ -81,7 +81,7 @@
     "flatpickr": "^4.6.13",
     "moment-mini": "^2.29.4",
     "multiple-select-vanilla": "^0.6.3",
-    "slickgrid": "^4.1.5",
+    "slickgrid": "^4.1.6",
     "sortablejs": "^1.15.1",
     "un-flatten-tree": "^2.0.12"
   },

packages/common/src/extensions/__tests__/slickCellSelectionModel.spec.ts

@@ -10,11 +10,13 @@ const NB_ITEMS = 200;
 const CALCULATED_PAGE_ROW_COUNT = 23; // pageRowCount with our mocked sizes is 23 => ((600 - 17) / 25)
 jest.mock('flatpickr', () => { });
 
-const addVanillaEventPropagation = function (event, commandKey = '', keyName = '') {
+const addVanillaEventPropagation = function (event, commandKeys: string[] = [], keyName = '') {
   Object.defineProperty(event, 'isPropagationStopped', { writable: true, configurable: true, value: jest.fn() });
   Object.defineProperty(event, 'isImmediatePropagationStopped', { writable: true, configurable: true, value: jest.fn() });
-  if (commandKey) {
-    Object.defineProperty(event, commandKey, { writable: true, configurable: true, value: true });
+  if (commandKeys.length) {
+    for (const commandKey of commandKeys) {
+      Object.defineProperty(event, commandKey, { writable: true, configurable: true, value: true });
+    }
   }
   if (keyName) {
     Object.defineProperty(event, 'key', { writable: true, configurable: true, value: keyName });
@@ -38,6 +40,12 @@ const dataViewStub = {
   getPagingInfo: () => ({ pageSize: 0 }),
 };
 
+const mockColumns = [
+  { id: 'firstName', field: 'firstName' },
+  { id: 'lastName', field: 'lastName' },
+  { id: 'age', field: 'age' },
+]
+
 const gridStub = {
   canCellBeSelected: jest.fn(),
   getActiveCell: jest.fn(),
@@ -46,12 +54,13 @@ const gridStub = {
   getCellFromEvent: jest.fn(),
   getCellFromPoint: jest.fn(),
   getCellNodeBox: jest.fn(),
+  getColumns: () => mockColumns,
   getData: () => dataViewStub,
   getDataLength: jest.fn(),
   getEditorLock: () => getEditorLockMock,
   getOptions: () => mockGridOptions,
   getUID: () => GRID_UID,
-  getScrollbarDimensions: () => ({ height: 17, width: 17}),
+  getScrollbarDimensions: () => ({ height: 17, width: 17 }),
   getViewportNode: jest.fn(),
   focus: jest.fn(),
   registerPlugin: jest.fn(),
@@ -229,7 +238,7 @@ describe('CellSelectionModel Plugin', () => {
     plugin.setSelectedRanges(mockRanges);
 
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'ArrowLeft');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'ArrowLeft');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     expect(setSelectRangeSpy).toHaveBeenCalledWith([{
@@ -250,7 +259,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: 3, toCell: 3, toRow: 4 }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'ArrowRight');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'ArrowRight');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     expect(setSelectRangeSpy).toHaveBeenCalledWith([{
@@ -268,7 +277,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: 3, toCell: 3, toRow: 4 }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'ArrowUp');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'ArrowUp');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     expect(setSelectRangeSpy).toHaveBeenCalledWith([{
@@ -286,7 +295,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: 3, toCell: 3, toRow: 4, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'ArrowDown');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'ArrowDown');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     expect(setSelectRangeSpy).toHaveBeenCalledWith([{
@@ -308,7 +317,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: 3, toCell: 3, toRow: 4, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'ArrowDown');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'ArrowDown');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [
@@ -337,7 +346,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 4, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'PageDown');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'PageDown');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [
@@ -364,7 +373,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 4, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'PageDown');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'PageDown');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [
@@ -391,7 +400,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 120, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'PageUp');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'PageUp');
     gridStub.onKeyDown.notify({ cell: 2, row: 101, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [
@@ -418,7 +427,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 4, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'PageUp');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'PageUp');
     gridStub.onKeyDown.notify({ cell: 2, row: 3, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [
@@ -432,7 +441,61 @@ describe('CellSelectionModel Plugin', () => {
     expect(scrollCellSpy).toHaveBeenCalledWith(firstRowIndex, 2, false);
   });
 
-  it('should call "setSelectedRanges" with Slick Range from current position to row index 0 when using Shift+Home key combo when triggered by "onKeyDown"', () => {
+  it('should call "setSelectedRanges" with Slick Range from current position to row index 0 horizontally when using Shift+Home key combo when triggered by "onKeyDown"', () => {
+    const notifyingRowNumber = 100;
+    const expectedRowZeroIdx = 0;
+    jest.spyOn(gridStub, 'getActiveCell').mockReturnValue({ cell: 2, row: notifyingRowNumber });
+    jest.spyOn(gridStub, 'canCellBeSelected').mockReturnValue(true);
+    const scrollCellSpy = jest.spyOn(gridStub, 'scrollCellIntoView');
+
+    plugin.init(gridStub);
+    plugin.setSelectedRanges([
+      { fromCell: 1, fromRow: 99, toCell: 3, toRow: 120, contains: () => false },
+      { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 120, contains: () => false }
+    ] as unknown as SlickRange[]);
+    const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'Home');
+    gridStub.onKeyDown.notify({ cell: 2, row: 101, grid: gridStub }, keyDownEvent, gridStub);
+
+    const expectedRangeCalled = [
+      { fromCell: 1, fromRow: 99, toCell: 3, toRow: 120, contains: expect.toBeFunction(), } as unknown as SlickRange,
+      {
+        fromCell: expectedRowZeroIdx, fromRow: notifyingRowNumber, toCell: 2, toRow: 100,
+        contains: expect.toBeFunction(), toString: expect.toBeFunction(), isSingleCell: expect.toBeFunction(), isSingleRow: expect.toBeFunction(),
+      },
+    ];
+    expect(setSelectRangeSpy).toHaveBeenCalledWith(expectedRangeCalled);
+    expect(scrollCellSpy).toHaveBeenCalledWith(100, 2, false);
+  });
+
+  it('should call "setSelectedRanges" with Slick Range from current position to same row last cell index horizontally when using Shift+End key combo when triggered by "onKeyDown"', () => {
+    const notifyingRowNumber = 100;
+    const columnsLn = mockColumns.length;
+    jest.spyOn(gridStub, 'getActiveCell').mockReturnValue({ cell: 2, row: notifyingRowNumber });
+    jest.spyOn(gridStub, 'canCellBeSelected').mockReturnValue(true);
+    const scrollCellSpy = jest.spyOn(gridStub, 'scrollCellIntoView');
+
+    plugin.init(gridStub);
+    plugin.setSelectedRanges([
+      { fromCell: 1, fromRow: 99, toCell: 3, toRow: 120, contains: () => false },
+      { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 120, contains: () => false }
+    ] as unknown as SlickRange[]);
+    const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['shiftKey'], 'End');
+    gridStub.onKeyDown.notify({ cell: 1, row: 101, grid: gridStub }, keyDownEvent, gridStub);
+
+    const expectedRangeCalled = [
+      { fromCell: 1, fromRow: 99, toCell: 3, toRow: 120, contains: expect.toBeFunction(), } as unknown as SlickRange,
+      {
+        fromCell: columnsLn - 1, fromRow: notifyingRowNumber, toCell: 2, toRow: 100,
+        contains: expect.toBeFunction(), toString: expect.toBeFunction(), isSingleCell: expect.toBeFunction(), isSingleRow: expect.toBeFunction(),
+      },
+    ];
+    expect(setSelectRangeSpy).toHaveBeenCalledWith(expectedRangeCalled);
+    expect(scrollCellSpy).toHaveBeenCalledWith(100, 2, false);
+  });
+
+  it('should call "setSelectedRanges" with Slick Range from current position to cell,row index 0 when using Ctrl+Shift+Home key combo when triggered by "onKeyDown"', () => {
     const notifyingRowNumber = 100;
     const expectedRowZeroIdx = 0;
     jest.spyOn(gridStub, 'getActiveCell').mockReturnValue({ cell: 2, row: notifyingRowNumber });
@@ -445,21 +508,21 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 120, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'Home');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['ctrlKey', 'shiftKey'], 'Home');
     gridStub.onKeyDown.notify({ cell: 2, row: 101, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [
       { fromCell: 1, fromRow: 99, toCell: 3, toRow: 120, contains: expect.toBeFunction(), } as unknown as SlickRange,
       {
-        fromCell: 2, fromRow: expectedRowZeroIdx, toCell: 2, toRow: 100,
+        fromCell: 0, fromRow: expectedRowZeroIdx, toCell: 2, toRow: 100,
         contains: expect.toBeFunction(), toString: expect.toBeFunction(), isSingleCell: expect.toBeFunction(), isSingleRow: expect.toBeFunction(),
       },
     ];
     expect(setSelectRangeSpy).toHaveBeenCalledWith(expectedRangeCalled);
     expect(scrollCellSpy).toHaveBeenCalledWith(expectedRowZeroIdx, 2, false);
   });
 
-  it('should call "setSelectedRanges" with Slick Range from current position to last row index when using Shift+End key combo when triggered by "onKeyDown"', () => {
+  it('should call "setSelectedRanges" with Slick Range from current position to last row index when using Ctrl+Shift+End key combo when triggered by "onKeyDown"', () => {
     const notifyingRowNumber = 100;
     const expectedLastRowIdx = NB_ITEMS - 1;
     jest.spyOn(gridStub, 'getActiveCell').mockReturnValue({ cell: 2, row: notifyingRowNumber });
@@ -472,7 +535,7 @@ describe('CellSelectionModel Plugin', () => {
       { fromCell: 2, fromRow: notifyingRowNumber, toCell: 3, toRow: 120, contains: () => false }
     ] as unknown as SlickRange[]);
     const setSelectRangeSpy = jest.spyOn(plugin, 'setSelectedRanges');
-    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), 'shiftKey', 'End');
+    const keyDownEvent = addVanillaEventPropagation(new Event('keydown'), ['ctrlKey', 'shiftKey'], 'End');
     gridStub.onKeyDown.notify({ cell: 2, row: 101, grid: gridStub }, keyDownEvent, gridStub);
 
     const expectedRangeCalled = [

packages/common/src/extensions/slickCellSelectionModel.ts

@@ -1,3 +1,5 @@
+import { isDefined } from '@slickgrid-universal/utils';
+
 import type { CellRange, OnActiveCellChangedEventArgs, SlickDataView, SlickEventHandler, SlickGrid, SlickNamespace, SlickRange } from '../interfaces/index';
 import { SlickCellRangeSelector } from './index';
 
@@ -159,9 +161,12 @@ export class SlickCellSelectionModel {
 
   protected handleActiveCellChange(_e: Event, args: OnActiveCellChangedEventArgs) {
     this._prevSelectedRow = undefined;
-    if (this._addonOptions?.selectActiveCell && args.row !== null && args.cell !== null) {
+    const isCellDefined = isDefined(args.cell);
+    const isRowDefined = isDefined(args.row);
+
+    if (this._addonOptions?.selectActiveCell && isRowDefined && isCellDefined) {
       this.setSelectedRanges([new Slick.Range(args.row, args.cell)]);
-    } else if (!this._addonOptions?.selectActiveCell) {
+    } else if (!this._addonOptions?.selectActiveCell || (!isRowDefined && !isCellDefined)) {
       // clear the previous selection once the cell changes
       this.setSelectedRanges([]);
     }
@@ -186,8 +191,8 @@ export class SlickCellSelectionModel {
   protected handleKeyDown(e: KeyboardEvent) {
     let ranges: CellRange[];
     let last: SlickRange;
+    const colLn = this._grid.getColumns().length;
     const active = this._grid.getActiveCell();
-    const metaKey = e.ctrlKey || e.metaKey;
 
     let dataLn = 0;
     if (this._dataView) {
@@ -196,7 +201,7 @@ export class SlickCellSelectionModel {
       dataLn = this._grid.getDataLength();
     }
 
-    if (active && e.shiftKey && !metaKey && !e.altKey && this.isKeyAllowed(e.key)) {
+    if (active && (e.shiftKey || e.ctrlKey) && !e.altKey && this.isKeyAllowed(e.key)) {
       ranges = this.getSelectedRanges().slice();
       if (!ranges.length) {
         ranges.push(new Slick.Range(active.row, active.cell));
@@ -216,9 +221,10 @@ export class SlickCellSelectionModel {
         const dirRow = active.row === last.fromRow ? 1 : -1;
         const dirCell = active.cell === last.fromCell ? 1 : -1;
         const isSingleKeyMove = e.key.startsWith('Arrow');
+        let toCell: undefined | number;
         let toRow = 0;
 
-        if (isSingleKeyMove) {
+        if (isSingleKeyMove && !e.ctrlKey) {
           // single cell move: (Arrow{Up/ArrowDown/ArrowLeft/ArrowRight})
           if (e.key === 'ArrowLeft') {
             dCell -= dirCell;
@@ -239,9 +245,17 @@ export class SlickCellSelectionModel {
             this._prevSelectedRow = active.row;
           }
 
-          if (e.key === 'Home') {
+          if (e.shiftKey && !e.ctrlKey && e.key === 'Home') {
+            toCell = 0;
+            toRow = active.row;
+          } else if (e.shiftKey && !e.ctrlKey && e.key === 'End') {
+            toCell = colLn - 1;
+            toRow = active.row;
+          } else if (e.ctrlKey && e.shiftKey && e.key === 'Home') {
+            toCell = 0;
             toRow = 0;
-          } else if (e.key === 'End') {
+          } else if (e.ctrlKey && e.shiftKey && e.key === 'End') {
+            toCell = colLn - 1;
             toRow = dataLn - 1;
           } else if (e.key === 'PageUp') {
             if (this._prevSelectedRow >= 0) {
@@ -262,7 +276,8 @@ export class SlickCellSelectionModel {
         }
 
         // define new selection range
-        const newLast = new Slick.Range(active.row, active.cell, toRow, active.cell + dirCell * dCell);
+        toCell ??= active.cell + dirCell * dCell;
+        const newLast = new Slick.Range(active.row, active.cell, toRow, toCell);
         if (this.removeInvalidRanges([newLast]).length) {
           ranges.push(newLast);
           const viewRow = dirRow > 0 ? newLast.toRow : newLast.fromRow;

packages/utils/src/utils.spec.ts

@@ -9,6 +9,7 @@ import {
   objectAssignAndExtend,
   emptyObject,
   hasData,
+  isDefined,
   isEmptyObject,
   isNumber,
   isPrimitiveValue,
@@ -98,6 +99,26 @@ describe('Service/Utilies', () => {
     });
   });
 
+  describe('isDefined method', () => {
+    it('should be truthy when comparing against any defined variable', () => {
+      const result1 = isDefined({ firstName: 'John', lastName: 'Doe' });
+      const result2 = isDefined('hello');
+
+      expect(result1).toBeTruthy();
+      expect(result2).toBeTruthy();
+    });
+
+    it('should be falsy when comparing against empty string, null or undefined', () => {
+      const result1 = isDefined('');
+      const result2 = isDefined(null);
+      const result3 = isDefined(undefined);
+
+      expect(result1).toBeFalsy();
+      expect(result2).toBeFalsy();
+      expect(result3).toBeFalsy();
+    });
+  });
+
   describe('isEmptyObject method', () => {
     it('should return True when comparing against an object that has properties', () => {
       const result = isEmptyObject({ firstName: 'John', lastName: 'Doe' });

packages/utils/src/utils.ts

@@ -195,6 +195,10 @@ export function isEmptyObject(obj: any): boolean {
   return Object.entries(obj).length === 0;
 }
 
+export function isDefined<T>(value: T | undefined | null): value is T {
+  return <T>value !== undefined && <T>value !== null && <T>value !== '';
+}
+
 /**
  * Simple object check.
  * @param item

packages/vanilla-bundle/package.json

@@ -66,7 +66,7 @@
     "@slickgrid-universal/utils": "workspace:~",
     "dequal": "^2.0.3",
     "flatpickr": "^4.6.13",
-    "slickgrid": "^4.1.5",
+    "slickgrid": "^4.1.6",
     "sortablejs": "^1.15.1",
     "whatwg-fetch": "^3.6.19"
   },