[EuiDataGrid] Fix multiple grid focus restoration issues #5530

cee-chen · 2022-01-11T20:28:15Z

Summary

This PR fixes 3 bugs with EuiDataGrid around restoring correct focus/keyboard navigation to the grid:

`c06db86` and `7f031ac` closes #5517

If the user scrolls the currently focused cell out of view (via mouse) and then attempts to tab back into the grid (via keyboard), the grid should intelligently focus into the first visible and interactive cell (instead of the entire grid becoming unfocusable).

refocus_grid.mp4

`d29d938` closes #4923

Problem: If the user hides the current column via the column header's actions popover, the popover cannot focus back into the now-removed cell, and sets the current focus back to the top of the document
Fix: Focus should be manually set to the grid by targeting its first visible and interactive cell (which is not perfect, but at least does not strand the user and require them tab all the way back to the grid)

hide_column.mp4

`8a035de` closes #5476

Fixes keyboard focus index being incorrect after moving a column left or right via the header's actions popover.

move_column.mp4

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
Revert [REVERT] commit

- and use this to determine whether or not the entire grid should have a tabindex/be focusable, rather than a simple 'hasFocus' check which doesn't account for virtualization - Add new mockFocusContext for easier testing for various tests that need to set a focus context - EuiDataGridCell - DRY out an `this.isFocusedCell` method - Call `setIsFocusedCellInView` on mount and on unmount (`setFocusedCell` handles setting true/false on new cell focus) - Write new unit tests for componentWillUnmount - Clean up unnecessary uncovered function fallback for this.unsubscribeCell - we're already checking for a falsy value before calling it

…ntent - see http://localhost:8030/#/tabular-content/data-grid-focus

…irtualized refs - see https://react-window.vercel.app/#/api/FixedSizeGrid for `onItemsRendered` API - this struck me as the quickest/easiest way to determine which virtualized cells are in view - I opted to save this as a ref instead of as state as I didn't see the need to cause a rerender - `focusFirstVisibleInteractiveCell` was split out to its own fn as it will shortly be used elsewhere to fix another focus bug in the grid

…umn via header

…er actions

cee-chen · 2022-01-11T20:31:06Z

@1Copenut Tagging you in for QA help, since you opened at least one of the issues I'm fixing in this PR and have familiarity with the first one. Let me know if you see any issues with the fixes!

src/components/datagrid/body/data_grid_body.tsx

src/components/datagrid/utils/focus.ts

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

src/components/datagrid/utils/focus.ts

kibanamachine · 2022-01-11T21:04:11Z

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

chandlerprall

LGTM, tested with the modified focus.js and the main example. Also messed with some of the tests locally to ensure their effectiveness 👍 .

…lling content" This reverts commit 7f031ac.

cee-chen · 2022-01-11T21:24:43Z

@1Copenut going to go ahead with turning auto merge on this PR, but feel free to confirm that the issue you opened is resolved after (+ ping me if you find anything else)!

kibanamachine · 2022-01-11T22:10:25Z

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

- gridItemsRendered will be falsey if rowCount is 0 and will cause an error on tab, so we should opt to simply leave focus on the grid body wrapper

kibanamachine · 2022-01-11T23:12:36Z

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

chandlerprall

Changes L even more GTM; tested the focus example with rowCounter={0} with both non- & interactive headers, and played more with the main example.

cee-chen · 2022-01-12T16:40:15Z

Trevor just found an issue that I'll push up another bugfix for:

Go to https://eui.elastic.co/pr_5530/#/tabular-content/data-grid-virtualization
Click or tab to first data cell (not header cell)
Scroll to bottom of grid (or enough so that cell is unmounted)
Press tab then shift tab back to the grid
Notice that while focus is returned to the sticky header, you cannot actually press arrow down from the sticky header into the data cells
This is because the sticky header is a row index of -1 and pressing down tries to return it to row 0 (first row) which is no longer rendered

I'm going to have to adjust the branching logic a bit further to account for this. Questions on how we prefer to treat this:

If row 0 is no longer visible/rendered, focus the first visible data cell instead of focusing the first header cell
Add some extra logic when pressing arrow down on the header row to try and find the first visible row index and jump to that instead of to row 0

Do we have any preference between the two options @1Copenut @chandlerprall? While 2 is kind of appealing as being "more intelligent", I think 1 is probably more straightforward and less likely to lead to edge cases/shenanigans - WDYT?
EDIT: Actually now that I think about it more I'm leaning towards more 2 haha...

1Copenut · 2022-01-12T16:55:26Z

I feel like the first option, focusing the first visible data cell, would be a good experience. I could arrow up once to get into the header row, and left, right, down to move through the virtualized grid cells as normal.

How difficult is it to detect if a cell is in view? User scrolling will leave us with inexact "stops" -- users might have half a cell in the visible frame when the stop scrolling, does that cell get "snapped" to just below the header row? I might be inventing problems that'll be addressed as code is updated.

cee-chen · 2022-01-12T16:57:58Z

After more testing, I'm going to go with option 2 because I actually realized the issue can be fully reproduced by an even simpler means:

Go to https://eui.elastic.co/pr_5530/#/tabular-content/data-grid-virtualization
Don't even click anything or tab into it, but scroll to the middle or bottom of the grid
Click the first header cell or tab into it, and try to press arrow down
Nothing happens

Fixing the arrow down key behavior on the sticky header will generally be a more robust fix IMO.

edit to add: we could also try to force row 0 into view with a manual scrollToItem call when pressing arrow down from the sticky header, but I kinda feel like that's scrolljacking and worse UX (see my second bullet point in #5517 (comment))

…t in view

cee-chen · 2022-01-12T17:26:03Z

@1Copenut sorry, totally forgot to address a few points in your comment:

