Skip to content

fix: auto-scroll outside the viewport should stop Row Move dragging #2053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ghiscoding merged 5 commits into from
Jul 15, 2025
Merged

fix: auto-scroll outside the viewport should stop Row Move dragging #2053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d5aa31e
fix: auto-scroll outside the viewport should stop Row Move dragging
ghiscoding Jul 15, 2025
30a392b
chore: listen to mouseup on window instead of document.body
ghiscoding Jul 15, 2025
03aa9ff
chore: fix failing unit tests
ghiscoding Jul 15, 2025
6b3e2d4
chore: fix test coverage
ghiscoding Jul 15, 2025
31e46ce
chore: add comment explaining code
ghiscoding Jul 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/common/src/core/__tests__/slickGrid.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2880,7 +2880,7 @@ describe('SlickGrid core file', () => {
Object.defineProperty(bodyMouseMoveEvent, 'target', { writable: true, value: slickCellElm });
container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);

expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).not.toHaveBeenCalled();
Expand All @@ -2906,7 +2906,7 @@ describe('SlickGrid core file', () => {

container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent1);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);

expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).not.toHaveBeenCalled();
Expand Down Expand Up @@ -2938,7 +2938,7 @@ describe('SlickGrid core file', () => {

container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent1);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);

expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).toHaveBeenCalled();
Expand Down Expand Up @@ -2969,7 +2969,7 @@ describe('SlickGrid core file', () => {

container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);

expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).toHaveBeenCalled();
Expand Down Expand Up @@ -3014,7 +3014,7 @@ describe('SlickGrid core file', () => {

container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);
expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).toHaveBeenCalled();
expect(onDragSpy).toHaveBeenCalled();
Expand Down Expand Up @@ -3073,7 +3073,7 @@ describe('SlickGrid core file', () => {

container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);
expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).toHaveBeenCalled();
expect(onDragSpy).toHaveBeenCalled();
Expand Down Expand Up @@ -3116,7 +3116,7 @@ describe('SlickGrid core file', () => {

container.dispatchEvent(cMouseDownEvent);
document.body.dispatchEvent(bodyMouseMoveEvent);
document.body.dispatchEvent(bodyMouseUpEvent);
window.dispatchEvent(bodyMouseUpEvent);

expect(onDragInitSpy).toHaveBeenCalled();
expect(onDragStartSpy).not.toHaveBeenCalled();
Expand Down
12 changes: 7 additions & 5 deletions packages/common/src/core/__tests__/slickInteractions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ describe('Draggable class', () => {
});

it('should trigger mousedown and expect a dragInit and a dragStart and drag to all happen since it was triggered by an allowed element and we did move afterward', () => {
const removeListenerSpy = vi.spyOn(document.body, 'removeEventListener');
const removeBodyListenerSpy = vi.spyOn(document.body, 'removeEventListener');
const removeWindowListenerSpy = vi.spyOn(window, 'removeEventListener');
const dragInitSpy = vi.fn();
const dragSpy = vi.fn();
const dragStartSpy = vi.fn();
Expand Down Expand Up @@ -100,7 +101,7 @@ describe('Draggable class', () => {
Object.defineProperty(muEvt, 'clientX', { writable: true, configurable: true, value: 12 });
Object.defineProperty(muEvt, 'clientY', { writable: true, configurable: true, value: 10 });
document.body.dispatchEvent(mmEvt);
document.body.dispatchEvent(muEvt);
window.dispatchEvent(muEvt);

expect(dg).toBeTruthy();
expect(dragInitSpy).toHaveBeenCalledWith(mdEvt, {
Expand All @@ -110,12 +111,13 @@ describe('Draggable class', () => {
deltaY: 0,
dragHandle: containerElement,
dragSource: containerElement,
target: document.body,
target: window,
});
expect(dragStartSpy).toHaveBeenCalled(); // TODO: revisit calledWith X/Y pos, after migrating to TS class
expect(dragSpy).toHaveBeenCalled();
expect(dragEndSpy).toHaveBeenCalled();
expect(removeListenerSpy).toHaveBeenCalledTimes(5 * 2);
expect(removeBodyListenerSpy).toHaveBeenCalledTimes(2 * 2); // 2x events
expect(removeWindowListenerSpy).toHaveBeenCalledTimes(3 * 2); // 3x events
});

it('should NOT trigger dragInit,dragStart events when user is pressing mousedown and mousemove + Meta key combo that we considered as forbidden via "preventDragFromKeys"', async () => {
Expand Down Expand Up @@ -148,7 +150,7 @@ describe('Draggable class', () => {
Object.defineProperty(muEvt, 'clientX', { writable: true, configurable: true, value: 12 });
Object.defineProperty(muEvt, 'clientY', { writable: true, configurable: true, value: 10 });
document.body.dispatchEvent(mmEvt);
document.body.dispatchEvent(muEvt);
window.dispatchEvent(muEvt);

expect(dg).toBeTruthy();
expect(dragInitSpy).not.toHaveBeenCalledWith(mdEvt, {
Expand Down
13 changes: 7 additions & 6 deletions packages/common/src/core/slickInteractions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,10 @@ export function Draggable(options: DraggableOption): {
if (result !== false) {
document.body.addEventListener('mousemove', userMoved);
document.body.addEventListener('touchmove', userMoved);
document.body.addEventListener('mouseup', userReleased);
document.body.addEventListener('touchend', userReleased);
document.body.addEventListener('touchcancel', userReleased);
// register mouseup/... events on the window object so that we can catch them even if the user moves the mouse outside the container element
window.addEventListener('mouseup', userReleased);
window.addEventListener('touchend', userReleased);
window.addEventListener('touchcancel', userReleased);
}
}
}
Expand All @@ -140,9 +141,9 @@ export function Draggable(options: DraggableOption): {
function userReleased(event: MouseEvent | TouchEvent): void {
document.body.removeEventListener('mousemove', userMoved);
document.body.removeEventListener('touchmove', userMoved);
document.body.removeEventListener('mouseup', userReleased);
document.body.removeEventListener('touchend', userReleased);
document.body.removeEventListener('touchcancel', userReleased);
window.removeEventListener('mouseup', userReleased);
window.removeEventListener('touchend', userReleased);
window.removeEventListener('touchcancel', userReleased);

// trigger a dragEnd event only after dragging started and stopped
if (dragStarted) {
Expand Down
6 changes: 6 additions & 0 deletions packages/common/src/extensions/__tests__/slickCellRangeSelector.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,7 @@ describe('CellRangeSelector Plugin', () => {
} as any);
const getCellFromPointSpy = vi.spyOn(gridStub, 'getCellFromPoint');
const onCellRangeSelectingSpy = vi.spyOn(plugin.onCellRangeSelecting, 'notify');
const stopIntervalSpy = vi.spyOn(plugin, 'stopIntervalTimer');

plugin.init(gridStub);
plugin.addonOptions.minIntervalToShowNextCell = 5;
Expand Down Expand Up @@ -634,6 +635,11 @@ describe('CellRangeSelector Plugin', () => {

vi.advanceTimersByTime(7);
expect(onCellRangeSelectingSpy).not.toHaveBeenCalled();

// calling dragEnd without a range should still call the stopIntervalTimer() which simulate auto-dragging outside the viewport
const dragEventEnd = addVanillaEventPropagation(new Event('dragEnd'));
gridStub.onDragEnd.notify({ startX: 3, startY: 4, grid: gridStub } as any, dragEventEnd, gridStub);
expect(stopIntervalSpy).toHaveBeenCalled();
});

it('should call onDrag and handle drag outside the viewport and expect drag to be moved to a new position', () => {
Expand Down
4 changes: 3 additions & 1 deletion packages/common/src/extensions/slickCellRangeSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,14 +321,16 @@ export class SlickCellRangeSelector {
protected handleDragEnd(e: any, dd: DragRowMove): void {
this._decorator.hide();

if (this._dragging) {
if (this._dragging && dd.range) {
this._dragging = false;
e.stopImmediatePropagation();

this.stopIntervalTimer();
this.onCellRangeSelected.notify({
range: new SlickRange(dd.range.start.row ?? 0, dd.range.start.cell ?? 0, dd.range.end.row, dd.range.end.cell),
});
} else if (this._autoScrollTimerId) {
this.stopIntervalTimer(); // stop the auto-scroll timer if it was running
}
}

Expand Down