Skip to content

Commit 5a05e2e

Browse files
committed
Fix mouse up cleanup while dragging
1 parent 68e2734 commit 5a05e2e

2 files changed

Lines changed: 257 additions & 7 deletions

File tree

packages/grid/src/Grid.test.tsx

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1181,3 +1181,243 @@ describe('column separators', () => {
11811181
});
11821182
});
11831183
});
1184+
1185+
describe('grid-block-events cleanup', () => {
1186+
const resizableTheme = { ...defaultTheme, allowColumnResize: true };
1187+
1188+
function getColumnSeparatorX(columnIndex: VisibleIndex): number {
1189+
const { rowHeaderWidth, columnWidth } = resizableTheme;
1190+
return rowHeaderWidth + columnWidth * (columnIndex + 1) - 2;
1191+
}
1192+
1193+
function getColumnHeaderY(): number {
1194+
return Math.floor(resizableTheme.columnHeaderHeight / 2);
1195+
}
1196+
1197+
function hasBlockEventsClass(): boolean {
1198+
return document.documentElement.classList.contains('grid-block-events');
1199+
}
1200+
1201+
beforeEach(() => {
1202+
// Ensure clean state before each test
1203+
document.documentElement.classList.remove('grid-block-events');
1204+
});
1205+
1206+
afterEach(() => {
1207+
// Clean up after each test
1208+
document.documentElement.classList.remove('grid-block-events');
1209+
});
1210+
1211+
it('should remove grid-block-events after normal drag and mouse up', () => {
1212+
const component = makeGridComponent(new MockGridModel(), resizableTheme);
1213+
const separatorX = getColumnSeparatorX(0);
1214+
const headerY = getColumnHeaderY();
1215+
1216+
expect(hasBlockEventsClass()).toBe(false);
1217+
1218+
// Start drag on column separator
1219+
mouseDown(0, 0, component, {}, separatorX, headerY);
1220+
1221+
// Drag to trigger handleMouseDrag which adds the class
1222+
fireEvent.mouseMove(window, {
1223+
clientX: separatorX + 50,
1224+
clientY: headerY,
1225+
});
1226+
1227+
expect(hasBlockEventsClass()).toBe(true);
1228+
1229+
// Normal mouse up should clean up
1230+
fireEvent.mouseUp(window, {
1231+
clientX: separatorX + 50,
1232+
clientY: headerY,
1233+
button: 0,
1234+
});
1235+
1236+
expect(hasBlockEventsClass()).toBe(false);
1237+
});
1238+
1239+
it('should remove grid-block-events when right-clicking during drag (case 1)', () => {
1240+
const component = makeGridComponent(new MockGridModel(), resizableTheme);
1241+
const separatorX = getColumnSeparatorX(0);
1242+
const headerY = getColumnHeaderY();
1243+
1244+
// Start drag
1245+
mouseDown(0, 0, component, {}, separatorX, headerY);
1246+
fireEvent.mouseMove(window, {
1247+
clientX: separatorX + 50,
1248+
clientY: headerY,
1249+
});
1250+
1251+
expect(hasBlockEventsClass()).toBe(true);
1252+
1253+
// Right-click during drag (button=2)
1254+
fireEvent.mouseUp(window, {
1255+
clientX: separatorX + 50,
1256+
clientY: headerY,
1257+
button: 2,
1258+
});
1259+
1260+
// Right-click during drag returns early, class should still be present
1261+
// Then complete with left mouse up
1262+
fireEvent.mouseUp(window, {
1263+
clientX: separatorX + 50,
1264+
clientY: headerY,
1265+
button: 0,
1266+
});
1267+
1268+
expect(hasBlockEventsClass()).toBe(false);
1269+
});
1270+
1271+
it('should ignore middle-click during drag and continue dragging (case 2)', () => {
1272+
const component = makeGridComponent(new MockGridModel(), resizableTheme);
1273+
const separatorX = getColumnSeparatorX(0);
1274+
const headerY = getColumnHeaderY();
1275+
1276+
// Start drag
1277+
mouseDown(0, 0, component, {}, separatorX, headerY);
1278+
fireEvent.mouseMove(window, {
1279+
clientX: separatorX + 50,
1280+
clientY: headerY,
1281+
});
1282+
1283+
expect(hasBlockEventsClass()).toBe(true);
1284+
1285+
// Middle mouse button up during drag (button=1) should be ignored
1286+
// Drag should continue
1287+
fireEvent.mouseUp(window, {
1288+
clientX: separatorX + 50,
1289+
clientY: headerY,
1290+
button: 1,
1291+
});
1292+
1293+
// Class should still be present since drag is ongoing
1294+
expect(hasBlockEventsClass()).toBe(true);
1295+
1296+
// Left mouse up should properly end the drag and clean up
1297+
fireEvent.mouseUp(window, {
1298+
clientX: separatorX + 50,
1299+
clientY: headerY,
1300+
button: 0,
1301+
});
1302+
1303+
expect(hasBlockEventsClass()).toBe(false);
1304+
});
1305+
1306+
it('should handle addDocumentCursor with null cursor (case 3)', () => {
1307+
const component = makeGridComponent(new MockGridModel(), resizableTheme);
1308+
1309+
expect(hasBlockEventsClass()).toBe(false);
1310+
1311+
// First add a real cursor - this sets documentCursor and adds grid-block-events
1312+
component.addDocumentCursor('move');
1313+
expect(hasBlockEventsClass()).toBe(true);
1314+
1315+
// Now simulate cursor being cleared to null (e.g., mouse handler sets cursor = null)
1316+
// This sets documentCursor to null but keeps grid-block-events
1317+
component.addDocumentCursor(null);
1318+
1319+
// grid-block-events should still be present since it was added
1320+
expect(hasBlockEventsClass()).toBe(true);
1321+
1322+
// removeDocumentCursor should still clean up
1323+
// BUG: Currently it doesn't because documentCursor is null and
1324+
// the entire function body is guarded by `if (this.documentCursor != null)`
1325+
component.removeDocumentCursor();
1326+
1327+
// After fix, this should be false
1328+
expect(hasBlockEventsClass()).toBe(false);
1329+
});
1330+
1331+
it('should handle multiple Grid instances without interference (case 5)', () => {
1332+
// Create two grid instances
1333+
const component1 = makeGridComponent(new MockGridModel(), resizableTheme);
1334+
const component2 = makeGridComponent(new MockGridModel(), resizableTheme);
1335+
1336+
const separatorX = getColumnSeparatorX(0);
1337+
const headerY = getColumnHeaderY();
1338+
1339+
expect(hasBlockEventsClass()).toBe(false);
1340+
1341+
// Start drag on Grid 1
1342+
fireEvent.mouseDown(component1.canvas!, {
1343+
clientX: separatorX,
1344+
clientY: headerY,
1345+
});
1346+
fireEvent.mouseMove(window, {
1347+
clientX: separatorX + 50,
1348+
clientY: headerY,
1349+
});
1350+
1351+
expect(hasBlockEventsClass()).toBe(true);
1352+
1353+
// Grid 2 unmounts while Grid 1 is dragging
1354+
// This simulates closing a panel or tab
1355+
component2.componentWillUnmount();
1356+
1357+
// Grid 2's unmount should NOT affect Grid 1's drag state
1358+
// The class should still be present for Grid 1
1359+
expect(hasBlockEventsClass()).toBe(true);
1360+
1361+
// Complete Grid 1's drag
1362+
fireEvent.mouseUp(window, {
1363+
clientX: separatorX + 50,
1364+
clientY: headerY,
1365+
button: 0,
1366+
});
1367+
1368+
expect(hasBlockEventsClass()).toBe(false);
1369+
});
1370+
1371+
it('should clean up grid-block-events when component unmounts during drag (case 6)', () => {
1372+
const component = makeGridComponent(new MockGridModel(), resizableTheme);
1373+
const separatorX = getColumnSeparatorX(0);
1374+
const headerY = getColumnHeaderY();
1375+
1376+
// Start drag
1377+
mouseDown(0, 0, component, {}, separatorX, headerY);
1378+
fireEvent.mouseMove(window, {
1379+
clientX: separatorX + 50,
1380+
clientY: headerY,
1381+
});
1382+
1383+
expect(hasBlockEventsClass()).toBe(true);
1384+
1385+
// Component unmounts (simulates user navigating away or closing panel)
1386+
// This could happen if user drags outside window and releases there
1387+
component.componentWillUnmount();
1388+
1389+
// componentWillUnmount should clean up the class
1390+
expect(hasBlockEventsClass()).toBe(false);
1391+
});
1392+
1393+
it('should not leave grid-block-events when drag ends outside grid boundaries', () => {
1394+
const component = makeGridComponent(new MockGridModel(), resizableTheme);
1395+
const separatorX = getColumnSeparatorX(0);
1396+
const headerY = getColumnHeaderY();
1397+
1398+
// Start drag
1399+
mouseDown(0, 0, component, {}, separatorX, headerY);
1400+
fireEvent.mouseMove(window, {
1401+
clientX: separatorX + 50,
1402+
clientY: headerY,
1403+
});
1404+
1405+
expect(hasBlockEventsClass()).toBe(true);
1406+
1407+
// Move mouse far outside grid (simulating dragging outside window)
1408+
fireEvent.mouseMove(window, {
1409+
clientX: -1000,
1410+
clientY: -1000,
1411+
});
1412+
1413+
// Mouse up outside grid boundaries
1414+
fireEvent.mouseUp(window, {
1415+
clientX: -1000,
1416+
clientY: -1000,
1417+
button: 0,
1418+
});
1419+
1420+
// Should still clean up properly
1421+
expect(hasBlockEventsClass()).toBe(false);
1422+
});
1423+
});

packages/grid/src/Grid.tsx

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,10 @@ class Grid extends PureComponent<GridProps, GridState> {
353353
// blocked pointer events that would otherwise prevent cursor styling from showing
354354
documentCursor: string | null;
355355

356+
// Track if this Grid instance added the grid-block-events class
357+
// Used to ensure only the Grid that added the class removes it
358+
hasAddedBlockEvents: boolean;
359+
356360
dragTimer: ReturnType<typeof setTimeout> | null;
357361

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

414419
this.dragTimer = null;
415420

@@ -1703,14 +1708,20 @@ class Grid extends PureComponent<GridProps, GridState> {
17031708
document.documentElement.classList.add(this.documentCursor);
17041709
}
17051710
document.documentElement.classList.add('grid-block-events');
1711+
this.hasAddedBlockEvents = true;
17061712
}
17071713

17081714
removeDocumentCursor(): void {
17091715
if (this.documentCursor != null) {
17101716
document.documentElement.classList.remove(this.documentCursor);
1711-
document.documentElement.classList.remove('grid-block-events');
17121717
this.documentCursor = null;
17131718
}
1719+
// Only remove grid-block-events if this Grid instance added it
1720+
// This prevents one Grid from removing the class on unmount while another Grid is still dragging
1721+
if (this.hasAddedBlockEvents) {
1722+
document.documentElement.classList.remove('grid-block-events');
1723+
this.hasAddedBlockEvents = false;
1724+
}
17141725
}
17151726

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

19201931
handleMouseUp(event: MouseEvent): void {
1921-
// Ignore right click while dragging
1932+
// Ignore non-left click while dragging to allow drag to continue
19221933
const { isDragging } = this.state;
1923-
if (isDragging && event.button === 2) {
1934+
if (isDragging && event.button !== 0) {
19241935
return;
19251936
}
19261937

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

1941+
this.stopDragTimer();
1942+
this.removeDocumentCursor();
1943+
19301944
if (event.button != null && event.button !== 0) {
19311945
return;
19321946
}
19331947

19341948
this.notifyMouseHandlers('onUp', event, false);
1935-
1936-
this.stopDragTimer();
1937-
1938-
this.removeDocumentCursor();
19391949
}
19401950

19411951
handleResize(): void {

0 commit comments

Comments
 (0)