[EuiDataGrid] Simplify & DRY out cell/header focus #7448

cee-chen · 2024-01-04T21:53:26Z

Summary

Extra context: I originally spiked most of this out during the cell redesigns (#7382) probably because the repeated logic or keyboard UX slightly bothered me 😅 It turned out a larger set of changes than I wanted for a single release, so I split it out to a separate branch for later. Since it was very close to completion, I figured I'd finish it up to clean out my lingering 2023 work.

As always, please follow along by commit. While it looks like more lines of code were added in this PR, those were primarily all tests (a good thing to have more of) and snapshots diffs. From actual source code, a net ~500 or so lines were removed.

Main changes in this PR:

I removed all logic around detecting headerIsInteractive. From now on, data grid headers can ALWAYS be navigated to via arrow keys, regardless of whether or not header cells have interactive content or not.
- I believe this logic was originally added to make EuiDataGrid behave like the "simple" data grid example in https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/data-grids/#examples.
- However, I don't think this is a pattern worth emulating. EuiDataGrids don't get used for simple data, otherwise devs would reach for EuiBasicTable instead. I strongly feel the extra code, state, and mutation observer was not worth the unnecessary UX, and frankly I also think the UX is confusing and creates inconsistencies between data grids.
Non-expandable cells that only contain 1 interactive child element no longer auto-focus that element, but instead render a focus trap
- Again, I believe this logic was trying to mimic the above WCAG demo behavior, but frankly, I don't consider that behavior worth emulating. To reiterate, our data grid usages in production are vastly more complex than these simplistic demos.
- The logic also introduced accessibility issues (see above [EuiDataGrid] Focus behavior on cells that are isExpandable: false and have 1 interactive child #5690, accessibility concerns) where other non-interactive content in the cell would not be accessible to keyboard users.
- A focus trap is overall more consistent with how other cells behave (Enter key) and more accessible to a variety of cell content.
Cell screen reader text now contains instructions for the Enter keypress behavior in addition to current column position.

For the majority of default EuiDataGrid use cases (expandable cells & column headers with actions), nothing should have changed or regressed from before.

QA

Go to https://eui.elastic.co/pr_7448/#/tabular-content/data-grid-cells-popovers#focus
- Confirm that pressing the Enter key on the non-expandable cells with interactive links/buttons inside them (both header and data cells) enters a focus trap
- Confirm that Tabbing within this focus trap only tabs to items within the cell
- Confirm the cell focus trap can be exited via the Escape key
Regression testing
- Go to https://eui.elastic.co/pr_7448/#/tabular-content/data-grid
- Confirm that tabbing in/out of the datagrid body and arrow navigation works as expected with no regressions
- Confirm that cell actions' hover/focus/popover open behavior behaves as before with no regressions

General checklist

Browser QA
~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA
- Updated documentation
  - [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
  ~~- [ ] Checked Code Sandbox works for any docs examples~~
Code quality checklist
- Added or updated jest and cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist
~~- [ ] Updated the Figma library counterpart~~

- there's enough now that it's clogging up the `body/` folder and it makes sense to

- all of the removed methods are now totally unnecessary and handled as-is by `HandleInteractiveChildren`

…state + misc imports reorder/reorg

- cell actions are rendered in the DOM, just not visible, which saves us from having to add a bunch of extra hover/focus state trackers

- all of the removed methods are now totally unnecessary and handled as-is by `HandleInteractiveChildren

- auto focus the column header actions button instead of the wrapping grid cell, as it contains more SR information than just the cell - fix cell focus border to appear correctly as before - write E2E Cypress tests to replace old Jest tests

+ better header focus trap testing

- removes the need for a bunch of extra state/props, a mutation observer, hooks, etc. - it really doesn't add anything except extra cruft and inconsistency to have the header not be navigable to, hence this entire PR removing it

- to account for header now always being navigable no matter what

- remove unnecessary `getGridData()` call which isn't asserting on anything

- my local vscode doesn't complain about this, but CLI does - not totally sure why or if this makes sense :|

cee-chen · 2024-01-04T23:32:40Z

@1Copenut Hey hey! I was hoping you could review this since this is primarily accessibility driven in improvements (in addition to the side benefit of lots of DRYing out/removal of code) 🙏

…havior

cee-chen · 2024-01-05T18:44:59Z

@1Copenut Header focus fighting should be fixed once staging is done updating! Thank you so much for catching that bug!

- caused by race condition with ref not updating `HandleInteractiveChildren`

kibanamachine · 2024-01-05T21:54:15Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7448/
Storybook: https://eui.elastic.co/pr_7448/storybook

elasticmachine · 2024-01-05T21:54:19Z

💚 Build Succeeded

Buildkite Build
Commit: 1d9e46f

History

💔 Build #1237 failed d6c70f6
💚 Build #1232 succeeded 41ed92e
💚 Build #1229 succeeded c767730

1Copenut

🚀 LGTM! I tested the QA requirements you laid out in two rounds--one before we discovered the regression in header row behavior and once after to verify the fix. This gave me two opportunities to drive the new UX for handling rows with one interactive element.

This was the right call, to make cells with 1 or N interactive elements behave the same. I was able to internalize the pattern quickly and didn't feel the extra click to do things like open the Actions menu was an inconvenience.

Testing groups:

Evergreen browsers [Chrome, Firefox, Edge, Safari]
MacOS Safari + VoiceOiver
Win10 Firefox + NVDA
Win10 Chrome + NVDA
Win10 Chrome + JAWS

1Copenut · 2024-01-05T21:19:11Z

src/components/datagrid/body/cell/focus_utils.tsx

+          <EuiI18n
+            // eslint-disable-next-line local/i18n
+            token="euiDataGridCell.focusTrapEnterPrompt"
+            default="Press the Enter key to interact with this cell's contents."


This is the one thing I've been mulling over in testing. I love the clear directions to enter the focus trap. I wonder if we need explanation to exit the focus trap via the ESC key. My hunch is no, at least until data tells us otherwise. I wonder if the Research team would be up for a screen reader testing session.

I was wondering that as well - thanks for raising it! I think I also lean towards waiting to see if it's needed before adding. I'd love for us to start doing screen reader research!

1Copenut · 2024-01-05T21:56:11Z

src/components/datagrid/body/cell/focus_utils.tsx

+ * This internal utility component is used by all cells, both header and body/footer cells.
+ * It always handles:
+ *   1. Removing any interactive children from keyboard tab order on cell mount
+ *   2. Listening for focus on any interactive children and updating the cell focus context
+ *
+ * It should *only* render focus traps for:
+ *   1. Header cells that are `actions: false` but still have interactive children
+ *   2. Body cells that are `isExpandable: false` but still have interactive children
+ */


This explanation of the utility was especially helpful in QA testing. Between this documentation and the clear separation of concerns, I was able to answer 90% of my own questions. This is greatly appreciated by me and will pay dividends for future consumers. Thanks Cee!

1Copenut · 2024-01-05T21:56:59Z

src/components/datagrid/body/cell/focus_utils.tsx

+ * focus trap that can be entered with the Enter key, which cycles keyboard tabs
+ * through the cell contents only, and exited with the Escape key
+ */
+export const FocusTrappedChildren: FunctionComponent<


This drives so much better with screen readers, having one interaction pattern for 1 or N number of interactive elements. It felt natural and I didn't notice the extra click.

cee-chen · 2024-01-08T17:51:33Z

Thanks a million for the QA and feedback Trevor!! Super happy this was a net screen reader improvement ✨

Going to merge this in after today's release to spread out changes a bit.

`v92.0.0-backport.0`⏩ `v92.1.1` --- ## [`v92.1.1`](https://github.com/elastic/eui/releases/v92.1.1) **Bug fixes** - Minor `EuiDataGrid` cell performance fixes ([#7465](elastic/eui#7465)) ## [`v92.1.0`](https://github.com/elastic/eui/releases/v92.1.0) - Updated `EuiResizableButton` to allow customizing the `indicator` style with either `handle` (default) or `border` ([#7455](elastic/eui#7455)) - Enhanced `EuiResizableContainer` to preserve the drag/resize event when the user's mouse leaves the parent container and re-enters ([#7456](elastic/eui#7456)) **Bug fixes** - Fixed an `EuiTreeView` JSX Typescript error ([#7452](elastic/eui#7452)) - Fixed a color console warning being generated by disabled `EuiStep`s ([#7454](elastic/eui#7454)) **Accessibility** - `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to be more consistent for varying complex data: ([#7448](elastic/eui#7448)) - Headers are now always navigable by arrow key, regardless of whether the header cells contain interactive content - Non-expandable cells containing any amount of interactive content now must be entered via Enter or F2 keypress - Expandable cells continue to be toggled via Enter or F2 keypress - `EuiDataGrid` now provides a direct screen reader hint for Enter key behavior for expandable & interactive cells ([#7448](elastic/eui#7448))

`v92.0.0-backport.0`⏩ `v92.1.1` --- ## [`v92.1.1`](https://github.com/elastic/eui/releases/v92.1.1) **Bug fixes** - Minor `EuiDataGrid` cell performance fixes ([elastic#7465](elastic/eui#7465)) ## [`v92.1.0`](https://github.com/elastic/eui/releases/v92.1.0) - Updated `EuiResizableButton` to allow customizing the `indicator` style with either `handle` (default) or `border` ([elastic#7455](elastic/eui#7455)) - Enhanced `EuiResizableContainer` to preserve the drag/resize event when the user's mouse leaves the parent container and re-enters ([elastic#7456](elastic/eui#7456)) **Bug fixes** - Fixed an `EuiTreeView` JSX Typescript error ([elastic#7452](elastic/eui#7452)) - Fixed a color console warning being generated by disabled `EuiStep`s ([elastic#7454](elastic/eui#7454)) **Accessibility** - `EuiDataGrid`'s keyboard/screenreader experience has been tweaked to be more consistent for varying complex data: ([elastic#7448](elastic/eui#7448)) - Headers are now always navigable by arrow key, regardless of whether the header cells contain interactive content - Non-expandable cells containing any amount of interactive content now must be entered via Enter or F2 keypress - Expandable cells continue to be toggled via Enter or F2 keypress - `EuiDataGrid` now provides a direct screen reader hint for Enter key behavior for expandable & interactive cells ([elastic#7448](elastic/eui#7448))

cee-chen added 8 commits January 3, 2024 10:27

[organization] Move cell-related files to their own folder

d17b6a5

- there's enough now that it's clogging up the `body/` folder and it makes sense to

Add new focus util

d670e85

[EuiDataGridCell] Replace focus logic with new focus util

a79faa3

- all of the removed methods are now totally unnecessary and handled as-is by `HandleInteractiveChildren`

[EuiDataGridCell] Continue removing now-unnecessary focus timeouts & …

3c04c75

…state + misc imports reorder/reorg

[downstream tests] Update tests to account for cellActions change

21ae96c

- cell actions are rendered in the DOM, just not visible, which saves us from having to add a bunch of extra hover/focus state trackers

[EuiDataGridCell] Remove Jest focus tests in favor of Cypress ones

1845f5a

[EuiDataGridHeaderCell] Replace focus logic with new util

fdb932a

- all of the removed methods are now totally unnecessary and handled as-is by `HandleInteractiveChildren

cee-chen added the data grid label Jan 4, 2024

cee-chen added 9 commits January 4, 2024 15:25

[Docs] Update intended final focus behavior

165e6e2

+ better header focus trap testing

[Focus] Make header always interactive & navigable

f39ba58

🔥 Remove headerIsInteractive logic entirely

31adcba

- removes the need for a bunch of extra state/props, a mutation observer, hooks, etc. - it really doesn't add anything except extra cruft and inconsistency to have the header not be navigable to, hence this entire PR removing it

[misc perf] Remove dependency on useCallback fn

5c9214e

[downstream tests] Update data grid tests w/ keyboard interactions

d844561

- to account for header now always being navigable no matter what

misc test cleanup

72df313

- remove unnecessary `getGridData()` call which isn't asserting on anything

[downstream tests] snapshot updates

45b3f31

changelog

292b7d8

Typescript lint workaround

0bcef9b

- my local vscode doesn't complain about this, but CLI does - not totally sure why or if this makes sense :|

cee-chen force-pushed the datagrid/cell-focus branch from c767730 to e0a1584 Compare January 4, 2024 23:25

Add screen reader text for Enter key behavior

41ed92e

cee-chen force-pushed the datagrid/cell-focus branch from e0a1584 to 41ed92e Compare January 4, 2024 23:30

cee-chen marked this pull request as ready for review January 4, 2024 23:31

cee-chen requested a review from a team as a code owner January 4, 2024 23:31

cee-chen requested a review from 1Copenut January 4, 2024 23:31

[PR feedback] Fix focus fighting bug + add E2E regression test for be…

d6c70f6

…havior

cee-chen force-pushed the datagrid/cell-focus branch from 8a2868d to d6c70f6 Compare January 5, 2024 18:44

Fix flaky Cypress test

1d9e46f

- caused by race condition with ref not updating `HandleInteractiveChildren`

cee-chen force-pushed the datagrid/cell-focus branch from 9f8601e to 1d9e46f Compare January 5, 2024 21:26

1Copenut approved these changes Jan 5, 2024

View reviewed changes

cee-chen merged commit e67ef5d into elastic:main Jan 8, 2024
7 checks passed

cee-chen deleted the datagrid/cell-focus branch January 8, 2024 19:16

This was referenced Jan 16, 2024

Upgrade EUI to v92.1.1 elastic/kibana#174955

Merged

[EuiDataGrid] Fix cell performance #7465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Simplify & DRY out cell/header focus #7448

[EuiDataGrid] Simplify & DRY out cell/header focus #7448

cee-chen commented Jan 4, 2024 •

edited by 1Copenut

Loading

cee-chen commented Jan 4, 2024 •

edited

Loading

cee-chen commented Jan 5, 2024

kibanamachine commented Jan 5, 2024

elasticmachine commented Jan 5, 2024

1Copenut left a comment

1Copenut Jan 5, 2024

cee-chen Jan 8, 2024

1Copenut Jan 5, 2024

1Copenut Jan 5, 2024

cee-chen commented Jan 8, 2024

[EuiDataGrid] Simplify & DRY out cell/header focus #7448

[EuiDataGrid] Simplify & DRY out cell/header focus #7448

Conversation

cee-chen commented Jan 4, 2024 • edited by 1Copenut Loading

Summary

Main changes in this PR:

QA

General checklist

cee-chen commented Jan 4, 2024 • edited Loading

cee-chen commented Jan 5, 2024

kibanamachine commented Jan 5, 2024

elasticmachine commented Jan 5, 2024

💚 Build Succeeded

History

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Jan 5, 2024

Choose a reason for hiding this comment

cee-chen Jan 8, 2024

Choose a reason for hiding this comment

1Copenut Jan 5, 2024

Choose a reason for hiding this comment

1Copenut Jan 5, 2024

Choose a reason for hiding this comment

cee-chen commented Jan 8, 2024

cee-chen commented Jan 4, 2024 •

edited by 1Copenut

Loading

cee-chen commented Jan 4, 2024 •

edited

Loading