From 92efe3405b820e5234bb9261d943816b0f4751ad Mon Sep 17 00:00:00 2001 From: Luke Cotter <4013877+lukecotter@users.noreply.github.com> Date: Wed, 6 May 2026 10:59:03 +0100 Subject: [PATCH] fix(MiddleRowFocus): preserve scroll anchor across table operations Replace the always-recenter behavior with operation-aware scroll preservation, addressing four distinct symptoms: - Single tree expand/collapse no longer jumps to center: a one-shot skipNextRender flag is armed on the first toggle of a burst and cleared by a second toggle in the same tick, so single toggles preserve scrollTop while expand-all/collapse-all still recenters. - Column sort no longer scrolls to the top: the anchor is captured in the dataSorting pre-event instead of renderStarted, since Tabulator resets scrollTop before renderStarted fires for sort. - Filter no longer leaves a blank strip across the top of the table: detect the inflated .tabulator-table paddingTop left by Tabulator's rerenderRows desync (pre-filter vDomTop/vDomBottom against post-filter rows) and zero both paddingTop and renderer.vDomTopPad on the next frame. Upstream fix tracked in tabulator-virtual-scroll-fixes.md. - At-top / at-bottom no longer jumps off the edge: capture wasAtTop / wasAtBottom alongside the middle row using a 10px threshold, and on restore snap to scrollTop = 0 / scrollHeight - clientHeight instead of centering. Anchor capture/restore is consolidated behind \_captureAnchor / \_restoreAnchor / \_clearAnchor so future fields snapshot in one place. Add unit tests covering each path: single-toggle skip, bulk-toggle fall-through, dataSorting pre-capture, paddingTop reset (stale and legitimate), and at-top / at-bottom boundary snapping. --- .../src/tabulator/module/MiddleRowFocus.ts | 141 +++++++- .../module/__mocks__/tabulator-tables.ts | 7 + .../module/__tests__/MiddleRowFocus.test.ts | 334 ++++++++++++++++++ 3 files changed, 469 insertions(+), 13 deletions(-) create mode 100644 log-viewer/src/tabulator/module/__mocks__/tabulator-tables.ts create mode 100644 log-viewer/src/tabulator/module/__tests__/MiddleRowFocus.test.ts diff --git a/log-viewer/src/tabulator/module/MiddleRowFocus.ts b/log-viewer/src/tabulator/module/MiddleRowFocus.ts index 474586b6..c1a31611 100644 --- a/log-viewer/src/tabulator/module/MiddleRowFocus.ts +++ b/log-viewer/src/tabulator/module/MiddleRowFocus.ts @@ -17,7 +17,24 @@ export class MiddleRowFocus extends Module { static moduleName = 'middleRowFocus'; tableHolder: HTMLElement | null = null; + private tableEl: HTMLElement | null = null; middleRow: RowComponent | null = null; + private pendingFilterRaf: number | null = null; + + // Single tree toggle: skip the next renderComplete recenter so scrollTop stays put. + // Bulk toggle (expand-all / collapse-all): the second toggle in the synchronous burst + // clears the skip flag, so the recenter runs as before. + private skipNextRender = false; + private toggleSeenInBurst = false; + + // Boundary anchoring: when the user is at the top or bottom of the scroll range + // before an operation, recentering on the middle row pushes them away from the + // edge they were at. Capture the boundary at snapshot time and restore it + // (scrollTop = 0 / scrollHeight - clientHeight) instead of centering. + private static readonly boundaryThresholdPx = 10; + private wasAtTop = false; + private wasAtBottom = false; + constructor(table: Tabulator) { super(table); this.registerTableOption(middleRowFocusOption, false); @@ -28,30 +45,128 @@ export class MiddleRowFocus extends Module { if (this.options(middleRowFocusOption)) { this.tableHolder = this.table.element.querySelector('.tabulator-tableholder'); - this.table.on('dataTreeRowExpanded', () => { - this._clearFocusRow(); - }); + this.table.on('dataTreeRowExpanded', () => this._onTreeToggle()); + this.table.on('dataTreeRowCollapsed', () => this._onTreeToggle()); - this.table.on('dataTreeRowCollapsed', () => { - this._clearFocusRow(); - }); + // Sort resets scrollTop before renderStarted fires, so capture the anchor here + // (pre-sort) instead. The renderStarted handler below is a no-op once middleRow + // is set — see the !this.middleRow guard. + this.table.on('dataSorting', () => this._captureAnchor()); + + this.table.on('renderStarted', () => this._captureAnchor()); - this.table.on('renderStarted', () => { - if (this.table && this.tableHolder && !this.middleRow) { - this.middleRow = this._findMiddleVisibleRow(this.tableHolder); + // Tabulator bug workaround: rerenderRows on filter can leave .tabulator-table's + // paddingTop inflated when the pre-filter vDomTop/vDomBottom point past the + // post-filter row count. Detect and zero on the next frame after the render. + // See tabulator-virtual-scroll-fixes.md "Fix 7" for the upstream patch. + this.table.on('dataFiltered', () => { + if (this.pendingFilterRaf !== null) { + cancelAnimationFrame(this.pendingFilterRaf); } + this.pendingFilterRaf = requestAnimationFrame(() => { + this.pendingFilterRaf = null; + this._resetStaleTopPadding(); + }); }); this.table.on('renderComplete', async () => { - const rowToScrollTo = this.middleRow; - this._scrollToRow(rowToScrollTo); - this.middleRow = null; + if (this.skipNextRender) { + this.skipNextRender = false; + this._clearAnchor(); + } else { + this._restoreAnchor(); + } + this.toggleSeenInBurst = false; }); } } - private _clearFocusRow() { + /** + * Capture the user's scroll anchor: the row at the visual middle and whether + * the user was at the top / bottom edge. Idempotent within one operation — + * subsequent calls are no-ops once an anchor is already set. + */ + private _captureAnchor() { + if (!this.tableHolder || this.middleRow) { + return; + } + const holder = this.tableHolder; + const max = Math.max(0, holder.scrollHeight - holder.clientHeight); + this.wasAtTop = holder.scrollTop <= MiddleRowFocus.boundaryThresholdPx; + this.wasAtBottom = max - holder.scrollTop <= MiddleRowFocus.boundaryThresholdPx; + this.middleRow = this._findMiddleVisibleRow(holder); + } + + /** + * Restore the captured anchor. Boundary cases (was-at-top / was-at-bottom) snap + * to the edge so we don't push the user off it. Mid-table centers on middleRow. + */ + private _restoreAnchor() { + const holder = this.tableHolder; + if (!holder) { + this._clearAnchor(); + return; + } + if (this.wasAtTop) { + holder.scrollTop = 0; + } else if (this.wasAtBottom) { + holder.scrollTop = Math.max(0, holder.scrollHeight - holder.clientHeight); + } else { + this._scrollToRow(this.middleRow); + } + this._clearAnchor(); + } + + private _clearAnchor() { this.middleRow = null; + this.wasAtTop = false; + this.wasAtBottom = false; + } + + /** + * Tabulator bug workaround: after a filter, rerenderRows() (`tabulator_esm.mjs`, + * line ~25265) iterates pre-filter `vDomTop..vDomBottom` against the post-filter + * `rows()` array. The resulting `topOffset` flows into `_virtualRenderFill` and + * inflates `vDomTopPad` → `paddingTop` on `.tabulator-table` → blank strip across + * the top of the holder. We detect the symptom (`scrollTop < paddingTop`, which + * implies more empty space above the rendered window than the user has scrolled + * past) and reset the padding plus Tabulator's internal `vDomTopPad` so future + * renders are coherent. + */ + private _resetStaleTopPadding() { + if (!this.tableHolder) { + return; + } + if (!this.tableEl) { + this.tableEl = this.tableHolder.querySelector('.tabulator-table'); + } + const tableEl = this.tableEl; + if (!tableEl) { + return; + } + const paddingTop = parseFloat(tableEl.style.paddingTop) || 0; + const scrollTop = this.tableHolder.scrollTop; + if (paddingTop > 0 && scrollTop < paddingTop) { + tableEl.style.paddingTop = '0px'; + const renderer = this.table.rowManager?.renderer as Record | undefined; + if (renderer) { + renderer.vDomTopPad = 0; + } + } + } + + private _onTreeToggle() { + if (!this.toggleSeenInBurst) { + // First toggle in this burst: assume single, arm the skip. + this.toggleSeenInBurst = true; + this.skipNextRender = true; + } else { + // A second toggle arrived synchronously => it's a bulk operation; let the + // existing recenter run so the user's middle row stays in view. + this.skipNextRender = false; + } + // Clear the anchor so renderStarted captures fresh (with up-to-date boundary flags). + this._clearAnchor(); } private _scrollToRow(row: RowComponent | null) { diff --git a/log-viewer/src/tabulator/module/__mocks__/tabulator-tables.ts b/log-viewer/src/tabulator/module/__mocks__/tabulator-tables.ts new file mode 100644 index 00000000..0d5283fe --- /dev/null +++ b/log-viewer/src/tabulator/module/__mocks__/tabulator-tables.ts @@ -0,0 +1,7 @@ +// __mocks__/tabulator-tables.ts +export class Module { + constructor(table?: any) { + // this.table = table; + } + registerTableOption() {} +} diff --git a/log-viewer/src/tabulator/module/__tests__/MiddleRowFocus.test.ts b/log-viewer/src/tabulator/module/__tests__/MiddleRowFocus.test.ts new file mode 100644 index 00000000..f80256a8 --- /dev/null +++ b/log-viewer/src/tabulator/module/__tests__/MiddleRowFocus.test.ts @@ -0,0 +1,334 @@ +import { describe, expect, it, jest } from '@jest/globals'; +import { MiddleRowFocus } from '../MiddleRowFocus'; + +function rect(top: number, height: number) { + return { + top, + bottom: top + height, + left: 0, + right: 0, + width: 0, + height, + x: 0, + y: top, + toJSON: () => ({}), + }; +} + +function makeRow(top: number, height = 20) { + return { + getElement: () => ({ getBoundingClientRect: () => rect(top, height) }), + }; +} + +function makeBareTable() { + const handlers: Record void)[]> = {}; + return { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn() }, + getRows: jest.fn(() => []), + rowManager: { getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; +} + +function setup() { + const table = makeBareTable(); + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + return { plugin, table }; +} + +describe('MiddleRowFocus', () => { + it('returns the row whose cumulative visible height first crosses half of the holder height', () => { + // holder height 100, target 50. r1 contributes 30 (running 30), r2 contributes 30 + // (running 60 — first row at/over 50), r3 never reached. + const r1 = makeRow(0, 30); + const r2 = makeRow(30, 30); + const r3 = makeRow(60, 30); + const holder = { getBoundingClientRect: () => rect(0, 100) }; + + const table = { + on: jest.fn(), + element: { querySelector: jest.fn() }, + getRows: jest.fn((type?: string) => { + if (type === 'visible') return [r1, r2, r3]; + return []; + }), + }; + + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + + const found = ( + plugin as unknown as { _findMiddleVisibleRow: (h: unknown) => unknown } + )._findMiddleVisibleRow(holder); + expect(found).toBe(r2); + // r3 should never be reached. + void r3; + }); + + it('skips the recenter on a single tree toggle (preserves scrollTop)', () => { + const { table, plugin } = setup(); + + table.handlers.dataTreeRowExpanded?.[0]?.(); + expect((plugin as unknown as { skipNextRender: boolean }).skipNextRender).toBe(true); + + // Pretend Tabulator runs its render cycle. + table.handlers.renderStarted?.[0]?.(); + table.handlers.renderComplete?.[0]?.(); + + // No scroll attempted; flag cleared. + expect(table.scrollToRow).not.toHaveBeenCalled(); + expect((plugin as unknown as { skipNextRender: boolean }).skipNextRender).toBe(false); + }); + + it('captures the middle row in dataSorting (before Tabulator resets scrollTop)', () => { + // Build a table with a holder + visible rows so dataSorting can run + // _findMiddleVisibleRow successfully. + const r1 = makeRow(0, 30); + const r2 = makeRow(30, 30); + const r3 = makeRow(60, 30); + const holder = { getBoundingClientRect: () => rect(0, 100) }; + const handlers: Record void)[]> = {}; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn((type?: string) => { + if (type === 'visible') return [r1, r2, r3]; + return []; + }), + rowManager: { getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + // Sort starts — pre-sort middle row should be captured now (= r2). + handlers.dataSorting?.[0]?.(); + expect((plugin as unknown as { middleRow: unknown }).middleRow).toBe(r2); + + // Tabulator now rebuilds DOM with sorted rows in a different order. If our + // dataSorting capture didn't happen, renderStarted would capture the wrong + // row. Simulate that by changing the visible set and firing renderStarted — + // the guard `!this.middleRow` should make this a no-op. + (table.getRows as jest.Mock).mockImplementation((...args: unknown[]) => { + if (args[0] === 'visible') return [r3, r2, r1]; // reversed + return []; + }); + handlers.renderStarted?.[0]?.(); + expect((plugin as unknown as { middleRow: unknown }).middleRow).toBe(r2); + }); + + it('zeros stale paddingTop after filter when scrollTop is less than paddingTop', () => { + // Build a holder containing a .tabulator-table child whose style.paddingTop + // simulates the inflated value Tabulator's rerenderRows leaves behind. + const tableEl = { style: { paddingTop: '120px' } }; + const holder = { + scrollTop: 0, + querySelector: jest.fn((sel: string) => (sel === '.tabulator-table' ? tableEl : null)), + }; + const handlers: Record void)[]> = {}; + const renderer: Record = { vDomTopPad: 120 }; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn(() => []), + rowManager: { renderer, getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + // Drive the workaround directly (rAF-free) — same code path the rAF schedules. + (plugin as unknown as { _resetStaleTopPadding: () => void })._resetStaleTopPadding(); + + expect(tableEl.style.paddingTop).toBe('0px'); + expect(renderer.vDomTopPad).toBe(0); + }); + + it('captures wasAtTop when the user is at the scroll top', () => { + const r1 = makeRow(0, 20); + const r2 = makeRow(20, 20); + const holder = { + scrollTop: 0, + scrollHeight: 1000, + clientHeight: 100, + getBoundingClientRect: () => rect(0, 100), + }; + const handlers: Record void)[]> = {}; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn((type?: string) => (type === 'visible' ? [r1, r2] : [])), + rowManager: { getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + handlers.renderStarted?.[0]?.(); + + expect((plugin as unknown as { wasAtTop: boolean }).wasAtTop).toBe(true); + expect((plugin as unknown as { wasAtBottom: boolean }).wasAtBottom).toBe(false); + }); + + it('captures wasAtBottom when the user is at the scroll bottom', () => { + const r1 = makeRow(0, 20); + const r2 = makeRow(20, 20); + // scrollHeight 1000, clientHeight 100 → max scrollTop = 900. + const holder = { + scrollTop: 900, + scrollHeight: 1000, + clientHeight: 100, + getBoundingClientRect: () => rect(0, 100), + }; + const handlers: Record void)[]> = {}; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn((type?: string) => (type === 'visible' ? [r1, r2] : [])), + rowManager: { getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + handlers.renderStarted?.[0]?.(); + + expect((plugin as unknown as { wasAtBottom: boolean }).wasAtBottom).toBe(true); + expect((plugin as unknown as { wasAtTop: boolean }).wasAtTop).toBe(false); + }); + + it('on renderComplete with wasAtTop, snaps scrollTop to 0 instead of centering', () => { + const holder: { scrollTop: number; scrollHeight: number; clientHeight: number } = { + scrollTop: 0, + scrollHeight: 1000, + clientHeight: 100, + }; + const handlers: Record void)[]> = {}; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn(() => []), + rowManager: { getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + // Simulate post-snapshot state where wasAtTop is set. + (plugin as unknown as { wasAtTop: boolean }).wasAtTop = true; + holder.scrollTop = 200; // pretend Tabulator moved scrollTop during render + handlers.renderComplete?.[0]?.(); + + expect(holder.scrollTop).toBe(0); + expect(table.scrollToRow).not.toHaveBeenCalled(); + }); + + it('on renderComplete with wasAtBottom, snaps scrollTop to max instead of centering', () => { + const holder: { scrollTop: number; scrollHeight: number; clientHeight: number } = { + scrollTop: 0, + scrollHeight: 1000, + clientHeight: 100, + }; + const handlers: Record void)[]> = {}; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn(() => []), + rowManager: { getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + (plugin as unknown as { wasAtBottom: boolean }).wasAtBottom = true; + handlers.renderComplete?.[0]?.(); + + expect(holder.scrollTop).toBe(900); // 1000 - 100 + expect(table.scrollToRow).not.toHaveBeenCalled(); + }); + + it('does NOT zero paddingTop when scrollTop has accounted for it (legitimate state)', () => { + // Mid-table: scrollTop matches paddingTop, meaning the user has genuinely scrolled + // past the rows the padding represents. Mitigation must leave this alone. + const tableEl = { style: { paddingTop: '500px' } }; + const holder = { + scrollTop: 500, + querySelector: jest.fn((sel: string) => (sel === '.tabulator-table' ? tableEl : null)), + }; + const handlers: Record void)[]> = {}; + const renderer: Record = { vDomTopPad: 500 }; + const table = { + handlers, + on: jest.fn((evt: string, fn: (...args: unknown[]) => void) => { + (handlers[evt] ??= []).push(fn); + }), + element: { querySelector: jest.fn(() => holder) }, + getRows: jest.fn(() => []), + rowManager: { renderer, getDisplayRows: () => [] }, + scrollToRow: jest.fn(() => Promise.resolve()), + }; + + const plugin = new MiddleRowFocus(table as never); + (plugin as unknown as { table: typeof table }).table = table; + (plugin as unknown as { options: () => boolean }).options = () => true; + plugin.initialize(); + + (plugin as unknown as { _resetStaleTopPadding: () => void })._resetStaleTopPadding(); + + expect(tableEl.style.paddingTop).toBe('500px'); + expect(renderer.vDomTopPad).toBe(500); + }); + + it('a second toggle in the same burst clears the skip flag (bulk recenter runs)', () => { + const { table, plugin } = setup(); + + // Synchronous burst — expand-all. + table.handlers.dataTreeRowExpanded?.[0]?.(); + table.handlers.dataTreeRowExpanded?.[0]?.(); + table.handlers.dataTreeRowExpanded?.[0]?.(); + + // After the first toggle skip was armed; subsequent toggles cleared it. + expect((plugin as unknown as { skipNextRender: boolean }).skipNextRender).toBe(false); + expect((plugin as unknown as { toggleSeenInBurst: boolean }).toggleSeenInBurst).toBe(true); + }); +});