Skip to content

Revert "refactor: avoid unnecessary re-renders on virtualizer size change (#5650)" #7205

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

Merged
vursen merged 13 commits into from
Mar 19, 2024
Merged

Revert "refactor: avoid unnecessary re-renders on virtualizer size change (#5650)" #7205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4cac75a
Revert "refactor: avoid unnecessary re-renders on virtualizer size ch…
vursen Mar 13, 2024
d815ad1
add tests
vursen Mar 13, 2024
81477cc
test: try to fix tests
vursen Mar 13, 2024
65222e9
test: try to fix tests
vursen Mar 13, 2024
e758e3f
another attempt to fix tests
vursen Mar 13, 2024
e094f2d
test: add test for all rows visible grid with varying row heights
ugur-vaadin Mar 14, 2024
2d01e7e
chore: revert virtualizer dev view update
ugur-vaadin Mar 15, 2024
cd66b8e
chore: run formatter
ugur-vaadin Mar 15, 2024
2f5cf29
add a JSDoc comment
vursen Mar 18, 2024
4acc605
uncomment code lines
vursen Mar 18, 2024
d1b48a3
set fixed height for test items
vursen Mar 18, 2024
8de7147
add more test styles
vursen Mar 18, 2024
32248b6
wip
vursen Mar 18, 2024
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
46 changes: 31 additions & 15 deletions packages/component-base/src/virtualizer-iron-list-adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,24 +300,41 @@ export class IronListAdapter {
this._debouncers._increasePoolIfNeeded.cancel();
}

// Prevent element update while the scroll position is being restored
this.__preventElementUpdates = true;

// Record the scroll position before changing the size
let fvi; // First visible index
let fviOffsetBefore; // Scroll offset of the first visible index
if (size > 0) {
fvi = this.adjustedFirstVisibleIndex;
fviOffsetBefore = this.__getIndexScrollOffset(fvi);
}

// Change the size
this.__size = size;

