From 36e14b6ee73b5f02184b0a2fd636059eb4904550 Mon Sep 17 00:00:00 2001 From: Brad Christensen Date: Wed, 23 Nov 2016 15:54:54 +1300 Subject: [PATCH] Prevent internal scroll position overwriting scroll position from props * 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 --- source/Grid/Grid.js | 40 +++++++++++++++------- source/Grid/Grid.test.js | 71 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/source/Grid/Grid.js b/source/Grid/Grid.js index 15f6f4302..31fd583d9 100644 --- a/source/Grid/Grid.js +++ b/source/Grid/Grid.js @@ -766,10 +766,16 @@ export default class Grid extends Component { } if (scrollLeft >= 0) { + // Track scrolling direction so we can more efficiently overscan rows to reduce empty space around the edges while scrolling. + const scrollDirectionHorizontal = scrollLeft > this.state.scrollLeft ? SCROLL_DIRECTION_FORWARD : SCROLL_DIRECTION_BACKWARD + newState.scrollDirectionHorizontal = scrollDirectionHorizontal newState.scrollLeft = scrollLeft } if (scrollTop >= 0) { + // Track scrolling direction so we can more efficiently overscan rows to reduce empty space around the edges while scrolling. + const scrollDirectionVertical = scrollTop > this.state.scrollTop ? SCROLL_DIRECTION_FORWARD : SCROLL_DIRECTION_BACKWARD + newState.scrollDirectionVertical = scrollDirectionVertical newState.scrollTop = scrollTop } @@ -865,18 +871,30 @@ export default class Grid extends Component { this.state.scrollLeft !== scrollLeft || this.state.scrollTop !== scrollTop ) { - // Track scrolling direction so we can more efficiently overscan rows to reduce empty space around the edges while scrolling. - const scrollDirectionVertical = scrollTop > this.state.scrollTop ? SCROLL_DIRECTION_FORWARD : SCROLL_DIRECTION_BACKWARD - const scrollDirectionHorizontal = scrollLeft > this.state.scrollLeft ? SCROLL_DIRECTION_FORWARD : SCROLL_DIRECTION_BACKWARD - - this.setState({ + const newState = { isScrolling: true, - scrollDirectionHorizontal, - scrollDirectionVertical, - scrollLeft, - scrollPositionChangeReason: SCROLL_POSITION_CHANGE_REASONS.OBSERVED, - scrollTop - }) + scrollPositionChangeReason: SCROLL_POSITION_CHANGE_REASONS.OBSERVED + } + + // For each axis, only update the internal scroll state if it is not already being controlled + // by the parent component. + + // In each case, also track the scrolling direction so we can more efficiently overscan rows + // to reduce empty space around the edges while scrolling. + + if (this.props.scrollLeft === undefined) { + const scrollDirectionHorizontal = scrollLeft > this.state.scrollLeft ? SCROLL_DIRECTION_FORWARD : SCROLL_DIRECTION_BACKWARD + newState.scrollDirectionHorizontal = scrollDirectionHorizontal + newState.scrollLeft = scrollLeft + } + + if (this.props.scrollTop === undefined) { + const scrollDirectionVertical = scrollTop > this.state.scrollTop ? SCROLL_DIRECTION_FORWARD : SCROLL_DIRECTION_BACKWARD + newState.scrollDirectionVertical = scrollDirectionVertical + newState.scrollTop = scrollTop + } + + this.setState(newState) } this._invokeOnScrollMemoizer({ scrollLeft, scrollTop, totalColumnsWidth, totalRowsHeight }) diff --git a/source/Grid/Grid.test.js b/source/Grid/Grid.test.js index 848a0f9a4..a89803a83 100644 --- a/source/Grid/Grid.test.js +++ b/source/Grid/Grid.test.js @@ -830,10 +830,16 @@ describe('Grid', () => { }) it('should set the correct scroll direction', () => { - const grid = render(getMarkup({ + // Do not pass in the initial state as props, otherwise the internal state is forbidden from + // updating itself + const grid = render(getMarkup()) + + // Simulate a scroll to set the initial internal state + simulateScroll({ + grid, scrollLeft: 50, scrollTop: 50 - })) + }) expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_FORWARD) expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) @@ -857,6 +863,67 @@ describe('Grid', () => { expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) }) + it('should set the correct scroll direction when scroll position is updated from props', () => { + let grid = render(getMarkup({ + scrollLeft: 50, + scrollTop: 50 + })) + + expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_FORWARD) + expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) + + grid = render(getMarkup({ + scrollLeft: 0, + scrollTop: 0 + })) + + expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_BACKWARD) + expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_BACKWARD) + + grid = render(getMarkup({ + scrollLeft: 100, + scrollTop: 100 + })) + + expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_FORWARD) + expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) + }) + + it('should not change the scroll position or direction if the axis is controlled by the parent component', () => { + // Setting the scrollTop prop here implies it is controlled by the parent component + // (e.g. WindowScroller), while scrollLeft is controlled internally by the Grid + const grid = render(getMarkup({ + scrollTop: 50 + })) + + expect(grid.state.scrollLeft).toEqual(0) + expect(grid.state.scrollTop).toEqual(50) + expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_FORWARD) + expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) + + simulateScroll({ + grid, + scrollLeft: 100, + scrollTop: 100 + }) + + expect(grid.state.scrollLeft).toEqual(100) + expect(grid.state.scrollTop).toEqual(50) + expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_FORWARD) + expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) + + simulateScroll({ + grid, + scrollLeft: 0, + scrollTop: 0 + }) + + expect(grid.state.scrollLeft).toEqual(0) + expect(grid.state.scrollTop).toEqual(50) + expect(grid.state.scrollDirectionHorizontal).toEqual(SCROLL_DIRECTION_BACKWARD) + expect(grid.state.scrollDirectionVertical).toEqual(SCROLL_DIRECTION_FORWARD) + }) + it('should overscan in the direction being scrolled', async (done) => { const helper = createHelper()