Skip to content

Commit

Permalink
fix: Edt cell mouseout should save & excel copy buffer should still w…
Browse files Browse the repository at this point in the history
…ork (#917)

- fixes a regression caused by previous PR #901 and identified in Angular-Slickgrid issue [1103](ghiscoding/Angular-Slickgrid#1103), the previous PR #901 was put in place to fix cell external copy in a bug reported in Slickgrid-React issue [36](ghiscoding/slickgrid-react#36)
- this bug actually came from a very old patch that was put in place via `suppressActiveCellChangeOnEdit: true` in SlickGrid [PR 243](6pac/SlickGrid#243) and this flag was to avoid trigger an event when the active cell changes, this event was being listened by SlickCellSelectionModel and when triggered was then sending another event with the cell ranges that changed, which then sent another event onSelectedRangesChanged which was itself listened by SlickCellExternalCopyManager and when triggered was calling a `grid.focus()` and when it did that, it was making the editor loses its focus (hence the implementation of `suppressActiveCellChangeOnEdit`) which is required by SlickCellExternalCopyManager to be able to copy & paste cell ranges. After investigating, I was able to remove the use of `suppressActiveCellChangeOnEdit` (now disabled globally) by simply calling `grid.focus()` in each editor prior to itself getting its own focus, so at least our focus remains in the grid and our editor no longer loses its focus and we are also able to finally use Editor + CellExternalCopyManager at the same time without conflict anymore
- hopefully this fixes all regressions and it also removes some old hacks (suppressActiveCellChangeOnEdit) that was put in place, in summary, it should be a lot cleaner now
  • Loading branch information
ghiscoding authored Feb 23, 2023
1 parent 40086db commit 18ba0fc
Show file tree
Hide file tree
Showing 29 changed files with 91 additions and 47 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"build:watch": "tsc --build ./tsconfig.packages.json --watch",
"predev": "pnpm build:esm:styles",
"dev": "run-p dev:watch webpack:watch",
"dev:watch": "lerna watch --no-bail --file-delimiter=\",\" --glob=\"src/**/*.{ts,scss}\" --ignored=\"src/**/*.spec.ts\" -- cross-env-shell pnpm run -r --filter $LERNA_PACKAGE_NAME dev",
"dev:watch": "lerna watch --no-bail --file-delimiter=\",\" --glob=\"src/**/*.{ts,scss}\" --ignored=\"**/*.spec.ts\" -- cross-env-shell pnpm run -r --filter $LERNA_PACKAGE_NAME dev",
"webpack:watch": "pnpm -r --parallel run webpack:dev",
"preview:publish": "lerna publish from-package --dry-run",
"preview:version": "lerna version --dry-run",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getOptions: () => gridOptionMock,
getColumns: jest.fn(),
Expand Down Expand Up @@ -265,6 +266,7 @@ describe('AutocompleterEditor', () => {
const spy = jest.spyOn(editorElm, 'focus');
editor.focus();

expect(gridStub.focus).toHaveBeenCalled();
expect(spy).toHaveBeenCalled();
});

Expand Down
2 changes: 2 additions & 0 deletions packages/common/src/editors/__tests__/checkboxEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getOptions: () => gridOptionMock,
getColumns: jest.fn(),
Expand Down Expand Up @@ -200,6 +201,7 @@ describe('CheckboxEditor', () => {
editor.focus();
editor.editorDomElement.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(false);
});
});
Expand Down
2 changes: 2 additions & 0 deletions packages/common/src/editors/__tests__/dateEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -190,6 +191,7 @@ describe('DateEditor', () => {
editor.show();
editor.focus();

expect(gridStub.focus).toHaveBeenCalled();
expect(calendarElm).toBeTruthy();
expect(spy).toHaveBeenCalled();
});
Expand Down
2 changes: 2 additions & 0 deletions packages/common/src/editors/__tests__/dualInputEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -215,6 +216,7 @@ describe('DualInputEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand Down
4 changes: 4 additions & 0 deletions packages/common/src/editors/__tests__/floatEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -178,6 +179,7 @@ describe('FloatEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand All @@ -191,6 +193,7 @@ describe('FloatEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand All @@ -205,6 +208,7 @@ describe('FloatEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
});

Expand Down
4 changes: 4 additions & 0 deletions packages/common/src/editors/__tests__/inputEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -188,6 +189,7 @@ describe('InputEditor (TextEditor)', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
expect(editor.isValueTouched()).toBe(true);
});
Expand All @@ -202,6 +204,7 @@ describe('InputEditor (TextEditor)', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand All @@ -216,6 +219,7 @@ describe('InputEditor (TextEditor)', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -188,6 +189,7 @@ describe('InputPasswordEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
expect(editor.isValueTouched()).toBe(true);
});
Expand All @@ -202,6 +204,7 @@ describe('InputPasswordEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand All @@ -216,6 +219,7 @@ describe('InputPasswordEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
});

Expand All @@ -229,6 +233,7 @@ describe('InputPasswordEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(false);
});

Expand All @@ -242,6 +247,7 @@ describe('InputPasswordEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
});
});
Expand Down
7 changes: 7 additions & 0 deletions packages/common/src/editors/__tests__/integerEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -180,6 +181,7 @@ describe('IntegerEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand All @@ -193,6 +195,7 @@ describe('IntegerEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(spyEvent).toHaveBeenCalled();
});

Expand All @@ -207,6 +210,7 @@ describe('IntegerEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
});

Expand All @@ -220,6 +224,7 @@ describe('IntegerEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(false);
});

Expand All @@ -233,6 +238,7 @@ describe('IntegerEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(false);
});

Expand All @@ -246,6 +252,7 @@ describe('IntegerEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
});
});
Expand Down
6 changes: 6 additions & 0 deletions packages/common/src/editors/__tests__/longTextEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -253,6 +254,7 @@ describe('LongTextEditor', () => {
editorElm.dispatchEvent(eventKeyDown);
editorElm.dispatchEvent(eventInput);

expect(gridStub.focus).toHaveBeenCalled();
expect(currentTextLengthElm.textContent).toBe('1');
expect(maxTextLengthElm.textContent).toBe('255');
expect(editor.isValueChanged()).toBe(true);
Expand All @@ -269,6 +271,7 @@ describe('LongTextEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(false);
expect(editor.isValueTouched()).toBe(true);
});
Expand All @@ -283,6 +286,7 @@ describe('LongTextEditor', () => {
editor.focus();
editorElm.dispatchEvent(event);

expect(gridStub.focus).toHaveBeenCalled();
expect(editor.isValueChanged()).toBe(true);
expect(editor.isValueTouched()).toBe(true);
});
Expand Down Expand Up @@ -742,6 +746,7 @@ describe('LongTextEditor', () => {
const currentTextLengthElm = document.body.querySelector('.editor-footer .text-length') as HTMLDivElement;
const maxTextLengthElm = document.body.querySelector('.editor-footer .max-length') as HTMLDivElement;

expect(gridStub.focus).toHaveBeenCalled();
expect(editorElm.value).toBe('some extra');
expect(currentTextLengthElm.textContent).toBe('10');
expect(maxTextLengthElm.textContent).toBe('10');
Expand All @@ -763,6 +768,7 @@ describe('LongTextEditor', () => {
const currentTextLengthElm = document.body.querySelector('.editor-footer .text-length') as HTMLDivElement;
const maxTextLengthElm = document.body.querySelector('.editor-footer .max-length') as HTMLDivElement;

expect(gridStub.focus).toHaveBeenCalled();
expect(editorElm.value).toBe('some extra');
expect(currentTextLengthElm.textContent).toBe('10');
expect(maxTextLengthElm.textContent).toBe('10');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getOptions: () => gridOptionMock,
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down
2 changes: 2 additions & 0 deletions packages/common/src/editors/__tests__/selectEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -146,6 +147,7 @@ describe('SelectEditor', () => {
editor.focus();
const editorCount = document.body.querySelectorAll('select.ms-filter.editor-gender').length;

expect(gridStub.focus).toHaveBeenCalled();
expect(editorCount).toBe(1);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getOptions: () => gridOptionMock,
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down
3 changes: 3 additions & 0 deletions packages/common/src/editors/__tests__/sliderEditor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const getEditorLockMock = {
};

const gridStub = {
focus: jest.fn(),
getActiveCell: jest.fn(),
getColumns: jest.fn(),
getEditorLock: () => getEditorLockMock,
Expand Down Expand Up @@ -421,9 +422,11 @@ describe('SliderEditor', () => {
const spy = jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit');

editor = new SliderEditor(editorArguments);
editor.focus();
editor.loadValue(mockItemData);
editor.save();

expect(gridStub.focus).toHaveBeenCalled();
expect(spy).toHaveBeenCalled();
});

Expand Down
3 changes: 3 additions & 0 deletions packages/common/src/editors/autocompleterEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,9 @@ export class AutocompleterEditor<T extends AutocompleteItem = any> implements Ed
}

focus() {
// always set focus on grid first so that plugin to copy range (SlickCellExternalCopyManager) would still be able to paste at that position
this.grid.focus();

if (this._inputElm) {
this._inputElm.focus();
this._inputElm.select();
Expand Down
3 changes: 3 additions & 0 deletions packages/common/src/editors/checkboxEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ export class CheckboxEditor implements Editor {
}

focus(): void {
// always set focus on grid first so that plugin to copy range (SlickCellExternalCopyManager) would still be able to paste at that position
this.grid.focus();

if (this._input) {
this._input.focus();
}
Expand Down
8 changes: 5 additions & 3 deletions packages/common/src/editors/dateEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,11 @@ export class DateEditor implements Editor {
}

focus() {
if (this._inputElm?.focus) {
this._inputElm.focus();
}
// always set focus on grid first so that plugin to copy range (SlickCellExternalCopyManager) would still be able to paste at that position
this.grid.focus();

this.show();
this._inputElm?.focus();
if (this._inputWithDataElm?.focus) {
this._inputWithDataElm.focus();
this._inputWithDataElm.select();
Expand Down
Loading

0 comments on commit 18ba0fc

Please sign in to comment.