Merge pull request #482 from lic-nz/fix-scroll-position-from-props

Prevent internal scroll position overwriting scroll position from props
bvaughn · Dec 1, 2016 · cc83019 · cc83019
2 parents c7bb0b7 + 36e14b6
commit cc83019
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 13 deletions.
diff --git 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
@@ -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()