[v4] [table] fix: prevent scrolling to ghost rows (#5115)

adidahiya · web-flow · commit 8c038a0a8ea3 · 2022-02-10T14:40:41.000-05:00
diff --git a/packages/table/src/common/grid.ts b/packages/table/src/common/grid.ts
@@ -49,6 +49,12 @@ export class Grid {
 
     public static DEFAULT_GHOST_WIDTH = 150;
 
+    // defined in headers/_common.scss
+    public static MIN_COLUMN_HEADER_HEIGHT = 30;
+
+    // defined in headers/_common.scss
+    public static MIN_ROW_HEADER_WIDTH = 30;
+
     public numCols: number;
 
     public numRows: number;
diff --git a/packages/table/src/headers/columnHeader.tsx b/packages/table/src/headers/columnHeader.tsx
@@ -88,8 +88,6 @@ export class ColumnHeader extends React.Component<IColumnHeaderProps> {
         } = this.props;
 
         return (
-            // HACKHACK(adahiya): strange shouldComponentUpdate type error with strict null checks
-            // @ts-ignore
             <Header
                 convertPointToIndex={this.convertPointToColumn}
                 fullRegionCardinality={RegionCardinality.FULL_COLUMNS}
diff --git a/packages/table/src/headers/rowHeader.tsx b/packages/table/src/headers/rowHeader.tsx
@@ -74,8 +74,6 @@ export class RowHeader extends React.Component<IRowHeaderProps> {
         } = this.props;
 
         return (
-            // HACKHACK(adahiya): strange shouldComponentUpdate type error with strict null checks
-            // @ts-ignore
             <Header
                 convertPointToIndex={this.convertPointToRow}
                 fullRegionCardinality={RegionCardinality.FULL_ROWS}
diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts
@@ -158,6 +158,33 @@ export class Locator implements ILocator {
         return maxHeight;
     }
 
+    /**
+     * Pass in an already-computed viewport rect here, if available, to reduce DOM reads.
+     *
+     * @returns whether the rendered rows overflow the visible viewport vertically, helpful for scrolling calculations
+     */
+    public hasVerticalOverflow(
+        columnHeaderHeight = Grid.MIN_COLUMN_HEADER_HEIGHT,
+        viewportRect = this.getViewportRect(),
+    ) {
+        if (this.grid === undefined) {
+            return false;
+        }
+        return this.grid.getHeight() > viewportRect.height - columnHeaderHeight;
+    }
+
+    /**
+     * Pass in an already-computed viewport rect here, if available, to reduce DOM reads.
+     *
+     * @returns whether the rendered columns overflow the visible viewport horizontally, helpful for scrolling calculations
+     */
+    public hasHorizontalOverflow(rowHeaderWidth = Grid.MIN_ROW_HEADER_WIDTH, viewportRect = this.getViewportRect()) {
+        if (this.grid === undefined) {
+            return false;
+        }
+        return this.grid.getWidth() > viewportRect.width - rowHeaderWidth;
+    }
+
     // Converters
     // ==========
 
diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx
@@ -207,23 +207,37 @@ export class Table2 extends AbstractComponent2<TableProps, TableState, TableSnap
 
     private refHandlers = {
         cellContainer: (ref: HTMLElement | null) => (this.cellContainerElement = ref),
-        columnHeader: (ref: HTMLElement | null) => (this.columnHeaderElement = ref),
+        columnHeader: (ref: HTMLElement | null) => {
+            this.columnHeaderElement = ref;
+            if (ref != null) {
+                this.columnHeaderHeight = ref.clientHeight;
+            }
+        },
         quadrantStack: (ref: TableQuadrantStack) => (this.quadrantStackInstance = ref),
         rootTable: (ref: HTMLElement | null) => (this.rootTableElement = ref),
-        rowHeader: (ref: HTMLElement | null) => (this.rowHeaderElement = ref),
+        rowHeader: (ref: HTMLElement | null) => {
+            this.rowHeaderElement = ref;
+            if (ref != null) {
+                this.rowHeaderWidth = ref.clientWidth;
+            }
+        },
         scrollContainer: (ref: HTMLElement | null) => (this.scrollContainerElement = ref),
     };
 
     private cellContainerElement?: HTMLElement | null;
 
     private columnHeaderElement?: HTMLElement | null;
 
+    private columnHeaderHeight = Grid.MIN_COLUMN_HEADER_HEIGHT;
+
     private quadrantStackInstance?: TableQuadrantStack;
 
     private rootTableElement?: HTMLElement | null;
 
     private rowHeaderElement?: HTMLElement | null;
 
+    private rowHeaderWidth = Grid.MIN_ROW_HEADER_WIDTH;
+
     private scrollContainerElement?: HTMLElement | null;
 
     /*
@@ -761,23 +775,32 @@ export class Table2 extends AbstractComponent2<TableProps, TableState, TableSnap
             selectedRegionTransform,
         } = this.props;
 
-        if (this.grid === null || this.locator === undefined || viewportRect === undefined) {
-            return undefined;
-        }
-
         const classes = classNames(Classes.TABLE_COLUMN_HEADERS, {
             [Classes.TABLE_SELECTION_ENABLED]: isSelectionModeEnabled(
                 this.props as TablePropsWithDefaults,
                 RegionCardinality.FULL_COLUMNS,
             ),
         });
 
-        const columnIndices = this.grid.getColumnIndicesInRect(viewportRect, enableGhostCells);
+        if (this.grid === null || this.locator === undefined || viewportRect === undefined) {
+            // if we haven't mounted yet (which we need in order for grid/viewport calculations),
+            // we still want to hand a DOM ref over to TableQuadrantStack for later
+            return <div className={classes} ref={refHandler} />;
+        }
+
+        // if we have horizontal overflow, no need to render ghost columns
+        // (this avoids problems like https://github.com/palantir/blueprint/issues/5027)
+        const hasHorizontalOverflow = this.locator.hasHorizontalOverflow(this.rowHeaderWidth, viewportRect);
+        const columnIndices = this.grid.getColumnIndicesInRect(
+            viewportRect,
+            hasHorizontalOverflow ? false : enableGhostCells,
+        );
+
         const columnIndexStart = showFrozenColumnsOnly ? 0 : columnIndices.columnIndexStart;
         const columnIndexEnd = showFrozenColumnsOnly ? this.getMaxFrozenColumnIndex() : columnIndices.columnIndexEnd;
 
         return (
-            <div className={classes}>
+            <div className={classes} ref={refHandler}>
                 <ColumnHeader
                     defaultColumnWidth={defaultColumnWidth!}
                     enableMultipleSelection={enableMultipleSelection}
@@ -789,7 +812,6 @@ export class Table2 extends AbstractComponent2<TableProps, TableState, TableSnap
                     loading={hasLoadingOption(loadingOptions, TableLoadingOption.COLUMN_HEADERS)}
                     locator={this.locator}
                     maxColumnWidth={maxColumnWidth!}
-                    measurableElementRef={refHandler}
                     minColumnWidth={minColumnWidth!}
                     onColumnWidthChanged={this.handleColumnWidthChanged}
                     onFocusedCell={this.handleFocus}
@@ -831,18 +853,24 @@ export class Table2 extends AbstractComponent2<TableProps, TableState, TableSnap
             selectedRegionTransform,
         } = this.props;
 
-        if (this.grid === null || this.locator === undefined || viewportRect === undefined) {
-            return undefined;
-        }
-
         const classes = classNames(Classes.TABLE_ROW_HEADERS, {
             [Classes.TABLE_SELECTION_ENABLED]: isSelectionModeEnabled(
                 this.props as TablePropsWithDefaults,
                 RegionCardinality.FULL_ROWS,
             ),
         });
 
-        const rowIndices = this.grid.getRowIndicesInRect(viewportRect, enableGhostCells);
+        if (this.grid === null || this.locator === undefined || viewportRect === undefined) {
+            // if we haven't mounted yet (which we need in order for grid/viewport calculations),
+            // we still want to hand a DOM ref over to TableQuadrantStack for later
+            return <div className={classes} ref={refHandler} />;
+        }
+
+        // if we have vertical overflow, no need to render ghost rows
+        // (this avoids problems like https://github.com/palantir/blueprint/issues/5027)
+        const hasVerticalOverflow = this.locator.hasVerticalOverflow(this.columnHeaderHeight, viewportRect);
+        const rowIndices = this.grid.getRowIndicesInRect(viewportRect, hasVerticalOverflow ? false : enableGhostCells);
+
         const rowIndexStart = showFrozenRowsOnly ? 0 : rowIndices.rowIndexStart;
         const rowIndexEnd = showFrozenRowsOnly ? this.getMaxFrozenRowIndex() : rowIndices.rowIndexEnd;
 
@@ -926,8 +954,15 @@ export class Table2 extends AbstractComponent2<TableProps, TableState, TableSnap
             return undefined;
         }
 
-        const rowIndices = this.grid.getRowIndicesInRect(viewportRect, enableGhostCells);
-        const columnIndices = this.grid.getColumnIndicesInRect(viewportRect, enableGhostCells);
+        // if we have vertical/horizontal overflow, no need to render ghost rows/columns (respectively)
+        // (this avoids problems like https://github.com/palantir/blueprint/issues/5027)
+        const hasVerticalOverflow = this.locator.hasVerticalOverflow(this.columnHeaderHeight, viewportRect);
+        const hasHorizontalOverflow = this.locator.hasHorizontalOverflow(this.rowHeaderWidth, viewportRect);
+        const rowIndices = this.grid.getRowIndicesInRect(viewportRect, hasVerticalOverflow ? false : enableGhostCells);
+        const columnIndices = this.grid.getColumnIndicesInRect(
+            viewportRect,
+            hasHorizontalOverflow ? false : enableGhostCells,
+        );
 
         // start beyond the frozen area if rendering unrelated quadrants, so we
         // don't render duplicate cells underneath the frozen ones.
@@ -1232,13 +1267,12 @@ export class Table2 extends AbstractComponent2<TableProps, TableState, TableSnap
     };
 
     private handleBodyScroll = (event: React.SyntheticEvent<HTMLElement>) => {
-        // Prevent the event from propagating to avoid a resize event on the
-        // resize sensor.
+        // Prevent the event from propagating to avoid a resize event on the resize sensor.
         event.stopPropagation();
 
         if (this.locator != null && !this.state.isLayoutLocked) {
-            const viewportRect = this.locator.getViewportRect();
-            this.updateViewportRect(viewportRect);
+            const newViewportRect = this.locator.getViewportRect();
+            this.updateViewportRect(newViewportRect);
         }
     };
 
diff --git a/packages/table/src/tableBody.tsx b/packages/table/src/tableBody.tsx
@@ -62,8 +62,9 @@ export class TableBody extends AbstractComponent2<ITableBodyProps> {
         renderMode: RenderMode.BATCH,
     };
 
-    // TODO: Does this method need to be public?
-    // (see: https://github.com/palantir/blueprint/issues/1617)
+    /**
+     * @deprecated, will be removed from public API in the next major version
+     */
     public static cellClassNames(rowIndex: number, columnIndex: number) {
         return cellClassNames(rowIndex, columnIndex);
     }
diff --git a/packages/table/test/table2Tests.tsx b/packages/table/test/table2Tests.tsx
@@ -31,6 +31,7 @@ import type { ColumnIndices, RowIndices } from "../src/common/grid";
 import { Rect } from "../src/common/rect";
 import { RenderMode } from "../src/common/renderMode";
 import { TableQuadrant } from "../src/quadrants/tableQuadrant";
+import { TableQuadrantStack } from "../src/quadrants/tableQuadrantStack";
 import { IRegion, Regions } from "../src/regions";
 import { TableState } from "../src/tableState";
 import { CellType, expectCellLoading } from "./cellTestUtils";
@@ -208,12 +209,35 @@ describe("<Table2>", function (this) {
             });
         });
 
-        function mountTable(tableProps: Partial<TableProps> = {}) {
+        it("does not render ghost columns when there is horizontal overflow", () => {
+            const { containerElement } = mountTable(
+                { numRows: 2, defaultRowHeight: 20, defaultColumnWidth: 100 },
+                {
+                    height: 200,
+                    // 300px leaves just enough space for the 3 columns, but there is 30px taken up by
+                    // the row header, which will overflow.
+                    width: 300,
+                },
+            );
+            const numGhostCellsInFirstRow = containerElement.querySelectorAll(
+                `.${Classes.TABLE_CELL_GHOST}.${Classes.rowCellIndexClass(0)}`,
+            ).length;
+            expect(numGhostCellsInFirstRow).to.be.eq(0);
+
+            // cleanup
+            document.body.removeChild(containerElement);
+        });
+
+        function mountTable(
+            tableProps: Partial<TableProps> = {},
+            tableDimensions: { width: number; height: number } = { width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT },
+        ) {
             const containerElement = document.createElement("div");
-            containerElement.style.width = `${CONTAINER_WIDTH}px`;
-            containerElement.style.height = `${CONTAINER_HEIGHT}px`;
+            containerElement.style.width = `${tableDimensions.width}px`;
+            containerElement.style.height = `${tableDimensions.height}px`;
             document.body.appendChild(containerElement);
 
+            TableQuadrantStack.defaultProps.throttleScrolling = false;
             const table = mount(
                 <Table2 numRows={0} enableGhostCells={true} {...tableProps}>
                     <Column cellRenderer={renderDummyCell} />
@@ -226,6 +250,65 @@ describe("<Table2>", function (this) {
         }
     });
 
+    describe("Vertically scrolling", () => {
+        runTestToEnsureScrollingIsEnabled(true);
+        runTestToEnsureScrollingIsEnabled(false);
+
+        it("does not render ghost rows when there is vertical overflow", () => {
+            const { containerElement } = mountTable(
+                { defaultRowHeight: 20, enableGhostCells: true },
+                {
+                    // we need _some_ amount of vertical overflow to avoid the code path which disables vertical scroll
+                    // in the table altogether. 200px leaves just enough space for the rows, but there is 30px taken up by
+                    // the column header, which will overflow.
+                    height: 200,
+                    width: 300,
+                },
+            );
+            const numGhostCellsInFirstColumn = containerElement.querySelectorAll(
+                `.${Classes.TABLE_CELL_GHOST}.${Classes.columnCellIndexClass(0)}`,
+            ).length;
+            expect(numGhostCellsInFirstColumn).to.be.eq(0);
+
+            // cleanup
+            document.body.removeChild(containerElement);
+        });
+
+        function runTestToEnsureScrollingIsEnabled(enableGhostCells: boolean) {
+            it(`isn't disabled when there is half a row left to scroll to and enableGhostCells is set to ${enableGhostCells}`, () => {
+                const { containerElement, table } = mountTable(
+                    { defaultRowHeight: 30, enableGhostCells },
+                    {
+                        height: 320,
+                        width: 300,
+                    },
+                );
+                const tableContainer = table.find(`.${Classes.TABLE_CONTAINER}`);
+                // There should be 10px left of scrolling. Height is 320, rows take up 300, and headerRow takes up 30
+                expect(tableContainer.hasClass(Classes.TABLE_NO_VERTICAL_SCROLL)).to.be.false;
+
+                // clean up created div
+                document.body.removeChild(containerElement);
+            });
+        }
+
+        function mountTable(tableProps: Partial<TableProps> = {}, tableDimensions: { width: number; height: number }) {
+            const containerElement = document.createElement("div");
+            containerElement.style.width = `${tableDimensions.width}px`;
+            containerElement.style.height = `${tableDimensions.height}px`;
+            document.body.appendChild(containerElement);
+
+            TableQuadrantStack.defaultProps.throttleScrolling = false;
+            const table = mount(
+                <Table2 numRows={10} {...tableProps}>
+                    <Column cellRenderer={renderDummyCell} />
+                </Table2>,
+                { attachTo: containerElement },
+            );
+            return { containerElement, table };
+        }
+    });
+
     describe("Instance methods", () => {
         describe("resizeRowsByApproximateHeight", () => {
             const STR_LENGTH_SHORT = 10;
diff --git a/packages/table/test/tableBodyTests.tsx b/packages/table/test/tableBodyTests.tsx
@@ -28,6 +28,7 @@ import { RenderMode } from "../src/common/renderMode";
 import { MenuContext } from "../src/interactions/menus/menuContext";
 import { IRegion, Regions } from "../src/regions";
 import { ITableBodyProps, TableBody } from "../src/tableBody";
+import { cellClassNames } from "../src/tableBodyCells";
 
 describe("TableBody", () => {
     // use enough rows that batching won't render all of them in one pass.
@@ -40,11 +41,8 @@ describe("TableBody", () => {
     const ROW_HEIGHT = 20;
 
     it("cellClassNames", () => {
-        expect(TableBody.cellClassNames(0, 0)).to.deep.equal([
-            Classes.rowCellIndexClass(0),
-            Classes.columnCellIndexClass(0),
-        ]);
-        expect(TableBody.cellClassNames(4096, 1024)).to.deep.equal([
+        expect(cellClassNames(0, 0)).to.deep.equal([Classes.rowCellIndexClass(0), Classes.columnCellIndexClass(0)]);
+        expect(cellClassNames(4096, 1024)).to.deep.equal([
             Classes.rowCellIndexClass(4096),
             Classes.columnCellIndexClass(1024),
         ]);
