Merge FixedSizeList/FixedSizeGrid scroll alignment and visibility bugfixes (PR #274)

Author: Brian Vaughn

src/FixedSizeGrid.js

@@ -31,12 +31,13 @@ const FixedSizeGrid = createGridComponent({
     instanceProps: typeof undefined,
     scrollbarSize: number
   ): number => {
-    const maxOffset = Math.max(
+    const lastColumnOffset = Math.max(
       0,
-      Math.min(
-        columnCount * ((columnWidth: any): number) - width,
-        columnIndex * ((columnWidth: any): number)
-      )
+      columnCount * ((columnWidth: any): number) - width
+    );
+    const maxOffset = Math.min(
+      lastColumnOffset,
+      columnIndex * ((columnWidth: any): number)
     );
     const minOffset = Math.max(
       0,
@@ -60,7 +61,18 @@ const FixedSizeGrid = createGridComponent({
       case 'end':
         return minOffset;
       case 'center':
-        return Math.round(minOffset + (maxOffset - minOffset) / 2);
+        // "Centered" offset is usually the average of the min and max.
+        // But near the edges of the list, this doesn't hold true.
+        const middleOffset = Math.round(
+          minOffset + (maxOffset - minOffset) / 2
+        );
+        if (middleOffset < Math.ceil(width / 2)) {
+          return 0; // near the beginning
+        } else if (middleOffset > lastColumnOffset + Math.floor(width / 2)) {
+          return lastColumnOffset; // near the end
+        } else {
+          return middleOffset;
+        }
       case 'auto':
       default:
         if (scrollLeft >= minOffset && scrollLeft <= maxOffset) {
@@ -69,7 +81,7 @@ const FixedSizeGrid = createGridComponent({
           // Because we only take into account the scrollbar size when calculating minOffset
           // this value can be larger than maxOffset when at the end of the list
           return minOffset;
-        } else if (scrollLeft - minOffset < maxOffset - scrollLeft) {
+        } else if (scrollLeft < minOffset) {
           return minOffset;
         } else {
           return maxOffset;
@@ -85,12 +97,13 @@ const FixedSizeGrid = createGridComponent({
     instanceProps: typeof undefined,
     scrollbarSize: number
   ): number => {
-    const maxOffset = Math.max(
+    const lastRowOffset = Math.max(
       0,
-      Math.min(
-        rowCount * ((rowHeight: any): number) - height,
-        rowIndex * ((rowHeight: any): number)
-      )
+      rowCount * ((rowHeight: any): number) - height
+    );
+    const maxOffset = Math.min(
+      lastRowOffset,
+      rowIndex * ((rowHeight: any): number)
     );
     const minOffset = Math.max(
       0,
@@ -114,7 +127,18 @@ const FixedSizeGrid = createGridComponent({
       case 'end':
         return minOffset;
       case 'center':
-        return Math.round(minOffset + (maxOffset - minOffset) / 2);
+        // "Centered" offset is usually the average of the min and max.
+        // But near the edges of the list, this doesn't hold true.
+        const middleOffset = Math.round(
+          minOffset + (maxOffset - minOffset) / 2
+        );
+        if (middleOffset < Math.ceil(height / 2)) {
+          return 0; // near the beginning
+        } else if (middleOffset > lastRowOffset + Math.floor(height / 2)) {
+          return lastRowOffset; // near the end
+        } else {
+          return middleOffset;
+        }
       case 'auto':
       default:
         if (scrollTop >= minOffset && scrollTop <= maxOffset) {
@@ -123,7 +147,7 @@ const FixedSizeGrid = createGridComponent({
           // Because we only take into account the scrollbar size when calculating minOffset
           // this value can be larger than maxOffset when at the end of the list
           return minOffset;
-        } else if (scrollTop - minOffset < maxOffset - scrollTop) {
+        } else if (scrollTop < minOffset) {
           return minOffset;
         } else {
           return maxOffset;
@@ -149,14 +173,14 @@ const FixedSizeGrid = createGridComponent({
     scrollLeft: number
   ): number => {
     const left = startIndex * ((columnWidth: any): number);
+    const numVisibleColumns = Math.ceil(
+      (width + scrollLeft - left) / ((columnWidth: any): number)
+    );
     return Math.max(
       0,
       Math.min(
         columnCount - 1,
-        startIndex +
-          Math.floor(
-            (width + (scrollLeft - left)) / ((columnWidth: any): number)
-          )
+        startIndex + numVisibleColumns - 1 // -1 is because stop index is inclusive
       )
     );
   },
@@ -175,13 +199,15 @@ const FixedSizeGrid = createGridComponent({
     startIndex: number,
     scrollTop: number
   ): number => {
-    const left = startIndex * ((rowHeight: any): number);
+    const top = startIndex * ((rowHeight: any): number);
+    const numVisibleRows = Math.ceil(
+      (height + scrollTop - top) / ((rowHeight: any): number)
+    );
     return Math.max(
       0,
       Math.min(
         rowCount - 1,
-        startIndex +
-          Math.floor((height + (scrollTop - left)) / ((rowHeight: any): number))
+        startIndex + numVisibleRows - 1 // -1 is because stop index is inclusive
       )
     );
   },

src/FixedSizeList.js

@@ -23,12 +23,13 @@ const FixedSizeList = createListComponent({
     // TODO Deprecate direction "horizontal"
     const isHorizontal = direction === 'horizontal' || layout === 'horizontal';
     const size = (((isHorizontal ? width : height): any): number);
-    const maxOffset = Math.max(
+    const lastItemOffset = Math.max(
       0,
-      Math.min(
-        itemCount * ((itemSize: any): number) - size,
-        index * ((itemSize: any): number)
-      )
+      itemCount * ((itemSize: any): number) - size
+    );
+    const maxOffset = Math.min(
+      lastItemOffset,
+      index * ((itemSize: any): number)
     );
     const minOffset = Math.max(
       0,
@@ -51,13 +52,25 @@ const FixedSizeList = createListComponent({
         return maxOffset;
       case 'end':
         return minOffset;
-      case 'center':
-        return Math.round(minOffset + (maxOffset - minOffset) / 2);
+      case 'center': {
+        // "Centered" offset is usually the average of the min and max.
+        // But near the edges of the list, this doesn't hold true.
+        const middleOffset = Math.round(
+          minOffset + (maxOffset - minOffset) / 2
+        );
+        if (middleOffset < Math.ceil(size / 2)) {
+          return 0; // near the beginning
+        } else if (middleOffset > lastItemOffset + Math.floor(size / 2)) {
+          return lastItemOffset; // near the end
+        } else {
+          return middleOffset;
+        }
+      }
       case 'auto':
       default:
         if (scrollOffset >= minOffset && scrollOffset <= maxOffset) {
           return scrollOffset;
-        } else if (scrollOffset - minOffset < maxOffset - scrollOffset) {
+        } else if (scrollOffset < minOffset) {
           return minOffset;
         } else {
           return maxOffset;
@@ -83,14 +96,14 @@ const FixedSizeList = createListComponent({
     const isHorizontal = direction === 'horizontal' || layout === 'horizontal';
     const offset = startIndex * ((itemSize: any): number);
     const size = (((isHorizontal ? width : height): any): number);
+    const numVisibleItems = Math.ceil(
+      (size + scrollOffset - offset) / ((itemSize: any): number)
+    );
     return Math.max(
       0,
       Math.min(
         itemCount - 1,
-        startIndex +
-          Math.floor(
-            (size + (scrollOffset - offset)) / ((itemSize: any): number)
-          )
+        startIndex + numVisibleItems - 1 // -1 is because stop index is inclusive
       )
     );
   },

src/VariableSizeGrid.js

@@ -278,7 +278,7 @@ const getOffsetForIndexAndAlignment = (
         // Because we only take into account the scrollbar size when calculating minOffset
         // this value can be larger than maxOffset when at the end of the list
         return minOffset;
-      } else if (scrollOffset - minOffset < maxOffset - scrollOffset) {
+      } else if (scrollOffset < minOffset) {
         return minOffset;
       } else {
         return maxOffset;

src/VariableSizeList.js

@@ -227,7 +227,7 @@ const VariableSizeList = createListComponent({
       default:
         if (scrollOffset >= minOffset && scrollOffset <= maxOffset) {
           return scrollOffset;
-        } else if (scrollOffset - minOffset < maxOffset - scrollOffset) {
+        } else if (scrollOffset < minOffset) {
           return minOffset;
         } else {
           return maxOffset;

src/__tests__/FixedSizeGrid.js

@@ -29,6 +29,7 @@ describe('FixedSizeGrid', () => {
 
     // JSdom does not do actual layout and so doesn't return meaningful values here.
     // For the purposes of our tests though, we can mock out semi-meaningful values.
+    // This mock is required for e.g. "onScroll" tests to work properly.
     Object.defineProperties(HTMLElement.prototype, {
       clientWidth: {
         configurable: true,
@@ -86,7 +87,7 @@ describe('FixedSizeGrid', () => {
 
   it('should render a grid of items', () => {
     ReactTestRenderer.create(<FixedSizeGrid {...defaultProps} />);
-    expect(itemRenderer).toHaveBeenCalledTimes(24);
+    expect(itemRenderer).toHaveBeenCalledTimes(15);
     expect(onItemsRendered.mock.calls).toMatchSnapshot();
   });
 
@@ -493,9 +494,9 @@ describe('FixedSizeGrid', () => {
       expect(onItemsRendered).toHaveBeenLastCalledWith(
         expect.objectContaining({
           visibleColumnStartIndex: 1,
-          visibleColumnStopIndex: 3,
+          visibleColumnStopIndex: 2,
           visibleRowStartIndex: 4,
-          visibleRowStopIndex: 8,
+          visibleRowStopIndex: 7,
         })
       );
 
@@ -505,7 +506,7 @@ describe('FixedSizeGrid', () => {
       expect(onItemsRendered).toHaveBeenLastCalledWith(
         expect.objectContaining({
           visibleColumnStartIndex: 1,
-          visibleColumnStopIndex: 3,
+          visibleColumnStopIndex: 2,
         })
       );
 
@@ -515,7 +516,7 @@ describe('FixedSizeGrid', () => {
       expect(onItemsRendered).toHaveBeenLastCalledWith(
         expect.objectContaining({
           visibleRowStartIndex: 8,
-          visibleRowStopIndex: 12,
+          visibleRowStopIndex: 11,
         })
       );
     });
@@ -569,6 +570,51 @@ describe('FixedSizeGrid', () => {
       expect(onItemsRendered.mock.calls).toMatchSnapshot();
     });
 
+    it('scroll with align = "auto" should work with partially-visible items', () => {
+      const rendered = ReactTestRenderer.create(
+        // Create list where items don't fit exactly into container.
+        // The container has space for 3 1/3 items.
+        <FixedSizeGrid {...defaultProps} columnWidth={70} rowHeight={30} />
+      );
+      // Scroll down enough to show row 10 at the bottom a nd column 10 at the right.
+      // Should show 4 rows: 3 full and one partial at the beginning
+      // Should show 3 columns: 2 full and one partial at the beginning
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 10, rowIndex: 10, align: 'auto' });
+      // No need to scroll again; row and column 9 are already visible.
+      // Because there's no scrolling, it won't call onItemsRendered.
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 9, rowIndex: 9, align: 'auto' });
+      // Scroll to near the end. row 96 and column 97 will be partly visible.
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 99, rowIndex: 99, align: 'auto' });
+      // Scroll back to row 91 and column 97.
+      // This will cause row 99 and column 99 to be partly viisble
+      // Even though a scroll happened,  none of the items rendered have changed.
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 97, rowIndex: 96, align: 'auto' });
+      // Scroll forward again. Because row and column #99 were already partly visible,
+      // all props of the onItemsRendered will be the same.
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 99, rowIndex: 99, align: 'auto' });
+      // Scroll to the second row and column.
+      // This should leave row 4 and column 3 partly visible.
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 1, rowIndex: 1, align: 'auto' });
+      // Scroll to the first row and column.
+      // This should leave row 3 and column 2 partly visible.
+      rendered
+        .getInstance()
+        .scrollToItem({ columnIndex: 0, rowIndex: 0, align: 'auto' });
+      expect(onItemsRendered.mock.calls).toMatchSnapshot();
+    });
+
     it('should scroll to the correct item for align = "auto" at the bottom of the grid', () => {
       getScrollbarSize.mockImplementation(() => 20);
 
@@ -948,6 +994,45 @@ describe('FixedSizeGrid', () => {
       simulateScroll(instance, { scrollLeft: 200, scrollTop: 200 });
       expect(onScroll.mock.calls[0][0].scrollUpdateWasRequested).toBe(false);
     });
+
+    it('scrolling should report partial items correctly in onItemsRendered', () => {
+      // Use ReactDOM renderer so the container ref works correctly.
+      const instance = ReactDOM.render(
+        <FixedSizeGrid
+          {...defaultProps}
+          initialScrollLeft={20}
+          initialScrollTop={10}
+        />,
+        document.createElement('div')
+      );
+      // grid 200w x 100h
+      // columnWidth: 100, rowHeight: 25,
+      // columnCount: 100, rowCount: 100
+      // Scroll 2 items fwd, but thanks to the initialScrollOffset, we should
+      // still be showing partials on both ends.
+      instance.scrollTo({ scrollLeft: 150, scrollTop: 40 });
+      // Scroll a little fwd to cause partials to be hidden
+      instance.scrollTo({ scrollLeft: 200, scrollTop: 50 });
+      // Scroll backwards to show partials again
+      instance.scrollTo({ scrollLeft: 150, scrollTop: 40 });
+      // Scroll near the end so that the last item is shown
+      // as a partial.
+      instance.scrollTo({
+        scrollLeft: 98 * 100 - 5,
+        scrollTop: 96 * 25 - 5,
+      });
+      // Scroll to the end. No partials.
+      instance.scrollTo({
+        scrollLeft: 98 * 100,
+        scrollTop: 96 * 25,
+      });
+      // Verify that backwards scrolling near the end works OK.
+      instance.scrollTo({
+        scrollLeft: 98 * 100 - 5,
+        scrollTop: 96 * 25 - 5,
+      });
+      expect(onItemsRendered.mock.calls).toMatchSnapshot();
+    });
   });
 
   describe('itemKey', () => {