if (!this._physicalItems) {
// Not initialized yet
this._itemsChanged({
path: 'items',
});
this.__preventElementUpdates = true;
flush();
this.__preventElementUpdates = false;
} else {
// Already initialized, just update _virtualCount
this._updateScrollerSize();
this._virtualCount = this.items.length;
this._render();
this._itemsChanged({
path: 'items',
});
flush();

// Try to restore the scroll position if the new size is larger than 0
if (size > 0) {
fvi = Math.min(fvi, size - 1);
// Note, calling scrollToIndex also updates the virtual index offset,
// causing the virtualizer to add more items when size is increased,
// and remove exceeding items when size is decreased.
this.scrollToIndex(fvi);

const fviOffsetAfter = this.__getIndexScrollOffset(fvi);
if (fviOffsetBefore !== undefined && fviOffsetAfter !== undefined) {
this._scrollTop += fviOffsetBefore - fviOffsetAfter;
}
}

this.__preventElementUpdates = false;

// When reducing size while invisible, iron-list does not update items, so
// their hidden state is not updated and their __lastUpdatedIndex is not
// reset. In that case force an update here.
Expand All @@ -329,8 +346,7 @@ export class IronListAdapter {
requestAnimationFrame(() => this._resizeHandler());
}

// Schedule and flush a resize handler. This will cause a
// re-render for the elements.
// Schedule and flush a resize handler
this._resizeHandler();
flush();
}
Expand Down
84 changes: 83 additions & 1 deletion packages/component-base/test/virtualizer-unlimited-size.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe('unlimited size', () => {

beforeEach(() => {
scrollTarget = fixtureSync(`
<div style="height: 100px;">
<div style="height: 250px;">
<div></div>
</div>
`);
Expand All @@ -22,6 +22,13 @@ describe('unlimited size', () => {
el.index = index;
el.id = `item-${index}`;
el.textContent = el.id;
el.style.right = '0';
Copy link
Copy Markdown
Contributor Author

@vursen vursen Mar 18, 2024

Choose a reason for hiding this comment

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

note: Most of the styles are added to make debugging easier.

el.style.left = '0';
el.style.display = 'flex';
el.style.alignItems = 'center';
el.style.background = index % 2 === 0 ? '#e7e7e7' : '#d0d0d0';
el.style.height = '30px';
Copy link
Copy Markdown
Contributor Author

@vursen vursen Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note: Setting a fixed height prevents test failures related to differences in font rendering across browsers.

el.style.padding = '0 10px';
},
scrollTarget,
scrollContainer,
Expand All @@ -30,6 +37,10 @@ describe('unlimited size', () => {
virtualizer.size = 1000000;
});

function getLastRenderedIndex() {
return [...elementsContainer.children].reduce((max, el) => Math.max(max, el.index), 0);
}

it('should scroll to a large index', () => {
const index = Math.floor(virtualizer.size / 2);
virtualizer.scrollToIndex(index);
Expand Down Expand Up @@ -202,4 +213,75 @@ describe('unlimited size', () => {
Math.ceil(itemRect.bottom),
);
});

it('should add more items on size increase', () => {
const index = virtualizer.size - 1;
virtualizer.scrollToIndex(index);
expect(getLastRenderedIndex()).to.equal(index);

virtualizer.size += 2000;
expect(getLastRenderedIndex()).to.be.above(index);
});

it('should remove exceeding items on size decrease', () => {
const index = virtualizer.size - 1;
virtualizer.scrollToIndex(index);
expect(getLastRenderedIndex()).to.equal(index);

virtualizer.size -= 1000;
expect(getLastRenderedIndex()).to.be.below(index);
});

it('should set scroll to end when size decrease affects a visible index', async () => {
virtualizer.scrollToIndex(virtualizer.size - 1000);
virtualizer.size = virtualizer.firstVisibleIndex - 20;
await oneEvent(scrollTarget, 'scroll');
const lastItem = elementsContainer.querySelector(`#item-${virtualizer.size - 1}`);
expect(lastItem.getBoundingClientRect().bottom).to.be.closeTo(scrollTarget.getBoundingClientRect().bottom, 1);
});

it('should preserve scroll position when size decrease affects a buffered index', async () => {
// Make sure there are at least 2 buffered items at the end.
expect(getLastRenderedIndex() - virtualizer.lastVisibleIndex).to.be.greaterThanOrEqual(2);

// Scroll to an index and add an additional scroll offset.
const index = virtualizer.size - 1000;
virtualizer.scrollToIndex(index);
scrollTarget.scrollTop += 10;

// Decrease the size so that the last buffered index exceeds the new size bounds.
virtualizer.size = virtualizer.lastVisibleIndex + 1;
await oneEvent(scrollTarget, 'scroll');

const item = elementsContainer.querySelector(`#item-${index}`);
expect(item.getBoundingClientRect().top).to.be.closeTo(scrollTarget.getBoundingClientRect().top - 10, 1);
});

// FIXME: Fails due to a scroll offset reset caused by _adjustVirtualIndexOffset on scroll event.
Copy link
Copy Markdown
Contributor

@ugur-vaadin ugur-vaadin Mar 14, 2024

Choose a reason for hiding this comment

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

Is there a related issue to fix this?

Copy link
Copy Markdown
Contributor Author

@vursen vursen Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good question. My plan was to report it after the merge because it's only reproducible with the scroll restoration logic.

it.skip('should preserve scroll position when size decrease does not affect any rendered indexes', async () => {
// Scroll to an index and add an additional scroll offset.
const index = virtualizer.size - 2000;
virtualizer.scrollToIndex(index);
scrollTarget.scrollTop += 10;

// Decrease the size so that no rendered indexes are affected.
virtualizer.size -= 1000;
await oneEvent(scrollTarget, 'scroll');

const item = elementsContainer.querySelector(`#item-${index}`);
expect(item.getBoundingClientRect().top).to.be.closeTo(scrollTarget.getBoundingClientRect().top - 10, 1);
});

// FIXME: Fails due to a scroll offset reset caused by _adjustVirtualIndexOffset on scroll event.
it.skip('should preserve scroll position on size increase', async () => {
const index = virtualizer.size - 2000;
virtualizer.scrollToIndex(index);
scrollTarget.scrollTop += 10;

virtualizer.size += 1000;
await oneEvent(scrollTarget, 'scroll');

const item = elementsContainer.querySelector(`#item-${index}`);
expect(item.getBoundingClientRect().top).to.be.closeTo(scrollTarget.getBoundingClientRect().top - 10, 1);
});
});
72 changes: 43 additions & 29 deletions packages/component-base/test/virtualizer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('virtualizer', () => {
}

scrollTarget = fixtureSync(`
<div style="height: 100px;">
<div style="height: 250px;">
<div></div>
</div>
`);
Expand All @@ -30,6 +30,13 @@ describe('virtualizer', () => {
el.index = index;
el.id = `item-${index}`;
el.textContent = el.id;
el.style.right = '0';
el.style.left = '0';
el.style.display = 'flex';
el.style.alignItems = 'center';
el.style.background = index % 2 === 0 ? '#e7e7e7' : '#d0d0d0';
el.style.height = '30px';
el.style.padding = '0 10px';
}),
scrollTarget,
scrollContainer,
Expand All @@ -40,6 +47,10 @@ describe('virtualizer', () => {

beforeEach(() => init({}));

function getLastRenderedIndex() {
return [...elementsContainer.children].reduce((max, el) => Math.max(max, el.index), 0);
}

it('should have the first item at the top', () => {
const item = elementsContainer.querySelector('#item-0');
expect(item.getBoundingClientRect().top).to.equal(scrollTarget.getBoundingClientRect().top);
Expand Down Expand Up @@ -174,7 +185,7 @@ describe('virtualizer', () => {
expect(item.getBoundingClientRect().top).to.equal(scrollTarget.getBoundingClientRect().top);
});

it('should restore scroll position on size change', () => {
it('should preserve scroll position on size increase', () => {
// Scroll to item 50 and an additional 10 pixels
virtualizer.scrollToIndex(50);
scrollTarget.scrollTop += 10;
Expand All @@ -184,40 +195,43 @@ describe('virtualizer', () => {
expect(item.getBoundingClientRect().top).to.equal(scrollTarget.getBoundingClientRect().top - 10);
});

it('should not request item updates on size increase', () => {
Copy link
Copy Markdown
Contributor Author

@vursen vursen Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note: After this PR is merged, I'll create an issue for further research on how to prevent item updates on size change without sacrificing the scroll restoration.

const updateElement = sinon.spy((el, index) => {
el.textContent = `item-${index}`;
});
init({ size: 100, updateElement });

// Scroll halfway down the list
updateElement.resetHistory();
it('should set scroll to end when size decrease affects a visible index', async () => {
virtualizer.scrollToIndex(50);
virtualizer.size = virtualizer.firstVisibleIndex - 20;
await oneEvent(scrollTarget, 'scroll');
const lastItem = elementsContainer.querySelector(`#item-${virtualizer.size - 1}`);
expect(lastItem.getBoundingClientRect().bottom).to.be.closeTo(scrollTarget.getBoundingClientRect().bottom, 1);
});

// Increase the size so it shouldn't affect the current viewport items
updateElement.resetHistory();
virtualizer.size = 200;
it('should preserve scroll position when size decrease affects a buffered index', async () => {
// Make sure there are at least 2 buffered items at the end.
expect(getLastRenderedIndex() - virtualizer.lastVisibleIndex).to.be.greaterThanOrEqual(2);

expect(updateElement.called).to.be.false;
});
// Scroll to an index and add an additional scroll offset.
const index = 50;
virtualizer.scrollToIndex(index);
scrollTarget.scrollTop += 10;

it('should not request item updates on size increase when scrolled', async () => {
const updateElement = sinon.spy((el, index) => {
el.textContent = `item-${index}`;
});
init({ size: 100, updateElement });
// Decrease the size so that the last buffered index exceeds the new size bounds.
virtualizer.size = virtualizer.lastVisibleIndex + 1;
await oneEvent(scrollTarget, 'scroll');

// Scroll halfway down the list
virtualizer.scrollToIndex(50);
// Manually scroll down a bit
scrollTarget.scrollTop += 100;
await nextFrame();
const item = elementsContainer.querySelector(`#item-${index}`);
expect(item.getBoundingClientRect().top).to.be.closeTo(scrollTarget.getBoundingClientRect().top - 10, 1);
});

// Increase the size so it shouldn't affect the current viewport items
updateElement.resetHistory();
virtualizer.size = 200;
it('should preserve scroll position when size decrease does not affect any rendered indexes', async () => {
// Scroll to an index and add an additional scroll offset.
const index = 50;
virtualizer.scrollToIndex(index);
scrollTarget.scrollTop += 10;

// Decrease the size so that no rendered indexes are affected.
virtualizer.size = 80;
await oneEvent(scrollTarget, 'scroll');

expect(updateElement.called).to.be.false;
const item = elementsContainer.querySelector(`#item-${index}`);
expect(item.getBoundingClientRect().top).to.be.closeTo(scrollTarget.getBoundingClientRect().top - 10, 1);
});

it('should request a different set of items on size decrease', () => {
Expand Down
21 changes: 21 additions & 0 deletions packages/grid/test/basic.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,27 @@ describe('basic features', () => {
grid.size = 1;
expect(getFirstCellRenderCount()).to.equal(1);
});

it('should render all items with varying row heights when all rows visible', async () => {
grid = fixtureSync(`
<vaadin-grid all-rows-visible>
<vaadin-grid-column></vaadin-grid-column>
</vaadin-grid>
`);
const rowHeights = [30, 100, 30, 30, 100, 30];
grid.items = rowHeights.map((value) => {
return { height: value };
});
column = grid.firstElementChild;
column.renderer = (root, _, model) => {
root.innerHTML = `<button style="height:${model.item.height}px">Button</button>`;
};
flushGrid(grid);
await nextFrame();
expect(getPhysicalItems(grid).length).to.equal(rowHeights.length);
const sum = rowHeights.reduce((acc, value) => acc + value, 0);
expect(grid.clientHeight).to.be.greaterThan(sum);
});
});

describe('flex child', () => {
Expand Down