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

Add portal prop to Combobox, Listbox, Menu and Popover components #3124

Merged
merged 22 commits into from
Apr 24, 2024

Conversation

RobinMalfait
Copy link
Collaborator

This PR introduces a new portal prop, and fine-tunes the meaning of the modal prop on the following components:

Combobox component

The ComboboxOptions component now accepts two new props:

  • portal (boolean, defaults to false): Whether to render the options in a portal. This will always be set to true if an anchor prop is passed.
  • modal (boolean, defaults to true):
    • Enables scroll locking, which means you can't scroll the page
    • Enables inert on other elements, which means you can't interact with other elements on the page while the combobox is open. An outside click typically requires two clicks. The first click will close the combobox, and the second click will interact with the element.

Listbox component

The ListboxOptions component now accepts two new props:

  • portal (boolean, defaults to false): Whether to render the options in a portal. This will always be set to true if an anchor prop is passed.
  • modal (boolean, defaults to true):
    • Enables scroll locking, which means you can't scroll the page
    • Enables inert on other elements, which means you can't interact with other elements on the page while the listbox is open. An outside click typically requires two clicks. The first click will close the listbox, and the second click will interact with the element.

Menu component

The MenuItems component now accepts two new props:

  • portal (boolean, defaults to false): Whether to render the options in a portal. This will always be set to true if an anchor prop is passed.
  • modal (boolean, defaults to true):
    • Enables scroll locking, which means you can't scroll the page
    • Does not enable inert on other elements, which means you can interact with other elements on the page while the menu is open. This also allows you to tab to the next focusable element.

Popover component

The PopoverPanel component now accepts two new props:

  • portal (boolean, defaults to false): Whether to render the options in a portal. This will always be set to true if an anchor prop is passed.
  • modal (boolean, defaults to false):
    • Enables scroll locking, which means you can't scroll the page
    • Does not enable inert on other elements, which means you can interact with other elements on the page while the menu is open. This also allows you to tab to the next focusable element.

This way we can always use `<Portal>`, but enable / disable it
conditionally.
This allows us to _not_ provide the ref is no ref was passed in.
moved logic from the `useEffect`, to module scope. We will re-use this
logic in a future commit.
Mark all elements on the page as inert, except for the ones that are allowed.

We move up the tree from the allowed elements, and mark all their
siblings as `inert`. If any of the children happens to be a parent of
one of the elements, then that child will not be marked as `inert`.

```
<body>                                    <!-- Stop at body -->
  <header></header>                       <!-- Inert, sibling of parent of allowed element -->
  <main>                                  <!-- Not inert, parent of allowed element -->
    <div>Sidebar</div>                    <!-- Inert, sibling of parent of allowed element -->
    <div>                                 <!-- Not inert, parent of allowed element -->
      <Listbox>                           <!-- Not inert, parent of allowed element -->
        <ListboxButton></ListboxButton>   <!-- Not inert, allowed element -->
        <ListboxOptions></ListboxOptions> <!-- Not inert, allowed element -->
      </Listbox>
    </div>
  </main>
  <footer></footer>                       <!-- Inert, sibling of parent of allowed element -->
</body>
```
- This adds a `portal` prop that renders the `MenuItems` in a portal.
  Defaults to `false`.
  - If you pass an `anchor` prop, the `portal` prop will always be set
    to `true`.
- The `modal` prop enables the following behavior:
  - Scroll locking is enabled when the `modal` prop is passed and the
    `Menu` is open.
  - Other elements but the `Menu` are marked as `inert`.
…ons`

- This adds a `portal` prop that renders the `ListboxOptions` in a
  portal. Defaults to `false`.
  - If you pass an `anchor` prop, the `portal` prop will always be set
    to `true`.
- The `modal` prop enables the following behavior:
  - Scroll locking is enabled when the `modal` prop is passed and the
    `Listbox` is open.
  - Other elements but the `Listbox` are marked as `inert`.
- This adds a `portal` prop that renders the `ComboboxOptions` in a
  portal. Defaults to `false`.
  - If you pass an `anchor` prop, the `portal` prop will always be set
    to `true`.
- The `modal` prop enables the following behavior:
  - Scroll locking is enabled when the `modal` prop is passed and the
    `Combobox` is open.
  - Other elements but the `Combobox` are marked as `inert`.
- This adds a `portal` prop that renders the `PopoverPanel` in a portal.
  Defaults to `false`.
  - If you pass an `anchor` prop, the `portal` prop will always be set
    to `true`.
- The `modal` prop enables the following behavior:
  - Scroll locking is enabled when the `modal` prop is passed and the
    `Panel` is open.
This is now implemented on a per component basis with some hooks.
The `Modal` component is removed, so there is no need to handle this in
the `Dialog`. It's also safe to remove because the components with
"portals" that are rendered inside the `Dialog` are portalled into the
`Dialog` and not as a sibling of the `Dialog`.
Before this, we were waiting for a "next render" to mount the portal if
it was used inside a specific group. This happens when using `<Portal/>`
inside of a `<Dialog/>`.
Copy link

vercel bot commented Apr 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 24, 2024 2:39pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 24, 2024 2:39pm

@RobinMalfait RobinMalfait changed the title Add portal prop to Menu, Listbox, Combobox and Popover components Add portal prop to Combobox, Listbox, Menu and Popover components Apr 23, 2024
Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

Everything looks good from what I can see. Does the new useInertOthers hook allow simplifying of the inert others code in Dialog?

Aside: I swear we had this hook already?! Maybe I'm remembering something else from a while back?

We used to have a `useInertOthers` hook, but it also made everything
inside `document.body` inert. This means that third party packages or
browser extensions that inject something in the `document.body` were
also marked as `inert`. This is something we don't want.

We fixed that previously by introducing a simpler `useInert` where we
explicitly marked certain elements as inert: #2290

But I believe this new implementation is better, especially with this
commit where we stop once we hit `document.body`. This means that we
will never mark `body > *` elements as `inert`.
@RobinMalfait
Copy link
Collaborator Author

@thecrypticace hah we indeed used to have it! But we removed it here: #2290

However, with the last commit (4fdef30), this should not be a problem anymore, and I think I like useInertOthers better because it's simpler to use.

I will update the places where we use useInert to account for that and further clean things up.

if (!ownerDocument) continue
d.add(markInert(element))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

for (let element of disallowed?.() ?? []) ?

// Mark the node as inert
d.add(markInert(node as HTMLElement))
}
for (let element of allowedElements) {
Copy link
Contributor

@thecrypticace thecrypticace Apr 24, 2024

Choose a reason for hiding this comment

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

ditto here: let allowedElements = allowed?.() ?? [] ?

Will let you remove the extra nesting in both spots.

This way we have a list of allowed and disallowed containers. The
`disallowed` elements will be marked as inert as-is.

The allowed elements will not be marked as `inert`, but it will mark its
children as inert. Then goes op the parent tree and repeats the process.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants