[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

cee-chen · 2022-01-05T23:11:22Z

General changes

Cell data attributes (`f444d56`)

4 new data attributes have been added to individual grid cells:

data-gridcell-column-id - Static column ID name, not affected by column order
data-gridcell-column-index - Affected by column reordering
data-gridcell-row-index - Index comes from data & is not affected by sorting or pagination
data-gridcell-visible-row-index - Affected by sorting & pagination

The separated props allows for flexibility in choosing specific cells by (e.g.) cell location vs cell data, or even choosing entire rows or columns.

⚠️ data-gridcell-id will be deprecated in 3 months in favor of using the new 4 separate data attrs.

Test hook helper (`961677c`)

I added a new helper for testing custom react hooks in this PR, which we'll continue to use as I write more unit tests for data grid.

Scroll/focus/keyboard nav fixes

closes #3151
closes #4456
closes #5481
closes #5516

Before

Note that using the up arrow to navigate back upwards is in particularly completely broken - see https://elastic.github.io/eui/#/tabular-content/data-grid-virtualization

After

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~

Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~

Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

src/components/datagrid/utils/focus.ts

kibanamachine · 2022-01-05T23:42:44Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5515/

chandlerprall · 2022-01-10T22:19:23Z

Firefox (v95.02) is acting weird for me on this one, it seems to process the changes to focus cell really late, even batching a few of the scroll operations together, so I can pretty easily press down a number of times before the selected cell jumps into focus.

Safari initially did some weird things where it didn't seem to be scrolling at all, but then it began working and I haven't seen it misbehave since 😕

cee-chen · 2022-01-10T22:37:29Z

Yeah I've seen that FF issue intermittently here but it works 80% of the time for me (screencaps were taken on FF). I'm tempted to add an isArrowKeyDown state to our onKeyDown handler instead of using a previousCell ref comparison - my best guess is that is where the staleness is occurring. Does that seem like an OK amount of extra code spaghetti to you?

chandlerprall

Fix for navigating up LGTM, fix for navigating down to the last row LGTM. We can at the footer interaction & non-last-row down separately.

thompsongl · 2022-01-11T23:05:00Z

Looks good! Just one scenario that may or may not be relevant:

Arrow to the last column in a grid with horizontal scroll
Tab out of the grid and scroll back to the first column
Tab into the grid

The correct cell is focused, but the focused cell is not in view. Too much of an edge case?

cee-chen · 2022-01-11T23:08:59Z

@thompsongl that specific edge case of focus restoration should be resolved separately in #5530!

FYI, I'm still wondering if there's a more robust way to handle downwards navigation + I might poke at this a little bit more to try and address the intermittent scrolling issues we're still seeing, so will hold off on merging this for today

thompsongl · 2022-01-11T23:09:59Z

that specific edge case of focus restoration should be resolved separately in #5530!

Just saw that! Thanks

…using keyboard navigation

+ small conversion example for gridStyles

This reverts commit 4068925. This reverts commit 99b370f.

cee-chen

PHEW, ALRIGHT. That was more math than I've done since high school but I've finally got this this working ✨ perfectly ✨

I banged my head around a couple different approaches and got super super close (accounting for scrollbars, sticky footers, etc), but it didn't really click until I realized I was relying too much on react-window's scrollToItem API. scrollToItem is inherently flawed because it doesn't account for sticky headers/footers, BUT I really only need it for cells that are totally out of view. So I stopped using it as a default and instead opted to query for the cell DOM node and use its scroll position/height/width + the grid's scroll container to calculate whether or not the cell actually needed to be fully scrolled into view on any sides.

I'm pretty happy with this final solution as it feels robust and has way less spaghetti/one-offs than other attempts I made, and if the various math positions are a headache to track, I at least tried to comment the everloving shit out of it as well as write (hopefully) helpful unit tests. LMK what you think!

(FYI, I basically scrapped everything from my original PR except the hook test helper, but I kept the commits in the git history just for reference.)

cee-chen · 2022-01-14T00:27:18Z

src/components/datagrid/body/data_grid_cell.tsx

@@ -616,7 +616,7 @@ export class EuiDataGridCell extends Component<
        ref={this.cellRef}
        {...cellProps}
        data-test-subj="dataGridRowCell"
-        data-gridcell-id={`${this.props.rowIndex},${this.props.colIndex}`}
+        data-gridcell-id={`${this.props.colIndex},${this.props.visibleRowIndex}`}


@chandlerprall This is possibly the dicey change in this approach (but unfortunately is necessary, as my entire approach requires being able to obtain the outermost cell DOM node).

I changed the order because it's confusingly the opposite of setFocusedCell which is [colIndex, rowIndex], and I thought the x,y coordinates made a bit more sense.

I also changed this from rowIndex to visibleRowIndex because that's what the focused cell behavior uses for indices, and it feels somewhat more accurate to how react-window behaves - that is to say, on pagination it doesn't actually destroy the cells that exist but instead replaces its contents.

That being said, if that's too big of a change for this PR, maybe we should add a new data attr instead like data-gridcell-location? Thoughts?

We can swap to x,y , but it would be a breaking change (+ Kibana has two affected unit tests) which is okay, and I agree that the x,y pattern fits better.

Instead of changing this attribute to be the visible row index, let's use that second data-gridcell-location idea and provide both.

f444d56

Per our sync call:

I've added 4 new props, data-gridcell-column-id, data-gridcell-column-index, data-gridcell-row-index, and data-gridcell-visible-row-index (with code comments for each prop to help indicate their use-cases).

This allows for flexibility in choosing specific cells by the cell location vs cell data, or even choosing entire rows or columns.

data-gridcell-id will be deprecated in 3 months in favor of using each 4 separated data attrs

cee-chen · 2022-01-14T00:30:55Z

src/components/datagrid/utils/scrolling.ts

+      // Obtain the outermost wrapper of the current cell in view in order to
+      // get scroll position/height/width calculations and determine what level
+      // of scroll adjustment the cell needs
+      const getCell = () =>
+        outerGridRef.current!.querySelector<HTMLDivElement>(
+          `[data-gridcell-id="${colIndex},${rowIndex}"]`
+        );
+      let cell = getCell();


See above comment - .querySelector was the fastest/easiest way I could think of to get the cell we wanted, but I'm aware it's not the most "React-y" approach. Definitely interested in hearing if y'all can think of some way of either obtaining or passing the current cell node in easily. Otherwise, I don't think plain vanilla JS is the worst here.

Quick note that I did also experiment with document.activeElement, but it doesn't work for a few reasons:

It's actually stale and not yet updated to the newly focused cell when this workaround fires

We focus into cell contents that have interactive elements which means sometimes the active element is not the outermost cell wrapper node (which is what we need to correctly calculate widths/heights/scroll positions)

I was planning on using this helper for the new openCellPopover API we're shortly going to be exposing as part of [EuiDataGrid] Expansion popover needs more customization #5310, which will not be triggering a focus element

Short of adding another context that cells can use to register/deregister through, I don't see another way to solve this. I think querySelector is great for navigating these internal-component contracts.

src/components/datagrid/utils/scrolling.spec.tsx

kibanamachine · 2022-01-14T00:38:08Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5515/

…atch setFocusedCell - setFocused cell uses `colIndex,rowIndex` not `rowIndex,colIndex` - this updates our data attr to match, and is more common as an x,y coordinates - NOTE: I'm relying heavily on the ability to find a cell via querySelector and this data-attr in my upcoming logic, hence the desire for standardization

- Less based on `scrollIntoView` than before - I now only call that specific API if the cell is completely out of viewport/not rendered - This allows us to stop fighting react-window's incorrect calculations due to not taking the sticky header into account, and only require the grid+cell's dimensions/scroll position to determine whether or not we need to tweak our scroll position

- Not super in love with the E2E tests, but not sure how to meaningfully spy on code otherwise - maybe visual snapshots would work better?

…e grid's dimensions - or for cells that are not *currently* out lf top/left bound but *would be* once the bottom/right bound was adjusted for (i.e., wider or taller than the grid) + simplify scroll adjustments for the left/top pos - we can simply statically use the cell's position instead of doing math relative to the current scroll position

kibanamachine · 2022-01-14T19:02:36Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5515/

src/components/datagrid/body/data_grid_body.tsx

kibanamachine · 2022-01-14T21:04:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5515/

chandlerprall

Wow this is cool and solves so many edge cases! Couple of thoughts, couple of 🥳 , I think there was one actual suggestion somewhere in here.

src-docs/src/views/datagrid/footer_row.js

chandlerprall · 2022-01-18T03:54:22Z

src/components/datagrid/body/data_grid_cell.tsx

@@ -616,7 +616,7 @@ export class EuiDataGridCell extends Component<
        ref={this.cellRef}
        {...cellProps}
        data-test-subj="dataGridRowCell"
-        data-gridcell-id={`${this.props.rowIndex},${this.props.colIndex}`}
+        data-gridcell-id={`${this.props.colIndex},${this.props.visibleRowIndex}`}


We can swap to x,y , but it would be a breaking change (+ Kibana has two affected unit tests) which is okay, and I agree that the x,y pattern fits better.

Instead of changing this attribute to be the visible row index, let's use that second data-gridcell-location idea and provide both.

src/components/datagrid/body/header/data_grid_header_cell_wrapper.tsx

src/components/datagrid/utils/scrolling.test.ts

src/components/datagrid/utils/scrolling.ts

chandlerprall · 2022-01-18T04:03:23Z

src/components/datagrid/utils/scrolling.ts

+        return; // Grid isn't rendered yet or is empty
+      }
+
+      const gridDoesNotScroll =


