tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
26.14k stars 1.08k forks source link

Dialog (Modal) headlessui.com docs are missing warning about how to support multiple-modals (sibling vs nested) #2689

Closed sgarcia-dev closed 1 year ago

sgarcia-dev commented 1 year ago

Describe your issue

It's been stated before Headless UI does NOT support multiple modals as siblings (requiring users to nest multiple modals instead). And has no plans to add support for this: https://github.com/tailwindlabs/headlessui/issues/825#issuecomment-1058981268

In response to this, the headlessui.com docs on Dialog (Modal) should be updated with a large disclaimer summarizing this large architectural limitation to users. Ideally, with its own dedicated "Multiple modal restrictions" section on the Sidebar to better communicate its importance.

This is especially important to add given the community's largely negative reception of the news. As well as the fact it's largely agreed upon the community that scalable global modal solutions often render them as siblings instead of nested children as Headless UI requires. Which can severely limit Headless UI's Dialog use-cases to smaller web applications where scalability and modal re-use isn't a strong requirement. https://github.com/tailwindlabs/headlessui/issues/825#issuecomment-1080718005

reinink commented 1 year ago

Hey! I think adding a section to the Headless UI dialogs documentation about how to render multiple dialogs using nested dialogs would be a good addition. I've made a note for this internally for when we have a moment to tackle this.

That said, I don't agree that the fact that you can't render sibling dialogs at the same time is a "large architectural limitation" of the project. I think you can do anything you need to do with nested dialogs, and I think nested dialogs are the right solution.

While we could provide an escape hatch for people to opt out of the dialog focus trapping, my concern is that this goes against the principles of this project. Our goal with Headless UI is to provide accessible-by-default components, so providing APIs to opt-out of our accessibility features isn't inline with our vision for the project.

Also, based on the discussion in #825, I wouldn't recommend creating a global dialog component instance that can be reused by other components. Rather, we recommend making a component for each specific dialog in your project, and then rendering them in each situation they're needed.

For example, if you have a list of users on a page and you can click a "Delete" button beside each one that opens a confirm delete dialog, don't create a single DeleteUser dialog instance that's shared by all of those buttons, but instead render a separate DeleteUser dialog instance for each delete button (user). This is also what the maintainers of Radix recommend: https://github.com/radix-ui/primitives/issues/1249#issuecomment-1072971306

So basically, instead of doing this:

export default function Users({ users }) {
  let [selectedUser, setSelectedUser] = useState(null)
  let [showDeleteDialog, setShowDeleteDialog] = useState(false)

  function deleteUser(user) {
    setSelectedUser(user)
    setShowDeleteDialog(true)
  }

  return (
    <main>
      <DeleteUser open={showDeleteDialog} user={selectedUser} />
      <ul>
        {users.map((user) => (
          <li key={user.id}>
            {user.name}
            <button onClick={() => deleteUser(user)}>Delete</button>
          </li>
        ))}
      </ul>
    </main>
  )
}

Do something like this instead, where the button/trigger is built directly into the dialog:

export default function Users() {
  return (
    <main>
      <ul>
        {users.map((user) => (
          <li key={user.id}>
            {user.name}
            <DeleteUser user={user}>Delete</DeleteUser>
          </li>
        ))}
      </ul>
    </main>
  )
}
sgarcia-dev commented 1 year ago

Thank you for the comments @reinink! I can totally respect Headless UI's commitment to nested modals for accessibility reasons, and I'm happy this is being documented on the docs.

However, I think we'll have to agree to disagree on this being a "large architectural limitation". As ultimately, I believe there's enough evidence online that global modal systems can sometimes promote code reuse, something many would agree is important for large scale web applications. Not to mention sibling modals help with keeping modals focused on the Single Responsibility Principle, deferring opening other modals to function props. Whereas declaring nested modals forces requires tighter coupling between modal components by handling their state and/or requiring importing them directly. Not saying I don't agree colocation is sometimes good. But there needs to be a balance. And some things are better left out of the component to avoid component complexity snowballing out of control and avoiding entropy over time.

I liked your final example's slight reduction of this overhead, though it is also less flexible. As it makes the assumption the element triggering a certain modal will always look the same. Something that is not always the case, and limits flexibility further.

Having said that, I agree not every modal needs to be global as that can lead to more problems. Still, it is nice to have that option. Either way, I'm just happy this is being communicated in the docs. As it helps set better set expectations from the start on what can/cannot be achieved with Headless UI.

clopezpro commented 1 year ago

