tailwindlabs / headlessui

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

[Bug]: Uncaught RangeError: Maximum call stack size exceeded. focusElement of Modal #825

Closed tianyingchun closed 2 years ago

tianyingchun commented 3 years ago

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

1.4.1

What browser are you using?

chrome

Reproduction repository

Not now.

Describe your issue

while we create Modal multiple (2+) it will throw me

focus-management.esm.js?d1e3:99 Uncaught RangeError: Maximum call stack size exceeded.
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)
    at handler (use-window-event.esm.js?b5e3:8)
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)
    at handler (use-window-event.esm.js?b5e3:8)
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)
    at handler (use-window-event.esm.js?b5e3:8)
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)
    at handler (use-window-event.esm.js?b5e3:8)
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)
    at handler (use-window-event.esm.js?b5e3:8)
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)
    at handler (use-window-event.esm.js?b5e3:8)
    at focusElement (focus-management.esm.js?d1e3:99)
    at eval (use-focus-trap.esm.js?25af:110)

Screenshot here

BTW, can Modal need to provider flag props to disabled focusElement feature, in most case we don't need this, i think.

kpratik2015 commented 2 years ago

Faced this too. +1

RobinMalfait commented 2 years ago

Hey! Thank you for your bug report! Much appreciated! πŸ™

This is definitely an error that should be fixed (by improving the error message), however the root cause is that you are rendering 2 dialogs as siblings, if you want nested dialogs, then you have to nest them inside one another, and not render them as a sibling.

The reason that this error is happening is because the dialog is implemented in a way so that if an element is being focused outside of a dialog, that the focus is restored into the dialog itself. However, with 2 sibling dialogs this happens:

Dialog A has focus
Dialog B gets rendered, steals focus
Dialog A loses focus, thus tries to restore focus into A
Dialog B loses focus, thus tries to restore focus into B
Dialog A loses focus, thus tries to restore focus into A
Dialog B loses focus, thus tries to restore focus into B
Dialog A loses focus, thus tries to restore focus into A
Dialog B loses focus, thus tries to restore focus into B
...
iandoe commented 2 years ago

We're having this issue too, it's a pretty common use case to have sibling modals, other libs like Baseweb handle this fine or at least provide a way to turn off autofocus

Upon opening, focus will be transferred to the first interactive element (unless autofocus is set to false) https://baseweb.design/components/modal/#accessibility

is there a way to do that with HeadlessUI, and if not would you be open to a PR allowing for autofocus to be turned off ?

tianyingchun commented 2 years ago

what's progress of this issue

gsidsid commented 2 years ago

Add an initialFocus prop to the dialog and set it to document.activeElement. For folks using vue:

<template>
    <Dialog as="div" :initialFocus="getActiveElement()">
   ...
    </Dialog>
</template>
<script>
...
   methods: {
       getActiveElement() {
           return document.activeElement;
       }
   }
...
</script>
Mangor1no commented 2 years ago

what's progress of this issue

@RobinMalfait replied the solution. Basically what you need to do is move one modal inside other.

`

`

kayvaninvemo commented 2 years ago

In our design we have stripe elements in a dialog, and stripe elements are rendered in iframe, the focus trap does not work well with iframe, i guess. That would be nice to have a "opt out of focus trap" option in dialogs.

tianyingchun commented 2 years ago

provider option to disable focus trap is make sense, in some edge case, the focus element won't work i think.

mockingjet commented 2 years ago

Same problem here. To be honest, I would rather abandon focus trap to prevent this error

tianyingchun commented 2 years ago

Oh extract new Component named FocusStrap for Modal, if we need, we can place FocusStrap into Modal children props by ourself.

tejasupmanyu commented 2 years ago

Is there a fix to this yet? any workaround for react?

obianachebe commented 2 years ago

I'm also experiencing issues with focus trap and Stripe elements in a dialog. When a Stripe element input component is currently focused and you then attempt to focus on another element outside of the Stripe iframe, the Stripe element will demand back focus making it so you can’t move on to other components in a form.

