Skip to content

Commit 1b71d95

Browse files
azmy60azmy60
committed
refactor
1 parent 232941a commit 1b71d95

1 file changed

Lines changed: 86 additions & 76 deletions

File tree

test/e2e/scroll-jump.spec.js renamed to test/e2e/scroll-jump.spec.ts

Lines changed: 86 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
1-
import { test, expect } from "@playwright/test";
1+
import { test, expect, Page } from "@playwright/test";
22
import { join } from "path";
33

4-
const holderSel = ".tabulator-tableholder";
5-
64
// Regression coverage for issue #3654 "Tabulator vertical scroll skipping / jumping".
75
//
86
// Root cause: _virtualRenderFill summed each row's getHeight() straight into
@@ -33,15 +31,14 @@ const holderSel = ".tabulator-tableholder";
3331
// The companion "gentle scrolling" test below demonstrates that this metric
3432
// reads ~0 for well-behaved scrolling, so a large reading is a real jump.
3533

36-
const DELTA = 250;
37-
38-
async function scrollToBottom(page, step) {
34+
async function scrollToBottom(page: Page, step: number) {
3935
for (let i = 0; i < 400; i++) {
40-
const done = await page.evaluate((sel) => {
41-
const h = document.querySelector(sel);
42-
return h.scrollTop >= h.scrollHeight - h.clientHeight - 1;
43-
}, holderSel);
44-
if (done) break;
36+
const done = await page
37+
.locator(".tabulator-tableholder")
38+
.evaluate((h) => h.scrollTop >= h.scrollHeight - h.clientHeight - 1);
39+
if (done) {
40+
break;
41+
}
4542
await page.mouse.wheel(0, step);
4643
await page.waitForTimeout(8);
4744
}
@@ -51,62 +48,89 @@ async function scrollToBottom(page, step) {
5148
// Scroll up `steps` times by `delta` px, returning the per-step jump samples.
5249
// `rowSelector` chooses which rows to track (grouped tables exclude the group
5350
// header rows so we follow a data row).
54-
async function scrollUpAndMeasure(page, delta, steps, rowSelector = ".tabulator-row") {
51+
52+
async function scrollUpAndMeasure(
53+
page: Page,
54+
delta: number,
55+
steps: number,
56+
rowSelector = ".tabulator-row",
57+
) {
5558
const jumps = [];
5659
for (let i = 0; i < steps; i++) {
57-
const before = await page.evaluate(({ sel, rowSel }) => {
58-
const holder = document.querySelector(sel);
59-
const mid =
60-
holder.getBoundingClientRect().top + holder.clientHeight / 2;
61-
const rows = [...holder.querySelectorAll(rowSel)];
62-
let best = null,
63-
bestDist = Infinity;
64-
for (const r of rows) {
65-
const d = Math.abs(r.getBoundingClientRect().top - mid);
66-
if (d < bestDist) {
67-
bestDist = d;
68-
best = r;
60+
/** The state BEFORE scrolling */
61+
const beforeScrolling = await page
62+
.locator(".tabulator-tableholder")
63+
.evaluate((holder, rowSelector) => {
64+
const mid =
65+
holder.getBoundingClientRect().top + holder.clientHeight / 2;
66+
const rows = [...holder.querySelectorAll(rowSelector)];
67+
68+
let best = null;
69+
let bestDist = Infinity;
70+
71+
for (const row of rows) {
72+
const dist = Math.abs(row.getBoundingClientRect().top - mid);
73+
if (dist < bestDist) {
74+
bestDist = dist;
75+
best = row;
76+
}
6977
}
70-
}
71-
if (!best) return null;
72-
const cell = best.querySelector(".tabulator-cell");
73-
return {
74-
id: cell ? cell.textContent : null,
75-
top: best.getBoundingClientRect().top,
76-
scrollTop: holder.scrollTop,
77-
};
78-
}, { sel: holderSel, rowSel: rowSelector });
78+
79+
if (!best) {
80+
return null;
81+
}
82+
83+
const cell = best.querySelector(".tabulator-cell");
84+
85+
return {
86+
id: cell ? cell.textContent : null,
87+
top: best.getBoundingClientRect().top,
88+
scrollTop: holder.scrollTop,
89+
};
90+
}, rowSelector);
7991

8092
await page.mouse.wheel(0, -delta);
8193
await page.waitForTimeout(20);
8294

83-
const after = await page.evaluate(
84-
({ sel, rowSel, id }) => {
85-
const holder = document.querySelector(sel);
86-
const rows = [...holder.querySelectorAll(rowSel)];
87-
for (const r of rows) {
88-
const cell = r.querySelector(".tabulator-cell");
95+
/** The state AFTER scrolling */
96+
const after = await page.locator(".tabulator-tableholder").evaluate(
97+
(holder, { rowSelector, id }) => {
98+
const rows = [...holder.querySelectorAll(rowSelector)];
99+
100+
for (const row of rows) {
101+
const cell = row.querySelector(".tabulator-cell");
89102
if (cell && cell.textContent === id) {
90103
return {
91-
found: true,
92-
top: r.getBoundingClientRect().top,
104+
found: true as const,
105+
top: row.getBoundingClientRect().top,
93106
scrollTop: holder.scrollTop,
94107
};
95108
}
96109
}
97-
return { found: false, scrollTop: holder.scrollTop };
110+
111+
return {
112+
found: false as const,
113+
scrollTop: holder.scrollTop,
114+
};
98115
},
99-
{ sel: holderSel, rowSel: rowSelector, id: before ? before.id : null },
116+
{ rowSelector, id: beforeScrolling ? beforeScrolling.id : null },
100117
);
101118

102-
if (!before) break;
119+
if (!beforeScrolling) {
120+
break;
121+
}
122+
103123
if (after.found) {
104-
const moved = after.top - before.top; // screen movement (down +)
105-
const scrolled = before.scrollTop - after.scrollTop; // scroll dist
124+
const moved = after.top - beforeScrolling.top; // screen movement (down +)
125+
const scrolled = beforeScrolling.scrollTop - after.scrollTop; // scroll dist
126+
106127
// Once nothing moves any more we have reached the top of the table.
107-
if (Math.abs(moved) < 3 && Math.abs(scrolled) < 3) break;
128+
if (Math.abs(moved) < 3 && Math.abs(scrolled) < 3) {
129+
break;
130+
}
131+
108132
jumps.push({
109-
id: before.id,
133+
id: beforeScrolling.id,
110134
moved: Math.round(moved),
111135
scrolled: Math.round(scrolled),
112136
jump: Math.round(moved - scrolled),
@@ -119,8 +143,8 @@ async function scrollUpAndMeasure(page, delta, steps, rowSelector = ".tabulator-
119143
test.describe("Vertical scroll jumping with variable height rows (#3654)", () => {
120144
test.beforeEach(async ({ page }) => {
121145
await page.goto(`file://${join(__dirname, "scroll-jump.html")}`);
122-
await page.waitForSelector(".tabulator-row");
123-
await page.locator(holderSel).hover();
146+
await page.waitForSelector(".tabulator-tableholder");
147+
await page.locator(".tabulator-tableholder").hover();
124148
});
125149

126150
test("scrolling up after a fast scroll does not make content jump", async ({
@@ -130,21 +154,18 @@ test.describe("Vertical scroll jumping with variable height rows (#3654)", () =>
130154
await scrollToBottom(page, 900);
131155

132156
// 2. Engage the horizontal scrollbar (part of the reproduction).
133-
await page.evaluate((sel) => {
134-
document.querySelector(sel).scrollLeft = 300;
135-
}, holderSel);
157+
await page
158+
.locator(".tabulator-tableholder")
159+
.evaluate((holder) => (holder.scrollLeft = 300));
136160
await page.waitForTimeout(50);
137161

138162
// 3. Scroll back up and measure the content jump.
139-
const jumps = await scrollUpAndMeasure(page, DELTA, 40);
163+
const jumps = await scrollUpAndMeasure(page, 250, 40);
140164
const worstJump = jumps.reduce(
141165
(max, j) => Math.max(max, Math.abs(j.jump)),
142166
0,
143167
);
144168

145-
console.log("scroll-up steps:", JSON.stringify(jumps));
146-
console.log("worst jump (px):", worstJump);
147-
148169
// Sanity: the scroll-up actually moved content.
149170
expect(jumps.length).toBeGreaterThan(0);
150171

@@ -163,9 +184,9 @@ test.describe("Vertical scroll jumping with variable height rows (#3654)", () =>
163184
await page.mouse.wheel(0, 120);
164185
await page.waitForTimeout(20);
165186
}
166-
await page.evaluate((sel) => {
167-
document.querySelector(sel).scrollLeft = 300;
168-
}, holderSel);
187+
await page
188+
.locator(".tabulator-tableholder")
189+
.evaluate((holder) => (holder.scrollLeft = 300));
169190
await page.waitForTimeout(50);
170191

171192
const jumps = await scrollUpAndMeasure(page, 120, 30);
@@ -174,40 +195,32 @@ test.describe("Vertical scroll jumping with variable height rows (#3654)", () =>
174195
0,
175196
);
176197

177-
console.log("gentle scroll-up steps:", JSON.stringify(jumps));
178-
console.log("gentle worst jump (px):", worstJump);
179-
180198
expect(jumps.length).toBeGreaterThan(5);
181199
expect(worstJump).toBeLessThanOrEqual(5);
182200
});
183201
});
184202

185-
// Several reporters on the issue noted the jump only appeared with the groupBy
186-
// feature enabled and disappeared when it was removed. It is the same bug - a
187-
// grouped table also renders variable height data rows through the virtual DOM
188-
// - so the same fix covers it. This guards the grouped path explicitly.
189-
// (Before the fix this jumped ~276px per step; after the fix it is 0px.)
190203
test.describe("Vertical scroll jumping with grouped variable height rows (#3654)", () => {
191204
test.beforeEach(async ({ page }) => {
192205
await page.goto(`file://${join(__dirname, "scroll-jump-group.html")}`);
193206
await page.waitForSelector(".tabulator-row");
194-
await page.locator(holderSel).hover();
207+
await page.locator(".tabulator-tableholder").hover();
195208
});
196209

197210
test("scrolling up after a fast scroll does not make content jump", async ({
198211
page,
199212
}) => {
200213
await scrollToBottom(page, 900);
201214

202-
await page.evaluate((sel) => {
203-
document.querySelector(sel).scrollLeft = 300;
204-
}, holderSel);
215+
await page
216+
.locator(".tabulator-tableholder")
217+
.evaluate((holder) => (holder.scrollLeft = 300));
205218
await page.waitForTimeout(50);
206219

207220
// Track data rows only, never the group header rows.
208221
const jumps = await scrollUpAndMeasure(
209222
page,
210-
DELTA,
223+
250,
211224
40,
212225
".tabulator-row:not(.tabulator-group)",
213226
);
@@ -216,9 +229,6 @@ test.describe("Vertical scroll jumping with grouped variable height rows (#3654)
216229
0,
217230
);
218231

219-
console.log("grouped scroll-up steps:", JSON.stringify(jumps));
220-
console.log("grouped worst jump (px):", worstJump);
221-
222232
expect(jumps.length).toBeGreaterThan(0);
223233
expect(worstJump).toBeLessThanOrEqual(20);
224234
});

0 commit comments

Comments
 (0)