Fix: Table not keeping focused cell visible #6882
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| type: fix | ||
| fix: | ||
| description: |- | ||
| `Table` and `Table2` now keep the focused cell cell visible if the focused cell is changed by: | ||
|
|
||
| - The `focusedCell` prop (provided that `enableFocusedCell` is `true`), OR | ||
| - The built-in navigation key handlers for arrow, tab and enter keys | ||
| links: | ||
| - https://github.com/palantir/blueprint/pull/6882 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /* ! | ||
| * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. | ||
| */ | ||
|
|
||
| export interface TableHeaderDimensions { | ||
| readonly columnHeaderHeight: number; | ||
| readonly rowHeaderWidth: number; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ import * as ScrollUtils from "./common/internal/scrollUtils"; | |
| import { Rect } from "./common/rect"; | ||
| import { RenderMode } from "./common/renderMode"; | ||
| import { ScrollDirection } from "./common/scrollDirection"; | ||
| import type { TableHeaderDimensions } from "./common/TableHeaderDimensions"; | ||
| import { Utils } from "./common/utils"; | ||
| import { ColumnHeader } from "./headers/columnHeader"; | ||
| import { ColumnHeaderCell, type ColumnHeaderCellProps } from "./headers/columnHeaderCell"; | ||
|
|
@@ -231,15 +232,13 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| private refHandlers = { | ||
| cellContainer: (ref: HTMLElement | null) => (this.cellContainerElement = ref), | ||
| columnHeader: (ref: HTMLElement | null) => { | ||
| if (ref != null) { | ||
| this.columnHeaderHeight = Math.max(ref.clientHeight, Grid.MIN_COLUMN_HEADER_HEIGHT); | ||
| } | ||
| }, | ||
| quadrantStack: (ref: TableQuadrantStack) => (this.quadrantStackInstance = ref), | ||
| rootTable: (ref: HTMLElement | null) => (this.rootTableElement = ref), | ||
| rowHeader: (ref: HTMLElement | null) => { | ||
| if (ref != null) { | ||
| this.rowHeaderWidth = ref.clientWidth; | ||
| } | ||
|
|
@@ -249,16 +248,12 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
|
|
||
| private cellContainerElement?: HTMLElement | null; | ||
|
|
||
| private columnHeaderHeight = Grid.MIN_COLUMN_HEADER_HEIGHT; | ||
|
|
||
| private quadrantStackInstance?: TableQuadrantStack; | ||
|
|
||
| private rootTableElement?: HTMLElement | null; | ||
|
|
||
| private rowHeaderWidth = Grid.MIN_ROW_HEADER_WIDTH; | ||
|
|
||
| private scrollContainerElement?: HTMLElement | null; | ||
|
|
@@ -328,6 +323,7 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
|
|
||
| this.hotkeysImpl = new TableHotkeys(props, this.state, { | ||
| getEnabledSelectionHandler: this.getEnabledSelectionHandler, | ||
| getHeaderDimensions: this.getHeaderDimensions, | ||
| handleFocus: this.handleFocus, | ||
| handleSelection: this.handleSelection, | ||
| syncViewportPosition: this.syncViewportPosition, | ||
|
|
@@ -726,15 +722,15 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| // ============= | ||
|
|
||
| private shouldDisableVerticalScroll() { | ||
| const { enableGhostCells } = this.props; | ||
| const { viewportRect } = this.state; | ||
|
|
||
| if (this.grid === null || viewportRect === undefined) { | ||
| return false; | ||
| } | ||
|
|
||
| const rowIndices = this.grid.getRowIndicesInRect({ | ||
| columnHeaderHeight: this.getColumnHeaderHeight(), | ||
| includeGhostCells: enableGhostCells!, | ||
| rect: viewportRect, | ||
| }); | ||
|
|
@@ -870,7 +866,6 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| enableGhostCells, | ||
| enableColumnReordering, | ||
| enableColumnResizing, | ||
| loadingOptions, | ||
| maxColumnWidth, | ||
| minColumnWidth, | ||
|
|
@@ -893,7 +888,7 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| // if we have horizontal overflow or exact fit, no need to render ghost columns | ||
| // (this avoids problems like https://github.com/palantir/blueprint/issues/5027) | ||
| const hasHorizontalOverflowOrExactFit = this.locator.hasHorizontalOverflowOrExactFit( | ||
| this.getRowHeaderWidth(), | ||
| viewportRect, | ||
| ); | ||
| const columnIndices = this.grid.getColumnIndicesInRect( | ||
|
|
@@ -948,7 +943,6 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| const { focusedCell, selectedRegions, viewportRect } = this.state; | ||
| const { | ||
| defaultRowHeight, | ||
| enableMultipleSelection, | ||
| enableGhostCells, | ||
| enableRowReordering, | ||
|
|
@@ -976,7 +970,7 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| // if we have vertical overflow or exact fit, no need to render ghost rows | ||
| // (this avoids problems like https://github.com/palantir/blueprint/issues/5027) | ||
| const hasVerticalOverflowOrExactFit = this.locator.hasVerticalOverflowOrExactFit( | ||
| this.getColumnHeaderHeight(), | ||
| viewportRect, | ||
| ); | ||
| const rowIndices = this.grid.getRowIndicesInRect({ | ||
|
|
@@ -1060,7 +1054,6 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| enableMultipleSelection, | ||
| enableColumnHeader, | ||
| enableGhostCells, | ||
| loadingOptions, | ||
| bodyContextMenuRenderer, | ||
| selectedRegionTransform, | ||
|
|
@@ -1077,7 +1070,7 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| viewportRect, | ||
| ); | ||
| const hasHorizontalOverflowOrExactFit = this.locator.hasHorizontalOverflowOrExactFit( | ||
| this.getRowHeaderWidth(), | ||
| viewportRect, | ||
| ); | ||
| const rowIndices = this.grid.getRowIndicesInRect({ | ||
|
|
@@ -1478,30 +1471,21 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| private syncViewportPosition = ({ nextScrollLeft, nextScrollTop }: TableSnapshot) => { | ||
| const { viewportRect } = this.state; | ||
|
|
||
| if (this.scrollContainerElement == null || viewportRect === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| if (nextScrollLeft !== undefined || nextScrollTop !== undefined) { | ||
| if (nextScrollTop !== undefined) { | ||
| this.scrollContainerElement.scrollTop = nextScrollTop; | ||
| } | ||
| if (nextScrollLeft !== undefined) { | ||
| this.scrollContainerElement.scrollLeft = nextScrollLeft; | ||
|
There was a problem hiding this comment. the change here removing the "correction" looks like the scrollContainer and viewportRect will have the same scroll top/left now - which seems to be different than what https://github.com/palantir/blueprint/pull/6882/files#diff-8cd23c90b1959192d311f6112803042130c4c7709017e6cac6fd991ea89221beR313-R318 is saying |
||
| } | ||
|
|
||
| const nextViewportRect = new Rect( | ||
| nextScrollLeft ?? viewportRect.left, | ||
| nextScrollTop ?? viewportRect.top, | ||
|
Comment on lines
-1503
to
+1488
There was a problem hiding this comment. Unless both There was a problem hiding this comment. I think this change makes sense. In so using the current viewportRect top/width instead of 0 sounds like a better default I think we could merge this now if you'd like to split this little bit off |
||
| viewportRect.width, | ||
| viewportRect.height, | ||
| ); | ||
|
|
@@ -1624,4 +1608,19 @@ export class Table2 extends AbstractComponent<Table2Props, TableState, TableSnap | |
| private handleRowResizeGuide = (horizontalGuides: number[]) => { | ||
| this.setState({ horizontalGuides }); | ||
| }; | ||
|
|
||
| private getHeaderDimensions = (): TableHeaderDimensions => { | ||
| return { | ||
| columnHeaderHeight: this.getColumnHeaderHeight(), | ||
| rowHeaderWidth: this.getRowHeaderWidth(), | ||
| }; | ||
| }; | ||
|
|
||
| private getColumnHeaderHeight = (): number => { | ||
|
There was a problem hiding this comment. my preference would be to merge all no-op refactors like this in a setup PR so we can really be reviewing a minimal diff here There was a problem hiding this comment. I actually tried putting up a PR for noops but they are so minimal they should hardly be a distractor. For instance, |
||
| return this.props.enableColumnHeader ? this.columnHeaderHeight : 0; | ||
| }; | ||
|
|
||
| private getRowHeaderWidth = (): number => { | ||
| return this.props.enableRowHeader ? this.rowHeaderWidth : 0; | ||
| }; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true.
scrollTopshould not be dependent on header size here:scrolltop.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that's the case then I'd expect the viewportRect to not include space for the headers, re https://github.com/palantir/blueprint/pull/6882/files#diff-c2ca442c1f8367c203de64884a8526876c32365d9dd5f071887fb16183ada632R100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewportRectdoes include space for the headers (it contains headers, frozen cells, and the visible "scrollable cells"). The absolute placement of the scrollable cells within the scrollable elementSis such thatS.scrollTopneed not take frozen cell or header height into account. Hopefully the diagram in #6882 (comment) helps explain this a bit better.