As @kayvaninvemo mentioned, I don't think FocusTrap plays well with iframes.

This happens sporadically on Chrome and consistently on Firefox.

Is there any workaround for this? I just need to be able to move my cursor through form elements without FocusTrap getting in the way.

adamhp commented 2 years ago

I know this isn't ideal, but in my case, I have a confirmation modal that then opens another modal afterwards. I was running into this focus issue on Safari, but I added a slight timeout to the second modal, so that it basically waits for the other to be completely closed and unmounted (I think?).

Something like

<Confirm onConfirm={() => { setTimeout(openOtherModal, 500) }}/>

Like I said, not ideal, but may help in certain cases that can deal with the timeout.

Mhmdhammoud commented 2 years ago

I'm also experiencing issues with focus trap and Stripe elements in a dialog. When a Stripe element input component is currently focused and you then attempt to focus on another element outside of the Stripe iframe, the Stripe element will demand back focus making it so you can’t move on to other components in a form.

As @kayvaninvemo mentioned, I don't think FocusTrap plays well with iframes.

This happens sporadically on Chrome and consistently on Firefox.

Is there any workaround for this? I just need to be able to move my cursor through form elements without FocusTrap getting in the way.

I've experienced the same issue, please let me know if this get's fixed anytime soon.

memiljamel commented 2 years ago

@Mhmdhammoud if you don't use transition component it should work but if you use transition component make sure on TransitionRoot component apply props :show and also on Dialog component apply props :open with same value.

RobinMalfait commented 2 years ago

Hey! Thank you for your bug report! Much appreciated! πŸ™

For everyone finding this issue.

The problem: You want 2 Dialogs, but it won't work Solution: Dialogs must be nested if you want multiple Dialogs. Dialogs can't be siblings because if you happen to open them at the same time each Dialog will try to steal focus again because a Dialog should be considered a separate page where you can't interact with the content behind it.

MLDMoritz commented 2 years ago

This shouldn't be closed. There should be a proper solution for Dialogs that get opened one after another or at least proper reporting of how to resolve the issue.

ThenTech commented 2 years ago

I also continue to run into this, each dialog is always sibling in a portal outside of root, as they can be opened by global methods and are not redefined in other components.

My use case is a base modal that has a form the user can fill in, and on error, I show another modal with the error message, thus creating a second sibling, triggering the focus loop when the error modal closes.

Setting initialFocus={document.activeElement} does not seem to work. (React)

Can't we just disable the Dialog component from trying to get focus in the first place? Like mentioned by @mockingjet and @tianyingchun, the FocusTrap behaviour could be opted out of.

tianyingchun commented 2 years ago

now i resolved this issue, close previous dialog before new dialog open.

fedorinoGore commented 2 years ago

@RobinMalfait Guys, this is a real issue. Having modals as siblings is not a whim it is a real requirement. Please provide an option to disable this behaviour...

M1ck0 commented 2 years ago

Workaround for this.

Set open prop on Modal to false and it will work without throwing that error. If anyone knows why it behaves this way, comment please.

wrod7 commented 2 years ago

im having this issue when i have a Dialog inside a Tab.List

solomonhawk commented 2 years ago

πŸ‘ Building a payments form inside an accessible dialog - using Stripe Elements (which renders its own inputs inside iframes) results in some serious issues related to focus management. Is there anything that can be done?

Mhmdhammoud commented 2 years ago

πŸ‘ Building a payments form inside an accessible dialog - using Stripe Elements (which renders its own inputs inside iframes) results in some serious issues related to focus management. Is there anything that can be done?

I had the same issue with Stripe element, but it only happens on Firefox Try Google Chrome it's working just fine there....

RobinMalfait commented 2 years ago

@solomonhawk try installing the insiders build

kayvaninvemo commented 2 years ago

πŸ‘ Building a payments form inside an accessible dialog - using Stripe Elements (which renders its own inputs inside iframes) results in some serious issues related to focus management. Is there anything that can be done?

I had the same issue with Stripe element, but it only happens on Firefox Try Google Chrome it's working just fine there....

