Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cee-chen opened this issue Jan 6, 2022 · 4 comments · Fixed by #5530
Closed

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

cee-chen opened this issue Jan 6, 2022 · 4 comments · Fixed by #5530

Comments

@cee-chen
Copy link
Member

cee-chen commented Jan 6, 2022

Apologies for the nebulous title, this bug is a bit of an edge case so I'm not 100% sure how we want to attempt to solve it or even if we want to do so.

Repro steps:

  1. Go to https://elastic.github.io/eui/#/tabular-content/data-grid-virtualization
  2. Click the Columns toolbar button, then click it again to close it
  3. Press tab 3 times to focus into the grid. It should automatically focus onto the "Name" cell
  4. Click any visible non-sticky data cell (e.g. the first data cell)
  5. Press Tab and then Shift-Tab: notice that focus is restored to the data cell you clicked
  6. Now with your mouse, scroll all the way to the middle or bottom of the grid. The cell you clicked should be long gone from the DOM due to virtualization.
  7. Now with your keyboard, press Tab (should be on pagination). Press Shift-Tab. Notice focus is no longer on any meaningful cell, and the arrow keys do nothing.

I think my reservations on how to fix this are mainly around the fact that I'm not sure if it's unexpected to have a user that relies on both mouse/scroll and keyboard nav/focus. With just keyboard nav alone, the user will be fine (after #5515 lands), and with mouse/scroll alone, the user is always fine. It's just this weird odd combination of both that can end up stranding the data grid's focus state.

So I guess with that in mind, here are some options I can think of:

  1. Leave it alone if it's an edge case 🤷‍♀️
  2. If a focused cell is no longer virtualized, attempt to set a new focus on whatever first cell is visible and focusable
  3. Or, if the user tabs into the grid and a focused cell was set but is no longer virtualized, always jump to that cell using react-window's scrollToItem API

Of those 3 options, I think I prefer 3 as the most straightforward and not potentially interfering with #5047. Thoughts @chandlerprall and @1Copenut?

@cee-chen
Copy link
Member Author

cee-chen commented Jan 6, 2022

Worth noting that #4923 may also generally be related to this and if we want to generally tailor our approach to "how to handle stale focusedCell state"

@1Copenut
Copy link
Contributor

1Copenut commented Jan 10, 2022

@constancecchen I agree that option 3 is the best one. I am probably an edge case, but I do use the mouse for broad UI changes like scrolling, then switch to the keyboard for finer navigation. Being able to return to the cell that had focus was my expected behavior. Moving focus to another part of the UI feels like it goes against WCAG Success Criteria 2.4.3: Focus Order.

@cee-chen
Copy link
Member Author

cee-chen commented Jan 11, 2022

Quick heads up, I chatted with Chandler yesterday and will likely going to go with option 2 as a fix instead of 3. My rationale for that was:

  • With [EuiDataGrid] Keep focus on grid if column gets removed #4923 in mind, option 2 as a final solution is DRYer - both issues can use "find the first visible interactable cell" as a fallback action
  • After more thought, I actually think jumping the user to a non-visible cell is potentially more disruptive and confusing than simply keeping the current scroll position in place. Presumably the user chose to manually scroll up and down the grid, therefore I don't think we should destroy that, but instead preserve their current scroll location
  • I spiked this out and found a way for option 2 to not interfere with [EuiDataGrid] Should not lose focus when scrolling  #5047. As long as the user does not keyboard tab back into the grid before scrolling back to their previously focused cell, the cell will retain focus.

Should hopefully have a PR open by EOD here to test the fix on

@1Copenut
Copy link
Contributor

Good discussion and points in favor of the second solution!

cee-chen added a commit to cee-chen/eui that referenced this issue Jan 11, 2022
- 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
cee-chen added a commit to cee-chen/eui that referenced this issue Jan 11, 2022
…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
cee-chen pushed a commit that referenced this issue Jan 12, 2022
* [#5517] Add `isFocusedCellInView` state to track visibility

- 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

* [#5517] Add `onItemsRendered` ref to store currently visible/virtualized 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

* [#4923] Fix focus returning to document body when hiding a column via header

* [#5476] Fix keyboard navigation after hiding a column via header actions

* Add changelog entry

* [PR feedback] Add explanatory comment in unit test

* [PR feedback] Account for empty grids

- 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

* [bug] Handle arrow keydown behavior on sticky header when row 0 is not in view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants