Ensure moving focus within a Portal component, does not close the Popover component
#2492
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Comment on lines
+9
to
17
| 'Portal', | ||
| 'Combobox', | ||
| 'Dialog', | ||
| 'Disclosure', | ||
| 'FocusTrap', | ||
| 'Listbox', | ||
| 'Menu', | ||
| 'Popover', | ||
| 'Portal', | ||
| 'RadioGroup', |
There was a problem hiding this comment.
Should be sorted alphabetically? :)
There was a problem hiding this comment.
They are, but the exports are sorted alphabetically and the export for the portal itself changed which is now hoisted to the top in the test.
This way we can reuse it in other components when needed.
RobinMalfait
force-pushed
the
fix/issue-2481
branch
from
8d1fda3
to
488a206
Compare
RobinMalfait
force-pushed
the
fix/issue-2481
branch
from
488a206
to
00ce35e
Compare
This allows us to find all the `Portal` components that are nested in a given component without manually adding refs to every `Portal` component itself. This will come in handy in the `Popover` component where we will allow focus in the child `Portal` components otherwise a focus outside of the `Popover` will close the it. In other components we often crawl the DOM directly using `[data-headlessui-portal]` data attributes, however this will fetch _all_ the `Portal` components, not the ones that started in the current component.
The `Popover` component will close by default if focus is moved outside of it. However, if you use a `Portal` comopnent inside the `Popover.Panel` then from a DOM perspective you are moving the focus outside of the `Popover.Panel`. This prevents the closing, and allows the focus into the `Portal`. It currently only allows for `Portal` components that originated from the `Popover` component. This means that if you open a `Dialog` component from within the `Popover` component, the `Dialog` already renders a `Portal` but since this is part of the `Dialog` and not the `Popover` it will close the `Popover` when focus is moved to the `Dialog` component.
This ensures that if you have a structure like this:
```jsx
<Dialog> {/* Renders a portal internally */}
<Popover>
<Portal> {/* First level */}
<Popover.Panel>
<Menu>
<Portal> {/* Second level */}
<Menu.Items>
{/* ... */}
</Menu.Items>
</Portal>
</Menu>
</Popover.Panel>
</Portal>
</Popover>
</Dialog>
```
That working with the `Menu` doesn't close the `Popover` or the `Dialog`.
This will allow you to pass in portal elements as well. + cleanup of the resolving of all DOM nodes.
Shorthand to check if any of the root containers contains the given element.
RobinMalfait
force-pushed
the
fix/issue-2481
branch
from
00ce35e
to
5ceed37
Compare
To make it similar between React & Vue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Popovercomponent closes automatically if the focus is moved outside of thePopover.Panel. However, this also happens if you render aPortalcomponent inside thePopover.Paneland move focus within thatPortalcomponent.From a DOM perspective, the focus is not in the
Popover.Panelanymore, so therefore it will close. This PR fixes that issue and will allow focus inPortalcomponents that originate from within thePopover.Panelcomponent. We can use the React tree (using the Context API) to see if a certain DOM element is still nested from a React's perspective.This PR also makes sure that only
Portalcomponents that originate from thePopovercomponent will be marked as a validPortallocation. If you trigger the opening of a siblingDialogcomponent from within thePopover.Panelcomponent, then focus moving into theDialogwill close thePopovercomponent.Fixes: #2481