[DataGrid] Virtualization not respecting scrollTop

[oliviertassinari]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

End-users can feel a difference between virtualization enable and disabled. The scrolling experience is not continuous, it jumps. The scroll height is also wrong, it's too short.

Expected Behavior 🤔

End-users shouldn't feel any difference between virtualization enable and disabled. Scrolling should be:

• Continous (e.g. not in jumps like it currently do)
• Move at the same speed (e.g. not like https://ag-grid.com/react-grid/massive-row-count/)
• Have the correct height (e.g. not be too short to host all the rows)

Steps to Reproduce 🕹

Steps:

1. Open https://codesandbox.io/s/material-uix-gridscroll-to-indexes-demo-simple-forked-on0dc
2. See how the scroll-top is discontinue disorienting end-users.
   

You can also see there is a blank space at the bottom, but it's #414.

Context 🔦

Raised by @m4theushw in #1871.

I found that the amount of scroll needed to make a row visible is less than what I would normally expect (rowIndex * rowHeight)

It's KO. It would only make sense for this use case: https://ag-grid.com/react-grid/massive-row-count/.

I suspect the best course of action is to rewrite the virtualization logic. Use this as an opportunity to take the following constraints into account in the design.

• Built-in support for no virtualization [DataGrid] Allow to disable virtualization #1781
• Variable row height [DataGrid] Variable row height #438
• Dynamic row height [DataGrid] Dynamic row height #417
• Performance, this one suggests an inserting path [DataGrid] Use binary search to find column indexes in virtualization #1903, it seems to match what react-virtuoso, react-virtual, react-window, and react-virtualized as using as a technique. We could maybe even market it as a standalone module.
• Research into overscan [RFC] Row virtualization overscanning tradeoff #270

[oliviertassinari]

Looking at the reproduction, we are completely off. We have 34 rows of 50px height with a viewport of 290px height. So the max scroll top should be = 34 * 50 - 290 = 1410px. Instead, it's 1250px 🙃.

[m4theushw]

Looking at the reproduction, we are completely off. We have 34 rows of 50px height with a viewport of 290px height. So the max scroll top should be = 34 * 50 - 290 = 1410px. Instead, it's 1250px 🙃.

Yeah, when I was playing with the virtualization to understand it I noticed that we use a different calculation for the height of the container. If we do the right thing the user will have to scroll more, but there will be no discontinuation when reaching the top.

There's also a blank space that might appear when calling scrollToIndexes. Making the scrollTop to honor the theoretical position of the row will fix it too. BTW, we could improve this API to allow the user to determine if he wants: 1. scroll as little as possible to make the row visible or 2. scroll as much necessary so the row is at the top of the viewport. AG Grid in ensureIndexVisible even allows to scroll to place the row at the middle or bottom of the viewport.

[oliviertassinari]

we could improve this API to allow the user to determine if he wants: 1. scroll as little as possible to make the row visible or 2. scroll as much necessary so the row is at the top of the viewport. AG Grid in ensureIndexVisible even allows to scroll to place the row at the middle or bottom of the viewport.

True, makes me think about the different options of https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

[m4theushw]

Fixed by #2673