schrodinger/fixed-data-table-2

When using variable row heights, setting `scrollTop` doesn't work well. Fix proposed

cristian-spiescu opened this issue · 4 comments

cristian-spiescu commented

Current Behavior

We have created a sample where we have variable row heights provided via rowHeightGetter property: first row height = 30px and the rest of the rows height = 500px, cf. here. rowheight property = 30.

When the scroll button is pressed (scrollTop changes from 0 to 5000), the first row displayed is being 41. This is wrong. The correct value is 11: because 1 (i.e. the first row) x 30 + 9 (the following rows) x 500 = 4800.

Possible Solution

This happens because the rowHeightGetter is not invoked several times, in order to inspect the custom height of each row.

Our fix

We have done the following modification. Not knowing if this has a big performance impact, for the moment we also added a flag to enable/disable it.

Screenshot, before the fix

image

Screenshot, after the fix

image

Questions

1/ Before submitting the PR, we'd like to hear if there are preliminary thoughts.
2/ The flag: should we keep it? Or remove it and always apply the new behavior? We don't know how to benchmark the performance impact. A "manual"/subjective evaluation of the samples w/ a lot of data, doesn't seem to show degraded perf.
3/ Do you think we should also include (in the PR) the example we created?

Context

We work on a Gantt component:

image

On the right, our component. On the left: your component. The main use case is to synchronize the heights of the rows and scrollTop:

  • on scroll in Gantt
  • on scroll in Table
  • on expand an item
  • on programmatic scroll
  • etc.
pradeepnschrodinger commented

Hey @cristian-spiescu , thanks for filing this in a very detailed manner!
The context behind the issue, example/code changes, and screenshots have been very helpful 😄 .

FDT calculates row heights lazily, so the offsets for the currently visible rows at any given point of time will be inaccurate until all the row heights of the previous rows are calculated.
This was a design decision focusing around lazy loading data and maximising performance against huge:no of rows with moderate sacrifices to offset accuracy.

As you noted, this doesn't work out well when scrolling to an arbitrary offset and expecting it to match with other scrollable containers.

I think having some way to make FDT obey accurate offsets will be awesome.
Please put the PR in. We can continue the discssion there.

Some immediate questions for the PR:

  • Could you test exactScrollTopInCaseOfVariableRowHeights with controlled scrolling?
    • Controlled scrolling is a typical use case in FDT where the scroll offsets are kept continously in sync with FDT via scroll handlers like onVerticalScroll (See AutoScroll example).
    • I expect FDT will preload the heights of all previous rows on EVERY render in this case.
    • This could be expensive for really large tables, especially if we need 60fps renders.
  • Should we employ a cache (or something to skip previous fetches) for performance?
    • If we do, then we'll need to figure out how a user can invalidate the cache (i.e, update row heights later) ?
    • eg: react-window has a similar use case and their design is to expose an API method to let the user manually clear the cached size of a specific item, causing the container to rerender and refetch data after that item.
cristian-spiescu commented

Hi @pradeepnschrodinger,

My colleague Daniela has submitted the PR #701. We have implemented a cache logic. Now on every componentDidUpdate() a new loop for calculation IS NOT made, as in the initial commit. We are waiting for your feedback.

  • 🎉1
pradeepnschrodinger commented

Nice, thanks! We'll checkout the PR with the devs sometime next week.

cc: @AmanGupta2708, @karry08, @abhisheksingh2410

pradeepnschrodinger commented

Released the fix for this issue with v2.0.5 via #701.
cc: @daniela-mateescu, @cristian-spiescu, thanks for your contribution!

Summary:
The new optional table prop isVerticalScrollExact (defaulted as `false) can be turned on to use precise vertical scrolls in FDT.
This works by making FDT fetch row heights for ALL rows in the table.
There's also an internal cache so that FDT won't keep re-requesting all rows on every render.

Incase the cached row height needs to be reset, the new public API updateRowHeights(rowIndex) can be used to reset the row heights starting from rowIndex until the end of the table.