-
Notifications
You must be signed in to change notification settings - Fork 843
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
Comments
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 |
@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. |
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:
Should hopefully have a PR open by EOD here to test the fix on |
Good discussion and points in favor of the second solution! |
- 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
…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
* [#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
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:
Columns
toolbar button, then click it again to close itI 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:
scrollToItem
APIOf those 3 options, I think I prefer 3 as the most straightforward and not potentially interfering with #5047. Thoughts @chandlerprall and @1Copenut?
The text was updated successfully, but these errors were encountered: