react-virtualized icon indicating copy to clipboard operation
react-virtualized copied to clipboard

scrollToRow on List with dynamic heights and CellMeasurer doesn't always scroll to the proper location

Open josephong opened this issue 7 years ago • 16 comments

On a List with many items, all of dynamic height that are measured using a CellMeasurer, sometimes, jumping to a smaller row index n from larger row m index fails. This more frequently happens when m is much larger than n.

Here's a plunkr to reproduce this behavior: https://plnkr.co/edit/B1463DyuByuIlmWTynRn

Steps:

  1. Enter a number between 600-800 in the box, and hit "Jump to row..."
  2. Enter a number between 200-400 in the box, and hit "Jump to row...". Check whether the row is visible.
  3. Repeat steps 1 and 2, using a different number in the ranges each time, until you notice that the row in step 2 isn't visible after the jump.

It usually takes anywhere from 1-5 repetitions for it to first occur. I've also noticed the error occurs with decreasing frequency the more jumps are done.

josephong avatar Jan 31 '18 19:01 josephong

Sounds like a bug, although I haven't looked at it yet. I doubt it ranks very high on our priorities list, but if you'd like to dig in and submit a PR- it would be welcome!

bvaughn avatar Feb 02 '18 01:02 bvaughn

I have solution

/**
 * Scroll to bottom of List/Grid
 */
const step = () => {
  const element = // .ReactVirtualized__Grid or List element
  const maxScrollTop = element.scrollHeight - element.clientHeight;

  if (maxScrollTop > element.scrollTop) {
     listRef.scrollToPosition(maxScrollTop); // Ref to <List /> component instance (or Grid)
     requestAnimationFrame(step);
  }
};

step();

You should do scroll until row index is >= startIndex and <= stopIndex scrollTop can be found by List#getOffsetForRow

yogurt1 avatar Mar 27 '18 22:03 yogurt1

I am also having this problem.

I think the issue is that scrollToRow functionality is using the value for defaultHeight in the CellMeasurerCache.

To see this in action, try going to @josephong's original Plunkr link. Try increasing the defaultHeight value in the cache to 10000, and scrollToRow will overshoot. Do the opposite and it will undershoot.

Looks like a "quick and dirty" solution is to call this.ref.measureAllRows() each time scrollToRow is called.

I would submit a PR myself, but to be quite honest it would take too long for me to understand the codebase.

I have a feeling that for someone familiar with the codebase this should be an easy fix! Would really appreciate if someone could solve it :)

ollie-o avatar Apr 09 '18 23:04 ollie-o

I have the same issue, my workaround is to call scrollToRow again in the setState callback, like:

this.listNode.scrollToRow(idx);
this.setState({ scrollingToRow: idx }, () => {
    this.listNode.scrollToRow(idx);
});

nullnoid avatar May 03 '18 09:05 nullnoid

Are there any other workarounds or solutions ?

BTW @oliverodaa, calling this.ref.measureAllRows() not solving for me

hlandao avatar May 22 '18 17:05 hlandao

I was having same problem in List solved it by using hack of setting an estimatedRowSize parameter to average row size value. StackOverflow Answer

rbgpt avatar Nov 12 '18 06:11 rbgpt

I was using the scrollToIndex prop to do the scrolling but due to this bug I also had to use scrollToRow() on componentDidUpdate as my workaround. The workaround is not ideal as it still has a little bounce/jump effect when the component updates.

zuus-keith avatar Dec 06 '18 05:12 zuus-keith

I found calling scrollToRow twice, once as normal and once in a setTimeout 100ms later was effective. calling measureAllRows or any of the other list/grid methods did nothing to solve the problem.

ryanwilliamquinn avatar Aug 26 '19 02:08 ryanwilliamquinn

Okay I have revisited this after a year to try to get a better solution. Here's what I came up with:

  • ScrollToRow should be a promise that resolves only after the row is rendered.
  • As @ryanwilliamquinn said, calling twice is really the only workable solution I found.
  • It's not necessary to call twice if you have already measured the height of the row you are scrolling to

So we basically just try twice and then resolve the promise after the second try.

const getElementById = id => listRef.current.Grid._scrollingContainer.querySelector(`#${id}`);

let promisedRow = null;

const scrollToRow = index =>
  new Promise(async resolve => {
    // If the elem is already on the page, do nothing
    if (getElementById(`row-${index}`)) return resolve();

    // If onRowsRendered is not called enough times to resolve the promise, this resolves it
    setTimeout(() => promisedRow && resolve(), 250);

    // If we've seen the row before, scroll once. If we haven't, scroll twice.
    // See this issue for explanation: https://github.com/bvaughn/react-virtualized/issues/995
    promisedRow = { index, resolve, remainingTries: cache._rowHeightCache[`${index}-0`] ? 0 : 1 };

    listRef.current.scrollToRow(index);
  });

// This function is called in onRowsRendered as a kind of "callback" after the scroll event occurs
const scrollToRowCallback = () => {
  if (!promisedRow) return;

  const { index, resolve, remainingTries } = promisedRow;
  if (remainingTries) {
    promisedRow.remainingTries--;
    listRef.current.scrollToRow(index);
  } else {
    promisedRow = null;
    resolve();
  }
};

<List {...otherProps} onRowsRendered={scrollToRowCallback} />

ollie-o avatar Aug 28 '19 20:08 ollie-o

I am also having this problem. I have to use dom to get element's top and pass it to scrollToPosition dynamiclly...

woshixiaoqianbi avatar Oct 30 '19 09:10 woshixiaoqianbi

Just want to add that the only solution that I have made work is the following:

this.listRef.current?.scrollToRow(this.state.messages.length - 1);
setTimeout(() => {
    this.listRef.current?.scrollToRow(this.state.messages.length - 1);
}, 0);

Essentially, like others have said, you must call it twice.

Unfortunately, those trying to use the measureAllCells() method before the call will not get the correct data. The method doesn't actually measure all the cells - it will leverage defaultHeight (which means no calculating is actually being done). Because of this, you're out of luck.

Luckily, I've been messing around with the double scrollTo trick with all kinds of nonsensical defaultHeight values and it seems to consistently work.

dilizarov avatar Apr 05 '20 06:04 dilizarov

A final comment:

You could actually get measureAllCells() to give correct truly measured data if you set defaultHeight: 0 within the cache - after which you would not need the double scrollToRow.

Of course, depending on your product, this is either not feasible, or totally feasible. Tread lightly.

dilizarov avatar Apr 05 '20 09:04 dilizarov

A final comment:

You could actually get measureAllCells() to give correct truly measured data if you set defaultHeight: 0 within the cache - after which you would not need the double scrollToRow.

Of course, depending on your product, this is either not feasible, or totally feasible. Tread lightly.

If you use defaultHeight: 0, your list will render all cells in one time for the first rendering, so using the library doesn't make any sense.

sfsr avatar May 14 '20 13:05 sfsr

Yep. Hence, tread carefully. Usually, it doesn't make sense.

dilizarov avatar May 16 '20 06:05 dilizarov

this works in my case

const [cb, setCb] = useState(0)

useEffect(() => { listRef.current.scrollToRow(messages.length) setCb((x) => x + 1) }, [activeChannel, messages.length])

useEffect(() => { listRef.current.scrollToRow(messages.length) }, [cb])

rifky-rangkuti avatar Jul 29 '21 13:07 rifky-rangkuti

Okay I have revisited this after a year to try to get a better solution. Here's what I came up with:

  • ScrollToRow should be a promise that resolves only after the row is rendered.
  • As @ryanwilliamquinn said, calling twice is really the only workable solution I found.
  • It's not necessary to call twice if you have already measured the height of the row you are scrolling to

So we basically just try twice and then resolve the promise after the second try. ...

This is a really nice solution. I just needed to call the scrollToRow function after a timeout (inside scrollToRowCallback). Like this:

scrollToRowCallback = () => {
    if (!this.promisedRow) return;

    const { index, resolve, remainingTries } = this.promisedRow;
    if (remainingTries) {
      this.promisedRow.remainingTries--;
      setTimeout(() => {
        this.listRef.current.scrollToRow(index);
      }, 100);
    } else {
      this.promisedRow = null;
      resolve();
    }
  };

batatop avatar Apr 15 '22 17:04 batatop