I have a global modal in nuxt that I use to confirm an action image I have a modal that shows a table I have a global modal in nuxt that I use to confirm an action image when Confirm is clicked out all modals are closed image

clopezpro commented 1 year ago

I think I can get used to this, to invoke the modal only in the component that is going to use it and not globally since it works correctly there image If I hadn't found this issue #2689 I couldn't solve it

Emiliano-Bucci commented 9 months ago

Sorry guys, but what you're saying is just silly. If I have a global modal that can and must be opened from different parts of my application, how the hell should I do this? I mean, should I add that Dialog in ALL places? That's totally insane, and totally not reusable and extendable.

@sgarcia-dev Did you found any solution to this? I can't believe this can't be done...

sgarcia-dev commented 9 months ago

Sorry guys, but what you're saying is just silly. If I have a global modal that can and must be opened from different parts of my application, how the hell should I do this? I mean, should I add that Dialog in ALL places? That's totally insane, and totally not reusable and extendable.

@sgarcia-dev Did you found any solution to this? I can't believe this can't be done...

Yes, we sadly had to stop using Headless UI for our modals šŸ˜… it was an existing large project migration for a massive codebase, and refactoring our entire global modal system to accommodate this modal limitation from Headless UI was just not an option. However, I can respect their reasoning to maintain accessibility.

Emiliano-Bucci commented 9 months ago

@sgarcia-dev Thanks; I feel a bit frustrated thinking that I have to remove headless ui because of this; or I have to do some hack to temporary bypass this limitation. :/

sgarcia-dev commented 9 months ago

@reinink, are there still plans to add this important caveat to the docs? Or is there perhaps any way I could help contribute to our docs by adding a dedicated section on how to properly use nested modals? It seems that a lot of unnecessary frustration could have been saved by communicating something as important as this upfront before people integrated headless UI into their projects.

Emiliano-Bucci commented 9 months ago

All this can be avoided by giving developers the possiblity to disable or manage the focus trap manually. Something to opt-in if needed.

@reinink Also it's not totally correct to say that we shouldn't use the same instance for some dialogs, like in the example above; what if I have a list of 1000 users? Should I render 1000 instances of the same components? What are your concerns about performances by having 1000 components instances rendered in the virtual DOM even if not used? Maybe if you're using the latest mac book you wouldn't notice any issue, but what if users use a mobile device?

sgarcia-dev commented 9 months ago

@Emiliano-Bucci the library authors already explained they don't feel right making this opt-in as it essentially breaks major accessibility rules. And the entire mantra/goal behind Headless UI is providing accessible headless components. Truth be told, I respect and understand that. And if you don't care about accessibility enough to be willing to remove the focus trap aspect, Headless UI was not the right library for you anyways.

Realistically, I don't think it's ok to ask the maintainers to add for global modal support just because the community doesn't respect accessibility principles.

But yeah, I agree this restriction alongside a candid explanation behind the accessibility reasoning should have been explained early in the modal docs.

Emiliano-Bucci commented 9 months ago

@sgarcia-dev But I do concern about accessibility, what I don't understand is why nested modals are ok but sibilings don't. From a dom tree perspective they're the same thing, is just the internals that change. It would be enough to handle dialogs accessibility by looking the dom tree. So, to me, it seems more a whim from developers.

And yes, also the lack of explanation and transparency, because now I probably would have to waste days of refactoring (in the best case scenarios) jut because they omit to inform everyone about the lack of a crucial functionality.

sgarcia-dev commented 9 months ago

why nested modals are ok but sibilings don't.

I'm pretty sure it's because focus trapping is essential for accessibility, and having sibling modals makes the focus trap of each one fight each other. Whereas nesting them allows focus traps to be nested seamlessly the same way native DOM event bubbling works (element boundaries can probably call event.stopPropagation where applicable, and so on) .

It would be enough to handle dialogs accessibility by looking the dom tree.

If you're certain of this, you're welcome to browse the source code and come up with a PR of your own on how to fix this yourself while maintaining the focus trap aspect that's essential for accessibility šŸ˜‰ Let's remember you and I aren't maintaining this and we have no right to demand anything. Specially when we aren't even aware of how difficult it is to fix focus trap issues in sibling modals.

Anyways, let's try to keep this civil as Headless UI itself is amazing and being maintained + actively developed for free.

dmarvasti commented 5 months ago

@sgarcia-dev @Emiliano-Bucci - This has been fixed, @RobinMalfait has addressed https://github.com/tailwindlabs/headlessui/pull/3242 https://github.com/tailwindlabs/headlessui/issues/2876#issuecomment-2134069416