petyosi / react-virtuoso

The most powerful virtual list component for React
https://virtuoso.dev
MIT License
5.25k stars 301 forks source link

[BUG] Items get calculated incorrectly when child of flex parent #522

Closed genepaul closed 2 years ago

genepaul commented 2 years ago

Describe the bug This may be a bug, or it may be incorrect usage. I am using Virtuoso inside a Material UI Popover, and placing it inside a flex container so it takes up the remaining portion of the popover after some content (see codesandbox link). My understanding is I should be able to have a structure like this:

<div style={{
   display: 'flex',
   flexDirection: 'column',
   height: '100%'
  }}
>
  <div> {'some content of unknown or variable height'}</div>
  <div style={{flex: 1}}>
      <Virtuoso height={'100%'} .../>
  </div>
</div>

And it seems this works just fine when this is just in the regular DOM, as shown in the codesandbox here. But when I put it in a Popover with this same sort of structure, it doesn't seem to be able to calculate the right number of items to show, and you'll notice when scrolling to the bottom of the list there's a gap and not all of them render.

Reproduction https://codesandbox.io/s/vigilant-rgb-xugwm?file=/src/components/App.js

To Reproduce Steps to reproduce the behavior:

  1. Go to the codesandbox
  2. Click the Expand button
  3. Scroll the list down

Expected behavior Should render all items with no gap at the bottom

Desktop (please complete the following information):

genepaul commented 2 years ago

I will note that the example codesandbox I linked to where it seemed to be working is using version 0.12.0, while my codesandbox is using 2.2.7 (and I've been able to reproduce up to the latest version). But if I update the example (not in a popover) to latest, it still does not reproduce the issue. It seems I'm only seeing this in a popover, so I'm not sure what's different.

genepaul commented 2 years ago

Actually, I took the time to go through the versions of react-virtuoso in my codesandbox, and it seems the issue starts showing up with v1.10.7. The version before that, v.1.10.6, does not seem to have scrolling or gap issues.

petyosi commented 2 years ago

v1.10.7 switched from using offsetHeight to using getBoundingClientRect for item sizing. I'm not sure about the internals of the Expand component, but from what I see, the viewport size is not recalculated properly. This can happen for variety of reasons, including margins or animations.

It's unlikely for me to investigate that in a reasonable timeframe, especially given that it needs mui for reproduction (which in general has a lot going on in terms of styles and positioning). If you're looking for a workaround, you can try overriding itemSize to use offsetHeight (https://virtuoso.dev/virtuoso-api-reference/).

genepaul commented 2 years ago

So yeah, it's definitely not getting the right height on mount. Here's what this looks like in my own code:

image

image

Above is what the elements look like in the DOM, but here's what Virtuoso thinks about the first one:

image

Interestingly, if I click any of the checkboxes, causing a re-render of the Virtuoso, it updates the data-known-size on all the list items to the correct value. So yeah, there's definitely an incorrect height getting reported. And indeed, when I log the height (either getBoundingClientRect or offsetHeight) in itemSize, it's incorrect when it initially mounts. I appreciate you looking at this, and it's definitely something with the interaction between Material and Virtuoso, so I don't expect you to dig into Material's code. But if any of this triggers any ideas for you, let me know. I'll keep digging. Thanks!

genepaul commented 2 years ago

Actually, one thing above is incorrect: in itemSize, el.offsetHeight is correct, but if I return that from itemSize I still have the rendering issue. So I'm not sure what else might be going on here.

petyosi commented 2 years ago

I'm not sure if the green part from the above is a margin - if so, it will confuse the layout a lot. offsetHeight does not include it.

genepaul commented 2 years ago

I'm fairly certain it's padding. I tried to clear margins.

genepaul commented 2 years ago

@petyosi, I determined what the issue is. It appears to be due to the fact that the Material UI Popover animates a transition when it enters. The Virtuoso tries to grab dimensions while the Popover is transitioning and gets incorrect information. If I defer the rendering of the Virtuoso until the Popover has fully entered, the Virtuoso works correctly. So this is not an issue with Virtuoso. I just wanted to add the update here for anyone else who happens upon this issue and wonders what the problem was. If I come up with working code I'll try to update with a codesandbox. I did a really dirty hack in my testing where I just did a setTimeout in a useEffect and that proved the issue. I think there's probably a better way to do it though. Thanks for creating such an awesome library!

trainoasis commented 6 months ago

@petyosi, I determined what the issue is. It appears to be due to the fact that the Material UI Popover animates a transition when it enters. The Virtuoso tries to grab dimensions while the Popover is transitioning and gets incorrect information. If I defer the rendering of the Virtuoso until the Popover has fully entered, the Virtuoso works correctly. So this is not an issue with Virtuoso. I just wanted to add the update here for anyone else who happens upon this issue and wonders what the problem was. If I come up with working code I'll try to update with a codesandbox. I did a really dirty hack in my testing where I just did a setTimeout in a useEffect and that proved the issue. I think there's probably a better way to do it though. Thanks for creating such an awesome library!

Did you manage to do it in a nicer way? I have the exact same issue :)

genepaul commented 6 months ago

@trainoasis - I think I ended up moving away from Popover to using Popper and didn't have the same issue. I may have also moved away from flex display. I don't remember exactly, unfortunately. I've also since upgraded MUI and a lot of other libraries.

Sushant-Rana commented 4 months ago

@trainoasis Hi , facing a similar issue , any luck with the solution ?