recomputeRowHeights is not always re-rendering

[chiss2906]

recomputeRowHeights() is not re-rendering some of the VirtualScroll cells properly, even though the function passed to rowHeight is receiving a proper value and I call recomputeRowHeights(index) when the height changes.

Mostly it happens to cells which contains an image, which means their height changes. Less often it happens to rows without it. So basically I would say not all rows that need re-rendering are re-rendered.

Also, sometimes the cell gets re-rendered, but the height does not get updated. I know this because I show the height in the cell and it doesn't match the actual height.

I'm storing the row height in a heights object, which has been filled with id: height pairs from dimensions received by react-measure component. The rowHeight gets returned from that.

I tried adding a prop to VirtualScroll that counts the number of height updates, and thus is different every time, but that does not help either.

[bvaughn]

Hey @chiss2906,

I'll need you to provide a Plunker that demonstrates what you're seeing. I'm fairly confident that the issue here lies somewhere in your props/state tracking and not in VirtualScroll - but I'll be happy to take a look if you can point me at a runnable demo with source code.

[chiss2906]

@bvaughn Here is the link to the Plunker: https://plnkr.co/edit/o4ArW2Os4XDdlTV8OuRx?p=preview :)

[bvaughn]

When resize is requested, your VirtualList component is logging "not yet!", meaning that it isn't passing this call through to VirtualScroll. Since VirtualScroll is pre-rendering a few rows below (overscanRowCount) this means that those rows are never getting remeasured. If you set overscanRowCount={0} on the inner VirtualScroll you can see that they do get updated as you scroll- but still not the first visible set.

Here's a fixed version:
https://plnkr.co/edit/lTFmKOQlJLIehL1LIY47?p=preview

The key basically boiled down to checking to see if any heights had been recomputed before the ref was set, and letting VirtualScroll know about them on-ref-set.

[wmertens]

@bvaughn I believe this is not the whole story: https://plnkr.co/edit/fy1GmIbjs255ZqAPi6ew?p=preview

I added something that blinks every few seconds, and if you enable the alternatel height detector, it will adjust.

However, I noticed that if there are a number of recalculation requests coming in before VirtualScroll gets to it, it will handle the last one and not all the ones requested. I think the callback should keep track of the lowest index requested.

[bvaughn]

However, I noticed that if there are a number of recalculation requests coming in before VirtualScroll gets to it, it will handle the last one and not all the ones requested. I think the callback should keep track of the lowest index requested.

This doesn't sound correct to me, but maybe I'm misunderstanding you. VirtualScroll.recomputeRowHeights calls Grid.recomputeGridSize to clear the cached sizes after the specified index and then immediately re-renders the Grid. The CellSizeAndPositionManager.resetCell method synchronously marks all cells after (and including) the specified index as stale which means the next time CellSizeAndPositionManager. getSizeAndPositionOfCell is called, those cells will be re-measured.

This means if you called recomputeRowHeights(5); recomputeRowHeights(2); recomputeRowHeights(10); the next time you asked for row 2- it would still be re-measured.

[bvaughn]

This seems to be what I'm seeing, looking at your demo too. Looks like rows are being expanded and shrunk as "blink" is added and removed.

[bvaughn]

However, I noticed that if there are a number of recalculation requests coming in before VirtualScroll gets to it, it will handle the last one and not all the ones requested.

Ah, I think I understand. And I think this is just how your test harness is setup. Each NewsCell is setting a timeout that clears and triggers a re-render. Let's say cells A, B, and C are visible. If cell A triggers a re-render, VirtualScroll will immediately re-measure A, B, and C. Then if cell B triggers a re-render right after this, VirtualScroll will immediately re-measure B and C. Then if cell C triggers a re-render right after that, VirtualScroll will immediately re-measure cell C. You may only notice the most recent message in the log - but they're all being re-measured.

[wmertens]

I think in my desire to make the blinking remeasuring work, I muddled my point. I made the plunker more obvious.

The problem is that, if you keep the regular row height detector (which can't detect when its sub-children change height and therefore won't call recalc a lot), you can quickly scroll down to some location, and then there will be cells that do not have the correct height. Slow scrolling works fine.

Here's an example:

the console log for that section is:

VM111283 script.js:263 vs render 24
VM111283 script.js:263 vs render 25
VM111283 script.js:263 vs render 26
VM17858 script.js:189 22 => 266
VM17858 script.js:192 recompute 22
VM17858 script.js:189 23 => 79
VM17858 script.js:192 recompute 23
VM17858 script.js:189 24 => 266
VM17858 script.js:192 recompute 24
VM17858 script.js:189 25 => 79
VM17858 script.js:192 recompute 25
VM17858 script.js:189 26 => 96
VM17858 script.js:192 recompute 26
VM130744 script.js:274 req 26 => 96
VM130744 script.js:274 req 27 => 150
VM130744 script.js:274 req 28 => 150

So as you can see, a recompute is requested for 24, but it doesn't trigger a request for the new height.

[bvaughn]

Sorry @wmertens but I'm not sure I understand. If you scroll quickly, you see some temporary overlap- but this is due to the way the test harness is written from what I can see.

1. When a row's height is first requested, it defaults to 150 (const height = heights[index] || 150)
2. Its content may not actually fit inside of that height so the rows sometimes overflow initially.
3. Once the interval hits, VirtualScroll is told to remeasure and the overflow fixes itself.

