Skip to content

fix vdom scroll #4796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
azmy60 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix vdom scroll #4796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/js/core/RowManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,16 @@ export default class RowManager extends CoreFeature{
data.reverse();
}

// Store initial state for large batch additions (issue #4730)
var initialScrollInfo;
if(data.length >= 100 && this.renderer && this.renderer.elementVertical){
Copy link

Copilot AI Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 100 should be extracted to a named constant or configuration option to make the threshold for scroll position preservation configurable and more maintainable.

Copilot uses AI. Check for mistakes.
initialScrollInfo = {
scrollTop: this.renderer.elementVertical.scrollTop,
scrollHeight: this.renderer.elementVertical.scrollHeight,
clientHeight: this.renderer.elementVertical.clientHeight
};
}

data.forEach((item, i) => {
var row = this.addRow(item, pos, index, true);
rows.push(row);
Expand All @@ -344,6 +354,32 @@ export default class RowManager extends CoreFeature{

this.regenerateRowPositions();

// Preserve relative scroll position for large batch additions (issue #4730)
if(initialScrollInfo && !pos && data.length >= 100){
Copy link

Copilot AI Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same magic number 100 is repeated here. This should use the same constant as defined for the condition above to avoid duplication and ensure consistency.

Copilot uses AI. Check for mistakes.
var newScrollHeight = this.renderer.elementVertical.scrollHeight;
var newClientHeight = this.renderer.elementVertical.clientHeight;

if(newScrollHeight > initialScrollInfo.scrollHeight){
var initialMaxScroll = Math.max(initialScrollInfo.scrollHeight - initialScrollInfo.clientHeight, 0);
var newMaxScroll = Math.max(newScrollHeight - newClientHeight, 0);

if(initialMaxScroll > 0 && newMaxScroll > 0){
// Don't use relative positioning if we were at the exact bottom
// This prevents the scrollbar from appearing to be at the bottom when more content exists
if(Math.abs(initialScrollInfo.scrollTop - initialMaxScroll) < 2){
Copy link

Copilot AI Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 2 for determining if the user was at the bottom should be extracted to a named constant with a descriptive name like BOTTOM_SCROLL_TOLERANCE for better readability and maintainability.

Copilot uses AI. Check for mistakes.
// We were at the bottom, but don't stick exactly to new bottom
// Leave some space to indicate there's more content
this.renderer.elementVertical.scrollTop = Math.max(0, newMaxScroll - 50);
Copy link

Copilot AI Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 50 representing the offset from bottom should be extracted to a named constant like BOTTOM_OFFSET_PIXELS to make the intent clear and allow for easier adjustment.

Copilot uses AI. Check for mistakes.
} else {
// Use relative positioning for middle positions
var relativePosition = initialScrollInfo.scrollTop / initialMaxScroll;
var newScrollTop = relativePosition * newMaxScroll;
this.renderer.elementVertical.scrollTop = newScrollTop;
}
}
}
}

if(this.displayRowsCount){
this._clearPlaceholder();
}
Expand Down
10 changes: 7 additions & 3 deletions src/js/core/rendering/renderers/VirtualDomVertical.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,16 +352,20 @@ export default class VirtualDomVertical extends Renderer{
}
}

//adjust row height to match average of rendered elements
this.vDomRowHeight = Math.floor((rowsHeight + topPadHeight) / totalRowsRendered);

if(!position){
this.vDomTopPad = 0;
//adjust row height to match average of rendered elements
this.vDomRowHeight = Math.floor((rowsHeight + topPadHeight) / totalRowsRendered);
this.vDomBottomPad = this.vDomRowHeight * (rowsCount - this.vDomBottom -1);

this.vDomScrollHeight = topPadHeight + rowsHeight + this.vDomBottomPad - containerHeight;
}else {
this.vDomTopPad = !forceMove ? this.scrollTop - topPadHeight : (this.vDomRowHeight * this.vDomTop) + offset;
this.vDomBottomPad = this.vDomBottom == rowsCount-1 ? 0 : Math.max(this.vDomScrollHeight - this.vDomTopPad - rowsHeight - topPadHeight, 0);
this.vDomBottomPad = this.vDomBottom == rowsCount-1 ? 0 : this.vDomRowHeight * (rowsCount - this.vDomBottom -1);

// Recalculate scroll height to ensure correct bottom padding
this.vDomScrollHeight = topPadHeight + rowsHeight + this.vDomBottomPad - containerHeight;
}

element.style.paddingTop = this.vDomTopPad+"px";
Expand Down
312 changes: 312 additions & 0 deletions test/e2e/basic.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,316 @@ test.describe("Tabulator functionality", () => {
expect(rowCountAfterClearingFilter).toBe(5); // Back to original count
});
});

