Skip to content

Commit

Permalink
fix: cell selection range with key combos were incorrect (#1244)
Browse files Browse the repository at this point in the history 
* fix: revamp all cell selection range with key combos
the previous key combo were incorrect
  • Loading branch information
@ghiscoding
ghiscoding authored Dec 2, 2023
1 parent adf2054 commit 79d86fe
Show file tree
Hide file tree
Showing 12 changed files with 296 additions and 77 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/common/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
99 changes: 81 additions & 18 deletions packages/common/src/extensions/__tests__/slickCellSelectionModel.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand All @@ -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(),
Expand All @@ -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(),
Expand Down Expand Up @@ -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([{
Expand All @@ -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([{
Expand All @@ -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([{
Expand All @@ -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([{
Expand All @@ -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 = [
Expand Down Expand Up @@ -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 = [
Expand All @@ -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 = [
Expand All @@ -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 = [
Expand All @@ -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 = [
Expand All @@ -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 });
Expand All @@ -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 });
Expand All @@ -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 = [
Expand Down
31 changes: 23 additions & 8 deletions packages/common/src/extensions/slickCellSelectionModel.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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([]);
}
Expand All @@ -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) {
Expand All @@ -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));
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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;
Expand Down
21 changes: 21 additions & 0 deletions packages/utils/src/utils.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
objectAssignAndExtend,
emptyObject,
hasData,
isDefined,
isEmptyObject,
isNumber,
isPrimitiveValue,
Expand Down Expand Up @@ -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' });
Expand Down
4 changes: 4 additions & 0 deletions packages/utils/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/vanilla-bundle/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
Loading

0 comments on commit 79d86fe

Please sign in to comment.