Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
240 changes: 240 additions & 0 deletions packages/grid/src/Grid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1181,3 +1181,243 @@ describe('column separators', () => {
});
});
});

describe('grid-block-events cleanup', () => {
const resizableTheme = { ...defaultTheme, allowColumnResize: true };

function getColumnSeparatorX(columnIndex: VisibleIndex): number {
const { rowHeaderWidth, columnWidth } = resizableTheme;
return rowHeaderWidth + columnWidth * (columnIndex + 1) - 2;
}

function getColumnHeaderY(): number {
return Math.floor(resizableTheme.columnHeaderHeight / 2);
}
Comment on lines +1188 to +1195
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getColumnSeparatorX / getColumnHeaderY helpers are duplicated from the earlier "column separators" tests in this file. To reduce drift and make future updates easier, consider hoisting these helpers to a shared scope (or reusing the existing ones) instead of redefining them in each describe block.

Copilot uses AI. Check for mistakes.

function hasBlockEventsClass(): boolean {
return document.documentElement.classList.contains('grid-block-events');
}

beforeEach(() => {
// Ensure clean state before each test
document.documentElement.classList.remove('grid-block-events');
});

afterEach(() => {
// Clean up after each test
document.documentElement.classList.remove('grid-block-events');
});

it('should remove grid-block-events after normal drag and mouse up', () => {
const component = makeGridComponent(new MockGridModel(), resizableTheme);
const separatorX = getColumnSeparatorX(0);
const headerY = getColumnHeaderY();

expect(hasBlockEventsClass()).toBe(false);

// Start drag on column separator
mouseDown(0, 0, component, {}, separatorX, headerY);

// Drag to trigger handleMouseDrag which adds the class
fireEvent.mouseMove(window, {
clientX: separatorX + 50,
clientY: headerY,
});

expect(hasBlockEventsClass()).toBe(true);

// Normal mouse up should clean up
fireEvent.mouseUp(window, {
clientX: separatorX + 50,
clientY: headerY,
button: 0,
});

expect(hasBlockEventsClass()).toBe(false);
});

it('should remove grid-block-events when right-clicking during drag (case 1)', () => {
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test title is misleading: the test asserts grid-block-events remains after the right-click mouseup and is only removed after the subsequent left mouseup. Rename the test (or adjust assertions) so the title matches the behavior being verified.

Suggested change
it('should remove grid-block-events when right-clicking during drag (case 1)', () => {
it('should keep grid-block-events after right-click during drag until left mouse up (case 1)', () => {

Copilot uses AI. Check for mistakes.
const component = makeGridComponent(new MockGridModel(), resizableTheme);
const separatorX = getColumnSeparatorX(0);
const headerY = getColumnHeaderY();

// Start drag
mouseDown(0, 0, component, {}, separatorX, headerY);
fireEvent.mouseMove(window, {
clientX: separatorX + 50,
clientY: headerY,
});

expect(hasBlockEventsClass()).toBe(true);

// Right-click during drag (button=2)
fireEvent.mouseUp(window, {
clientX: separatorX + 50,
clientY: headerY,
button: 2,
});

// Right-click during drag returns early, class should still be present
// Then complete with left mouse up
fireEvent.mouseUp(window, {
clientX: separatorX + 50,
clientY: headerY,
button: 0,
});

expect(hasBlockEventsClass()).toBe(false);
});

it('should ignore middle-click during drag and continue dragging (case 2)', () => {
const component = makeGridComponent(new MockGridModel(), resizableTheme);
const separatorX = getColumnSeparatorX(0);
const headerY = getColumnHeaderY();

// Start drag
mouseDown(0, 0, component, {}, separatorX, headerY);
fireEvent.mouseMove(window, {
clientX: separatorX + 50,
clientY: headerY,
});

expect(hasBlockEventsClass()).toBe(true);

// Middle mouse button up during drag (button=1) should be ignored
// Drag should continue
fireEvent.mouseUp(window, {
clientX: separatorX + 50,
clientY: headerY,
button: 1,
});

// Class should still be present since drag is ongoing
expect(hasBlockEventsClass()).toBe(true);

// Left mouse up should properly end the drag and clean up
fireEvent.mouseUp(window, {
clientX: separatorX + 50,
clientY: headerY,
button: 0,
});

expect(hasBlockEventsClass()).toBe(false);
});

it('should handle addDocumentCursor with null cursor (case 3)', () => {
const component = makeGridComponent(new MockGridModel(), resizableTheme);

expect(hasBlockEventsClass()).toBe(false);

// First add a real cursor - this sets documentCursor and adds grid-block-events
component.addDocumentCursor('move');
expect(hasBlockEventsClass()).toBe(true);

// Now simulate cursor being cleared to null (e.g., mouse handler sets cursor = null)
// This sets documentCursor to null but keeps grid-block-events
component.addDocumentCursor(null);

// grid-block-events should still be present since it was added
expect(hasBlockEventsClass()).toBe(true);

// removeDocumentCursor should still clean up
// BUG: Currently it doesn't because documentCursor is null and
// the entire function body is guarded by `if (this.documentCursor != null)`
component.removeDocumentCursor();

// After fix, this should be false
Comment on lines +1322 to +1327
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments describe behavior as a current bug ("BUG: Currently it doesn't...") but the implementation in this PR makes removeDocumentCursor() remove grid-block-events even when documentCursor is null. Please update/remove the bug explanation so the test documents the new expected behavior rather than the pre-fix state.

Suggested change
// removeDocumentCursor should still clean up
// BUG: Currently it doesn't because documentCursor is null and
// the entire function body is guarded by `if (this.documentCursor != null)`
component.removeDocumentCursor();
// After fix, this should be false
// removeDocumentCursor should clean up grid-block-events
// even if documentCursor is currently null
component.removeDocumentCursor();
// grid-block-events should be removed after calling removeDocumentCursor

Copilot uses AI. Check for mistakes.
expect(hasBlockEventsClass()).toBe(false);
});

it('should handle multiple Grid instances without interference (case 5)', () => {
// Create two grid instances
const component1 = makeGridComponent(new MockGridModel(), resizableTheme);
const component2 = makeGridComponent(new MockGridModel(), resizableTheme);

const separatorX = getColumnSeparatorX(0);
const headerY = getColumnHeaderY();

expect(hasBlockEventsClass()).toBe(false);

// Start drag on Grid 1
fireEvent.mouseDown(component1.canvas!, {
clientX: separatorX,
clientY: headerY,
});
fireEvent.mouseMove(window, {
clientX: separatorX + 50,
clientY: headerY,
});

expect(hasBlockEventsClass()).toBe(true);

// Grid 2 unmounts while Grid 1 is dragging
// This simulates closing a panel or tab
component2.componentWillUnmount();

// Grid 2's unmount should NOT affect Grid 1's drag state
// The class should still be present for Grid 1
expect(hasBlockEventsClass()).toBe(true);

// Complete Grid 1's drag
fireEvent.mouseUp(window, {
clientX: separatorX + 50,
clientY: headerY,
button: 0,
});

expect(hasBlockEventsClass()).toBe(false);
});

it('should clean up grid-block-events when component unmounts during drag (case 6)', () => {
const component = makeGridComponent(new MockGridModel(), resizableTheme);
const separatorX = getColumnSeparatorX(0);
const headerY = getColumnHeaderY();

// Start drag
mouseDown(0, 0, component, {}, separatorX, headerY);
fireEvent.mouseMove(window, {
clientX: separatorX + 50,
clientY: headerY,
});

expect(hasBlockEventsClass()).toBe(true);

// Component unmounts (simulates user navigating away or closing panel)
// This could happen if user drags outside window and releases there
component.componentWillUnmount();

// componentWillUnmount should clean up the class
expect(hasBlockEventsClass()).toBe(false);
});

it('should not leave grid-block-events when drag ends outside grid boundaries', () => {
const component = makeGridComponent(new MockGridModel(), resizableTheme);
const separatorX = getColumnSeparatorX(0);
const headerY = getColumnHeaderY();

// Start drag
mouseDown(0, 0, component, {}, separatorX, headerY);
fireEvent.mouseMove(window, {
clientX: separatorX + 50,
clientY: headerY,
});

expect(hasBlockEventsClass()).toBe(true);

// Move mouse far outside grid (simulating dragging outside window)
fireEvent.mouseMove(window, {
clientX: -1000,
clientY: -1000,
});

// Mouse up outside grid boundaries
fireEvent.mouseUp(window, {
clientX: -1000,
clientY: -1000,
button: 0,
});

// Should still clean up properly
expect(hasBlockEventsClass()).toBe(false);
});
});
24 changes: 17 additions & 7 deletions packages/grid/src/Grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ class Grid extends PureComponent<GridProps, GridState> {
// blocked pointer events that would otherwise prevent cursor styling from showing
documentCursor: string | null;

// Track if this Grid instance added the grid-block-events class
// Used to ensure only the Grid that added the class removes it
hasAddedBlockEvents: boolean;

dragTimer: ReturnType<typeof setTimeout> | null;

keyHandlers: readonly KeyHandler[];
Expand Down Expand Up @@ -410,6 +414,7 @@ class Grid extends PureComponent<GridProps, GridState> {
// Note: on document, not body so that cursor styling can be combined with
// blocked pointer events that would otherwise prevent cursor styling from showing
this.documentCursor = null;
this.hasAddedBlockEvents = false;

this.dragTimer = null;

Expand Down Expand Up @@ -1703,14 +1708,20 @@ class Grid extends PureComponent<GridProps, GridState> {
document.documentElement.classList.add(this.documentCursor);
}
document.documentElement.classList.add('grid-block-events');
this.hasAddedBlockEvents = true;
Comment on lines 1710 to +1711
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasAddedBlockEvents is set to true unconditionally after classList.add('grid-block-events'). If another Grid instance (or any other code) already added grid-block-events, this instance will later remove it in removeDocumentCursor(), reintroducing the cross-instance interference this change is trying to prevent. Consider setting hasAddedBlockEvents only when the class was not present before (or using a shared ref-count / global ownership token) so removal only happens when this instance actually transitioned the class from absent→present.

Suggested change
document.documentElement.classList.add('grid-block-events');
this.hasAddedBlockEvents = true;
const docEl = document.documentElement;
const hadBlockEventsClass = docEl.classList.contains('grid-block-events');
docEl.classList.add('grid-block-events');
this.hasAddedBlockEvents = !hadBlockEventsClass;

Copilot uses AI. Check for mistakes.
}

removeDocumentCursor(): void {
if (this.documentCursor != null) {
document.documentElement.classList.remove(this.documentCursor);
document.documentElement.classList.remove('grid-block-events');
this.documentCursor = null;
}
// Only remove grid-block-events if this Grid instance added it
// This prevents one Grid from removing the class on unmount while another Grid is still dragging
if (this.hasAddedBlockEvents) {
document.documentElement.classList.remove('grid-block-events');
this.hasAddedBlockEvents = false;
}
}

startDragTimer(event: React.MouseEvent): void {
Expand Down Expand Up @@ -1918,24 +1929,23 @@ class Grid extends PureComponent<GridProps, GridState> {
}

handleMouseUp(event: MouseEvent): void {
// Ignore right click while dragging
// Ignore non-left click while dragging to allow drag to continue
const { isDragging } = this.state;
if (isDragging && event.button === 2) {
if (isDragging && event.button !== 0) {
return;
}

window.removeEventListener('mousemove', this.handleMouseDrag, true);
window.removeEventListener('mouseup', this.handleMouseUp, true);

this.stopDragTimer();
this.removeDocumentCursor();

if (event.button != null && event.button !== 0) {
return;
}

this.notifyMouseHandlers('onUp', event, false);

this.stopDragTimer();

this.removeDocumentCursor();
}

handleResize(): void {
Expand Down
Loading