Am I misunderstanding or missing something?

So as you can see, a recompute is requested for 24, but it doesn't trigger a request for the new height.

Not sure about this one. My guess would be that row 24 is no longer visible which is why react-virtualized only calculates 25+. This would seem to be backed up by the errors I sometimes see in the console:

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the ReactElementResize component.

You can trace the flow of code (I pasted it above) for resetting a row's height. It's pretty straight forward I think. This is also an area I have automated test coverage on (eg here and here).

[wmertens]

The temporary overlap is not temporary, the screenshot is what it looks like when I stop scrolling. (using react-measure and react-height, the other one forces recalcs all the time)

Here's how I can always reproduce it: scroll quickly to the end and then scroll back up at normal speed. You will see some overlapping cells that are not the correct height.

I noticed the setState errors too, weird because I do clear the interval handler before unmount…

All in all, it seems like the problem is incorrect height caching, it may be better if you always call rowHeight?

[wmertens]

the tests don't seem to cover the case where the cell is no longer visible by the time the measurement needs to be done…

[bvaughn]

Ah! Scrolling backwards after skipping...

resetCell (index: number): void {
    this._lastMeasuredIndex = index - 1
}

This is the problem. I was not anticipating frequent calls to resetCell and I missed an obvious Math.min check there. Give me a minute to write a new test and then push a fix.

[bvaughn]

Thanks for sticking with it @wmertens :)

7.12.1 should fix this edge case. I'm still not able to repro what you describe in the test harness, but maybe you can bump the react-virtualized version and verify the fix?

[wmertens]

🎉 7.12.1 fixes it 👍

I wonder if dynamic rows with VirtualScroll + react-height is not a common enough need that it should be part of react-virtualized? In my very un-scientific eyeballing, it seems to be faster than react-measure, and tiny.

So when you pass e.g. autoHeight to VirtualScroll, it would automatically add ReactHeight on the rows and measure things itself?

[wmertens]

BTW, I was using Chrome 51 on OS X for the repro, and you? Just curious…

[bvaughn]

Same, Chrome 51 on OS X El Capitan.

I wonder if dynamic rows with VirtualScroll + react-height is not a common enough need that it should be part of react-virtualized? In my very un-scientific eyeballing, it seems to be faster than react-measure, and tiny.

How's react-height different from CellMeasurer?

[wmertens]

Hmmm, the docs for CellMeasurer say not to use it for VirtualScroll, and the ones for react-height don't mention VirtualScroll :)

Actually, I find its API a bit confusing…

[bvaughn]

CellMeasurer's interface was written for Grid; that's why the docs say that. But since VirtualScroll and FlexTable are really just Grids, you definitely can use it with them. You would just need to adapt the parameters to match the expected format. Sorry you find the interface confusing. I think the docs are pretty straight forward but feel free to suggest improvements.

[wmertens]

Well, for me the ideal API would be rowHeight="auto" or whatever on VirtualScroll and it takes care of the rest.

So what I find confusing about the API, I didn't use it before so I'll just call it as I see it:

• cellRenderer and children, why are they both needed? Also, not super fond of function-as-children, I'd rather pass that as a prop.
• column/rowCount: I presume these are used to know when an entire row/column is measured, but it seems a bit magical, it presumes some sort of data sharing behind the scenes. I would expect to give this information to the Grid only
• Why is CellMeasurer a parent of Grid? When I use ReactHeight, it measures its children, but here it seems that somehow Grid is altered by it. Perhaps it should be called MakeCellsMeasurable?
• Passing props called e.g. "getColumnWidth" makes me think that I'm supposed to use them. However then need to be passed to Grid, so why not call it widthGetter?

So the way I understand it now is that CellMeasurer alters Grid somehow, and I would expect that to work more like Redux's connect(), which takes a class and returns an augmented class…

Mind you, you did an excellent job on this library, I'm just confused…

[bvaughn]

Sorry @wmertens. Didn't mean to ignore. Traveling in Japan at the moment and have spotty Internet access. :)

I understand your confusion. Let me try to address the things you mentioned briefly.

• cellRenderer and children: Child functions are pretty powerful when it comes to composition. Without then, it would be very hard to combine HOCs like InfiniteLoader, AutoSizer, CellMeasurer, etc because each would essentially have to be aware of the properties/methods of the others in order to pass things through correctly. I initially did this (children instead of child functions) and things were greatly simplified when I made the switch. (This change was made in version 5.0.0 so you can look at an older release if you're curious what the signatures used to look like.)
• column/rowCount: It isn't mean to be magical. :) In order for CellMeasurer to report the measured height of a row, it needs to measure all columns in the row and take the max. Ditto for the width of a column (need to measure that column in all rows and take the max).
• Why is CellMeasurer a parent of Grid? Because Grid needs a function that returns a cell's width or height. That function is passed as a parameter to the child function of CellMeasurer. I agree it's a little counter-intuitive at first, since the cells measured are inside of the Grid but I am not sure of another way to accomplish this. :)
• Passing props called e.g. getColumnWidth ...: Naming is hard. This is why I write lots of docs. I'd be willing to change the name, except that doing so would require a major release (since it would be non-backwards compatible) so it seems ... not worth it. :)