bvaughn / react-virtualized

React components for efficiently rendering large lists and tabular data
http://bvaughn.github.io/react-virtualized/
MIT License
26.27k stars 3.06k forks source link

Dynamic rows go missing with WindowScroller+AutoSizer+Collection #551

Closed djeeg closed 7 years ago

djeeg commented 7 years ago

Hi Brian,

Thought I would report an oddity I am seeing with dynamic loaded items when using this HOC combo

Here is my test https://plnkr.co/edit/86TUSrHmQ8y9wFrEc8iK?p=preview

If I set the cell size to something small, say [weight: 20], then I dont see the issue

If I set the cell size to something larger, say [weight: 300], a few things go odd

  1. not all the rows display per page, they sometimes stop either at 12 or 16 cells displayed
  2. when I click the button to dynamically load the next page, I see the scroll bar gets bigger, so assume the items get detected, however the extra items dont render
  3. if I then make a single scroll movement down, the cells appear

I have tried a few things, though they didnt seem to work

bvaughn commented 7 years ago

I assume this is just a much-reduced version of your application, but I feel like I should point out that if your UI looks anything like this- you should be using Grid or List instead of Collection for improved performance. 😄

That being said, this looks like a WindowScroller+Collection bug. Can't repro if I replace Collection with Grid or if I remove WindowScroller. It's interesting to note that in my local copy of the react-virtualized demo site, WindowScroller+Collection play nicely together as well.

Examining the DOM while scrolling, it seems like Collection is bouncing back and forth between the rows you've scrolled to and the initial set of rows (at scrollTop 0)- but only when you're viewing rows near the end of the set.

To be honest, I don't think I'll have the time to dig into this any time soon. I don't use Collection in anything other than the demo site and so it's a bit harder to support. If you'd like to dig in though, I'd be happy to review a PR from you!

mauron85 commented 7 years ago

Not sure if I'm facing same issue. But my problem seems to be AutoSizer with ScrollSync.

I'm trying to develop timeline component

image

It should render 48 hour intervals between two dates (1/30/2007 - 1/31/2007) in table header and courier routes (rendered as cascade of squares), courier working hours and striped background in the table body.

My problem is that scrolling is only possible up to last courier route (3PM). There are no other routes, but I would like to enable scrolling to very last hour interval (11PM)

It's working when there is only one component in table body, but it's not when there more of them. In this case there is only stripe background.

image

Pseudo code:

this is working perfectly

<ScrollSync>
  <AutoSizer disableHeight>
    <Grid className={styles.HeaderGrid} /> {/* hour intervals */}
    <div className={styles.Body}>
       <Grid /> {/* table striped background */}
    </div>
  </AutoSizer>
</ScrollSync>

but scrolling is limited when multiple children in AutoSizer:

<ScrollSync>
  <AutoSizer disableHeight>
    <div>
       <Grid className={styles.HeaderGrid} /> {/* hour intervals */}
       <div className={styles.Body}>
         <Grid /> {/* table striped background */}
         <Collection/> {/* courier routes has to be collection */}
         <Collection/> {/* courier working hours has to be collection */}
       </div>
    </div>
  </AutoSizer>
</ScrollSync>
mauron85 commented 7 years ago

I've made sample project. https://jsfiddle.net/mauron85/5vverasn/

Comment out block: {/* when following commented scrolling works as expected */} to get expected scrolling behaviour.

bvaughn commented 7 years ago

@mauron85 This doesn't look like the same issue to me.

Have you tried posting your issue on Stack Overflow?

mauron85 commented 7 years ago

@bvaughn sorry for posting my question here. I've created question on stackoverflow: http://stackoverflow.com/questions/41956669/unable-to-scroll-pass-the-last-child-with-scrollsync-with-multiple-grids-and-col

Still investigating what is the issue and how to fix. Will appreciate your oppinion of last paragraf on stackoverflow.

Not 100% sure where is the catch, but I believe it's because every child component Grid, Collection ... has its own ReactVirtualizedCollectioninnerScrollContainer, which has different width and only very last in DOM tree is actually scrollable/visible. Playing with z-index of components confirms this.

Could it be the issue?

bvaughn commented 7 years ago

@mauron85 I've responded to your issue on Stack Overflow. Sorry it took me a while but I get busy and sometimes things fall through the cracks.

As for @djeeg's original report, I've copy-pasted the code you have in Plnkr into a stand-alone page (so I could test it against my local build of react-virtualized) and it worked fine. Removing the styles.css file from your Plnkr also resolved the problem.

Turns out the problem is caused by the border style. Remove it and the problem goes away. Or specify a box-sizing: border-box style for your cells and the problem also goes away. Without the box-sizing attribute, the cells are actually slightly too large (by the size of the border) for the container and I think this is causing some horizontal scroll or something else funky that wipes out the vertical scroll offset.

box-sizing: border-box should fix you up though 😄

djeeg commented 7 years ago

Thanks @bvaughn for looking into this.

Your suggestion does fix the plnkr, and I have tried to apply the concept to my use case (constrain the tile dimensions), however unfortuately it does not solve my underlying issue. :cry:

I have done a bit more investigation, and it seems I only get this issue in IE Firefox/Chrome look to be okay.

I started debugging /dist/es/Collection/CollectionView.js to try to see what was going on.

Narrowed it down to this part https://github.com/bvaughn/react-virtualized/blob/ad6200c15844a5585032e2848b8fd90af3e66143/source/Collection/CollectionView.js#L528

It seems that in IE, I get an extra scroll event which resets the scrollTop from halfway down the page (2000+), back to 0, even though the scrollTop is correctly passed in as props from <WindowScroller> Which then makes <Collection> think it is scrolled to the top, so it renders out the tiles at the top of the page, which i can't see halfway down the page. When i check the HTML source, I can see the top couple of tiles there.

See below, if I add a hack to ignore this "extra" scroll event, then the problem is solved in IE There doesnt seem to be any knockon issue in any other parts of my usage, thats not to say this doesnt cause a bug somewhere else though.

    key: '_onScroll',
    value: function _onScroll(event) {
      if (event.target !== this._scrollingContainer) {
        return;
      }
        if (event.target.scrollTop < 10 && this.state.scrollTop > 100) {
            return;
        }

Note, there is a rebind of the <Collection> here, as I am adding items However the <WindowScroller> state should be unchanged

Component1

Component2 (redux bound to a immutableJS list)

bvaughn commented 7 years ago

Collection shouldn't be scrolling (or scrollable). It should overflow and scrolling should be managed entirely by the WindowScroller (based on document scrolling). But the borders overflowing their size (as they do with the box-sizing: content-box) causes Collection to be slightly scrollable- b'c of the border overflow- and that causes these undesirable scroll events to reset its state.

You can see this problem demonstrated by the double scrollbars it causes (if you have scrollbars enabled):

screen shot 2017-02-22 at 9 58 29 pm

For me this problem goes away entirely if you add the box-sizing style I mentioned above.

To be honest I'm not sure of the best way to tackle this. It's weird. We could, perhaps, ignore the scrollTop value of (internal) scroll events for Collection if the autoHeight property is enabled. Because we shouldn't get any in that case. But I don't know why that's necessary...