fix(common): Excel copy cell ranges shouldn't lose its cell focus (#901)

* fix(common): Excel copy cell ranges shouldn't lose its cell focus - when using Excel copy buffer to copy cell ranges, the cell loses its focus after the copy execution, so we need to reapply the focus on the active cell that the user clicked
ghiscoding · Feb 16, 2023 · 1dc8b76 · 1dc8b76
1 parent 66c8b4d
commit 1dc8b76
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 75 deletions.
diff --git a/examples/webpack-demo-vanilla-bundle/src/examples/example13.ts b/examples/webpack-demo-vanilla-bundle/src/examples/example13.ts
@@ -1,6 +1,8 @@
 import {
   BindingEventService,
   Column,
+  Editors,
+  FieldType,
   GridOption,
 } from '@slickgrid-universal/common';
 import { ExcelExportService } from '@slickgrid-universal/excel-export';
@@ -59,6 +61,9 @@ export class Example13 {
       enableAutoResize: true,
       enableHeaderButton: true,
       enableHeaderMenu: false,
+      autoCommitEdit: true,
+      autoEdit: true,
+      editable: true,
       autoResize: {
         container: '.demo-container',
       },
@@ -124,9 +129,11 @@ export class Example13 {
         id: i,
         name: 'Column ' + String.fromCharCode('A'.charCodeAt(0) + i),
         field: i + '',
-        width: i === 0 ? 70 : 100, // have the 2 first columns wider
+        width: i === 0 ? 70 : 100, // make the first 2 columns wider
         filterable: true,
         sortable: true,
+        type: FieldType.number,
+        editor: { model: Editors.integer },
         formatter: (_row, _cell, value, columnDef) => {
           if (gridNo === 1 && columns1WithHighlightingById[columnDef.id] && value < 0) {
             return `<div style="color:red; font-weight:bold;">${value}</div>`;

diff --git a/package.json b/package.json
@@ -75,7 +75,7 @@
     "rimraf": "^3.0.2",
     "rxjs": "^7.5.7",
     "servor": "^4.0.2",
-    "slickgrid": "^3.0.3",
+    "slickgrid": "^3.0.4",
     "sortablejs": "^1.15.0",
     "ts-jest": "^29.0.5",
     "ts-node": "^10.9.1",

diff --git a/packages/common/package.json b/packages/common/package.json
@@ -82,7 +82,7 @@
     "jquery": "^3.6.3",
     "moment-mini": "^2.29.4",
     "multiple-select-modified": "^1.3.17",
-    "slickgrid": "^3.0.3",
+    "slickgrid": "^3.0.4",
     "sortablejs": "^1.15.0",
     "un-flatten-tree": "^2.0.12"
   },

diff --git a/packages/common/src/services/__tests__/gridEvent.service.spec.ts b/packages/common/src/services/__tests__/gridEvent.service.spec.ts
@@ -179,46 +179,22 @@ describe('GridEventService', () => {
       });
     });
 
-    it('should execute the column "onCellClick" callback method and "setActiveCell" cell navigation is enabled but column is not editable', () => {
+    it('should execute the column "onCellClick" callback method and "setActiveCell" only when enableExcelCopyBuffer is enabled', () => {
       gridStub.getOptions = jest.fn();
-      jest.spyOn(gridStub, 'getOptions').mockReturnValue({ enableCellNavigation: true, editable: false });
+      jest.spyOn(gridStub, 'getOptions').mockReturnValue({ enableCellNavigation: true, enableExcelCopyBuffer: true });
       const spyActive = jest.spyOn(gridStub, 'setActiveCell');
       const spyGetCols = jest.spyOn(gridStub, 'getColumns').mockReturnValue([mockColumn]);
       const spyGetData = jest.spyOn(gridStub, 'getDataItem').mockReturnValue(mockRowData);
       const spyOnChange = jest.spyOn(mockColumn, 'onCellClick');
 
       service.bindOnClick(gridStub);
-      gridStub.onClick.notify({ cell: 0, row: 0, grid: gridStub }, new Slick.EventData(), gridStub);
+      gridStub.onClick.notify({ cell: 0, row: 2, grid: gridStub }, new Slick.EventData(), gridStub);
 
-      expect(spyActive).toHaveBeenCalled();
+      expect(spyActive).toHaveBeenCalledWith(2, 0);
       expect(spyGetCols).toHaveBeenCalled();
       expect(spyGetData).toHaveBeenCalled();
       expect(spyOnChange).toHaveBeenCalledWith(expect.anything(), {
-        row: 0,
-        cell: 0,
-        dataView: dataViewStub,
-        grid: gridStub,
-        columnDef: mockColumn,
-        dataContext: mockRowData
-      });
-    });
-
-    it('should execute the column "onCellClick" callback method and "setActiveCell" when cell is editable and autoCommitEdit', () => {
-      gridStub.getOptions = jest.fn();
-      jest.spyOn(gridStub, 'getOptions').mockReturnValue({ enableCellNavigation: true, editable: true, autoCommitEdit: true });
-      const spyActive = jest.spyOn(gridStub, 'setActiveCell');
-      const spyGetCols = jest.spyOn(gridStub, 'getColumns').mockReturnValue([mockColumn]);
-      const spyGetData = jest.spyOn(gridStub, 'getDataItem').mockReturnValue(mockRowData);
-      const spyOnChange = jest.spyOn(mockColumn, 'onCellClick');
-
-      service.bindOnClick(gridStub);
-      gridStub.onClick.notify({ cell: 0, row: 0, grid: gridStub }, new Slick.EventData(), gridStub);
-
-      expect(spyActive).toHaveBeenCalled();
-      expect(spyGetCols).toHaveBeenCalled();
-      expect(spyGetData).toHaveBeenCalled();
-      expect(spyOnChange).toHaveBeenCalledWith(expect.anything(), {
-        row: 0,
+        row: 2,
         cell: 0,
         dataView: dataViewStub,
         grid: gridStub,

diff --git a/packages/common/src/services/gridEvent.service.ts b/packages/common/src/services/gridEvent.service.ts
@@ -28,7 +28,7 @@ export class GridEventService {
 
   /* OnCellChange Event */
   bindOnBeforeEditCell(grid: SlickGrid) {
-    const dataView = grid?.getData && grid.getData() as SlickDataView;
+    const dataView = grid?.getData?.() as SlickDataView;
 
     // subscribe to this Slickgrid event of onBeforeEditCell
     this._eventHandler.subscribe(grid.onBeforeEditCell, (e, args) => {
@@ -57,7 +57,7 @@ export class GridEventService {
 
   /* OnCellChange Event */
   bindOnCellChange(grid: SlickGrid) {
-    const dataView = grid?.getData && grid.getData() as SlickDataView;
+    const dataView = grid?.getData?.() as SlickDataView;
 
     // subscribe to this Slickgrid event of onCellChange
     this._eventHandler.subscribe(grid.onCellChange, (e, args) => {
@@ -86,22 +86,19 @@ export class GridEventService {
 
   /* OnClick Event */
   bindOnClick(grid: SlickGrid) {
-    const dataView = grid?.getData && grid.getData() as SlickDataView;
+    const dataView = grid?.getData?.() as SlickDataView;
 
     this._eventHandler.subscribe(grid.onClick, (e, args) => {
       if (!e || !args || !grid || args.cell === undefined || !grid.getColumns || !grid.getDataItem) {
         return;
       }
-      const column: Column = grid && grid.getColumns && grid.getColumns()[args.cell];
-      const gridOptions: GridOption = grid && grid.getOptions && grid.getOptions() || {};
-
-      // only when the grid option "autoCommitEdit" is enabled, we will make the cell active (in focus) when clicked
-      // setting the cell as active as a side effect and if "autoCommitEdit" is set to false then the Editors won't save correctly
-      if (gridOptions.enableCellNavigation && (!gridOptions.editable || (gridOptions.editable && gridOptions.autoCommitEdit))) {
-        try {
-          grid.setActiveCell(args.row, args.cell, false, false, true);
-        // eslint-disable-next-line @typescript-eslint/no-shadow, no-empty
-        } catch(e) {}
+      const column: Column = grid.getColumns?.()[args.cell];
+      const gridOptions: GridOption = grid.getOptions?.() || {};
+
+      // when using Excel copy buffer to copy cell ranges, the cell loses its focus after the copy execution
+      // so we need to reapply the focus on the active cell that the user clicked
+      if (gridOptions.enableCellNavigation && gridOptions.enableExcelCopyBuffer) {
+        grid.setActiveCell(args.row, args.cell);
       }
 
       // if the column definition has a onCellClick property (a callback function), then run it

diff --git a/packages/vanilla-bundle/package.json b/packages/vanilla-bundle/package.json
@@ -66,7 +66,7 @@
     "dequal": "^2.0.3",
     "flatpickr": "^4.6.13",
     "jquery": "^3.6.3",
-    "slickgrid": "^3.0.3",
+    "slickgrid": "^3.0.4",
     "sortablejs": "^1.15.0",
     "whatwg-fetch": "^3.6.2"
   },

diff --git a/packages/vanilla-force-bundle/webpack.config.js b/packages/vanilla-force-bundle/webpack.config.js
@@ -41,9 +41,5 @@ module.exports = ({ production } = {}) => ({
         options: { loader: 'ts', target: 'es2018' }
       },
     ],
-  },
-  watchOptions: {
-    ignored: '**/node_modules',
-    poll: 1000, // Check for changes every second
   }
 });
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/test/cypress/e2e/example11.cy.ts b/test/cypress/e2e/example11.cy.ts
@@ -21,18 +21,17 @@ describe('Example 11 - Batch Editing', { retries: 1 }, () => {
     cy.get('h3').should('contain', 'Example 11 - Batch Editing');
   });
 
+  it('should click on "Clear Local Storage" and expect to be back to original grid with all the columns', () => {
+    cy.get('[data-test="clear-storage-btn"]')
+      .click();
 
-  it('should have exact Column Titles in the grid', () => {
     cy.get('.grid11')
       .find('.slick-header-columns')
       .children()
       .each(($child, index) => expect($child.text()).to.eq(fullTitles[index]));
   });
 
-  it('should click on "Clear Local Storage" and expect to be back to original grid with all the columns', () => {
-    cy.get('[data-test="clear-storage-btn"]')
-      .click();
-
+  it('should have exact Column Titles in the grid', () => {
     cy.get('.grid11')
       .find('.slick-header-columns')
       .children()
@@ -56,36 +55,41 @@ describe('Example 11 - Batch Editing', { retries: 1 }, () => {
       .should('contain', '0 day')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
 
-    cy.get('.editor-duration').type('1').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 1}px"] > .slick-cell:nth(2)`).click().type('1{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 1}px"] > .slick-cell:nth(2)`).should('contain', '1 day')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
 
-    cy.get('.editor-duration').type('2').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(2)`).click().type('2{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(2)`).should('contain', '2 days')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
 
-    cy.get('.editor-duration').type('3').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 3}px"] > .slick-cell:nth(2)`).click().type('3{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 3}px"] > .slick-cell:nth(2)`).should('contain', '3 days')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
-    cy.get('.editor-duration').type('{esc}');
+
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 3}px"] > .slick-cell:nth(2)`).click().type('{esc}');
     cy.get('.editor-duration').should('not.exist');
   });
 
   it('should be able to change "Title" values of row indexes 1-3', () => {
     // change title
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 1}px"] > .slick-cell:nth(1)`).should('contain', 'TASK 1').click();
-    cy.get('.editor-title').type('task 1111').type('{enter}', { force: true });
+    cy.get('.editor-title').type('task 1111').type('{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 1}px"] > .slick-cell:nth(1)`).should('contain', 'TASK 1111')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
 
-    cy.get('.editor-title').type('task 2222').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(1)`).click()
+      .type('task 2222{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(1)`).should('contain', 'TASK 2222')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
 
-    cy.get('.editor-title').type('task 3333').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 3}px"] > .slick-cell:nth(1)`).click()
+      .type('task 3333').type('{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 3}px"] > .slick-cell:nth(1)`).should('contain', 'TASK 3333')
       .should('have.css', 'background-color').and('eq', UNSAVED_RGB_COLOR);
-    cy.get('.editor-title').type('{esc}');
+
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 3}px"] > .slick-cell:nth(1)`).click()
+      .type('{esc}');
     cy.get('.editor-title').should('not.exist');
 
     cy.get('.slick-viewport.slick-viewport-top.slick-viewport-left')

diff --git a/test/cypress/e2e/example12.cy.ts b/test/cypress/e2e/example12.cy.ts
@@ -1,6 +1,6 @@
 import { changeTimezone, zeroPadding } from '../plugins/utilities';
 
-describe('Example 12 - Composite Editor Modal', { retries: 1 }, () => {
+describe('Example 12 - Composite Editor Modal', { retries: 0 }, () => {
   const fullPreTitles = ['', 'Common Factor', 'Analysis', 'Period', 'Item', ''];
   const fullTitles = ['', ' Title', 'Duration', 'Cost', '% Complete', 'Complexity', 'Start', 'Completed', 'Finish', 'Product', 'Country of Origin', 'Action'];
 
@@ -43,20 +43,20 @@ describe('Example 12 - Composite Editor Modal', { retries: 1 }, () => {
   it('should be able to change "Duration" values of first 4 rows', () => {
     // change duration
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 0}px"] > .slick-cell:nth(2)`).should('contain', 'days').click();
-    cy.get('.editor-duration').type('0').type('{enter}', { force: true });
+    cy.get('.editor-duration').type('0{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 0}px"] > .slick-cell:nth(2)`)
       .should('contain', '0 day')
       .get('.editing-field')
       .should('have.css', 'border')
       .and('contain', `solid ${UNSAVED_RGB_COLOR}`);
 
-    cy.get('.editor-duration').type('1').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 1}px"] > .slick-cell:nth(2)`).click().type('1{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 1}px"] > .slick-cell:nth(2)`).should('contain', '1 day')
       .get('.editing-field')
       .should('have.css', 'border')
       .and('contain', `solid ${UNSAVED_RGB_COLOR}`);
 
-    cy.get('.editor-duration').type('2').type('{enter}', { force: true });
+    cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(2)`).click().type('2{enter}');
     cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(2)`).should('contain', '2 days')
       .get('.editing-field')
       .should('have.css', 'border')