From 3e9711fc8312c2a92d0e79fb46dbcee0a5ed31d4 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 3 Feb 2022 20:30:12 +0000 Subject: [PATCH 1/9] [table] WIP fix ghost cells bug --- packages/table/src/common/rect.ts | 1 - packages/table/src/locator.ts | 7 +++++++ packages/table/src/table2.tsx | 10 ++++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/packages/table/src/common/rect.ts b/packages/table/src/common/rect.ts index 77c7bbb95b..9e7398b6e9 100644 --- a/packages/table/src/common/rect.ts +++ b/packages/table/src/common/rect.ts @@ -18,7 +18,6 @@ import * as React from "react"; export type AnyRect = Rect | ClientRect; -// HACKHACK: workaround for https://github.com/palantir/tslint/issues/1768 // eslint-disable @typescript-eslint/adjacent-overload-signatures /** diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts index 91dd573a7c..82fc2d45ea 100644 --- a/packages/table/src/locator.ts +++ b/packages/table/src/locator.ts @@ -58,6 +58,8 @@ export interface ILocator { export class Locator implements ILocator { public static CELL_HORIZONTAL_PADDING = 10; + public hasVerticalOverflow = false; + private grid: Grid | undefined; // these values affect how we map a mouse coordinate to a cell coordinate. @@ -78,6 +80,7 @@ export class Locator implements ILocator { ) { this.numFrozenRows = 0; this.numFrozenColumns = 0; + this.updateOverflow(); } // Setters @@ -98,6 +101,10 @@ export class Locator implements ILocator { return this; } + public updateOverflow() { + this.hasVerticalOverflow = this.scrollContainerElement.scrollHeight > this.scrollContainerElement.clientHeight; + } + // Getters // ======= diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 9c10bed99d..7b7f51df17 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -647,11 +647,12 @@ export class Table2 extends AbstractComponent2 { From 386a7c388ef1671c764e84c2c67840ddf6cf5ed5 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 3 Feb 2022 20:33:03 +0000 Subject: [PATCH 2/9] fix formatting --- packages/table/src/table2.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 7b7f51df17..1287d82a20 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -650,7 +650,8 @@ export class Table2 extends AbstractComponent2 Date: Fri, 4 Feb 2022 15:41:32 +0000 Subject: [PATCH 3/9] Revert "[table] WIP fix ghost cells bug" This reverts commit 3e9711fc8312c2a92d0e79fb46dbcee0a5ed31d4. --- packages/table/src/common/rect.ts | 1 + packages/table/src/locator.ts | 7 ------- packages/table/src/table2.tsx | 11 ++++------- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/table/src/common/rect.ts b/packages/table/src/common/rect.ts index 9e7398b6e9..77c7bbb95b 100644 --- a/packages/table/src/common/rect.ts +++ b/packages/table/src/common/rect.ts @@ -18,6 +18,7 @@ import * as React from "react"; export type AnyRect = Rect | ClientRect; +// HACKHACK: workaround for https://github.com/palantir/tslint/issues/1768 // eslint-disable @typescript-eslint/adjacent-overload-signatures /** diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts index 82fc2d45ea..91dd573a7c 100644 --- a/packages/table/src/locator.ts +++ b/packages/table/src/locator.ts @@ -58,8 +58,6 @@ export interface ILocator { export class Locator implements ILocator { public static CELL_HORIZONTAL_PADDING = 10; - public hasVerticalOverflow = false; - private grid: Grid | undefined; // these values affect how we map a mouse coordinate to a cell coordinate. @@ -80,7 +78,6 @@ export class Locator implements ILocator { ) { this.numFrozenRows = 0; this.numFrozenColumns = 0; - this.updateOverflow(); } // Setters @@ -101,10 +98,6 @@ export class Locator implements ILocator { return this; } - public updateOverflow() { - this.hasVerticalOverflow = this.scrollContainerElement.scrollHeight > this.scrollContainerElement.clientHeight; - } - // Getters // ======= diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 1287d82a20..9c10bed99d 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -647,13 +647,11 @@ export class Table2 extends AbstractComponent2 { From 01b4aea355c5ca661e74406971e594e6e88d4971 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Fri, 4 Feb 2022 18:35:07 +0000 Subject: [PATCH 4/9] [table] fix ghost cells scrolling --- packages/table/src/locator.ts | 12 +++++ packages/table/src/table2.tsx | 26 ++++++++-- packages/table/test/table2Tests.tsx | 74 +++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts index 91dd573a7c..34f328ccf9 100644 --- a/packages/table/src/locator.ts +++ b/packages/table/src/locator.ts @@ -58,6 +58,13 @@ export interface ILocator { export class Locator implements ILocator { public static CELL_HORIZONTAL_PADDING = 10; + /** + * Marks whether the rendered rows overflow the table scroll container. + * This should be updated via calls to `.updateOverflow()` whenever updating + * the Locator. + */ + public hasVerticalOverflow = false; + private grid: Grid | undefined; // these values affect how we map a mouse coordinate to a cell coordinate. @@ -78,6 +85,7 @@ export class Locator implements ILocator { ) { this.numFrozenRows = 0; this.numFrozenColumns = 0; + this.updateOverflow(); } // Setters @@ -98,6 +106,10 @@ export class Locator implements ILocator { return this; } + public updateOverflow() { + this.hasVerticalOverflow = this.scrollContainerElement.scrollHeight > this.scrollContainerElement.clientHeight; + } + // Getters // ======= diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 9c10bed99d..5b21e55293 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -1230,13 +1230,28 @@ export class Table2 extends AbstractComponent2) => { - // 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(); + const oldScrollTop = this.state.viewportRect?.top ?? 0; + const enableGhostCells = this.props.enableGhostCells!; + if (this.locator != null && !this.state.isLayoutLocked) { - const viewportRect = this.locator.getViewportRect(); - this.updateViewportRect(viewportRect); + const newViewportRect = this.locator.getViewportRect(); + + // if there are ghost rows, we must take care to avoid unnecessarily scrolling them into view + // (see https://github.com/palantir/blueprint/issues/5027) + if (enableGhostCells && this.grid != null) { + const didScrollDownVertically = newViewportRect.top > oldScrollTop; + const rowIndices = this.grid.getRowIndicesInRect(newViewportRect, enableGhostCells); + // subtract 1 to give one row of buffer, to ensure that the last row does not get hidden in some edge cases + const areGhostRowsVisible = this.grid.isGhostIndex(rowIndices.rowIndexEnd - 1, 0); + if (didScrollDownVertically && areGhostRowsVisible && this.locator.hasVerticalOverflow) { + // reached bottom of table, not scrolling further + return; + } + } + this.updateViewportRect(newViewportRect); } }; @@ -1334,7 +1349,8 @@ export class Table2 extends AbstractComponent2 { diff --git a/packages/table/test/table2Tests.tsx b/packages/table/test/table2Tests.tsx index d5ea252127..7dea09224b 100644 --- a/packages/table/test/table2Tests.tsx +++ b/packages/table/test/table2Tests.tsx @@ -226,6 +226,80 @@ describe("", function (this) { } }); + describe("Vertically scrolling", () => { + runTestToEnsureScrollingIsEnabled(true); + runTestToEnsureScrollingIsEnabled(false); + + it("does not continue scrolling to ghost rows at the bottom of table", () => { + const onVisibleCellsChange = sinon.spy(); + const { containerElement, table } = mountTable({ defaultRowHeight: 20, enableGhostCells: true, onVisibleCellsChange }, { + width: 300, + // 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, + }); + const OVERFLOW_DISTANCE_TO_SCROLL = 30; + const locator = table.instance().locator!; + const prevViewportRect = locator.getViewportRect(); + + // first: scroll the table to the bottom + // HACKHACK: consider exposing this for testing somehow + ((locator as any).scrollContainerElement as HTMLElement).scrollTop = OVERFLOW_DISTANCE_TO_SCROLL; + updateLocatorElements(table, 0, OVERFLOW_DISTANCE_TO_SCROLL, prevViewportRect.width, prevViewportRect.height); + // need to simulate the scroll event to trigger React event handlers after we touched the real DOM + table + .find(`.${Classes.TABLE_QUADRANT_MAIN} .${Classes.TABLE_QUADRANT_SCROLL_CONTAINER}`) + .simulate("scroll"); + onVisibleCellsChange.resetHistory(); + + // next, try to scroll a bit farther + // N.B. this is tricky to test via unit testing (perhaps better handled by an integration test), so your results may vary + ((locator as any).scrollContainerElement as HTMLElement).scrollTop = OVERFLOW_DISTANCE_TO_SCROLL + 5; + // updateLocatorElements(table, 0, OVERFLOW_DISTANCE_TO_SCROLL + 5, prevViewportRect.width, prevViewportRect.height); + // again, we need to simulate the scroll event to trigger React event handlers after we touched the real DOM + table + .find(`.${Classes.TABLE_QUADRANT_MAIN} .${Classes.TABLE_QUADRANT_SCROLL_CONTAINER}`) + .simulate("scroll"); + + // we expect the body scroll handler to have exited early, and not triggered a visible cells change + expect(onVisibleCellsChange.callCount).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 }, { + width: 300, + height: 320, + }); + 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 = {}, 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); + + const table = mount( + + + , + { attachTo: containerElement }, + ); + return { containerElement, table }; + } + }); + describe("Instance methods", () => { describe("resizeRowsByApproximateHeight", () => { const STR_LENGTH_SHORT = 10; From 63106e3eea3725c748b6cb98dc471e60aeb70fa5 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Fri, 4 Feb 2022 18:52:53 +0000 Subject: [PATCH 5/9] Fix linting, formatting --- packages/table/test/table2Tests.tsx | 38 +++++++++++++++++++---------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/packages/table/test/table2Tests.tsx b/packages/table/test/table2Tests.tsx index 7dea09224b..55318f2901 100644 --- a/packages/table/test/table2Tests.tsx +++ b/packages/table/test/table2Tests.tsx @@ -232,13 +232,16 @@ describe("", function (this) { it("does not continue scrolling to ghost rows at the bottom of table", () => { const onVisibleCellsChange = sinon.spy(); - const { containerElement, table } = mountTable({ defaultRowHeight: 20, enableGhostCells: true, onVisibleCellsChange }, { - width: 300, - // 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, - }); + const { containerElement, table } = mountTable( + { defaultRowHeight: 20, enableGhostCells: true, onVisibleCellsChange }, + { + // 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 OVERFLOW_DISTANCE_TO_SCROLL = 30; const locator = table.instance().locator!; const prevViewportRect = locator.getViewportRect(); @@ -246,7 +249,13 @@ describe("", function (this) { // first: scroll the table to the bottom // HACKHACK: consider exposing this for testing somehow ((locator as any).scrollContainerElement as HTMLElement).scrollTop = OVERFLOW_DISTANCE_TO_SCROLL; - updateLocatorElements(table, 0, OVERFLOW_DISTANCE_TO_SCROLL, prevViewportRect.width, prevViewportRect.height); + updateLocatorElements( + table, + 0, + OVERFLOW_DISTANCE_TO_SCROLL, + prevViewportRect.width, + prevViewportRect.height, + ); // need to simulate the scroll event to trigger React event handlers after we touched the real DOM table .find(`.${Classes.TABLE_QUADRANT_MAIN} .${Classes.TABLE_QUADRANT_SCROLL_CONTAINER}`) @@ -271,10 +280,13 @@ describe("", function (this) { 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 }, { - width: 300, - height: 320, - }); + 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; @@ -284,7 +296,7 @@ describe("", function (this) { }); } - function mountTable(tableProps: Partial = {}, tableDimensions: { width: number, height: number }) { + function mountTable(tableProps: Partial = {}, tableDimensions: { width: number; height: number }) { const containerElement = document.createElement("div"); containerElement.style.width = `${tableDimensions.width}px`; containerElement.style.height = `${tableDimensions.height}px`; From 42d394c1502848ef9d1b111b3a4a3f368215a543 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Wed, 9 Feb 2022 20:57:07 +0000 Subject: [PATCH 6/9] WIP another strategy --- packages/table/src/locator.ts | 20 ++++++++++++++------ packages/table/src/table2.tsx | 26 ++++++++++++++++++-------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts index 34f328ccf9..e8b8be969b 100644 --- a/packages/table/src/locator.ts +++ b/packages/table/src/locator.ts @@ -59,11 +59,16 @@ export class Locator implements ILocator { public static CELL_HORIZONTAL_PADDING = 10; /** - * Marks whether the rendered rows overflow the table scroll container. + * The amount of vertical and horizontal overflow, recorded as width/height dimensions. + * If all the visible cells fit into the viewport, this should be 0/0. + * * This should be updated via calls to `.updateOverflow()` whenever updating - * the Locator. + * the Locator, and when the table root container element is resized. */ - public hasVerticalOverflow = false; + public overflowDimensions = { + height: 0, + width: 0, + }; private grid: Grid | undefined; @@ -85,7 +90,7 @@ export class Locator implements ILocator { ) { this.numFrozenRows = 0; this.numFrozenColumns = 0; - this.updateOverflow(); + this.updateOverflowDimensions(); } // Setters @@ -106,8 +111,11 @@ export class Locator implements ILocator { return this; } - public updateOverflow() { - this.hasVerticalOverflow = this.scrollContainerElement.scrollHeight > this.scrollContainerElement.clientHeight; + public updateOverflowDimensions() { + this.overflowDimensions = { + height: this.scrollContainerElement.scrollHeight - this.scrollContainerElement.clientHeight, + width: this.scrollContainerElement.scrollWidth - this.scrollContainerElement.clientWidth, + }; } // Getters diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 05346b548b..e3a1028100 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -502,6 +502,7 @@ export class Table2 extends AbstractComponent2 { if (!this.state.isLayoutLocked) { + this.locator?.updateOverflowDimensions(); this.updateViewportRect(this.locator?.getViewportRect()); } }); @@ -761,10 +762,6 @@ export class Table2 extends AbstractComponent2; + } + const columnIndices = this.grid.getColumnIndicesInRect(viewportRect, enableGhostCells); const columnIndexStart = showFrozenColumnsOnly ? 0 : columnIndices.columnIndexStart; const columnIndexEnd = showFrozenColumnsOnly ? this.getMaxFrozenColumnIndex() : columnIndices.columnIndexEnd; @@ -1235,7 +1237,7 @@ export class Table2 extends AbstractComponent2 oldScrollTop; + + // HACKHACK(adahiya): cache this value somehow + // const columnHeaderHeight = this.columnHeaderElement?.clientHeight ?? 0; + // const didScrollToBottom = newViewportRect.top + columnHeaderHeight >= this.locator.overflowDimensions.height; + + const didScrollDownVertically = newViewportRect.top - oldViewportRect.top > 1; // ignore subpixel scrolls const rowIndices = this.grid.getRowIndicesInRect(newViewportRect, enableGhostCells); // subtract 1 to give one row of buffer, to ensure that the last row does not get hidden in some edge cases const areGhostRowsVisible = this.grid.isGhostIndex(rowIndices.rowIndexEnd - 1, 0); - if (didScrollDownVertically && areGhostRowsVisible && this.locator.hasVerticalOverflow) { + + if (didScrollDownVertically && areGhostRowsVisible && this.locator.overflowDimensions.height > 0) { // reached bottom of table, not scrolling further + event.preventDefault(); return; } } + this.updateViewportRect(newViewportRect); } }; @@ -1352,7 +1362,7 @@ export class Table2 extends AbstractComponent2 { From c10d2b0177b9a82c9b61eae2107b7d18ef552079 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 10 Feb 2022 18:39:12 +0000 Subject: [PATCH 7/9] impelment a better fix --- packages/table/src/common/grid.ts | 6 ++ packages/table/src/headers/columnHeader.tsx | 2 - packages/table/src/headers/rowHeader.tsx | 2 - packages/table/src/locator.ts | 30 ++++++++ packages/table/src/table2.tsx | 78 +++++++++++---------- packages/table/src/tableBody.tsx | 5 +- packages/table/test/table2Tests.tsx | 72 +++++++++---------- packages/table/test/tableBodyTests.tsx | 5 +- 8 files changed, 116 insertions(+), 84 deletions(-) diff --git a/packages/table/src/common/grid.ts b/packages/table/src/common/grid.ts index eb2dde20fa..76ad447e93 100644 --- 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 index 298b5935b4..4932847b28 100644 --- a/packages/table/src/headers/columnHeader.tsx +++ b/packages/table/src/headers/columnHeader.tsx @@ -88,8 +88,6 @@ export class ColumnHeader extends React.Component { } = this.props; return ( - // HACKHACK(adahiya): strange shouldComponentUpdate type error with strict null checks - // @ts-ignore
{ } = this.props; return ( - // HACKHACK(adahiya): strange shouldComponentUpdate type error with strict null checks - // @ts-ignore
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 index e3a1028100..6984620cfe 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -207,10 +207,20 @@ export class Table2 extends AbstractComponent2 (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), }; @@ -218,12 +228,16 @@ export class Table2 extends AbstractComponent2; } - const columnIndices = this.grid.getColumnIndicesInRect(viewportRect, enableGhostCells); + // if we have horizontal overflow, no need to render ghost rows + // (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 ( -
+
; + } + + // 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; @@ -928,8 +952,12 @@ export class Table2 extends AbstractComponent2= this.locator.overflowDimensions.height; - - const didScrollDownVertically = newViewportRect.top - oldViewportRect.top > 1; // ignore subpixel scrolls - const rowIndices = this.grid.getRowIndicesInRect(newViewportRect, enableGhostCells); - // subtract 1 to give one row of buffer, to ensure that the last row does not get hidden in some edge cases - const areGhostRowsVisible = this.grid.isGhostIndex(rowIndices.rowIndexEnd - 1, 0); - - if (didScrollDownVertically && areGhostRowsVisible && this.locator.overflowDimensions.height > 0) { - // reached bottom of table, not scrolling further - event.preventDefault(); - return; - } - } - this.updateViewportRect(newViewportRect); } }; diff --git a/packages/table/src/tableBody.tsx b/packages/table/src/tableBody.tsx index 85baea4b3b..98bf25854f 100644 --- a/packages/table/src/tableBody.tsx +++ b/packages/table/src/tableBody.tsx @@ -62,8 +62,9 @@ export class TableBody extends AbstractComponent2 { 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 index 55318f2901..a27338a0d4 100644 --- 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,32 @@ describe("", function (this) { }); }); - function mountTable(tableProps: Partial = {}) { + 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 = {}, 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( @@ -230,10 +251,9 @@ describe("", function (this) { runTestToEnsureScrollingIsEnabled(true); runTestToEnsureScrollingIsEnabled(false); - it("does not continue scrolling to ghost rows at the bottom of table", () => { - const onVisibleCellsChange = sinon.spy(); - const { containerElement, table } = mountTable( - { defaultRowHeight: 20, enableGhostCells: true, onVisibleCellsChange }, + 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 @@ -242,37 +262,10 @@ describe("", function (this) { width: 300, }, ); - const OVERFLOW_DISTANCE_TO_SCROLL = 30; - const locator = table.instance().locator!; - const prevViewportRect = locator.getViewportRect(); - - // first: scroll the table to the bottom - // HACKHACK: consider exposing this for testing somehow - ((locator as any).scrollContainerElement as HTMLElement).scrollTop = OVERFLOW_DISTANCE_TO_SCROLL; - updateLocatorElements( - table, - 0, - OVERFLOW_DISTANCE_TO_SCROLL, - prevViewportRect.width, - prevViewportRect.height, - ); - // need to simulate the scroll event to trigger React event handlers after we touched the real DOM - table - .find(`.${Classes.TABLE_QUADRANT_MAIN} .${Classes.TABLE_QUADRANT_SCROLL_CONTAINER}`) - .simulate("scroll"); - onVisibleCellsChange.resetHistory(); - - // next, try to scroll a bit farther - // N.B. this is tricky to test via unit testing (perhaps better handled by an integration test), so your results may vary - ((locator as any).scrollContainerElement as HTMLElement).scrollTop = OVERFLOW_DISTANCE_TO_SCROLL + 5; - // updateLocatorElements(table, 0, OVERFLOW_DISTANCE_TO_SCROLL + 5, prevViewportRect.width, prevViewportRect.height); - // again, we need to simulate the scroll event to trigger React event handlers after we touched the real DOM - table - .find(`.${Classes.TABLE_QUADRANT_MAIN} .${Classes.TABLE_QUADRANT_SCROLL_CONTAINER}`) - .simulate("scroll"); - - // we expect the body scroll handler to have exited early, and not triggered a visible cells change - expect(onVisibleCellsChange.callCount).to.be.eq(0); + const numGhostCellsInFirstColumn = containerElement.querySelectorAll( + `.${Classes.TABLE_CELL_GHOST}.${Classes.columnCellIndexClass(0)}` + ).length; + expect(numGhostCellsInFirstColumn).to.be.eq(0); // cleanup document.body.removeChild(containerElement); @@ -302,6 +295,7 @@ describe("", function (this) { containerElement.style.height = `${tableDimensions.height}px`; document.body.appendChild(containerElement); + TableQuadrantStack.defaultProps.throttleScrolling = false; const table = mount( @@ -835,7 +829,7 @@ describe("", function (this) { }); }); - describe("Resizing", () => { + describe.only("Resizing", () => { it("Resizes selected rows together", () => { const table = mountTable(); const rows = getRowHeadersWrapper(table)!; diff --git a/packages/table/test/tableBodyTests.tsx b/packages/table/test/tableBodyTests.tsx index 3224812773..10acd408e4 100644 --- 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,11 @@ describe("TableBody", () => { const ROW_HEIGHT = 20; it("cellClassNames", () => { - expect(TableBody.cellClassNames(0, 0)).to.deep.equal([ + expect(cellClassNames(0, 0)).to.deep.equal([ Classes.rowCellIndexClass(0), Classes.columnCellIndexClass(0), ]); - expect(TableBody.cellClassNames(4096, 1024)).to.deep.equal([ + expect(cellClassNames(4096, 1024)).to.deep.equal([ Classes.rowCellIndexClass(4096), Classes.columnCellIndexClass(1024), ]); From 4a54d645d7ea3da32300715ec03bcca4d86ac516 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 10 Feb 2022 18:40:16 +0000 Subject: [PATCH 8/9] Remove dead code --- packages/table/src/locator.ts | 20 -------------------- packages/table/src/table2.tsx | 4 +--- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts index a2db090b3b..1f97652d49 100644 --- a/packages/table/src/locator.ts +++ b/packages/table/src/locator.ts @@ -58,18 +58,6 @@ export interface ILocator { export class Locator implements ILocator { public static CELL_HORIZONTAL_PADDING = 10; - /** - * The amount of vertical and horizontal overflow, recorded as width/height dimensions. - * If all the visible cells fit into the viewport, this should be 0/0. - * - * This should be updated via calls to `.updateOverflow()` whenever updating - * the Locator, and when the table root container element is resized. - */ - public overflowDimensions = { - height: 0, - width: 0, - }; - private grid: Grid | undefined; // these values affect how we map a mouse coordinate to a cell coordinate. @@ -90,7 +78,6 @@ export class Locator implements ILocator { ) { this.numFrozenRows = 0; this.numFrozenColumns = 0; - this.updateOverflowDimensions(); } // Setters @@ -111,13 +98,6 @@ export class Locator implements ILocator { return this; } - public updateOverflowDimensions() { - this.overflowDimensions = { - height: this.scrollContainerElement.scrollHeight - this.scrollContainerElement.clientHeight, - width: this.scrollContainerElement.scrollWidth - this.scrollContainerElement.clientWidth, - }; - } - // Getters // ======= diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 6984620cfe..48c6401224 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -516,7 +516,6 @@ export class Table2 extends AbstractComponent2 { if (!this.state.isLayoutLocked) { - this.locator?.updateOverflowDimensions(); this.updateViewportRect(this.locator?.getViewportRect()); } }); @@ -1365,8 +1364,7 @@ export class Table2 extends AbstractComponent2 { From 63a78d1e6dfaeec0e37b396db96e630e757b9427 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 10 Feb 2022 18:49:17 +0000 Subject: [PATCH 9/9] fix lint, formatting --- packages/table/src/locator.ts | 5 +---- packages/table/src/table2.tsx | 12 +++++++++--- packages/table/test/table2Tests.tsx | 11 +++++++---- packages/table/test/tableBodyTests.tsx | 5 +---- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/packages/table/src/locator.ts b/packages/table/src/locator.ts index 1f97652d49..43004ffb46 100644 --- a/packages/table/src/locator.ts +++ b/packages/table/src/locator.ts @@ -178,10 +178,7 @@ export class Locator implements ILocator { * * @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(), - ) { + public hasHorizontalOverflow(rowHeaderWidth = Grid.MIN_ROW_HEADER_WIDTH, viewportRect = this.getViewportRect()) { if (this.grid === undefined) { return false; } diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 48c6401224..514e66279a 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -788,10 +788,13 @@ export class Table2 extends AbstractComponent2; } - // if we have horizontal overflow, no need to render ghost rows + // 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 columnIndices = this.grid.getColumnIndicesInRect( + viewportRect, + hasHorizontalOverflow ? false : enableGhostCells, + ); const columnIndexStart = showFrozenColumnsOnly ? 0 : columnIndices.columnIndexStart; const columnIndexEnd = showFrozenColumnsOnly ? this.getMaxFrozenColumnIndex() : columnIndices.columnIndexEnd; @@ -956,7 +959,10 @@ export class Table2 extends AbstractComponent2", function (this) { }, ); const numGhostCellsInFirstRow = containerElement.querySelectorAll( - `.${Classes.TABLE_CELL_GHOST}.${Classes.rowCellIndexClass(0)}` + `.${Classes.TABLE_CELL_GHOST}.${Classes.rowCellIndexClass(0)}`, ).length; expect(numGhostCellsInFirstRow).to.be.eq(0); @@ -228,7 +228,10 @@ describe("", function (this) { document.body.removeChild(containerElement); }); - function mountTable(tableProps: Partial = {}, tableDimensions: { width: number; height: number } = { width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT }) { + function mountTable( + tableProps: Partial = {}, + tableDimensions: { width: number; height: number } = { width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT }, + ) { const containerElement = document.createElement("div"); containerElement.style.width = `${tableDimensions.width}px`; containerElement.style.height = `${tableDimensions.height}px`; @@ -263,7 +266,7 @@ describe("", function (this) { }, ); const numGhostCellsInFirstColumn = containerElement.querySelectorAll( - `.${Classes.TABLE_CELL_GHOST}.${Classes.columnCellIndexClass(0)}` + `.${Classes.TABLE_CELL_GHOST}.${Classes.columnCellIndexClass(0)}`, ).length; expect(numGhostCellsInFirstColumn).to.be.eq(0); @@ -829,7 +832,7 @@ describe("", function (this) { }); }); - describe.only("Resizing", () => { + describe("Resizing", () => { it("Resizes selected rows together", () => { const table = mountTable(); const rows = getRowHeadersWrapper(table)!; diff --git a/packages/table/test/tableBodyTests.tsx b/packages/table/test/tableBodyTests.tsx index 10acd408e4..d3b0cddee1 100644 --- a/packages/table/test/tableBodyTests.tsx +++ b/packages/table/test/tableBodyTests.tsx @@ -41,10 +41,7 @@ describe("TableBody", () => { const ROW_HEIGHT = 20; it("cellClassNames", () => { - expect(cellClassNames(0, 0)).to.deep.equal([ - Classes.rowCellIndexClass(0), - Classes.columnCellIndexClass(0), - ]); + 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),