Skip to content

Commit 7b752ae

Browse files
authored
fix: DH-20229: Value is null or undefined when scrolling rollup (#2538)
The "Value is null or undefined" error was thrown from calling `IrisGridTreeTableModel.textAlignForCell`when scrolling a table with expanded rollups while hovering on a row. I think these errors are due to `textAlignForCell` not being able to find the row in the viewport when scrolling rapidly as it has already left the viewport, and so it shouldn't matter what is returned in this case if the cell isn't displayed on screen. - In this case, the row is only used to determine whether the constituent type should be used for formatting, and will now fallback to the column type when the row is null. To address potential issues with scrolling in the future, some tests involving scrolling a table were added. These mainly represent the failure scenario seen in the ticket, but more can be added in the future to test other scenarios. - From running the tests, a similar error was found in `IrisGridRenderer.getExpandButtonPosition`, and a fallback value has been added to replace the not null assertions there as well. This is the same pattern used in `IrisGridTextCellRenderer.ts.getCellOverflowButtonPosition`
1 parent 6938c47 commit 7b752ae

22 files changed

+139
-6
lines changed

packages/iris-grid/src/IrisGridRenderer.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -915,9 +915,10 @@ export class IrisGridRenderer extends GridRenderer {
915915
metrics
916916
);
917917

918-
assertNotNull(left);
919-
assertNotNull(rowHeight);
920-
assertNotNull(top);
918+
if (left == null || rowHeight == null || top == null) {
919+
return NULL_POSITION;
920+
}
921+
921922
const { cellHorizontalPadding } = theme;
922923

923924
const width = EXPAND_ICON_SIZE + 2 * cellHorizontalPadding;

packages/iris-grid/src/IrisGridTreeTableModel.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,8 +248,6 @@ class IrisGridTreeTableModel
248248
}
249249

250250
const row = this.row(y);
251-
assertNotNull(row);
252-
253251
let typeForFormatting = column.type;
254252

255253
// For tree table leaf nodes, use the constituent type for formatting

tests/table-operations.spec.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { test, expect, Page } from '@playwright/test';
1+
import { test, expect, type Page } from '@playwright/test';
22
import {
33
makeTableCommand,
44
pasteInMonaco,
@@ -113,6 +113,9 @@ test.beforeEach(async ({ page }) => {
113113
throw new Error(msg.text());
114114
}
115115
});
116+
page.on('pageerror', error => {
117+
throw error;
118+
});
116119

117120
await openTable(page, 'all_types');
118121

tests/table-scroll.spec.ts

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
import { test, expect, type Page } from '@playwright/test';
2+
import {
3+
waitForLoadingDone,
4+
openTableOption,
5+
openTable,
6+
gotoPage,
7+
} from './utils';
8+
9+
// Scroll the table by scrolling the mouse wheel, whilst moving the mouse up and down to test hovering functionality
10+
async function scrollTableWhileMovingMouse(page: Page) {
11+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
12+
const box = await gridCanvas.boundingBox();
13+
if (!box) throw new Error('No bounding box found for grid canvas');
14+
15+
let mouseY = box.y + 120;
16+
const mouseX = box.x + 20;
17+
const mouseStep = 40;
18+
19+
const scrollDeltas = [
20+
1000, -600, 1600, -1200, 2000, -2000, 1000, -400, 1400, -1000,
21+
];
22+
23+
for (let i = 0; i < scrollDeltas.length; i += 1) {
24+
mouseY += i % 2 === 0 ? -mouseStep : mouseStep;
25+
// eslint-disable-next-line no-await-in-loop
26+
await page.mouse.move(mouseX, mouseY);
27+
28+
let remaining = Math.abs(scrollDeltas[i]);
29+
const direction = Math.sign(scrollDeltas[i]);
30+
const scrollStep = 100;
31+
32+
while (remaining > 0) {
33+
const step = Math.min(scrollStep, remaining);
34+
// eslint-disable-next-line no-await-in-loop
35+
await page.mouse.wheel(0, step * direction);
36+
remaining -= step;
37+
}
38+
}
39+
}
40+
41+
test.beforeEach(async ({ page }) => {
42+
await gotoPage(page, '');
43+
44+
// Fail quickly if console errors are detected
45+
page.on('console', msg => {
46+
if (msg.type() === 'error') {
47+
throw new Error(msg.text());
48+
}
49+
});
50+
page.on('pageerror', error => {
51+
throw error;
52+
});
53+
});
54+
55+
test('scroll with keyboard shortcuts', async ({ page }) => {
56+
// Use simple_table for its scrollable number of rows
57+
await openTable(page, 'simple_table');
58+
59+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
60+
await gridCanvas.click({ position: { x: 10, y: 80 } });
61+
await waitForLoadingDone(page);
62+
63+
// Down
64+
await page.keyboard.press('ArrowDown');
65+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
66+
67+
// Up
68+
await page.keyboard.press('ArrowUp');
69+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
70+
71+
// Page Down
72+
await page.keyboard.press('PageDown');
73+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
74+
75+
// Page Up
76+
await page.keyboard.press('PageUp');
77+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
78+
79+
// End
80+
await page.keyboard.press('ControlOrMeta+ArrowDown');
81+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
82+
83+
// Home
84+
await page.keyboard.press('ControlOrMeta+ArrowUp');
85+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
86+
});
87+
88+
test('scroll while moving mouse over rows', async ({ page }) => {
89+
// Use simple_table for its scrollable number of rows
90+
await openTable(page, 'simple_table');
91+
92+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
93+
await gridCanvas.click({ position: { x: 10, y: 80 } });
94+
await waitForLoadingDone(page);
95+
96+
await scrollTableWhileMovingMouse(page);
97+
});
98+
99+
test('scroll expanded rollup while moving mouse over rows', async ({
100+
page,
101+
}) => {
102+
// Use simple_table for its scrollable number of rows
103+
await openTable(page, 'simple_table');
104+
105+
await test.step('Open Rollup Rows option', async () => {
106+
const tableOperationsMenu = page.locator(
107+
'data-testid=btn-iris-grid-settings-button-table'
108+
);
109+
await tableOperationsMenu.click();
110+
// Wait for Table Options menu to show
111+
await expect(page.locator('.table-sidebar')).toHaveCount(1);
112+
await openTableOption(page, 'Rollup Rows');
113+
114+
await test.step('Rollup column', async () => {
115+
const xColumn = page.getByRole('button', { name: 'y' });
116+
expect(xColumn).toBeTruthy();
117+
await xColumn.dblclick();
118+
await waitForLoadingDone(page);
119+
});
120+
121+
await test.step('Expand constituent with non-aggregated columns visible', async () => {
122+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
123+
await gridCanvas.click({ position: { x: 10, y: 80 } });
124+
await waitForLoadingDone(page);
125+
});
126+
127+
await test.step('Scroll while moving mouse over rows', async () => {
128+
await scrollTableWhileMovingMouse(page);
129+
});
130+
});
131+
});
26 KB
Loading
46.6 KB
Loading
16.1 KB
Loading
26 KB
Loading
46.6 KB
Loading
16.1 KB
Loading

0 commit comments

Comments
 (0)