Skip to content
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] When a cell expansion popover is closed, ensure focus is always returned to the originating cell for keyboard users #5761

Merged
merged 4 commits into from
Apr 5, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Apr 5, 2022

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

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
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

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

Copy link
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!

cy.realMount(<EuiDataGrid {...baseProps} />);
cy.get(
'[data-gridcell-row-index="0"][data-gridcell-column-index="0"]'
).click();
Copy link
Contributor

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
Copy link
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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants