-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve FocusTrap
behaviour
#1432
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
e240204
to
0516e4c
Compare
0516e4c
to
b8b1b2d
Compare
This new component will also make sure that it is visually hidden to sighted users. However, it contains a few more features that are going to be useful in other places as well. These features include: 1. Make visually hidden to sighted users (default) 2. Hide from assistive technology via `features={Features.Hidden}` (will add `display: none;`) 3. Hide from assistive technology but make the element focusable via `features={Features.Focusable}` (will add `aria-hidden="true"`)
This will behave the same (roughly) as the new to be released `useEvent` hook in React 18.X This hook allows you to have a stable function that can "see" the latest data it is using. We already had this concept using: ```js let handleX = useLatestValue(() => { // ... }) ``` But this returned a stable ref so you had to call `handleX.current()`. This new hook is a bit nicer to work with but doesn't change much in the end.
This keeps track of the direction people are tabbing in. This returns a ref so no re-renders happen because of this hook.
This is similar to the `useEffect` hook, but only executes if values are _actually_ changing... 😒
Using a component directly allows us to simplify the focus trap logic itself. Instead of intercepting the <kbd>Tab</kbd> keydown event and figuring out the correct element to focus, we will now add 2 "guard" buttons (hence why we require a component now). These buttons will receive focus and if they do, redirect the focus to the first/last element inside the focus trap. The sweet part is that all the tabs in between those buttons will now be handled natively by the browser. No need to find the first non disabled, non hidden with correct tabIndex element!
Also added a hidden button so that we know the correct "main" tree of the application. Before this we were assuming the previous active element which will still be correct in most cases but we don't have access to that anymore since the logic is encapsulated inside the FocusTrap component.
We make sure that the Portal is cleaning up its `element` properly. We also make sure to call the `target.appendChild(element)` conditionally because I ran into a super annoying bug where a focused element got blurred because I believe that this re-mounts the element instead of 'moving' it or just ignoring it, if it already is in the correct spot.
Not really necessary, just cleaner.
b8b1b2d
to
33583ff
Compare
@RobinMalfait I think this may introduce an issue when the It seems the hidden buttons get rendered near the bottom of the page, and are focusable, so it can end up scrolling the page down to put them in view. If you try this sandbox, click open button, hit tab, there's no bug. If you bump the version to Potential fix, requires a bit of global CSS: #headlessui-portal-root {
position: fixed;
height: 100%;
width: 100%;
} |
This PR provides a bunch of improvements to the
FocusTrap
instead of trying to be "smart" about things.Instead of intercepting the
Tab
keydown event and moving the focus to the correct next/previous element, we will now render 2 hidden buttons at the beginning and the end of the Dialog. This means that all the Tab logic is handled by the browser and we don't do anything special in this case.The only case we have to handle is to make sure that when one of those hidden buttons receive the focus, that we get redirected to the correct element in the FocusTrap.
This also includes some cleanup!
Fixes: #1406
Fixes: #1391
Fixes: #1291