Skip to content

fix: adjust virtualizer indexes when scrolling at the end #9142

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
merged 9 commits into from
Jun 19, 2025
Merged

fix: adjust virtualizer indexes when scrolling at the end #9142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55d57ee
fix: adjust virtualizer indexes when scrolling at the end
ugur-vaadin May 14, 2025
5269198
fix: do not get reusables when there are no physical items
ugur-vaadin May 14, 2025
1d3a2bf
fix: fix jumpy scrolling at virtualizer end
ugur-vaadin May 15, 2025
6d6e26a
refactor: do not modify delta
ugur-vaadin May 15, 2025
ffc6ee4
refactor: revert changes in original path
ugur-vaadin May 15, 2025
f4aa239
refactor: move line back
ugur-vaadin May 15, 2025
da4316b
test: add grid container resize test and modify an existing test
ugur-vaadin May 16, 2025
81577f4
refactor: move the fix to resize handler and simplify index adjustment
ugur-vaadin Jun 17, 2025
7749ae0
refactor: update scroll top in order to have smooth scrolling
ugur-vaadin Jun 19, 2025
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
18 changes: 18 additions & 0 deletions packages/component-base/src/virtualizer-iron-list-adapter.js
View file
Open in desktop
Copy link
Member

@tomivirkki tomivirkki Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since the issue occurs on resize, I wonder if this should rather be fixed by overriding iron-list's _resizeHandler:

_resizeHandler() {
  super._resizeHandler();

  const lastIndexVisible = this.adjustedLastVisibleIndex === this.size - 1;
  const emptySpace = this._physicalTop - this._scrollPosition;
  if (lastIndexVisible && emptySpace > 0) {
    const idxAdjustment = Math.ceil(emptySpace / this._physicalAverage);
    this._virtualStart = Math.max(0, this._virtualStart - idxAdjustment);
    this._physicalStart = Math.max(0, this._physicalStart - idxAdjustment);

    // Scroll to end for smoother resize
    super.scrollToIndex(this._virtualCount - 1);
  }
}

EDIT: Since _resizeHandler also invokes on item resize, could also consider adding a dedicated ResizeObserver to scrollTarget for this purpose (maybe it's overoptimizing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pretty clean fix. Updated the PR to use this approach. However, separating the resize handling to apply the fix only for the scroll target still reproduces the issue under some cases. Therefore, I left the resize observer a single one that applies the fix to both.

Copy link
Contributor

@sissbruecker sissbruecker Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This change seems to introduce a new issue when resizing a grid. It keeps adjusting the indexes in a loop, decreasing the scrollable area until the scroll position snaps back to the start. It looks like this happens when there is still some empty space at the top that could be filled with another item. This is likely cause by __fixInvalidItemPositioning, which adjusts the scroll position by a single pixel, potentially in a loop.

Some other observations:

  • The new workaround doesn't always add enough items to fill up the empty space, it looks like the topmost item is missing sometimes. That seems to be the cause for the issue above.
  • There already is a __fixInvalidItemPositioning which seems to detect a similar case with items being missing at either top or bottom, but the fix it applies isn't enough to make new items show up. However maybe it would be more appropriate to try to make that method work properly rather than add another workaround in a different place.
  • It's not clear to my why we need to use the scroll delta as part of the calculation. I would expect that there would be some way to detect how much space is empty, and use that space to determine how many items need to be added. __fixInvalidItemPositioning has some calculations that might help there.
Bildschirmaufnahme.2025-06-17.um.11.08.13.mov
Reproduction 
<script type="module">
  // PolymerElement based imports
  import '@vaadin/grid/all-imports';
  import '@vaadin/dialog';

  const grid = document.querySelector('vaadin-grid');
  grid.items = [...Array(1000)].map((_, i) => ({
    name: `Item ${i}`,
  }));

  const scrollButton = document.getElementById('scroll');
  scrollButton.addEventListener('click', () => {
    // Scroll to the end of the grid
    grid.scrollToIndex(1000);
  });

  const dialog = document.querySelector('vaadin-dialog');
  const dialogContent = document.getElementById('dialog-content');
  dialog.renderer = (root) => {
    root.innerHTML = '';
    root.appendChild(dialogContent);
  }
  dialog.opened = true;

</script>

<vaadin-dialog resizable draggable></vaadin-dialog>

<div id="dialog-content" style="height: 100%; display: flex; flex-direction: column; gap: 10px;">
  <vaadin-grid item-id-path="name" style="flex: 1">
    <vaadin-grid-column path="name" width="200px" flex-shrink="0"></vaadin-grid-column>
    <vaadin-grid-column path="name" width="200px" flex-shrink="0"></vaadin-grid-column>
    <vaadin-grid-column path="name" width="200px" flex-shrink="0"></vaadin-grid-column>
  </vaadin-grid>
  <button id="scroll" style="flex: 0">Scroll to end</button>
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This endless loop around __fixInvalidItemPositioning under some conditions was present before the fix already. This change seemed to fix it for me. But probably not all the cases as per your finding. This workaround/fix is based on the index adjustment logic iron list core _scrollHandler, which handles large jumps. The delta calculation comes from there basically. I will reexamine __fixInvalidItemPositioning and also consider Tomi's suggestions above and try to come up with a more stable fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the fix based on the suggestions and tried it with multiple setups. I cannot reproduce a loop anymore. The fix resides in an overridden _resizeHandler. While this can still be considered a workaround, I could not find a better fix around __fixInvalidItemPositioning.

Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,24 @@ export class IronListAdapter {
}
}

/** @override */
_resizeHandler() {
super._resizeHandler();

// Fixes an issue where the new items are not created on scroll target resize when the scroll position is around the end.
// See https://github.com/vaadin/flow-components/issues/7307
const lastIndexVisible = this.adjustedLastVisibleIndex === this.size - 1;
const emptySpace = this._physicalTop - this._scrollPosition;
if (lastIndexVisible && emptySpace > 0) {
const idxAdjustment = Math.ceil(emptySpace / this._physicalAverage);
this._virtualStart = Math.max(0, this._virtualStart - idxAdjustment);
this._physicalStart = Math.max(0, this._physicalStart - idxAdjustment);
// Scroll to end for smoother resize
super.scrollToIndex(this._virtualCount - 1);
Copy link
Member

Choose a reason for hiding this comment

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

While this works well for virtualizer generally:

Kapture.2025-06-19.at.09.15.07.mp4

Resizing grid still looks strange

Kapture.2025-06-19.at.09.20.59.mp4

This could possibly be fixed by calling this.scrollTarget.scrollTop = this.scrollTarget.scrollHeight - this.scrollTarget.clientHeight; after the scrollToIndex call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it seems to make the scrolling smoother. Updated the PR.

this.scrollTarget.scrollTop = this.scrollTarget.scrollHeight - this.scrollTarget.clientHeight;
}
}

/**
* Work around an iron-list issue with invalid item positioning.
* See https://github.com/vaadin/flow-components/issues/4306
Expand Down
33 changes: 33 additions & 0 deletions packages/grid/test/resizing.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,39 @@ describe('resizing', () => {
expect(grid.$.scroller.getBoundingClientRect().bottom).to.equal(grid.$.table.getBoundingClientRect().bottom);
});

it('should create rows when resized while scrolled to bottom', async () => {
// Have a full width grid inside a fixed width container
component = fixtureSync(`
<div style="height: 200px;">
<vaadin-grid style="height: 100%;">
<vaadin-grid-column></vaadin-grid-column>
</vaadin-grid>
</div>
`);
grid = component.querySelector('vaadin-grid');
grid.querySelector('vaadin-grid-column').renderer = (root, _, model) => {
root.textContent = model.item.name;
};
const itemCount = 1000;
grid.items = Array.from({ length: itemCount }, (_, i) => ({
name: `Item ${i}`,
}));
// Scroll to end
grid.scrollToIndex(itemCount - 1);
await aTimeout(200);
// Resize container
for (let i = 0; i < 10; i++) {
component.style.height = `${component.offsetHeight + 200}px`;
flushGrid(grid);
await aTimeout(50);
}
const gridRect = grid.getBoundingClientRect();
// Get an element from the area where new rows should be created
const elementInResizedArea = document.elementFromPoint(gridRect.left + 1, gridRect.top + 50);
const isCell = elementInResizedArea && elementInResizedArea.tagName === 'VAADIN-GRID-CELL-CONTENT';
expect(isCell).to.be.true;
});

describe('flexbox parent', () => {
beforeEach(() => {
grid.style.height = grid.style.width = '';
Expand Down