Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two <WindowScroller>s on a page causes the second one to not render properly #1324

Open
apelsinet opened this issue Feb 8, 2019 · 9 comments

Comments

@apelsinet
Copy link

When I'm trying to render multiple lists (2 in my case) after each other, both using <AutoSizer> and <WindowScroller>, the second one does not render properly. It takes up the correct amount of space on the page, but the rows are gone except for a few at the bottom of the page. If you scroll up, a few more of them appears. This issue does not happen after resizing your window, or if you set the width of your <List> component to a static value.

See codesandbox example.

@nicolasschabram
Copy link

@apelsinet, did you make any progress on this? I'm dealing with the same problem right now. I've noticed though that it does seem to work after once resizing the window. 🤔

virtualized

@michellemdaraujo
Copy link

I was having this issue on a project, and I ended up putting everything inside a single and created a function to handle the height of everything, instead of having to use multiple autosizers and lists

@apelsinet
Copy link
Author

Nope, I ended up not using virtualised lists on pages with multiple lists unfortunately.

@nicolasschabram
Copy link

I've done some more digging. I'm not familiar at all with the codebase, but maybe my findings still help:

WindowScroller has an updatePosition() method which measures the dom element's offset and stores it in _positionFromTop and _positionFromLeft. updatePosition() is called in componentDidMount(), but at the time the offsets are stored, the lists themselves haven't been painted yet. For the first one, that isn't a problem because _positionFromTop is correct no matter its height. But the second list does depend on what has been rendered above. So what seems to be happening is that an incorrect _positionFromTop is stored during the initial render, breaking virtualization in the second list. Resizing the window triggers updatePosition() to be called again from _onResize(), which fixes the incorrect offsets - hence the behavior demonstrated in my comment above.

Another place where updatePosition() is called is in registerChild. A (rather unsatisfying) quick fix for the problem seems to be doing something like this:

<WindowScroller>
  {({ height, registerChild, scrollTop }) => (
    <div>
      <AutoSizer disableHeight>
        {({ width }) => (
          <div ref={el => registerChild(el)}> // <-- relevant line
            <List
              autoHeight
              height={height}
              rowCount={list1.length}
              rowHeight={32}
              rowRenderer={rowRenderer1}
              scrollTop={scrollTop}
              width={width}
            />
          </div>
        )}
      </AutoSizer>
    </div>
  )}
</WindowScroller>

Note that it only works when recreating the function passed to ref on every render, i.e. ref={el => registerChild(el)} instead of ref={registerChild}. I've tweaked @apelsinet's example accordingly: https://codesandbox.io/s/jjnr1v3r3w?fontsize=14

It's obviously a hack and probably not the right long-term solution. I'm not sure at all about possible negative implications.

But at least it seems like RV isn't very far from supporting this use case. :)

@apelsinet
Copy link
Author

@nicolasschabram thank you so much for doing the research and supplying this solution! It's definitely better than not using RV at all 😄

@Rogach
Copy link

Rogach commented Apr 30, 2019

One more workaround that seems to work: wrapping the WindowScrollers in <div style={{height: rowCount * rowHeight}}>. Obviously that works only with fixed-height rows.

@dmitrygalperin
Copy link

@nicolasschabram Thank you so much for this. Hack or not, it works for my use case without any apparent implications.

@uffou
Copy link

uffou commented Feb 1, 2022

@nicolasschabram your hack works like a charm for a default window scroller. However, once you set a scrollElement (and you need to do so if you have some nested components) it stops working.

Does anyone have a solution with 2 lists on top, a window scroller, and custom scroll element?

@uffou
Copy link

uffou commented Feb 1, 2022

OK, I figured out what the problem was with 2 WindowScrollers and the same scrollElement provided to each one, the registerScrollListener function (utils/onScroll) has a check which prevents the adding a scroll listener.

When I commented the if statement it works perfectly. I am not really sure what this check is for, probably something to prevent too many event listeners to the same component.

export function registerScrollListener(
  component: WindowScroller,
  element: Element,
) {
  // if (
  //   !mountedInstances.some(instance => instance.props.scrollElement === element)
  // ) {
  element.addEventListener('scroll', onScrollWindow);
  // }
  mountedInstances.push(component);
}

This only works thanks to @nicolasschabram relevant line ;)

<div ref={el => registerChild(el)}> // <-- relevant line

@bvaughn First of all. Big thanks to you for your incredible work! I know in your mind we are using the lib in the wrong way but for my case (React Beautiful DND) I really need multiple lists on top of each other and can't really combine it into one. I am sure many more have this same issue so maybe someone can help with this use case.

WindowScroller is used by many in conjunction with react-window as well.
So solving this use case would really be helpful to the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants