Fix: Table not keeping focused cell visible #6882

filipbalucha · 2024-06-28T15:49:09Z

Fixes #1609. Partially addresses #5114.

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

Make sure the focused cell is visible on click or keyboard navigation (including when controlled via props)
Improve internal docs
Add pointers for other improvements uncovered as part of the work here (see end of PR)

Reviewers should focus on:

Screenshot

Before:

before.mov

After:

after.mov

Not addressed in this PR

Polishing interactions with frozen cells. The scrollable part of the table should only scroll if the frozen cell would otherwise not be visible. I file a FR after this merges and maybe FLUP.

frozen.cells.mov
getColumnIndicesInRect operates on viewport rect but does not ignore headers or take frozen cells into account; this might be feeding into Table Whites Out With Mouse Scroll Greater Than 3 Rows #4780 and potentially contaminating the response of onVisibleCellsChange. Again, this is a side observation rather than a regression.

palantirtech · 2024-06-28T15:49:11Z

Thanks for your interest in palantir/blueprint, @filipbalucha! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

packages/table/src/table2.tsx

filipbalucha · 2024-06-28T18:47:57Z

packages/table/src/table2.tsx

-            // we need to modify the scroll container explicitly for the viewport to shift. in so
-            // doing, we add the size of the header elements, which are not technically part of the
-            // "grid" concept (the grid only consists of body cells at present).


This is not true. scrollTop should not be dependent on header size here:

scrolltop.mov

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

viewportRect does 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 element S is such that S.scrollTop need not take frozen cell or header height into account. Hopefully the diagram in #6882 (comment) helps explain this a bit better.

filipbalucha · 2024-06-28T19:02:23Z

packages/table/src/table2.tsx

-                nextScrollLeft ?? 0,
-                nextScrollTop ?? 0,
+                nextScrollLeft ?? viewportRect.left,
+                nextScrollTop ?? viewportRect.top,


Unless both nextScrollLeft and nextScrollTop were used, we would set one value to 0 here. However, viewportRect's top-left point is the value by which the scrollable cells have been scrolled. So, we would be scrolling this back to the first row / column (likely causing batcher to schedule the rendering of cells in that region before being corrected by some other prop change).

I think this change makes sense. In getSnapshotBeforeUpdate I'm seeing

// these will only be defined if they differ from viewportRect return { nextScrollLeft, nextScrollTop };

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

evansjohnson · 2024-07-03T13:42:20Z

docs-app preview: https://output.circle-artifacts.com/output/job/017ed34c-0b72-43cb-97fe-488fb0795dcf/artifacts/0/packages/docs-app/dist/index.html

table-app preview: https://output.circle-artifacts.com/output/job/017ed34c-0b72-43cb-97fe-488fb0795dcf/artifacts/0/packages/table-dev-app/dist/index.html

evansjohnson

This is my first time really diving into the table code so doing my best to review here.

Ideally I'd like to see minimal changes, ex if we can do something without introducing a new setState call, that would be preferred.

Adding to my worries is that it looks like some table tests are not running, or may only be running on Table and not Table2: https://github.com/palantir/blueprint/issues?q=is%3Aissue+is%3Aopen+table2 - and that there are no new tests introduced in this PR.

As a next step, I think it may help if you can help clarify my comments with your thoughts, and consider if we can avoid refactoring the header size into state.

packages/table/src/tableState.ts

packages/table/src/table2.tsx

evansjohnson · 2024-07-03T14:06:54Z

packages/table/src/table2.tsx

-                nextScrollLeft ?? 0,
-                nextScrollTop ?? 0,
+                nextScrollLeft ?? viewportRect.left,
+                nextScrollTop ?? viewportRect.top,


I think this change makes sense. In getSnapshotBeforeUpdate I'm seeing

// these will only be defined if they differ from viewportRect return { nextScrollLeft, nextScrollTop };

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

evansjohnson · 2024-07-03T14:09:03Z

packages/table/src/tableHotkeys.ts

+            (this.state.focusedCell == null || this.state.focusedCell !== newState.focusedCell)
+        ) {
+            // Ensure controlled focused cell is scrolled into view
+            this.scrollBodyToFocusedCell(newState.focusedCell);


it looks like focusedCell can be provided in props which may cause this scroll call to be missed

// only set focused cell state if not specified in props if (this.props.focusedCell == null) { this.setState({ focusedCell }); }

thoughts on adding this scrollBodyToFocusedCell here while leaving it in the places it was already? is there no concern with calling this twice?

what are the cases this handled that are currently missed by handleFocusMove and handleFocusMoveInternal where this is called at the end?

what are the cases this handled that are currently missed by handleFocusMove and handleFocusMoveInternal where this is called at the end?

BLUF: It helps when the focusedCell is controlled and we do not want to rely on native Table2 navigation logic (handleFocusMove, handleFocusMoveInternal).

handleFocusMove and handleFocusMoveInternal correctly call scrollBodyToFocusedCell. But they do this even if focusedCell is controlled:

blueprint/packages/table/src/table2Utils.ts

Lines 78 to 85 in f2e9d6e

if (enableFocusedCell != null) {

hotkeys.push(

{

combo: "left",

group: "Table",

label: "Move focus cell left",

onKeyDown: hotkeysImpl.handleFocusMoveLeft,

},

Now, the table should not use its own navigation mechanism when focusedCell is controlled.

In our case, navigation logic cannot rely on handleFocusMove or handleFocusMoveInternal, and so our controlled focusedCell would not get scrolled into view.

thoughts on adding this scrollBodyToFocusedCell here while leaving it in the places it was already?

Done!

is there no concern with calling this twice?

Assuming ref-consistency of consumers, this should early exit without calling scrollBodyToFocusedCell, so I'm not too concerned.

evansjohnson · 2024-07-03T14:15:53Z

packages/table/src/table2.tsx

-            // we need to modify the scroll container explicitly for the viewport to shift. in so
-            // doing, we add the size of the header elements, which are not technically part of the
-            // "grid" concept (the grid only consists of body cells at present).


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

evansjohnson · 2024-07-03T14:28:32Z

packages/table/src/table2.tsx

-                        : this.rowHeaderElement.clientWidth;
-
-                this.scrollContainerElement.scrollLeft = nextScrollLeft + leftCorrection;
+                this.scrollContainerElement.scrollLeft = nextScrollLeft;


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

packages/table/src/tableHotkeys.ts

filipbalucha · 2024-07-30T01:05:37Z

@evansjohnson This is now ready for another look. I have:

revamped the logic in TableHotkeys,
removed the state update,
re-enabled tests
created a diagram showing how Table2 (and Table) handles the scrollable are and outlining the 4 cases that require scroll (FigJam here):

evansjohnson

going to give final review on this one to @ggdouglas but leaving some last comments for consideration - sorry for the delayed review on this one

evansjohnson · 2024-08-21T18:51:47Z

packages/table/src/tableHotkeys.ts

+            newState.focusedCell != null &&
+            (this.state.focusedCell == null || this.state.focusedCell !== newState.focusedCell)
+        ) {
+            this.scrollBodyToFocusedCell(newState.focusedCell);


is it really true that we always want to scroll the table to the focused cell on any state update? it seems like maybe this should be happening as a more targeted change

ideally we can add this call to the specific internal places needed rather than just assuming this is fine to do on any state update

is it really true that we always want to scroll the table to the focused cell on any state update

Totally appreciate the concern. In short, yes.

The focused cell is a UI metaphor for the centre point of user interaction in the table. By that logic, it should always be visible. That's what Excel does, for instance, with the exception of being scrolled away:

excel.mov

For full disclosure, not keeping the focused cell in view is precisely what brought my team to this issue. The lack of alignment in the presence of headers and frozen rows and columns was a side-discovery made while working on this.

evansjohnson · 2024-08-21T19:00:09Z

packages/table/src/table2.tsx

+        };
+    };
+
+    private getColumnHeaderHeight = (): number => {


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

I actually tried putting up a PR for noops but they are so minimal they should hardly be a distractor. For instance, getColumnHeaderHeight here just encapsulates the implementation from elsewhere in the class.

ggdouglas

Having tested this locally and reviewed the work/conversations here in this PR, I'm satisfied that this meets the bar. Approving so that we can merge this fix in.

filipbalucha · 2024-08-27T10:28:00Z

Thanks @evansjohnson and @ggdouglas for reviewing!

changelog-app · 2024-08-27T10:53:36Z

Generate changelog in `packages/table/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Fix: Table not keeping focused cell visible

Check the box to generate changelog(s)

Generate changelog entry

Invalidated by push of 90587a2

filipbalucha force-pushed the fb/fix-table-does-not-keep-focused-cell-in-viewport branch 2 times, most recently from 1c5b9fe to 7b351a6 Compare June 28, 2024 16:28

filipbalucha changed the title ~~Fb/fix table does not keep focused cell in viewport~~ Fix: Table not keeping focused cell visible Jun 28, 2024

filipbalucha commented Jun 28, 2024

View reviewed changes

packages/table/src/table2.tsx Outdated Show resolved Hide resolved

filipbalucha commented Jun 28, 2024

View reviewed changes

filipbalucha marked this pull request as ready for review June 28, 2024 19:13

ericjeney assigned invliD and evansjohnson Jun 28, 2024

evansjohnson reviewed Jul 3, 2024

View reviewed changes

filipbalucha force-pushed the fb/fix-table-does-not-keep-focused-cell-in-viewport branch from 7bbc491 to 3f64d03 Compare July 30, 2024 00:11

filipbalucha added 2 commits July 30, 2024 01:15

Keep focused cell in viewport

a15af6f

Address comments

6e284f1

filipbalucha force-pushed the fb/fix-table-does-not-keep-focused-cell-in-viewport branch from 3f64d03 to 6e284f1 Compare July 30, 2024 00:15

filipbalucha requested a review from ggdouglas August 20, 2024 15:13

evansjohnson reviewed Aug 21, 2024

View reviewed changes

ggdouglas previously approved these changes Aug 26, 2024

View reviewed changes

filipbalucha added merge when ready no changelog and removed no changelog merge when ready labels Aug 27, 2024

Add changelog manually

90587a2

ggdouglas approved these changes Aug 27, 2024

View reviewed changes

filipbalucha added the merge when ready label Aug 27, 2024

filipbalucha merged commit 7fcf8d0 into palantir:develop Aug 27, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Table not keeping focused cell visible #6882

Fix: Table not keeping focused cell visible #6882

filipbalucha commented Jun 28, 2024 •

edited

Loading

palantirtech commented Jun 28, 2024

filipbalucha Jun 28, 2024

evansjohnson Jul 3, 2024

filipbalucha Jul 30, 2024

filipbalucha Jun 28, 2024

evansjohnson Jul 3, 2024

evansjohnson commented Jul 3, 2024

evansjohnson left a comment

evansjohnson Jul 3, 2024

evansjohnson Jul 3, 2024

evansjohnson Jul 3, 2024

filipbalucha Jul 3, 2024 •

edited

Loading

evansjohnson Jul 3, 2024

evansjohnson Jul 3, 2024

filipbalucha commented Jul 30, 2024

evansjohnson left a comment

evansjohnson Aug 21, 2024

filipbalucha Aug 22, 2024 •

edited

Loading

evansjohnson Aug 21, 2024

filipbalucha Aug 22, 2024 •

edited

Loading

ggdouglas left a comment

filipbalucha commented Aug 27, 2024

changelog-app bot commented Aug 27, 2024 •

edited by filipbalucha

Loading

	if (enableFocusedCell != null) {
	hotkeys.push(
	{
	combo: "left",
	group: "Table",
	label: "Move focus cell left",
	onKeyDown: hotkeysImpl.handleFocusMoveLeft,
	},

Fix: Table not keeping focused cell visible #6882

Fix: Table not keeping focused cell visible #6882

Conversation

filipbalucha commented Jun 28, 2024 • edited Loading

Fixes #1609. Partially addresses #5114.

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

Not addressed in this PR

palantirtech commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansjohnson commented Jul 3, 2024

evansjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipbalucha Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipbalucha commented Jul 30, 2024

evansjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipbalucha Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipbalucha Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

ggdouglas left a comment

Choose a reason for hiding this comment

filipbalucha commented Aug 27, 2024

changelog-app bot commented Aug 27, 2024 • edited by filipbalucha Loading

Generate changelog in packages/table/changelog/@unreleased

filipbalucha commented Jun 28, 2024 •

edited

Loading

filipbalucha Jul 3, 2024 •

edited

Loading

filipbalucha Aug 22, 2024 •

edited

Loading

filipbalucha Aug 22, 2024 •

edited

Loading

changelog-app bot commented Aug 27, 2024 •

edited by filipbalucha

Loading

Generate changelog in `packages/table/changelog/@unreleased`