Scroll position cache not purged on update

[kof]

Hi,
me and @giuseppeg stumbled over an issue in case of a chat with reverse scrolling behavior + scrollToIndex option. When a raw of a List gets updated and its height becomes bigger, RV is currently not recalculating the scroll position. Value of scrollToIndex remains the same before and after the update.

[kof]

@giuseppeg can you point to the line we traced it to?

[bvaughn]

Sounds like a misunderstanding.

RV assumes new rows are appended to the end of its data source. So new rows should not impact scroll position.

If you know they do then you can tell RV to recalculate the offset using the scrollToRow method. https://github.com/bvaughn/react-virtualized/blob/master/docs/List.md#scrolltorow-index-number

Reverse lists require some extra management in application land. They're much less common.

[kof]

If you know they do then you can tell RV to recalculate the offset using the scrollToRow method.

we tried, didn't work and we tracked it down to a specific line.

Basically RV receives a new array of rows, new data gets rendered, its just the problem that scrollToIndex doesn't change which is correct in case of an "update".

[bvaughn]

Pretty sure the error is in your application code. This particular functionality has been in place and tested for a long time. I'll need a repro case.

[giuseppeg]

@bvaughn just to clarify our issue is with last row updates not appending.

In a list of length 3 with scollToIndex set to 2 and scrollToAlignment set to end react virtualized scrolls to the bottom as expected.

However if we update the list, specifically if we update the item at index 2 (and its height changes), then Grid (we use List) doesn't update the scroll position – I believe because scollToIndex and scrollToAlignment didn't change.

If we are not mistaken the issue could be here https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/utils/CellSizeAndPositionManager.js#L53

Please let me know if this still sounds like a problem with our application code.

In that case I will try to put together a test case and try to reproduce in an isolated environment before I go back to debugging :) thank you!

[bvaughn]

Thanks for the additional details, @giuseppeg.

If the size of a row changes, you need to let react-virtualized know about it. Given that you mentioned a specific row resizing, I assume you're passing a function prop for rowHeight. In this case, there's nothing in the props passed to List that would let it know that the height of a specific row has changed; that's all internal to the function. (If you're using a fixed rowHeight number on the other hand, List would automatically handle invalidation. You can see that by going here, entering a "scroll to" index, and then changing row heights. I also just double checked when writing this to make sure I wasn't mistaken.)

With the List component, you let it know the underlying measurements have changed by calling recomputeRowHeights with the index of the row that has resized. (If you're also using CellMeasurer- I assume you are not, but in case- you would also need to call its resetMeasurementForRow method to purge the cached measurement for that row.)

Anyway, the recomputeRowHeights method resets the _lastMeasuredIndex in CellSizeAndPositionManager so the line you linked to should not be a problem.

If you still think you're seeing a problem, I'll need either a Plnkr or a failing unit test to be submitted from you guys so that I can see it too. 😄

[giuseppeg]

Thanks @bvaughn! That's very useful information.

I assume you're passing a function prop for rowHeight

correct

If you're also using CellMeasurer- I assume you are not, but in case- you would also need to call its resetMeasurementForRow method to purge the cached measurement for that row.

Yep we are using CellMeasurer. I will try to add a call to resetMeasurementForRow!

[kof]

I will try to add a call to resetMeasurementForRow!

We did try that, the height gets properly recalculated for setting the inline row height, but gets ignored by the scroll position measurement.

[bvaughn]

Give me a repro case or a failing unit test pls. 😄

[giuseppeg]

hi @bvaughn sorry for the delay.

I put together a test case that should reproduce our current behavior https://plnkr.co/edit/aQ27JepnpFIrtESFRJPz?p=preview

Basically I simulate an update after 2s and List doesn't update the scroll position.

In our real app we call recomputeRowHeights on componentWillReceiveProps.

[bvaughn]

Thanks for the repro. Will try to find some time to dig into it, but likely won't be too soon. Have a lot on my plate. If you get a chance to dig in first- I'd welcome the help.

[giuseppeg]

No worries! I've been debugging "a bit" but I would need more time as well :) As a workaround for now we are using List's scrollToRow to force scrolling.

[bvaughn]

That's a good workaround 😁 Maybe once I get 9.0.0 shipped and give it a few days for any bug reports to trickle in, I can dig into this more

[bvaughn]

FYI I've caught this in a small unit test:

it('should update scroll position to account for changed cell sizes', () => {
  let rowHeight = 20
  const props = {
    height: 100,
    rowCount: 100,
    rowHeight: ({ index }) => index === 99 ? rowHeight : 20,
    scrollToRow: 99
  }
  const grid = render(getMarkup(props))
  const node = findDOMNode(grid)
  expect(node.scrollTop).toBe(1900)
  rowHeight = 40
  grid.recomputeGridSize({
    rowIndex: 99
  })
  expect(node.scrollTop).toBe(1920)
})

The behavior demonstrated is because of this check. The helper doesn't have enough information to detect when a cell size has changed if the size is determined by a function property.

I think the most appropriate fix is to set a flag when cell sizes are recomputed to tip the next cDU off that its scroll offset may be invalid.

Will be fixed in 9.0.1 (releasing presently)

[kof]

Beautiful!

[noeljackson]

Was this issue fixed? I’m still seeing an issue where scrollHeight does not get recomputed.