[EuiDataGrid] resolve performance issues when interacting with a large grid

[chandlerprall]

Summary

Fixes #5115 by making our call to tabbable more exact.

Background

Application-provided content, rendered in EuiDataGrid cells, need to be removed from the page's tab order as we only want arrow keys to move focus between cells, and tab should jump into the grid's focused cell, and then continue to the next interactive element after the grid.

We had a previous performance issue with our approach to solving that, where every cell created its own mutation observer. We solved that by moving the mutation observer to the body component, but called tabbable with the entire grid instead of the more specific cell content containers.

However, on mouse over, cells render any associated actions which creates a DOM mutation, triggering the mutation observer, which slowed an entire page beyond a crawl just by mousing over a large grid.

Solve

This PR changes the call to tabbable, providing any affected/mutated cell content container instead of the entire grid. This greatly optimizes the work done by tabbable (it removes the work entirely for this case of just hovering over cells), making it an O(1) execution time amortized over cell contents' complexity instead of O(n) dependent on all cells currently rendered.

Alternative

I tried creating the mutation observer as part of the grid's context and allowing each individual cell to add an observed DOM node, which would avoid needing the getParentCellContent function, but observers have no way to unobserve without destroying the entire observer. This would have leaked cells' DOM into memory until the grid (and it's single observer) was GC'd.

Before

moving cursor over 10 rows

moving cursor over 100 rows

After

moving cursor over 10 rows

moving cursor over 100 rows

Result

Moving from 1-1.5s (2-5s in siem's table) stop-the-world when leaving a cell to ~40ms.

But you said this fix removes all work

Yes, but React Does Things™ to support onMouseEnter event handlers, which each cell uses. This is the cause of the execution time in the result screen shots. I want to understand this better, because of course I do, but it's unrelated to the issue at hand. Also, most of that time is spent by React doing development environment things, and a prod build is significantly better:

Testing

• Mouse your cursor over the grid, see how the links now underscore as you do it instead of the page locking up for seconds.
• The first example was modified (I'll remove before merging) to show the interactive-disabling logic still functions.

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 the any docs examples
  - [ ] Added or updated jest 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

[kibanamachine]

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

[kibanamachine]

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

[cee-chen]

This is super amazing, huge kudos for the fix Chandler! This isn't a blocker per se and it's definitely more important we get this resolved for the Security folks, but I might as well mention it since I have EuiDataGrid unit testing on the brain - it feels like a bit of a shame to not be able to write tests for these (although admittedly that would be pretty difficult in Jest), mostly as a way document the exact use-case/problem we're trying to solve here.

Any thoughts on it if it would be a significant dev improvement to add a longer comment block above getParentCellContent, or tweaking the comment on line 620 to note that this is being done for performance (iterating upwards to the closest cell rather than tabbable parsing the entire wrapper)? I think between the 2 I have a slight lean towards the former over the inline comment, but totally up to you :)

[chandlerprall]

Indeed! I was thinking the updated snapshots would suffice for now against regressions, and then we can really start codifying a lot of the functionality, etc, with cypress.

[chandlerprall]

@constancecchen pushed up two additional comments

[cee-chen]

Awesome, thanks!! Super appreciate the longer comment, it helped clarify things even more for me personally! I have some small option copy nits, but otherwise LGTM!

[kibanamachine]

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

[kibanamachine]

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

[snide]

Verified gains and double checked our example docs to make sure they worked. Have one small comment around the docs presentation.

[chandlerprall]

09:34:40 ERROR: Failed to set up Chromium r818858! Set "PUPPETEER_SKIP_DOWNLOAD" env variable to skip download.
09:34:40 { Error: read ECONNRESET
09:34:40 at TLSWrap.onStreamRead (internal/stream_base_commons.js:111:27) errno: 'ECONNRESET', code: 'ECONNRESET', syscall: 'read' }

gonna push the example revision and use that to re-kick CI

[kibanamachine]

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