-
Notifications
You must be signed in to change notification settings - Fork 834
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] Fix focus on pagination #5578
Conversation
- Currently the grid is sort of brokenly doing this due to `aria-controls` - we should just fix the buggy behavior and make this call manually
- as of recent scrolling/focus fixes, setFocusedCell will automatically scroll to the targeted cell
@1Copenut would love your eyes/testing on this and confirmation that this is indeed best keyboard/screen reader practice. I would have thought taking focus away from the pagination buttons would not be great, but it looks like we baked that logic into our pagination FWIW, the screen reader experience from this is now also the same as when the user uses the page up/page down keys on the grid |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5578/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 LGTM! I tested the branch locally with Safari and it worked really well. The focus was captured, and it read back the current cell information and new page N of NN. This will be a big boost for assistive technology and keyboard navigation. Thank you @constancecchen
Awesome, thanks a million Trevor!! |
* Manually focus into the first data cell on pagination change - Currently the grid is sort of brokenly doing this due to `aria-controls` - we should just fix the buggy behavior and make this call manually * Remove now-unnecessary gridRef/scrollToItem call - as of recent scrolling/focus fixes, setFocusedCell will automatically scroll to the targeted cell * Add pagination unit tests * [misc] clean up unused pagination prop type * Changelog
Summary
closes #5577 - please see the linked issue for a description of why the bug is happening.
Before
Notice the black outline (in Safari) that indicates the grid wrapper is getting focused instead of into a grid cell.
After
Notice that the first grid data cell is now focused on every new page.
Checklist
- [ ] Check against all themes for compatibility 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