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

feat(input-date-picker): add focus trap support #6816

Merged

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Apr 20, 2023

Related Issue: #6668

Summary

Adds focus-trap to date-picker dialog to improve a11y.

Note: This also adds a util to help retrieve props from the currently focused element.

Notable focusTrapComponent changes

  • connectFocusTrap now supports passing a different focus-trap target element (for setting the focus trap on a component's subtree) and overrides to certain options from focus-trap's focusTrap().
  • activateFocusTrap/deactivateFocusTrap now allow passing options to focus-trap's activate/deactivate methods.
  • updateFocusTrapElements is now optional.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Apr 21, 2023
@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Apr 21, 2023
@jcfranco
Copy link
Member Author

FYI, this is based on #6805. These are the relevant changes for review.

@jcfranco jcfranco marked this pull request as ready for review April 21, 2023 18:45
@jcfranco jcfranco requested a review from a team as a code owner April 21, 2023 18:45
@jcfranco
Copy link
Member Author

+@geospatialem for a11y testing.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

FocusTrap related changes look good!

@geospatialem geospatialem self-requested a review April 21, 2023 19:17
Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

Likely out of scope for the focus trap support, and also nitpick:

Would we want to support keyboard focus where the "previous month" button is focused on first?

Could see this being confusing for mouse users, too, but it seems like an odd behavior for AT to have to tab in after the dialog is displayed. Also makes it harder for AT users to close the dialog, as they would need to tab in first, then press esc.

@jcfranco
Copy link
Member Author

@geospatialem I updated it so that when the date-picker is opened via the ⬇ key, it gets focused. Clicking won't move focus to allow users to type the value if desired.

@jcfranco jcfranco merged commit 0d9ddc9 into master Apr 22, 2023
@jcfranco jcfranco deleted the jcfranco/6668-add-focus-trap-to-input-date-picker-popover branch April 22, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants