bvaughn / react-virtualized

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

Bug with scrolling with WindowScroller -> AutoSizer -> Table and a big Header #705

Closed maks-rafalko closed 6 years ago

maks-rafalko commented 7 years ago

When Table component is used in conjunction with WindowScroller and AutoSizer, there is a bug with empty space on top of the table when the user is scrolling content down. But it works fine when content is being scrolled up.

Gif: http://www.giphy.com/gifs/l4FGCdxL20UsKANgs

Here is a Plunkr to reproduce: https://plnkr.co/edit/joaaAQPJLPy26Do4tA3k

Please try to scroll down and up and you will see the empty space on the top of the page.

bvaughn commented 7 years ago

Table isn't really compatible with WindowScroller because of the header. If you'd like to dig in and find a way to make the 2 play nicely, I'll review a PR. I don't think I'm going to tackle it though due to limited time.

maks-rafalko commented 7 years ago

Do you have any directions on where to start? Maybe some advices? Interesting thing that it works when I scroll up, but does not work in other directon.

I will try to investigate the issue.

bvaughn commented 7 years ago

Not really. Just familiarize yourself with WindowScroller and Grid. 😄

conandk commented 7 years ago

I have a same problem. I am using WindowScroller.

anhdn commented 7 years ago

The same issue happens with me using WindowScroller on Safari Mobile. Please help us investigate the issue. It happens every time when i refresh page.

maks-rafalko commented 7 years ago

Guys, JFYI: I didn't manage to fix this bug, but as a workaround - you can have two separate tables

  1. the first is for header
  2. the second is for data (with disableHeader=true)

It works and fixes this ugly bug, but as I said still a workaround. Please seee renderFakeTableAsAHeader and a complete example

private renderFakeTableAsAHeader(): JSX.Element {
    return (
        <AutoSizer disableHeight={true}>
            {({width, height}) => (
                <Table
                    autoHeight={true}
                    width={width}
                    height={height}
                    headerHeight={HEADER_HEIGHT}
                    rowHeight={this.props.cache.rowHeight}
                    rowCount={0}
                    rowGetter={() => {throw new Error('Logic Error'); }}
                >
                    {this.renderExtraColumns()}
                </Table>
            )}
        </AutoSizer>
    );
}

public render(): JSX.Element {
    return (
        <div>
            {this.renderFakeTableAsAHeader()}
            <WindowScroller scrollElement={scrollElement}>
                {({height, isScrolling, scrollTop}) => (
                    <AutoSizer disableHeight={true}>
                        {({width}) => (
                            <Table
                                autoHeight={true}
                                isScrolling={isScrolling}
                                scrollTop={scrollTop}
                                width={width}
                                height={height}
                                headerHeight={0}
                                disableHeader={true}
                                rowHeight={this.props.cache.rowHeight}
                                rowCount={vehicles.length}
                                rowGetter={({index}) => vehicles[index]}
                                noRowsRenderer={this.noRowsRenderer}
                                ref={(element) => {
                                    this.tableElement = element;
                                }}
                                deferredMeasurementCache={this.props.cache}
                            >
                        )}
                    </AutoSizer>
                )}
            </WindowScroller>
        </div>
    );
}
bvaughn commented 7 years ago

Thanks for sharing your workaround, @borNfreee. 😄

bangert commented 7 years ago

Table isn't really compatible with WindowScroller because of the header.

The documentation at https://github.com/bvaughn/react-virtualized/blob/master/docs/WindowScroller.md says

[... WindowScroller] should be used with Table or List only.

bvaughn commented 7 years ago

Docs are sometimes wrong.

AziRafaaa commented 6 years ago

Hi @bvaughn I need to use the combination of WindowScroller with AutoSizer and Table. Actually I have to display a long list of data. I am facing two issues: 1- if user resize the browser the column header are overlapped. in other words, the horizontal scroll bar doesn't get appeared. 2- the border of the header is not aligned with table content border, although all the margins and padding are the same.

Would you please advice what can I do?

bvaughn commented 6 years ago

if user resize the browser the column header are overlapped. in other words, the horizontal scroll bar doesn't get appeared.

Table doesn't really support horizontal scrolling. You could try putting it in a container that scrolls though.

Would you please advice what can I do?

I suggest using Stack Overflow with a #react-virtualized tag or asking in the react-virtualized Slack channel.

AziRafaaa commented 6 years ago

Using container does help. Also I managed to make use of flexShrink and set it to zero and that solved the problem. Thanks for your help. I'll ask my question in Stack Overflow from now on.

bvaughn commented 6 years ago

The Table only appears to work correctly when scrolling because the inner Grid is over-rendering in that direction. Changing the overscanRowCount to zero will reveal that it works the same as scrolling downward. (It's worth pointing out quickly that RV also appears to work differently in this case if the headerHeight and rowHeight match, but that is only because it always overscans one row in both directions for accessibility purposes.)

In all cases, there is space left for the Table header row but it is not properly being absolutely positioned. I experimented briefly with pushing down the table header using the scrollTop value injected by WindowScroller but this feels laggy/jumpy because of scrolling being async.

Anyway, I think the suggested workaround (disabling the header and rendering a separate header) is probably reasonable enough that I am not going to invest time into this any more.

I'm always happy to review PRs for things like this though. That being said, I'm closing this for now due to inactivity.

zefj commented 6 years ago

@borNfreee just a quick heads-up - I've been able to successfully solve this issue on my complex Grid setup by forking defaultOverscanIndicesGetter and supplying a tweaked version to overscanIndicesGetter prop. As far as I can see, the docs do not mention that prop, however the code says otherwise, so this solution should work for Table as well.

maks-rafalko commented 6 years ago

Could you please share code of your forked version? @zefj

zefj commented 6 years ago

@borNfreee sure, here's a short write-up first:

The reason why the cells are disappearing is that the overscan is not applied to top-side rows when scrolling forwards. In line defaultOverscanIndicesGetter.js:25 you can see the overscan start index is set to 0 or startIndex (the first visible row determined by some other code), whichever is higher. The problem is the first visible row index value is incorrect if padding is added - some rows before that are still in view, so you need to take that into account and offset the overscanStartIndex by the value of however many rows your padding could fit, so Math.ceil(PADDING / ROW_HEIGHT) or something.

My padding is minuscule so I ended up just subtracting 1 from startIndex in the aforementioned line, but you can reuse the logic for backwards scrolling and subtract the overscanCellsCount argument as seen in line defaultOverscanIndicesGetter.js:33 instead, so that it uses the overscan values supplied via props.

export default function defaultOverscanIndicesGetter({
  cellCount,
  overscanCellsCount,
  scrollDirection,
  startIndex,
  stopIndex,
}: OverscanIndicesGetterParams): OverscanIndices {
  if (scrollDirection === SCROLL_DIRECTION_FORWARD) {
    return {
      // while scrolling forward, apply the overscan to top-side rows as well
      overscanStartIndex: Math.max(0, startIndex - overscanCellsCount), 
      overscanStopIndex: Math.min(
        cellCount - 1,
        stopIndex + overscanCellsCount,
      ),
    };
  } else {
    return {
      overscanStartIndex: Math.max(0, startIndex - overscanCellsCount),
      overscanStopIndex: Math.min(cellCount - 1, stopIndex),
    };
  }
}

This theoretically might need additional tweaking (or higher overscan values) for variable-height rows, but for fixed-height rows it works like a charm.

BradRyan commented 5 years ago

Running into this problem under similar circumstance to that of the original post, but I don't think this is just a table problem. You can easily replicate the problem by going to https://bvaughn.github.io/react-virtualized/#/components/WindowScroller and editing the section of header to have extra height (min-height: 200px). Then scroll down and you'll see the list getting clipped.

It seems to me that the actual virtual window is shifting in the event that there is a large header.

Increased header height: image

List clipping: image

emistorrs commented 4 years ago

Any resolution on this bug? I can still reproduce it on the website (above) and am now encountering this in my own app as well.

Edit: I recognize there is a work around here, but ideally the window scroller would work where ever it is put on the page.

BradRyan commented 4 years ago

@edwinwalda for what it's worth I have this issue in a react / redux application when a user clicks to expand a section in the page header. When the expand animation finishes, a redux action is fired which increments a counter in the redux store. The table component watches this attribute and fires updatePosition() when it changes... sorta hacky

ideal-life-generator commented 4 years ago

the same

Sojborg commented 3 years ago

I have a bug in a production application with the same issue. I'm using the latest version 9.22.3.

This can be fixed by writing your own overscanIndicesGetter and use that when using the List component:

overscanIndicesGetter={(props) => {
                    const {overscanCellsCount, startIndex, stopIndex, cellCount} = props;
                    return {
                      overscanStartIndex: startIndex <= props.overscanCellsCount ? 0 : startIndex - overscanCellsCount,
                      overscanStopIndex: stopIndex + overscanCellsCount < cellCount ? stopIndex + overscanCellsCount : props.stopIndex
                    }
                  }}

But i think this is a bug in the library so I located the issue down to that the defaultOverscanIndicesGetter function isn't using overscanCellsCount when calculating overscanStartIndex. The startIndex is indicating the current row index number so doing Math.max(0, startIndex - 1) on fx. index row 3 will result in returing overscanStartIndex 3 where i would expect 0 because i also want to show rowindex 0,1,2.

So replacing the code in accessibilityOverscanIndicesGetter with the following code will fix the issue at hand.

export var SCROLL_DIRECTION_FORWARD = 1;
export var SCROLL_DIRECTION_HORIZONTAL = 'horizontal';
export var SCROLL_DIRECTION_VERTICAL = 'vertical';
/**
 * Calculates the number of cells to overscan before and after a specified range.
 * This function ensures that overscanning doesn't exceed the available cells.
 */

export default function defaultOverscanIndicesGetter(_ref) {
  var cellCount = _ref.cellCount,
      overscanCellsCount = _ref.overscanCellsCount,
      scrollDirection = _ref.scrollDirection,
      startIndex = _ref.startIndex,
      stopIndex = _ref.stopIndex;
  // Make sure we render at least 1 cell extra before and after (except near boundaries)
  // This is necessary in order to support keyboard navigation (TAB/SHIFT+TAB) in some cases
  // For more info see issues #625
  overscanCellsCount = Math.max(1, overscanCellsCount);

  return {
    overscanStartIndex: startIndex <= overscanCellsCount ? 0 : startIndex - overscanCellsCount,
    overscanStopIndex: stopIndex + overscanCellsCount < cellCount ? stopIndex + overscanCellsCount : stopIndex
  }

  // if (scrollDirection === SCROLL_DIRECTION_FORWARD) {
  //   return {
  //     overscanStartIndex: Math.max(0, startIndex - 1),
  //     overscanStopIndex: Math.min(cellCount - 1, stopIndex + overscanCellsCount)
  //   };
  // } else {
  //   return {
  //     overscanStartIndex: Math.max(0, startIndex - overscanCellsCount),
  //     overscanStopIndex: Math.min(cellCount - 1, stopIndex + 1)
  //   };
  // }
}
import { bpfrpt_proptype_OverscanIndicesGetterParams } from "./types";
import { bpfrpt_proptype_OverscanIndices } from "./types";

What i haven't looked into yet is why this only happens when other dom elements changes height. In my use case user can show/hide a graph with a button. If they show it the above issue happens.

Anyone who can verify that the above changes also fixes the issue for you? @love-intent @BradRyan @edwinwalda

Sojborg commented 3 years ago

@bvaughn this is still a bug. Can we reopen this issue?