-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Modal] Improve the focus logic #14545
Conversation
a61eefa
to
3a69cb4
Compare
3a69cb4
to
307bbc4
Compare
307bbc4
to
63879f4
Compare
} | ||
}; | ||
|
||
const loopFocus = event => { |
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.
There is some option to avoid tabbing that sentinelEnd
and sentinelStart
elements ?
I noticed that if you have focus on the last element you need to press two times TAB
key to go to the first element on the dialog and vice versa. In this case, screen reader vocalized nothing and user can be feel lost.
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.
Btw, what do you think on option to disable looping.
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.
I have tried to reproduce Bootstrap Modal's behavior: https://getbootstrap.com/docs/4.3/components/modal/. In order to implement what you are suggesting, we need a way to list all the dom elements in the modal that can receive the focus. It's not obvious, you would have to rely on something like this: tabbable: +1kB gzipped.
(cc @ryancogswell, just for context)
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.
I've never tried, but it might work:
- on
sentinel
focus - list all nodes between
Start
andEnd
- try
.focus
each one - if
.activeElement
changed - then node is focusable, - break the loop.
Probably would not play well with radio buttons.
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.
Interesting :)
This pull request introduces a new
TrapFocus
component. There are different alternatives in the OSS word, this version optimizes the bundle size, it weighs less than 1 KB.We are using two sentinel DOM nodes instead of computing all the tabbable elements. Computing the tabbable elements is not straightforward. The drawback of using sentinels is that we have to create two extra DOM nodes and we can't implement the features suggested in #14545 (comment). The advantage is that it's more idiomatic to the web API, hence a smaller bundle size & CPU footprint.
We should be able to make the TrapFocus component public. Let's deploy this change in the wild and see how it performs before. We might be doing it all wrong, we need to battle test it.
A React RFC to follow: reactjs/rfcs#109.
Closes #13702
Closes #13693