From 1a17ada2ab60ec526bd4a7076c8a1d1f94c16ca8 Mon Sep 17 00:00:00 2001 From: Luke Cotter <4013877+lukecotter@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:14:28 +0100 Subject: [PATCH] fix(render): align scrollToRowPosition from the row's actual offsetTop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scrollToRowPosition's center/bottom math derived the offset from the current scrollTop, assuming scrollToRow had placed the target at the top of the window. For rows that cannot sit at the top after the fill (e.g. near the end of the list) this misaligned them — measured ~12px off for center. Compute the alignment from the row's actual offsetTop instead, and sync vDomScrollPosTop/Bottom afterwards so the next user scroll does not diff against a stale tracker and jump (guarded for the basic renderer). Adds a Playwright regression test asserting a near-bottom row lands within 6px of top/center/bottom. --- src/js/core/rendering/Renderer.js | 33 +++++------ test/e2e/scroll-to-row-position.html | 46 +++++++++++++++ test/e2e/scroll-to-row-position.spec.ts | 74 +++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 16 deletions(-) create mode 100644 test/e2e/scroll-to-row-position.html create mode 100644 test/e2e/scroll-to-row-position.spec.ts diff --git a/src/js/core/rendering/Renderer.js b/src/js/core/rendering/Renderer.js index 5f5824940..e95904c24 100644 --- a/src/js/core/rendering/Renderer.js +++ b/src/js/core/rendering/Renderer.js @@ -165,34 +165,35 @@ export default class Renderer extends CoreFeature{ //scroll to row this.scrollToRow(row); - //align to correct position + //align to correct position. After scrollToRow the target row is + //in the rendered window but NOT necessarily at the top, so derive + //the alignment from its actual offsetTop rather than assuming the + //current scrollTop places it at the top. switch(position){ case "middle": case "center": - - if(this.elementVertical.scrollHeight - this.elementVertical.scrollTop == this.elementVertical.clientHeight){ - this.elementVertical.scrollTop = this.elementVertical.scrollTop + (rowEl.offsetTop - this.elementVertical.scrollTop) - ((this.elementVertical.scrollHeight - rowEl.offsetTop) / 2); - }else{ - this.elementVertical.scrollTop = this.elementVertical.scrollTop - (this.elementVertical.clientHeight / 2); - } - + this.elementVertical.scrollTop = rowEl.offsetTop - (this.elementVertical.clientHeight / 2) + (rowEl.offsetHeight / 2); break; case "bottom": - - if(this.elementVertical.scrollHeight - this.elementVertical.scrollTop == this.elementVertical.clientHeight){ - this.elementVertical.scrollTop = this.elementVertical.scrollTop - (this.elementVertical.scrollHeight - rowEl.offsetTop) + rowEl.offsetHeight; - }else{ - this.elementVertical.scrollTop = this.elementVertical.scrollTop - this.elementVertical.clientHeight + rowEl.offsetHeight; - } - + this.elementVertical.scrollTop = rowEl.offsetTop - this.elementVertical.clientHeight + rowEl.offsetHeight; break; case "top": - this.elementVertical.scrollTop = rowEl.offsetTop; + this.elementVertical.scrollTop = rowEl.offsetTop; break; } + //keep the virtual renderer's scroll trackers in sync with the + //manual scrollTop write, so the next user scroll doesn't compute a + //large diff against a stale tracker and trigger a spurious full + //re-render/jump. Guarded for the basic renderer, which has no + //trackers. + if(this.vDomScrollPosTop !== undefined){ + this.vDomScrollPosTop = this.elementVertical.scrollTop; + this.vDomScrollPosBottom = this.elementVertical.scrollTop; + } + resolve(); }else{ diff --git a/test/e2e/scroll-to-row-position.html b/test/e2e/scroll-to-row-position.html new file mode 100644 index 000000000..fbbe8e113 --- /dev/null +++ b/test/e2e/scroll-to-row-position.html @@ -0,0 +1,46 @@ + + + + + Tabulator scrollToRow position test + + + + + +
+ + + diff --git a/test/e2e/scroll-to-row-position.spec.ts b/test/e2e/scroll-to-row-position.spec.ts new file mode 100644 index 000000000..69d2a2c6f --- /dev/null +++ b/test/e2e/scroll-to-row-position.spec.ts @@ -0,0 +1,74 @@ +import { test, expect, Page } from "@playwright/test"; +import { join } from "path"; + +// Regression coverage for scrollToRowPosition alignment. +// +// After scrollToRow(row) fills the virtual DOM, the target row is in the window +// but NOT necessarily at the top. The old center/bottom math derived the offset +// from the current scrollTop (assuming top placement), so for rows deep in the +// list it scrolled the wrong distance — the row could land far from the +// requested position or detach entirely. The fix reads the row's actual +// offsetTop. It also syncs vDomScrollPosTop/Bottom so a following user scroll +// doesn't see a stale tracker and jump. +// +// Metric: distance (px) between where the target row landed and where the +// requested position wants it. Lower is better; a large reading is the bug. + +// A row near the bottom of the list: after scrollToRow fills the window it +// cannot sit at the window top (not enough rows below it), so the old math that +// assumed top placement misaligns it — the documented trigger. +const TARGET_ID = 1490; + +async function landingMiss(page: Page, position: "top" | "center" | "bottom") { + await page.evaluate( + async ({ id, position }) => { + // @ts-expect-error test global + await window.testTable.scrollToRow(id, position, true); + }, + { id: TARGET_ID, position }, + ); + await page.waitForTimeout(80); + + return page.evaluate( + ({ id, position }) => { + const holder = document.querySelector(".tabulator-tableholder") as HTMLElement; + const rows = [...holder.querySelectorAll(".tabulator-row")]; + const target = rows.find((r) => { + const cell = r.querySelector(".tabulator-cell"); + return cell && cell.textContent === String(id); + }); + if (!target) { + return { found: false as const }; + } + const rr = target.getBoundingClientRect(); + const hr = holder.getBoundingClientRect(); + let miss: number; + if (position === "center") { + miss = Math.abs(rr.top + rr.height / 2 - (hr.top + hr.height / 2)); + } else if (position === "bottom") { + miss = Math.abs(rr.bottom - hr.bottom); + } else { + miss = Math.abs(rr.top - hr.top); + } + return { found: true as const, miss: Math.round(miss) }; + }, + { id: TARGET_ID, position }, + ); +} + +test.describe("scrollToRow position alignment", () => { + test.beforeEach(async ({ page }) => { + await page.goto(`file://${join(__dirname, "scroll-to-row-position.html")}`); + await page.waitForSelector(".tabulator-tableholder"); + }); + + for (const position of ["top", "center", "bottom"] as const) { + test(`scrollToRow lands the target at ${position}`, async ({ page }) => { + const res = await landingMiss(page, position); + expect(res.found).toBe(true); + // The target row (id 750 of 1500, variable heights) must land within a + // row-height of the requested position. + expect(res.miss).toBeLessThanOrEqual(6); + }); + } +});