Prevent internal scroll position overwriting scroll position from props

[bradchristensen]

This fixes the following issue: scrolling a Grid using an internal scrollbar causes the scroll position of the other axis in the grid to be reset to 0 (when that other axis is controlled externally).

In the example below, the internal horizontal scrollbar causes the vertical scroll position to be reset, where the vertical scroll position is controlled by a WindowScroller. (Note that I am using a Grid exactly like a List does - just rendering a single "column", so there is no horizontal overscanning etc)

• When the scroll position is updated internally (i.e. the inner container is scrolled), it previously updated the internal scroll state for both axes, which overwrote the scroll position that was passed in from props. If one axis was controlled externally (e.g. by WindowScroller), that axis's scroll position was being overwritten. This fix stops the internal state of an axis from being modified if an external scroll position is supplied for that axis.
• Additionally, when the external scroll position is modified (props are updated), the scroll direction is now updated, so that overscanning works for an axis that is controlled, e.g. by a WindowScroller

The first change could be breaking for users if for some reason they rely on scrolling being controlled in the way it is currently, but I can't think of a use case for it off the top of my head.

A side note - the GIF above shows some flickering of values in the grid, which is NOT fixed in this pull request, and I think may be related to #453. I'll add some comments to that thread shortly with my experiences.

[bvaughn]

Hi @bradchristensen! Thanks for the contribution and description.

I wonder if you have a Plnkr or repro case you could share with me that would allow me to see the before and after behavior myself? I could probably create one based on your description, but it looks like you may have already done that and I like saving time when I can 😁

[bradchristensen]

Not at the moment, but I'll put something together for you later today 😄

[bvaughn]

Thanks much!

[bradchristensen]

Sorry for the delay @bvaughn! I expected to get around to this when I was back in the office but ended up with other things to do.

I can reproduce the issue with this code: https://jsfiddle.net/bradchristensen/cj22s6gr/
And with the patch applied: https://jsfiddle.net/bradchristensen/vd49b6e5/

These demos will also reproduce #453.

[mbrevda]

While I don't have the best insight into the inner workings of Grid, this patch LGTM. Can you clarify what scrollDirectionVertical does when added to _setScrollPosition()?

[bradchristensen]

@mbrevda the scroll direction determines where extra rows are rendered when overscanning (e.g. if we are scrolling upwards, overscanned rows will be rendered above the visible rows, but if we are scrolling downwards, rows will be rendered below the visible rows). _onScroll currently sets the direction and therefore this works as expected when scrolling using a scrollbar that's internal to the Grid. However, when the scrollbar is external (e.g. scrolling the Grid using a WindowScroller), the scroll position gets updated through props and it does not pass through the _onScroll method, which is currently the only place that sets the scroll direction. _setScrollPosition is called when the scroll position is updated from an external source (_updateScrollLeftForScrollToColumn, _updateScrollTopForScrollToRow and componentWillUpdate). Therefore adding the same logic from _onScroll into _setScrollPosition will correctly set the direction and account for overscanned rows when any of these methods of scrolling are used (by comparing the new position against the previous position to determine the direction). In practice, the functions for scrolling to a column/row won't benefit from this, but if you're updating the scroll position using an HOC, correctly updating the direction for overscanning is very important.

[mbrevda]

Thank you!

[bvaughn]

Just chiming in here to say "thanks" as well! To @bradchristensen for the PR- and to @mbrevda for the review. ❤️ This change will go out with the 8.6 release (soon).

[bvaughn]

8.6.0 has just been released with this change.

[bradchristensen]

Thanks team 😄