From b156bfaf1b0774a5a8d24a42e46aaed77eb94344 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Fri, 23 Aug 2024 20:20:20 -0400 Subject: [PATCH 1/3] fix: SlickCellExternalCopyManager should work w/hidden cols fixes #1634 --- packages/common/src/core/slickGrid.ts | 6 ++-- .../slickCellExternalCopyManager.ts | 28 ++++++++++++------- .../src/extensions/slickCellSelectionModel.ts | 2 +- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/packages/common/src/core/slickGrid.ts b/packages/common/src/core/slickGrid.ts index a57e6e201..6d84c7045 100644 --- a/packages/common/src/core/slickGrid.ts +++ b/packages/common/src/core/slickGrid.ts @@ -4150,7 +4150,7 @@ export class SlickGrid = Column, O e const d = this.getDataItem(row); - // TODO: shorten this loop (index? heuristics? binary search?) + // TODO: shorten this loop (index? heuristics? binary search?) for (let i = 0, ii = this.columns.length; i < ii; i++) { if (!this.columns[i] || this.columns[i].hidden) { continue; } @@ -5104,7 +5104,7 @@ export class SlickGrid = Column, O e let w = 0; for (let i = 0; i < this.columns.length && w <= x; i++) { - if (!this.columns[i] || this.columns[i].hidden) { + if (!this.columns[i]) { continue; } w += this.columns[i].width as number; @@ -5232,7 +5232,7 @@ export class SlickGrid = Column, O e const y2 = y1 + this._options.rowHeight! - 1; let x1 = 0; for (let i = 0; i < cell; i++) { - if (!this.columns[i] || this.columns[i].hidden) { + if (!this.columns[i]) { continue; } diff --git a/packages/common/src/extensions/slickCellExternalCopyManager.ts b/packages/common/src/extensions/slickCellExternalCopyManager.ts index de47f9cbd..dd0cfe0b2 100644 --- a/packages/common/src/extensions/slickCellExternalCopyManager.ts +++ b/packages/common/src/extensions/slickCellExternalCopyManager.ts @@ -1,7 +1,7 @@ import { createDomElement, getHtmlStringOutput, stripTags } from '@slickgrid-universal/utils'; import { DataWrapperService } from '../services/dataWrapperService'; -import type { Column, Editor, EditorConstructor, ElementPosition, ExcelCopyBufferOption, ExternalCopyClipCommand, OnEventArgs } from '../interfaces/index'; +import type { Column, CssStyleHash, Editor, EditorConstructor, ElementPosition, ExcelCopyBufferOption, ExternalCopyClipCommand, OnEventArgs } from '../interfaces/index'; import { type SlickDataView, SlickEvent, SlickEventData, SlickEventHandler, type SlickGrid, SlickRange, Utils as SlickUtils } from '../core/index'; // using external SlickGrid JS libraries @@ -311,31 +311,40 @@ export class SlickCellExternalCopyManager { maxDestX: this._grid.getColumns().length, h: 0, w: 0, - execute: () => { clipCommand.h = 0; for (let y = 0; y < clipCommand.destH; y++) { clipCommand.oldValues[y] = []; clipCommand.w = 0; clipCommand.h++; + let xOffset = 0; // the x offset for hidden col + for (let x = 0; x < clipCommand.destW; x++) { - clipCommand.w++; const desty = activeRow + y; const destx = activeCell + x; + const column = columns[destx]; + + // paste on hidden column will be skipped, but we need to paste 1 cell further on X axis + // we'll increase our X and increase the offset` + if (column.hidden) { + clipCommand.destW++; + xOffset++; + continue; + } + clipCommand.w++; if (desty < clipCommand.maxDestY && destx < clipCommand.maxDestX) { - // const nd = this._grid.getCellNode(desty, destx); const dt = this._dataWrapper.getDataItem(desty); - if (this._grid.triggerEvent(this.onBeforePasteCell, { row: desty, cell: destx, dt, column: columns[destx], target: 'grid' }).getReturnValue() === false) { + if (this._grid.triggerEvent(this.onBeforePasteCell, { row: desty, cell: destx, dt, column, target: 'grid' }).getReturnValue() === false) { continue; } - clipCommand.oldValues[y][x] = dt[columns[destx]['field']]; + clipCommand.oldValues[y][x - xOffset] = dt[column['field']]; if (oneCellToMultiple) { - this.setDataItemValueForColumn(dt, columns[destx], clippedRange[0][0]); + this.setDataItemValueForColumn(dt, column, clippedRange[0][0]); } else { - this.setDataItemValueForColumn(dt, columns[destx], clippedRange[y] ? clippedRange[y][x] : ''); + this.setDataItemValueForColumn(dt, column, clippedRange[y] ? clippedRange[y][x - xOffset] : ''); } this._grid.updateCell(desty, destx); this._grid.onCellChange.notify({ @@ -359,7 +368,6 @@ export class SlickCellExternalCopyManager { this._grid.getSelectionModel()?.setSelectedRanges([bRange]); this.onPasteCells.notify({ ranges: [bRange] }); }, - undo: () => { for (let y = 0; y < clipCommand.destH; y++) { for (let x = 0; x < clipCommand.destW; x++) { @@ -522,7 +530,7 @@ export class SlickCellExternalCopyManager { this.clearCopySelection(); const columns = this._grid.getColumns(); - const hash: any = {}; + const hash: CssStyleHash = {}; for (const range of ranges) { for (let j = range.fromRow; j <= range.toRow; j++) { hash[j] = {}; diff --git a/packages/common/src/extensions/slickCellSelectionModel.ts b/packages/common/src/extensions/slickCellSelectionModel.ts index 0e1ce5886..420df16a1 100644 --- a/packages/common/src/extensions/slickCellSelectionModel.ts +++ b/packages/common/src/extensions/slickCellSelectionModel.ts @@ -33,7 +33,7 @@ export class SlickCellSelectionModel implements SelectionModel { this._selector = (options === undefined || options.cellRangeSelector === undefined) ? new SlickCellRangeSelector({ selectionCss: { border: '2px solid black' } as CSSStyleDeclaration }) - : this._selector = options.cellRangeSelector; + : options.cellRangeSelector; this._addonOptions = options; } From 3cfde4c55ac07608f30aa927f37539c11656d64a Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Fri, 23 Aug 2024 20:30:08 -0400 Subject: [PATCH 2/3] chore: fix failing unit tests --- packages/common/src/core/__tests__/slickGrid.spec.ts | 7 +++---- packages/common/src/core/slickGrid.ts | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/common/src/core/__tests__/slickGrid.spec.ts b/packages/common/src/core/__tests__/slickGrid.spec.ts index 541d6b0f6..626c513c9 100644 --- a/packages/common/src/core/__tests__/slickGrid.spec.ts +++ b/packages/common/src/core/__tests__/slickGrid.spec.ts @@ -4392,15 +4392,14 @@ describe('SlickGrid core file', () => { expect(result5).toEqual({ cell: -1, row: -1 }); }); - it('should skip a cell when column found at x/y coordinates is hidden (Gender) so cell will the last known visible column', () => { + it('should return hidden cells as well since skipping them would add an unwanted offset', () => { grid = new SlickGrid(container, data, columns, { ...defaultOptions, enableCellNavigation: true }); const result1 = grid.getCellFromPoint((DEFAULT_COLUMN_WIDTH * 5) + 5, (DEFAULT_COLUMN_HEIGHT * 5) + 5); const result2 = grid.getCellFromPoint((DEFAULT_COLUMN_WIDTH * 6) + 5, (DEFAULT_COLUMN_HEIGHT * 6) + 5); - // last 2 columns are hidden and last visible column is cell:4 - expect(result1).toEqual({ cell: 4, row: 5 }); - expect(result2).toEqual({ cell: 4, row: 6 }); + expect(result1).toEqual({ cell: 5, row: 5 }); + expect(result2).toEqual({ cell: 6, row: 6 }); }); }); diff --git a/packages/common/src/core/slickGrid.ts b/packages/common/src/core/slickGrid.ts index 6d84c7045..964fe8e3d 100644 --- a/packages/common/src/core/slickGrid.ts +++ b/packages/common/src/core/slickGrid.ts @@ -5232,7 +5232,7 @@ export class SlickGrid = Column, O e const y2 = y1 + this._options.rowHeight! - 1; let x1 = 0; for (let i = 0; i < cell; i++) { - if (!this.columns[i]) { + if (!this.columns[i] || this.columns[i].hidden) { continue; } From 7b485962024a4ce336ebe80d223ddba4b7e80824 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Fri, 23 Aug 2024 22:42:11 -0400 Subject: [PATCH 3/3] chore: add missing unit test coverage --- .../slickCellExternalCopyManager.spec.ts | 49 ++++++++++++++++++- .../slickCellExternalCopyManager.ts | 24 +++++---- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/packages/common/src/extensions/__tests__/slickCellExternalCopyManager.spec.ts b/packages/common/src/extensions/__tests__/slickCellExternalCopyManager.spec.ts index 5026f9ed3..81c01e8de 100644 --- a/packages/common/src/extensions/__tests__/slickCellExternalCopyManager.spec.ts +++ b/packages/common/src/extensions/__tests__/slickCellExternalCopyManager.spec.ts @@ -35,7 +35,7 @@ const gridStub = { getColumns: jest.fn().mockReturnValue([ { id: 'firstName', field: 'firstName', name: 'First Name', }, { id: 'lastName', field: 'lastName', name: 'Last Name' }, - ]), + ] as Column[]), getData: () => dataViewStub, getDataItem: jest.fn(), getDataLength: jest.fn(), @@ -688,6 +688,53 @@ describe('CellExternalCopyManager', () => { done(); }, 2); }); + + it('should copy selection but skip hidden column and then use window.clipboard when exist and Paste is performed', (done) => { + let clipCommand; + const clipboardCommandHandler = (cmd) => { + clipCommand = cmd; + cmd.execute(); + }; + const mockColumns = [ + { id: 'firstName', field: 'firstName', name: 'First Name', editor: { model: Editors.text }, editorClass: Editors.text }, + { id: 'lastName', field: 'lastName', name: lastNameElm, hidden: true, }, + { id: 'age', field: 'age', name: 'Age', editor: { model: Editors.text }, editorClass: Editors.text }, + { id: 'gender', field: 'gender', name: 'Gender' }, + ] as Column[]; + jest.spyOn(gridStub, 'getColumns').mockReturnValue(mockColumns); + jest.spyOn(gridStub.getSelectionModel() as SelectionModel, 'getSelectedRanges').mockReturnValueOnce([new SlickRange(0, 1, 1, 2)]).mockReturnValueOnce(null as any); + plugin.init(gridStub, { clipboardPasteDelay: 1, clearCopySelectionDelay: 1, includeHeaderWhenCopying: true, clipboardCommandHandler }); + + const keyDownCtrlCopyEvent = new Event('keydown'); + Object.defineProperty(keyDownCtrlCopyEvent, 'ctrlKey', { writable: true, configurable: true, value: true }); + Object.defineProperty(keyDownCtrlCopyEvent, 'key', { writable: true, configurable: true, value: 'c' }); + Object.defineProperty(keyDownCtrlCopyEvent, 'isPropagationStopped', { writable: true, configurable: true, value: jest.fn() }); + Object.defineProperty(keyDownCtrlCopyEvent, 'isImmediatePropagationStopped', { writable: true, configurable: true, value: jest.fn() }); + gridStub.onKeyDown.notify({ cell: 0, row: 0, grid: gridStub }, keyDownCtrlCopyEvent, gridStub); + + const updateCellSpy = jest.spyOn(gridStub, 'updateCell'); + const onCellChangeSpy = jest.spyOn(gridStub.onCellChange, 'notify'); + const getActiveCellSpy = jest.spyOn(gridStub, 'getActiveCell').mockReturnValue({ cell: 0, row: 1 }); + const keyDownCtrlPasteEvent = new Event('keydown'); + jest.spyOn(gridStub, 'getColumns').mockReturnValue(mockColumns); + Object.defineProperty(keyDownCtrlPasteEvent, 'ctrlKey', { writable: true, configurable: true, value: true }); + Object.defineProperty(keyDownCtrlPasteEvent, 'key', { writable: true, configurable: true, value: 'v' }); + Object.defineProperty(keyDownCtrlPasteEvent, 'isPropagationStopped', { writable: true, configurable: true, value: jest.fn() }); + Object.defineProperty(keyDownCtrlPasteEvent, 'isImmediatePropagationStopped', { writable: true, configurable: true, value: jest.fn() }); + gridStub.onKeyDown.notify({ cell: 0, row: 0, grid: gridStub }, keyDownCtrlPasteEvent, gridStub); + document.querySelector('textarea')!.value = `Doe\tserialized output`; + + setTimeout(() => { + expect(getActiveCellSpy).toHaveBeenCalled(); + expect(updateCellSpy).toHaveBeenCalledWith(1, 0); + expect(updateCellSpy).not.toHaveBeenCalledWith(1, 1); + expect(onCellChangeSpy).toHaveBeenCalledWith({ row: 1, cell: 2, item: { firstName: 'John', lastName: 'Doe' }, grid: gridStub, column: {} }); + const getDataItemSpy = jest.spyOn(gridStub, 'getDataItem'); + clipCommand.undo(); + expect(getDataItemSpy).toHaveBeenCalled(); + done(); + }, 2); + }); }); }); }); \ No newline at end of file diff --git a/packages/common/src/extensions/slickCellExternalCopyManager.ts b/packages/common/src/extensions/slickCellExternalCopyManager.ts index dd0cfe0b2..b31cc9387 100644 --- a/packages/common/src/extensions/slickCellExternalCopyManager.ts +++ b/packages/common/src/extensions/slickCellExternalCopyManager.ts @@ -467,22 +467,26 @@ export class SlickCellExternalCopyManager { if (clipTextRows.length === 0 && this._addonOptions.includeHeaderWhenCopying) { const clipTextHeaders: string[] = []; for (let j = range.fromCell; j < range.toCell + 1; j++) { - const colName: string = columns[j].name instanceof HTMLElement - ? stripTags((columns[j].name as HTMLElement).innerHTML) - : columns[j].name as string; - if (colName.length > 0 && !columns[j].hidden) { - clipTextHeaders.push(this.getHeaderValueForColumn(columns[j])); + if (columns[j]) { + const colName: string = columns[j].name instanceof HTMLElement + ? stripTags((columns[j].name as HTMLElement).innerHTML) + : columns[j].name as string; + if (colName.length > 0 && !columns[j].hidden) { + clipTextHeaders.push(this.getHeaderValueForColumn(columns[j])); + } } } clipTextRows.push(clipTextHeaders.join('\t')); } for (let j = range.fromCell; j < range.toCell + 1; j++) { - const colName: string = columns[j].name instanceof HTMLElement - ? stripTags((columns[j].name as HTMLElement).innerHTML) - : columns[j].name as string; - if (colName.length > 0 && !columns[j].hidden) { - clipTextCells.push(this.getDataItemValueForColumn(dt, columns[j], i, j, e)); + if (columns[j]) { + const colName: string = columns[j].name instanceof HTMLElement + ? stripTags((columns[j].name as HTMLElement).innerHTML) + : columns[j].name as string; + if (colName.length > 0 && !columns[j].hidden) { + clipTextCells.push(this.getDataItemValueForColumn(dt, columns[j], i, j, e)); + } } } clipTextRows.push(clipTextCells.join('\t'));