It has also issue on Chrome, if you notice the "tab" key does not work smoothly, I should press tab key 2 times to go to the next card element.

solomonhawk commented 2 years ago

@solomonhawk try installing the insiders build

Thank you for the suggestion.

Unfortunately insiders doesn't seem to have any impact in my case. Tabbing through the stripe elements fields still doesn't work properly.

For a little more context: I use the individual CardNumberElement / CardExpiryElement / CardCvcElement, wrapped in a custom connected form field component.

The issue in my case is that focus never leaves the card input (or it continuously is re-focused when attempting to tab past). Interestingly, I can Shift+Tab backwards through the form fields just fine but Tabbing through them is messed up.

I tried insiders and saw no change in this behavior. I also tested with Stripe's all-in-one CardElement (combined input for card number/expiry/cvc) without my custom component wrapper and the issue was mostly the same except I can tab through card number and expiry but focus gets "stuck" on the CVC input (which is the last input inside the iframe).

solomonhawk commented 2 years ago

I spent a little time trying to dig deeper and wasn't able to come to any conclusions. Interestingly, react-modal does not exhibit the same issue (but also traps focus). I'd really prefer not to introduce another separate dependency strictly for modals with worse accessibility but this is currently a huge blocker for the project I'm working on.

Appreciate all the hard work that goes into maintaining this library πŸ™

RobinMalfait commented 2 years ago

@solomonhawk sorry to hear about that. Can you create and share a reproduction repo / CodeSandbox that showcases the issue(s)?

solomonhawk commented 2 years ago

@RobinMalfait I hope to find time to create a minimal reproduction, but I'm not sure when I'll be able to. Appreciate the reply!

acatalina commented 2 years ago

hey @solomonhawk, I came across this issue as I had the same problem with stripe elements and the dialog. Just tried 1.6.0 and it solved the issue although I cannot use the new Dialog.Panel as it triggers the onClose method on the Dialog while clicking around. Hope it helps

mreduar commented 2 years ago

Add an initialFocus prop to the dialog and set it to document.activeElement. For folks using vue:

<template>
    <Dialog as="div" :initialFocus="getActiveElement()">
   ...
    </Dialog>
</template>
<script>
...
   methods: {
       getActiveElement() {
           return document.activeElement;
       }
   }
...
</script>

This trick works to avoid the error, but it seems that the recursion problem still affects the performance and also if the page has scroll it is not possible to scroll after the modal is closed.

mreduar commented 2 years ago

hey @solomonhawk, I came across this issue as I had the same problem with stripe elements and the dialog. Just tried 1.6.0 and it solved the issue although I cannot use the new Dialog.Panel as it triggers the onClose method on the Dialog while clicking around. Hope it helps

I confirm that this is solved by upgrading to the latest version. But the error I mentioned in the previous comment persists.

sgarcia-dev commented 1 year ago

@RobinMalfait Guys, this is a real issue. Having modals as siblings is not a whim it is a real requirement. Please provide an option to disable this behaviour...

While I (and the community) agrees with this being a real requirement. I think we need to remember it's an open source library maintained for free, and we have no right to demand anything.

However, it is strange how the Dialog (Modal) docs on headlessui.com do not ever mention this limitation. Despite the large impact on how large scale web applications are architected. If there are no plans to add support for multiple modal support as siblings, we should at the bare minimum communicate this publicly instead of burying this valuable piece of information in a Closed issue.

I went ahead and created https://github.com/tailwindlabs/headlessui/issues/2689 in hopes of giving this issue some closure.

Feb 6 2024 Update: no mention of multiple modal usage in the docs yet 😞 feel free to leave a comment/reaction on the above thread if you'd also like this mentioned early in the docs. As it seems early communicating of this restriction could have avoided a lot of frustration for developers who've already committed to headless UI for their projects unaware of it.

pinguluk commented 12 months ago

For me it was caused due a misconfiguration in webpack, that caused the Modal to be rendered twice https://github.com/mui/material-ui/issues/24641#issuecomment-1729167309