I'll make a note to look into if this does matter in any way, but I wonder if this logic should try to match the same scroll*/client* logic in https://github.com/elastic/eui/blob/main/src/components/datagrid/utils/grid_height_width.ts#L146-L148 ?

I think there's some overlapping logic here, in this case I think it's more safe to look at whether in the outer/inner elements share the same width/height to determine whether the grid scrolls. It's worth noting that for example in Firefox, I never actually see a scrollbar width (offsetWidth and clientWidth are the same for me because the scrollbars in FF are 'overlays' and don't actually occupy width), whereas in Chrome/Edge/Safari, the scrollbar actually takes up width.

chandlerprall · 2022-01-18T04:07:25Z

src/components/datagrid/utils/scrolling.ts

+      // Obtain the outermost wrapper of the current cell in view in order to
+      // get scroll position/height/width calculations and determine what level
+      // of scroll adjustment the cell needs
+      const getCell = () =>
+        outerGridRef.current!.querySelector<HTMLDivElement>(
+          `[data-gridcell-id="${colIndex},${rowIndex}"]`
+        );
+      let cell = getCell();


Short of adding another context that cells can use to register/deregister through, I don't see another way to solve this. I think querySelector is great for navigating these internal-component contracts.

src/components/datagrid/utils/scrolling.ts

