Skip to content

Commit

Permalink
Revert "refactor: avoid unnecessary re-renders on virtualizer size ch…
Browse files Browse the repository at this point in the history 
…ange (#5650)" (#7205) (#7236)

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
Co-authored-by: ugur-vaadin <ugur@vaadin.com>
  • Loading branch information
@vaadin-bot @vursen @ugur-vaadin
3 people committed Mar 19, 2024
1 parent bacafcb commit 2a919ed
Show file tree
Hide file tree
Showing 4 changed files with 178 additions and 45 deletions.
46 changes: 31 additions & 15 deletions packages/component-base/src/virtualizer-iron-list-adapter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,24 +297,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 @@ -326,8 +343,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
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';
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 @@ -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.
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
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', () => {
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
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

0 comments on commit 2a919ed

Please sign in to comment.