[EuiDataGrid] Fix sorting console errors + header cell focus cleanup/refactors by cee-chen · Pull Request #5209 · elastic/eui

cee-chen · 2021-09-22T17:02:10Z

Summary

This PR contains 2 console error fixes, a functionality removal that was already not working on master, several cleanup/refactors of the EuiHeaderGridCellWrapper focus code (that I noticed and wanted to work on while debugging/fixing the above bugs), and several unit test improvements. I strongly recommend following along by commit.

Console error fixes
- 6be8f53
  - I noticed that when dragging a column outside the popover, a destination is null error appears in the console:
  - This commit adds a simple object existence check to ensure it's not null.
- ac5e058
  - closes [EuiDataGrid] Sorting columns after focusing a header cell throws console errors #4384
  - Currently in Safari, clicking on a header cell and then opening the Columns popover and attemping to drag/drop reorder the clicked header cell causes a focus fight:
  - This happens because the focusin/focusout events are firing repeatedly on the moved headercell, triggering focus events on the header cell and causing a fight between the cell and the reorder popover.
  - Removing the manual focusInteractives() call and instead relying on setIsCellEntered(true) removes this lock/fight, because the state change does not repeatedly call itself
  - TBH, It's possible that this isFocused conditional branch isn't even needed with current header cell behavior, but this is a pretty simple fix, so I'm leaving it as is
Functionality removal
- 405b1c0
  - I chatted with @chandlerprall earlier to confirm that F2 keyboard event behavior is not working on master, and it makes sense that it isn't. We agreed that the logic for it can be removed, since we treat the header cell itself as the widget, which means there isn't a discernible focus state between cell focus or widget focus.
  - We also console warn when header control cells (which can contain any children) contain more than one interactable element.
Refactors
- The remaining commits deal with several cleanups (mostly moving and combining code / unit tests) and is easier to follow along by commit.

QA

Go to https://eui.elastic.co/pr_5209/#/tabular-content/data-grid
In the Columns popover, confirm that dragging and dropping a column outside the popover does not generate a console error
In the Safari browser, confirm that clicking a header cell and then opening the Columns popover and trying to drag/drop the clicked header cell does not create a stack call console error

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile (toolbar doesn't appear on 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 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

Setting isCellEntered to true already makes that same call, so *should* have that same effect as long as we're consistent about changing logic via isCellEntered This also fixes elastic#4384, which was a focus-lock issue caused focusin/focusout events firing multiple times. Now with the setIsCellEntered change, focus does not fire repeatedly

- now that we're not calling enableInteractives and focusInteractives directly but letting isCellEntered handle that for us, there's no need to include them + setIsCellEntered isn't necessary either/doesn't trigger the lint rule

- Separate focus tests for isFocused logic vs focusin / focusout events

- Removes the need for useCallback and setting the utility fns as dependencies, simplifying code - refactor for loops to forEach's - remove setIsCellEntered(true) in focusInteractables, since it now only gets called when isCellEntered is true

- they don't get called separately, so it makes sense to optimize the call and not make an extra tabbables call (+ reduces unnecessary branching)

- covers last uncovered branch in file - move to bottom of the file rather than top, since after talking to Chandler this is an edge case that only applies to control header cells + remove `data-euigrid-tab-managed` attr - tests should pass without it

- convert functions to arrow functions - use shorter headerNode var instead of ref

- it's had no effect since we switched header cells to use a popover for actions

kibanamachine · 2021-09-22T17:29:20Z

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

kertal

Approving that #4384 will be fixed with this PR, tested in Chrome, Firefox, Safari, works as expected 👍

cee-chen · 2021-09-27T16:09:06Z

@chandlerprall or @thompsongl - any chance one of you can review at this PR sometime this week? Or alternatively, is @kertal's approval with the fix sufficient for merge? Thanks as always for your time!

chandlerprall · 2021-09-27T19:14:41Z

src/components/datagrid/body/header/data_grid_header_cell_wrapper.test.tsx

    index: 0,
    headerIsInteractive: true,
-    children: <button data-euigrid-tab-managed="true" />,
+    children: <button />,


Ugh, good catch!!

chandlerprall

LGTM; good fixes & refactors! ship it 🦑

cee-chen · 2021-09-27T19:49:07Z

Thanks Chandler!!

kibanamachine · 2021-09-27T20:11:22Z

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

kibanamachine · 2021-09-27T20:48:36Z

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

cee-chen added 10 commits September 21, 2021 15:34

[Fix] Fix console error when dragging to reorder columns

6be8f53

[Refactor] Remove unnecessary useEffect dependencies

5cdd043

- now that we're not calling enableInteractives and focusInteractives directly but letting isCellEntered handle that for us, there's no need to include them + setIsCellEntered isn't necessary either/doesn't trigger the lint rule

[Refactor] Improve focus unit tests

ce98765

- Separate focus tests for isFocused logic vs focusin / focusout events

[Refactor] Combine enable and focus interactives action

2df8152

- they don't get called separately, so it makes sense to optimize the call and not make an extra tabbables call (+ reduces unnecessary branching)

[Refactor] Misc cleanup

a6f5c3b

- convert functions to arrow functions - use shorter headerNode var instead of ref

[Fix] Remove F2 key event

405b1c0

- it's had no effect since we switched header cells to use a popover for actions

Add changelog entry

cdb5a42

cee-chen marked this pull request as ready for review September 22, 2021 17:35

kertal approved these changes Sep 23, 2021

View reviewed changes

cee-chen requested a review from chandlerprall September 23, 2021 20:28

chandlerprall reviewed Sep 27, 2021

View reviewed changes

chandlerprall approved these changes Sep 27, 2021

View reviewed changes

cee-chen added 2 commits September 27, 2021 12:47

Merge branch 'master' into datagrid-header-cell-wrapper-refactors

b80f2ad

Update changelog

c042b1d

cee-chen enabled auto-merge (squash) September 27, 2021 19:49

cee-chen merged commit 6fe740c into elastic:master Sep 27, 2021

cee-chen deleted the datagrid-header-cell-wrapper-refactors branch September 27, 2021 20:23

cee-chen mentioned this pull request Sep 27, 2021

[EuiDataGrid] header cell throwing call stack errors #4484

Closed

breehall mentioned this pull request Oct 1, 2021

Bumping EUI to 39.0.0 elastic/kibana#113633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Fix sorting console errors + header cell focus cleanup/refactors#5209

[EuiDataGrid] Fix sorting console errors + header cell focus cleanup/refactors#5209
cee-chen merged 12 commits intoelastic:masterfrom
cee-chen:datagrid-header-cell-wrapper-refactors

cee-chen commented Sep 22, 2021 •

edited

Loading

Uh oh!

kibanamachine commented Sep 22, 2021

Uh oh!

kertal left a comment

Uh oh!

cee-chen commented Sep 27, 2021

Uh oh!

chandlerprall Sep 27, 2021

Uh oh!

chandlerprall left a comment

Uh oh!

cee-chen commented Sep 27, 2021

Uh oh!

kibanamachine commented Sep 27, 2021

Uh oh!

kibanamachine commented Sep 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cee-chen commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

Checklist

Uh oh!

kibanamachine commented Sep 22, 2021

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Sep 27, 2021

Uh oh!

chandlerprall Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Sep 27, 2021

Uh oh!

kibanamachine commented Sep 27, 2021

Uh oh!

kibanamachine commented Sep 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen commented Sep 22, 2021 •

edited

Loading