src/components/datagrid/utils/scrolling.spec.tsx

+ update query selectors and Cypress tests to use new dat attrs + deprecate `data-gridcell-id`

This reverts commit 1e5999b.

kibanamachine · 2022-01-18T19:10:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5515/

chandlerprall

🎉 changes lgtm, cannot find any bugs in user experience in Chrome, FF, or Safari across the various examples.

cee-chen commented Jan 5, 2022

View reviewed changes

src/components/datagrid/utils/focus.ts Outdated Show resolved Hide resolved

src/components/datagrid/utils/focus.ts Outdated Show resolved Hide resolved

cee-chen mentioned this pull request Jan 6, 2022

[EuiDataGrid] Virtualization+scroll+keyboard focus combination bug #5517

Closed

chandlerprall approved these changes Jan 11, 2022

View reviewed changes

cee-chen mentioned this pull request Jan 12, 2022

[EuiDataGrid] Fix multiple grid focus restoration issues #5530

Merged

5 tasks

cee-chen added 6 commits January 13, 2022 12:40

Workaround forcing the datagrid to fully scroll cells into view when …

4068925

…using keyboard navigation

Set up test helper for unit testing custom hooks

961677c

+ small conversion example for gridStyles

Add unit tests for focus keyboard scroll workaround

99b370f

Add changelog entry

e72953e

Revert previous approach

51e405d

This reverts commit 4068925. This reverts commit 99b370f.

[REVERT] Test sticky footer behavior

1e5999b

