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

[core] fix(Overlay): improve focus management #4963

Merged
merged 13 commits into from
Oct 15, 2021

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Oct 14, 2021

Fixes #4956, fixes #4920, and related issues

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Adjust Overlay's focus management behavior to avoid programmatically focussing elements inside the overlay contents in most cases, opting for keeping focus on the start/end focus trap elements instead

Reviewers should focus on:

We should still get the accessibility enhancements added by #4894, but without the overly invasive focus management which caused issues like #4956.

Screenshot

2021-10-14 03 26 21

@blueprint-bot
Copy link

[core] fix(Overlay): improve focus management

Previews: documentation | landing | table | modern colors demo

@blueprint-bot
Copy link

remove reference to bug repro code

Previews: documentation | landing | table | modern colors demo

@adidahiya
Copy link
Contributor Author

There are issues with reverse TAB key navigation with this change. Here's an example where I press TAB x4 times and then SHIFT+TAB x4 times after clicking the button to open this dialog:

2021-10-14 03 52 27

When pressing SHIFT+TAB, we expect the focus to cycle back from the end of the dialog and up through the focusable elements. Instead, it appears that nothing happens and the focus stays on the end focus trap element.

@adidahiya adidahiya requested a review from styu October 14, 2021 08:15
@blueprint-bot
Copy link

fix trap behavior, use regular react event handlers

Previews: documentation | landing | table | modern colors demo

@adidahiya
Copy link
Contributor Author

Things are better now, but there's still a minor issue where the second to last focusable element in the dialog seems to get focussed after pressing Shift+Tab at the end of the dialog. It's turning out to be a tricky one to fix... will probably just punt it for later, since this change seems like a strict improvement over the current state of things.

@blueprint-bot
Copy link

tweak variable name

Previews: documentation | landing | table | modern colors demo

@adidahiya adidahiya mentioned this pull request Oct 15, 2021
2 tasks
@adidahiya
Copy link
Contributor Author

Ok, I've now fixed the issue I mentioned in the previous comment. This should be good to go :shipit:

@blueprint-bot
Copy link

don't get trapped on start focus element

Previews: documentation | landing | table | modern colors demo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants