Skip to content

Commit 3d8dc71

Browse files
committed
fix: DH-22093: Fix web UI freezing bug from grid-block-events (#2646)
1 parent 9b60dc3 commit 3d8dc71

2 files changed

Lines changed: 261 additions & 5 deletions

File tree

packages/grid/src/Grid.test.tsx

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

packages/grid/src/Grid.tsx

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,10 @@ class Grid extends PureComponent<GridProps, GridState> {
328328
// blocked pointer events that would otherwise prevent cursor styling from showing
329329
documentCursor: string | null;
330330

331+
// Track if this Grid instance added the grid-block-events class
332+
// Used to ensure only the Grid that added the class removes it
333+
hasAddedBlockEvents: boolean;
334+
331335
dragTimer: ReturnType<typeof setTimeout> | null;
332336

333337
keyHandlers: readonly KeyHandler[];
@@ -385,6 +389,7 @@ class Grid extends PureComponent<GridProps, GridState> {
385389
// Note: on document, not body so that cursor styling can be combined with
386390
// blocked pointer events that would otherwise prevent cursor styling from showing
387391
this.documentCursor = null;
392+
this.hasAddedBlockEvents = false;
388393

389394
this.dragTimer = null;
390395

@@ -1652,14 +1657,20 @@ class Grid extends PureComponent<GridProps, GridState> {
16521657
document.documentElement.classList.add(this.documentCursor);
16531658
}
16541659
document.documentElement.classList.add('grid-block-events');
1660+
this.hasAddedBlockEvents = true;
16551661
}
16561662

16571663
removeDocumentCursor(): void {
16581664
if (this.documentCursor != null) {
16591665
document.documentElement.classList.remove(this.documentCursor);
1660-
document.documentElement.classList.remove('grid-block-events');
16611666
this.documentCursor = null;
16621667
}
1668+
// Only remove grid-block-events if this Grid instance added it
1669+
// This prevents one Grid from removing the class on unmount while another Grid is still dragging
1670+
if (this.hasAddedBlockEvents) {
1671+
document.documentElement.classList.remove('grid-block-events');
1672+
this.hasAddedBlockEvents = false;
1673+
}
16631674
}
16641675

16651676
startDragTimer(event: React.MouseEvent): void {
@@ -1868,18 +1879,23 @@ class Grid extends PureComponent<GridProps, GridState> {
18681879
}
18691880

18701881
handleMouseUp(event: MouseEvent): void {
1882+
// Ignore non-left click while dragging to allow drag to continue
1883+
const { isDragging } = this.state;
1884+
if (isDragging && event.button !== 0) {
1885+
return;
1886+
}
1887+
18711888
window.removeEventListener('mousemove', this.handleMouseDrag, true);
18721889
window.removeEventListener('mouseup', this.handleMouseUp, true);
18731890

1891+
this.stopDragTimer();
1892+
this.removeDocumentCursor();
1893+
18741894
if (event.button != null && event.button !== 0) {
18751895
return;
18761896
}
18771897

18781898
this.notifyMouseHandlers('onUp', event, false);
1879-
1880-
this.stopDragTimer();
1881-
1882-
this.removeDocumentCursor();
18831899
}
18841900

18851901
handleResize(): void {

0 commit comments

Comments
 (0)