Skip to content

[EuiDataGrid] When a cell expansion popover is closed, ensure focus is always returned to the originating cell for keyboard users#5761

Merged
cee-chen merged 4 commits intoelastic:mainfrom
cee-chen:5646/cell-popover-refocus
Apr 5, 2022
Merged

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Apr 5, 2022 •
edited
Loading

Summary

This PR closes #5646 by manually returning .focus() to the originating cell when a popover is closed by keyboard users (the escape key or the F2 key). This applies to two separate buggy use cases:

  1. When VoiceOver is open (see detailed convo in [EuiDataGrid][a11y] When cell popover contains interactive children, opening and closing the cell popover strands focus  #5646, this appears to be a very strange Mac/VO quirk)
  2. When the user uses a mouse button on the expand icon to open the popover, but uses the Escape key to close the popover

Before

before

After

after

Checklist

- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Updated the Figma library counterpart

  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress tests
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen force-pushed the 5646/cell-popover-refocus branch from 24cc559 to efaf0be Compare April 5, 2022 19:45
@cee-chen cee-chen requested a review from 1Copenut April 5, 2022 19:45
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 5, 2022

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

@cee-chen cee-chen force-pushed the 5646/cell-popover-refocus branch from 2836072 to 9b007ae Compare April 5, 2022 20:20
@cee-chen cee-chen force-pushed the 5646/cell-popover-refocus branch from 9b007ae to 44d3016 Compare April 5, 2022 20:21
@cee-chen
Copy link
Copy Markdown
Contributor Author

cee-chen commented Apr 5, 2022

Tested and confirmed that the last 2 added Cypress tests fail on main but pass on this branch.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 5, 2022

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

1Copenut
1Copenut approved these changes Apr 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I had one suggestion in the Cypress spec about using a Real Events method. If it makes sense to switch, let's do that, or if it's better as is, that's great too!

src/components/datagrid/body/data_grid_cell_popover.spec.tsx Outdated
cy.realMount(<EuiDataGrid {...baseProps} />);
cy.get(
'[data-gridcell-row-index="0"][data-gridcell-column-index="0"]'
).click();
Copy link
Copy Markdown
Contributor

@1Copenut 1Copenut Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about swapping these .click() events for .realClick() ones? https://github.com/dmtrKovalenko/cypress-real-events#cyrealclick

If that causes issues for clicks that need a { force: true } object, I'm okay sticking with the current implementation.

@cee-chen cee-chen mentioned this pull request Apr 5, 2022
Merged
4 tasks
@cee-chen
Copy link
Copy Markdown
Contributor Author

cee-chen commented Apr 5, 2022

Ooo, it looks like .realClick() actually lets me remove the force: true flag. Thanks Trevor!!

@cee-chen cee-chen enabled auto-merge (squash) April 5, 2022 22:31
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 5, 2022

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

@cee-chen cee-chen merged commit f39cfb6 into elastic:main Apr 5, 2022
@cee-chen cee-chen deleted the 5646/cell-popover-refocus branch April 5, 2022 23:30
@cee-chen cee-chen mentioned this pull request Apr 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@1Copenut 1Copenut 1Copenut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiDataGrid][a11y] When cell popover contains interactive children, opening and closing the cell popover strands focus

3 participants

@cee-chen @kibanamachine @1Copenut