test.describe("Scrolling operations", () => {
async function addRows(page, count, position) {
await page.evaluate(({ count, position }) => {
const newRows = [];
for (let i = 0; i < count; i++) {
newRows.push({
id: 11 + i,
name: `New Person ${i}`,
age: Math.floor(Math.random() * 30) + 20,
gender: "Male",
});
}
window.testTable.addData(newRows, position);
}, { count, position });
}

async function scrollToPosition(page, position) {
await page.evaluate((pos) => {
const tableHolder = document.querySelector('.tabulator-tableholder');
tableHolder.scrollTop = pos;
}, position);
await page.waitForTimeout(100);
}

async function getScrollPosition(page) {
return await page.evaluate(() => {
const tableHolder = document.querySelector('.tabulator-tableholder');
return tableHolder.scrollTop;
});
}

async function getTableHeight(page) {
return await page.evaluate(() => {
const tableHolder = document.querySelector('.tabulator-tableholder');
return {
scrollHeight: tableHolder.scrollHeight,
clientHeight: tableHolder.clientHeight,
maxScroll: tableHolder.scrollHeight - tableHolder.clientHeight
};
});
}

test("should stay in position if adding new rows at top", async ({ page }) => {
await addRows(page, 10, "top");
await page.waitForTimeout(300);

const scrollTop = await getScrollPosition(page);
expect(scrollTop).toBe(0);
});

test("should stay in position if adding new rows at bottom", async ({ page }) => {
await addRows(page, 10, "bottom");
await page.waitForTimeout(300);

const scrollTop = await getScrollPosition(page);
expect(scrollTop).toBe(0);
});

test("should maintain scroll position when adding rows at top while scrolled", async ({ page }) => {
await addRows(page, 20, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
const middlePosition = Math.floor(maxScroll / 2);
await scrollToPosition(page, middlePosition);

const initialScrollTop = await getScrollPosition(page);
await addRows(page, 10, "top");
await page.waitForTimeout(300);

const finalScrollTop = await getScrollPosition(page);
expect(finalScrollTop).toBeGreaterThan(initialScrollTop);
});

test("should maintain scroll position when adding rows at bottom while scrolled", async ({ page }) => {
await addRows(page, 20, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
const middlePosition = Math.floor(maxScroll / 2);
await scrollToPosition(page, middlePosition);

const initialScrollTop = await getScrollPosition(page);
await addRows(page, 10, "bottom");
await page.waitForTimeout(300);

const finalScrollTop = await getScrollPosition(page);
expect(Math.abs(finalScrollTop - initialScrollTop)).toBeLessThan(5);
});

test("should handle scrolling to specific row", async ({ page }) => {
await addRows(page, 50, "bottom");
await page.waitForTimeout(300);

await page.evaluate(() => {
window.testTable.scrollToRow(30, "center", false);
});
await page.waitForTimeout(300);

const scrollPosition = await getScrollPosition(page);
expect(scrollPosition).toBeGreaterThan(0);
});

test("should handle programmatic scroll to top", async ({ page }) => {
await addRows(page, 50, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
await scrollToPosition(page, maxScroll);

await page.evaluate(() => {
window.testTable.scrollToRow(1, "top", false);
});
await page.waitForTimeout(300);

const finalScrollPosition = await getScrollPosition(page);
expect(finalScrollPosition).toBe(0);
});

test("should handle programmatic scroll to bottom", async ({ page }) => {
await addRows(page, 50, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);

await page.evaluate(() => {
const lastRowId = window.testTable.getData().length;
window.testTable.scrollToRow(lastRowId, "bottom", false);
});
await page.waitForTimeout(300);

const finalScrollPosition = await getScrollPosition(page);
expect(Math.abs(finalScrollPosition - maxScroll)).toBeLessThan(150);
});

test("should handle deleting rows while scrolled", async ({ page }) => {
await addRows(page, 30, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
await scrollToPosition(page, maxScroll);
const initialScrollTop = await getScrollPosition(page);

await page.evaluate(() => {
const rows = window.testTable.getRows();
for (let i = 0; i < 10; i++) {
rows[rows.length - 1 - i].delete();
}
});
await page.waitForTimeout(300);

const finalScrollTop = await getScrollPosition(page);
expect(finalScrollTop).toBeLessThanOrEqual(initialScrollTop);
});

test("should handle updating row data while scrolled", async ({ page }) => {
await addRows(page, 30, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
const middlePosition = Math.floor(maxScroll / 2);
await scrollToPosition(page, middlePosition);
const initialScrollTop = await getScrollPosition(page);

await page.evaluate(() => {
window.testTable.updateRow(15, { name: "Updated Name", age: 99 });
});
await page.waitForTimeout(300);

const finalScrollTop = await getScrollPosition(page);
expect(finalScrollTop).toBe(initialScrollTop);
});

test("should handle filtering while scrolled", async ({ page }) => {
await addRows(page, 30, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
await scrollToPosition(page, maxScroll);
const initialScrollTop = await getScrollPosition(page);

await page.evaluate(() => {
window.testTable.setFilter("age", ">", 25);
});
await page.waitForTimeout(300);

const scrollTopAfterFilter = await getScrollPosition(page);
expect(scrollTopAfterFilter).toBeGreaterThanOrEqual(0);

const filteredRowCount = await page.locator(".tabulator-row").count();
expect(filteredRowCount).toBeGreaterThan(0);

await page.evaluate(() => {
window.testTable.clearFilter();
});
await page.waitForTimeout(300);

const scrollTopAfterClear = await getScrollPosition(page);
expect(scrollTopAfterClear).toBeGreaterThanOrEqual(0);
});

test("issue #4730 - scroll position after adding large amount of rows at bottom", async ({ page }) => {
await addRows(page, 100, "bottom");
await page.waitForTimeout(300);

const { maxScroll: initialMaxScroll } = await getTableHeight(page);
await scrollToPosition(page, initialMaxScroll);
const scrollTopBeforeAdd = await getScrollPosition(page);

await page.evaluate(() => {
const newRows = [];
for (let i = 0; i < 300; i++) {
newRows.push({
id: 1000 + i,
name: `Added Person ${i}`,
age: Math.floor(Math.random() * 30) + 20,
gender: "Male",
});
}
window.testTable.addRow(newRows);
});
await page.waitForTimeout(500);

const { maxScroll: finalMaxScroll } = await getTableHeight(page);
const scrollTopAfterAdd = await getScrollPosition(page);

const relativePositionBefore = scrollTopBeforeAdd / initialMaxScroll;
const relativePositionAfter = scrollTopAfterAdd / finalMaxScroll;

expect(Math.abs(relativePositionAfter - relativePositionBefore)).toBeLessThan(0.1);
});

test("issue #4730 - scroll position after adding rows at top while scrolled", async ({ page }) => {
await addRows(page, 100, "bottom");
await page.waitForTimeout(300);

const { maxScroll } = await getTableHeight(page);
const middlePosition = Math.floor(maxScroll / 2);
await scrollToPosition(page, middlePosition);

await page.evaluate(() => {
const newRows = [];
for (let i = 0; i < 50; i++) {
newRows.push({
id: 2000 + i,
name: `Top Person ${i}`,
age: Math.floor(Math.random() * 30) + 20,
gender: "Female",
});
}
window.testTable.addData(newRows, "top");
});
await page.waitForTimeout(500);

const visibleRowCount = await page.locator(".tabulator-row").count();
expect(visibleRowCount).toBeGreaterThan(0);

const hasBlankArea = await page.evaluate(() => {
const tableHolder = document.querySelector('.tabulator-tableholder');
return tableHolder.scrollHeight > tableHolder.clientHeight;
});
expect(hasBlankArea).toBe(true);
});

test("issue #4730 - blank space bug with cursor drag after adding rows", async ({ page }) => {
// Add initial rows and go to bottom
await addRows(page, 300, "bottom");
await page.waitForTimeout(300);

await page.evaluate(() => {
const tableHolder = document.querySelector('.tabulator-tableholder');
tableHolder.scrollTop = tableHolder.scrollHeight - tableHolder.clientHeight;
});
await page.waitForTimeout(100);

// Add more rows while at bottom
await addRows(page, 300, "bottom");
await page.waitForTimeout(500);

// Simulate rapid cursor drag to top (like your manual test)
await page.evaluate(() => {
const tableHolder = document.querySelector('.tabulator-tableholder');
tableHolder.scrollTop = 0;
});
await page.waitForTimeout(50);

// Check for blank space
const blankSpaceCheck = await page.evaluate(() => {
const tableHolder = document.querySelector('.tabulator-tableholder');
const table = document.querySelector('.tabulator-table');
const firstVisibleRow = table.querySelector('.tabulator-row');

if (!firstVisibleRow) {
return { hasBlankSpace: true, reason: 'No visible rows found' };
}

const tableStyle = window.getComputedStyle(table);
const topPadding = parseInt(tableStyle.paddingTop) || 0;
const hasBlankSpace = topPadding > 200 || firstVisibleRow.offsetTop > 100;

return {
hasBlankSpace,
topPadding,
firstRowTop: firstVisibleRow.offsetTop,
reason: hasBlankSpace ? 'Blank space detected after cursor drag' : 'Normal rendering'
};
});

expect(blankSpaceCheck.hasBlankSpace).toBe(false);
});
});
});
1 change: 1 addition & 0 deletions test/e2e/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ <h1>Tabulator Test</h1>
data: testData,
columns: columns,
layout: "fitColumns",
height: "200px",
});
});
</script>
Expand Down
Loading