I could arrow up once to get into the header row, and left, right, down to move through the virtualized grid cells as normal.

Just wanted to clarify that even with this new fix, users will still need to go all the way to the first row (row 0) to be able to up arrow key into the sticky header. Not sure there is any way around this and this is how it's always been UX-wise AFAICT. In theory the Home key should allow users to quickly jump to row 0.

How difficult is it to detect if a cell is in view?

We get this for free from react-window, they have an onItemsRendered API that feeds us back the current first visible row index (which I added a ref for in this PR).

User scrolling will leave us with inexact "stops" -- users might have half a cell in the visible frame when the stop scrolling, does that cell get "snapped" to just below the header row? I might be inventing problems that'll be addressed as code is updated.

Not inventing problems at all, this is an open issue (#3151) that I currently have a separate open PR for: #5515

One downside of the disparate modular PRs is that we don't get to see them altogether in action just yet. So currently this PR will still have the same issue where you can down arrow key from the sticky header into a cell that's just barely visible. Once this PR merges, I'll absolutely update that PR to test out the combined behavior and make sure that newly focused cells are always in view.

kibanamachine · 2022-01-12T17:54:12Z

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

1Copenut

LGTM! I did a fair bit of random key pressing after this latest update. The focus handling and setting to the first row, first cell on virtualized scroll all feels smooth and intuitive. Thank you Constance!

cee-chen · 2022-01-12T19:56:01Z

Going to merge this in now as I suspect I'll need gridItemsRendered for the upcoming 'cells should always be in view' fix. Feel free to ping if there are any code issues or change requests, I can absolutely address in a follow-up PR

cee-chen added 5 commits January 11, 2022 12:06

[REVERT] Update demo to test noninteractive headers with scrolling co…

7f031ac

…ntent - see http://localhost:8030/#/tabular-content/data-grid-focus

[elastic#4923] Fix focus returning to document body when hiding a col…

d29d938

…umn via header

[elastic#5476] Fix keyboard navigation after hiding a column via head…

8a035de

…er actions

cee-chen requested review from chandlerprall and 1Copenut January 11, 2022 20:28

Add changelog entry

129febd

cee-chen commented Jan 11, 2022

View reviewed changes

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

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

src/components/datagrid/body/header/column_actions.tsx Show resolved Hide resolved

chandlerprall reviewed Jan 11, 2022

View reviewed changes

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

chandlerprall approved these changes Jan 11, 2022

View reviewed changes

cee-chen added 2 commits January 11, 2022 13:17

Revert "[REVERT] Update demo to test noninteractive headers with scro…

0fce549

…lling content" This reverts commit 7f031ac.

[PR feedback] Add explanatory comment in unit test

4fad086

cee-chen enabled auto-merge (squash) January 11, 2022 21:25

Merge branch 'main' into datagrid/5517/4923/5476

bd7f192

cee-chen disabled auto-merge January 11, 2022 21:48

[PR feedback] Account for empty grids

9a69534

- gridItemsRendered will be falsey if rowCount is 0 and will cause an error on tab, so we should opt to simply leave focus on the grid body wrapper

cee-chen requested a review from chandlerprall January 11, 2022 22:40

cee-chen mentioned this pull request Jan 11, 2022

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

Merged

6 tasks

chandlerprall approved these changes Jan 12, 2022

View reviewed changes

[bug] Handle arrow keydown behavior on sticky header when row 0 is no…

694621b

…t in view

1Copenut approved these changes Jan 12, 2022

View reviewed changes

cee-chen merged commit 8cb2c3f into elastic:main Jan 12, 2022

cee-chen deleted the datagrid/5517/4923/5476 branch January 12, 2022 19:56

thompsongl mentioned this pull request Jan 24, 2022

Upgrade EUI to v46.1.0 elastic/kibana#123652

Merged

cee-chen mentioned this pull request Apr 18, 2024

[EuiDataGrid] Fix column header move left focus bug #7698

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Fix multiple grid focus restoration issues #5530

[EuiDataGrid] Fix multiple grid focus restoration issues #5530

cee-chen commented Jan 11, 2022 •

edited

Loading

cee-chen commented Jan 11, 2022

kibanamachine commented Jan 11, 2022

chandlerprall left a comment

cee-chen commented Jan 11, 2022

kibanamachine commented Jan 11, 2022

kibanamachine commented Jan 11, 2022

chandlerprall left a comment

cee-chen commented Jan 12, 2022 •

edited

Loading

1Copenut commented Jan 12, 2022

cee-chen commented Jan 12, 2022 •

edited

Loading

cee-chen commented Jan 12, 2022

kibanamachine commented Jan 12, 2022

1Copenut left a comment

cee-chen commented Jan 12, 2022

[EuiDataGrid] Fix multiple grid focus restoration issues #5530

[EuiDataGrid] Fix multiple grid focus restoration issues #5530

Conversation

cee-chen commented Jan 11, 2022 • edited Loading

Summary

c06db86 and 7f031ac closes #5517

d29d938 closes #4923

8a035de closes #5476

Checklist

cee-chen commented Jan 11, 2022

kibanamachine commented Jan 11, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 11, 2022

kibanamachine commented Jan 11, 2022

kibanamachine commented Jan 11, 2022

chandlerprall left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 12, 2022 • edited Loading

1Copenut commented Jan 12, 2022

cee-chen commented Jan 12, 2022 • edited Loading

cee-chen commented Jan 12, 2022

kibanamachine commented Jan 12, 2022

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 12, 2022

cee-chen commented Jan 11, 2022 •

edited

Loading

`c06db86` and `7f031ac` closes #5517

`d29d938` closes #4923

`8a035de` closes #5476

cee-chen commented Jan 12, 2022 •

edited

Loading

cee-chen commented Jan 12, 2022 •

edited

Loading