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

[Bug]: Multiple/nested Dialog instances bugs #426

Closed
Andrey-Bazhanov opened this issue Apr 22, 2021 · 14 comments
Closed

[Bug]: Multiple/nested Dialog instances bugs #426

Andrey-Bazhanov opened this issue Apr 22, 2021 · 14 comments
Assignees

Comments

@Andrey-Bazhanov
Copy link

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

1.0.0, 1.0.0-ef31bbe

What browser are you using?

Chrome MacOs, Chrome Android, Safari iOS

Reproduction repository

https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-sq7sn?file=/pages/index.js

Describe your issue

First of all, thanks for great library!

In complex app sometimes content of dialog can open another one... I played around dialog component and find some issues:

  • scroll jumping, described here: [Bug]: Modal jumps horizontally when closing (on Windows) #421 (and I found fix... maybe we should describe this behaviour in documentation)
  • if dialog open another dialog: FocusTrap - can handle focus only if autoFocusRef provided (compare pressing "Open 1" -> "Open 2" and "Open 1" -> "Open 3")
  • when 2 or more dialogs is open - the are no way to close only one of them
@RobinMalfait
Copy link
Member

RobinMalfait commented Apr 22, 2021

Hey! Thank you for your bug report!
Much appreciated! 🙏

When you want nested Modals, then you should also nest it in the component tree. This allows us to create a stacking context so that when you press esc that only the top most Dialog gets closed.

Dialogs have this feature where all other content is inert, meaning 2 open Dialogs at the same time is not possible when they are rendered as children. But the nesting should fix this.

Here is an example: https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-o2ykb?file=/pages/index.js

@RobinMalfait RobinMalfait self-assigned this Apr 22, 2021
@sveisvei
Copy link

@RobinMalfait Interesting enough, your sandbox demo has a bug - where the first escape removes 2 dialogs.

@RobinMalfait
Copy link
Member

RobinMalfait commented Apr 22, 2021

Good catch @sveisvei!

I saw it too when I made that example. Have a fix already: #430

Edit: Updated the codesandbox with the dev build of Headless UI.
You can already try it using npm install @headlessui/react@dev or yarn add @headlessui/react@dev.

@DoctorDerek
Copy link

@RobinMalfait Is it the same for nested Popovers? I seem to be seeing a tabIndex bug where nested popovers tab 2 items for focus, including with the new dev build. But maybe I'm missing something?

https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-534xd?file=/pages/index.js:171-187

Tab into the menu works:
image

But Tab skips the in-between item:
image

And Esc closes all the Popovers. Is that the intended effect or a similar bug to the nested dialogs bug?

@IrvingArmenta
Copy link

How about Vue 3 nested Dialogs?

@lorlab
Copy link

lorlab commented Dec 30, 2021

Hey! Thank you for your bug report! Much appreciated! 🙏

When you want nested Modals, then you should also nest it in the component tree. This allows us to create a stacking context so that when you press esc that only the top most Dialog gets closed.

Dialogs have this feature where all other content is inert, meaning 2 open Dialogs at the same time is not possible when they are rendered as children. But the nesting should fix this.

Here is an example: https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-o2ykb?file=/pages/index.js

@RobinMalfait Hi, I noticed that when the child modals should be hidden, you are simply not rendering them instead of passing the open prop. This however breaks transitions. Is there a way to achieve both? Thanks!

@YeungKC
Copy link

YeungKC commented Jan 14, 2022

Hey! Thank you for your bug report! Much appreciated! 🙏

When you want nested Modals, then you should also nest it in the component tree. This allows us to create a stacking context so that when you press esc that only the top most Dialog gets closed.

Dialogs have this feature where all other content is inert, meaning 2 open Dialogs at the same time is not possible when they are rendered as children. But the nesting should fix this.

Here is an example: https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-o2ykb?file=/pages/index.js

I have a global fullscreen loading dialog, there are multi dialogs, but they are not nested.
Wouldn't it be nice if we can close dialog in reverse order of appearance?

@RobinMalfait
Copy link
Member

Hey all!

Revisited this issue, and the main issue is that multiple open sibling Dialogs is not supported. You have 2 options:

  1. Only have 1 open Dialog at the same time: https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-72hgp2?file=/pages/index.js
  2. Nest your Dialogs instead of using siblings: https://codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-o2ykb?file=/pages/index.js

  • @DoctorDerek This is a different issue, and I noticed that you already created an issue for this, thanks!
  • @IrvingArmenta This is now supported using the latest insiders built: npm install @headlessui/vue@insiders
  • @lorlab not entirely sure what you mean, if this is still an issue for you please open a new issue with a minimal reproduction repo.