cee-chen force-pushed the datagrid/3151/4456 branch from 4a65407 to 6d5adbe Compare January 14, 2022 00:06

cee-chen requested a review from chandlerprall January 14, 2022 00:07

cee-chen commented Jan 14, 2022

View reviewed changes

cee-chen added 6 commits January 14, 2022 10:28

Write tests

b1cd532

- Not super in love with the E2E tests, but not sure how to meaningfully spy on code otherwise - maybe visual snapshots would work better?

[misc] Update comment

2c307ed

Merge branch 'main' into datagrid/3151/4456

9d950a9

cee-chen force-pushed the datagrid/3151/4456 branch from 6d5adbe to 9d950a9 Compare January 14, 2022 18:30

cee-chen commented Jan 14, 2022

View reviewed changes

src/components/datagrid/body/data_grid_body.tsx Outdated Show resolved Hide resolved

Comment copy tweak

d218634

chandlerprall reviewed Jan 18, 2022

View reviewed changes

cee-chen added 4 commits January 18, 2022 10:27

[PR feedback] Separate cell row/column data into individual data-attrs

f444d56

+ update query selectors and Cypress tests to use new dat attrs + deprecate `data-gridcell-id`

[PR feedback] Clarify clientWidth/Height comment

a410c56

Revert "[REVERT] Test sticky footer behavior"

d4f5293

This reverts commit 1e5999b.

Merge branch 'main' into datagrid/3151/4456

b7d4cee

cee-chen requested a review from chandlerprall January 18, 2022 18:47

chandlerprall approved these changes Jan 18, 2022

View reviewed changes

cee-chen merged commit ca4435a into elastic:main Jan 18, 2022

cee-chen deleted the datagrid/3151/4456 branch January 18, 2022 20:28

cee-chen mentioned this pull request Jan 18, 2022

[EuiDataGrid] Provide a way to highlight a row #4401

Closed

thompsongl mentioned this pull request Jan 24, 2022

Upgrade EUI to v46.1.0 elastic/kibana#123652

Merged

cee-chen mentioned this pull request Jan 27, 2022

[EuiDataGrid] DRY out scrolling/scrollbar detections and add scroll border overlays #5563

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

cee-chen commented Jan 5, 2022 •

edited

Loading

kibanamachine commented Jan 5, 2022

chandlerprall commented Jan 10, 2022

cee-chen commented Jan 10, 2022

chandlerprall left a comment

thompsongl commented Jan 11, 2022

cee-chen commented Jan 11, 2022

thompsongl commented Jan 11, 2022

cee-chen left a comment •

edited

Loading

cee-chen Jan 14, 2022

chandlerprall Jan 18, 2022

cee-chen Jan 18, 2022

cee-chen Jan 14, 2022

chandlerprall Jan 18, 2022

kibanamachine commented Jan 14, 2022

kibanamachine commented Jan 14, 2022

kibanamachine commented Jan 14, 2022

chandlerprall left a comment

chandlerprall Jan 18, 2022

chandlerprall Jan 18, 2022

cee-chen Jan 18, 2022

chandlerprall Jan 18, 2022

kibanamachine commented Jan 18, 2022

chandlerprall left a comment

[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

[EuiDataGrid] Fix keyboard navigation not fully scrolling cells into view #5515

Conversation

cee-chen commented Jan 5, 2022 • edited Loading

General changes

Cell data attributes (f444d56)

Test hook helper (961677c)

Scroll/focus/keyboard nav fixes

Before

After

Checklist

kibanamachine commented Jan 5, 2022

chandlerprall commented Jan 10, 2022

cee-chen commented Jan 10, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

thompsongl commented Jan 11, 2022

cee-chen commented Jan 11, 2022

thompsongl commented Jan 11, 2022

cee-chen left a comment • edited Loading

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

Choose a reason for hiding this comment

kibanamachine commented Jan 14, 2022

kibanamachine commented Jan 14, 2022

kibanamachine commented Jan 14, 2022

chandlerprall 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

Choose a reason for hiding this comment

kibanamachine commented Jan 18, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 5, 2022 •

edited

Loading

Cell data attributes (`f444d56`)

Test hook helper (`961677c`)

cee-chen left a comment •

edited

Loading