For anyone running into issues, please open a new issue (if it doesn't already exist) and attach a minimal reproduction repo or codesandbox.

@pix2D
Copy link

pix2D commented Nov 11, 2022

@RobinMalfait Your example 1 from above breaks scrolling if you open modal 1, then open 2 from within 1, then close. The page is no longer scrollable.

@jeanlambert17
Copy link

jeanlambert17 commented Jul 10, 2023

This is still and issue, I'm aware the library doesn't support this behavior, which I don't really see a reason why, but for the moment I come up with this solution:

import { useEffect, useMemo } from 'react'
import { isServer } from '../utils/ssr'
import { useLatestValue } from './use-latest-value'

// Since headlessui/react Dialog does not support having nested opened dialogs
// Need to come up with a logic to close the dialogs in the order they are opened
let idCounter = 0
let listeners: ({
  id: string
  cb: (evt: KeyboardEvent) => void
})[] = []

export const useEscapeOverride = (
  // Open/closed state
  open: boolean | undefined,
  // onClose function basically
  listener: (evt: KeyboardEvent) => void,
  _id?: string
) => {
  // Get id for the listener
  const id = useMemo(() => _id ?? `escape-override-${idCounter++}`, [_id])
  // We can replace this for useMemo(() => listener, [listener])
  const { current } = useLatestValue(listener)

  useEffect(() => {
    if (!open) return

    const handleKeyDown = (evt: KeyboardEvent) => {
      if (evt.code !== 'Escape') return
      evt.preventDefault()
      // Close the last opened modal
      listeners[listeners.length - 1].cb(evt)
      listeners.pop()
    }

    if (!isServer) {
      // Add current event handler
      if (!listeners.some(listener => listener.id === id)) {
        listeners.push({ id, cb: current })
      }
      document.addEventListener('keydown', handleKeyDown)
    }

    return () => {
      if (!isServer) {
        document.removeEventListener('keydown', handleKeyDown)
        // Remove current event handler if the component unmounts or "open" changes
        listeners = listeners.filter((listener) => listener.id !== id)
      }
    }
  }, [open])
}

Usage:

export const CustomModal = ({
  open,
  onClose,
  children
}: ModalProps) => {
  useEscapeOverride(open, onClose)

  return (
    // Pass an empty function to onClose, we don't want to trigger the onClose done by the Dialog automatically
    <Dialog open={open} onClose={() => {}}>
      {children}
    </Dialog>
  )
}

@vegawong
Copy link

I have such a scenario where there is a global dialog, such as a login or registration form, which can be triggered at any time, on the page layer, in the first-level dialog, or even in a second-level dialog. If I have to use component hierarchy to implement nesting, this global dialog component will be ubiquitous, leading to a lot of redundant code. Is there any solution for this situation?

@calvo-jp
Copy link

@RobinMalfait Your example 1 from above breaks scrolling if you open modal 1, then open 2 from within 1, then close. The page is no longer scrollable.

We also faced this issue recently due to modal being opened from a drawer which is also a modal. So we forced the body to be scrollable. But now the problem is the layout shift due to the right padding the lib adds whenever a modal is opened. Really hope they support nested modals. We end up using floating-ui

@Aravinda93
Copy link

Aravinda93 commented May 13, 2024

@RobinMalfait Just have a query why would opening the opening a DialogPanel2.vue on top of the DialogPanel1.vue would close the DialogPanel1.vue when interacting with DialogPanel2.vue.

I have posted the question with versions and code snippet here: IssueLink

CodeSandbox: CodeSandbox

@Archetipo95
Copy link

Hey! Thank you for your bug report! Much appreciated! 🙏

When you want nested Modals, then you should also nest it in the component tree. This allows us to create a stacking context so that when you press esc that only the top most Dialog gets closed.

Dialogs have this feature where all other content is inert, meaning 2 open Dialogs at the same time is not possible when they are rendered as children. But the nesting should fix this.

Here is an example: codesandbox.io/s/headlessui-dialog-multiple-modals-bugs-forked-o2ykb?file=/pages/index.js

Can we have this in the documentation? I was trying to debug it for quite some time and just found out this solution. Maybe others can benefit from it.

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

No branches or pull requests