[Bug]: Dialog: There are no focusable elements inside the <FocusTrap />

[Krystofee]

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

1.0.0

What browser are you using?

Chrome

Reproduction repository

https://codesandbox.io/s/gallant-butterfly-wnxjb

Description

When there is no focusable element inside modal, it fails with error There are no focusable elements inside the <FocusTrap />. I don't know if it's a feature or a bug, but there isn't said in the documentation that Dialog must include at least one focusable element and I totally see a use-case when there are modals without any focusable element. I think it at least should fail with warning instead of fatal error.

Anyways I would like to thank you for the great job you all are doing with the tailwind ecosystem, we really appreciate your work!

[etc-tiago]

I believe it is purposeful, as there are specific cases that the modal needs to be displayed without a button or input.

If you need to display this modal and do not want the button/input appear, you can use it like this:

<button className="h-0 w-0 overflow-hidden"/>

But I strongly recommend to add at least one "close" button in case of usability or smaller screens, where the space of the overlay is not large enough to be clickable / touchable.

[Krystofee]

You got a point, but I think there are many valid usecases where there are no focusable elements and you don't want any, e. g.

• Loading contents of the Dialog - while it's loading, you probably render only spinner and nothing else and don't expect the Dialog to crash.
• Or you just want to show some Dialog to the users when they were locked out of their account and there is no action that thay can take within the app.

Even then, there is no big red caveat that this error will happen if you somewhat forget to render anything in the Dialog.

So I propose two ways out:

1. Dialog should not crash when there is no focusable element within.
2. Add some caveat to the docs that the Dialog will crash if rendered without any focusable elements.

If it was the second, then I think many users (including me) which are building internal set of components on top of headless ui will just insert the hidden focusable element to prevent Dialogs from crashing.

[trm217]

@Krystofee I 100% agree with your proposal, but I would add another change:

• There should be an option for the overlay to not close the modal on click. For example, in a loading-modal you wouldn't want the user to able to cancel the modal.

[Krystofee]

@trm217 this is doable by managing the state of the modal by yourself and handling Dialog onClose prop according to the loading state. I think that's not the problem of this issue and should be resolved in the discussions.

[trm217]

@Krystofee I'm aware of that, yet I believe that this would be a fitting change in regards to your above-mentioned proposed changes. Therefore if your proposed changes would be implemented, my proposal would make sense to implement as well since it goes hand in hand with others.

[barnabaskecskes]

I've got a TableFilters component and several actual filter components, for example an ApplicationFilter. I use the provide/inject API of Vue to register the filter components to the main component when created(), so they can interact with each other in order to set initial states from the query string, etc. For that to work, I had to set the Dialog not to be unmounted. Doing that I get the same error as above.

In my case, there are focusable components in the dialog - but as it's not rendered, those are not actually focusable. Does the FocusTrap check for a visible state before trying to actually focus?

UPDATE: I could get around the issue by setting the initialFocus to an arbitrary element of the DOM.

[Krystofee]

I think components that are not rendered are generally not focusable. That's not a feature of headless ui but general feature of browser.

And yeah, I was reading the source code and also noticed the hack with the initialFocus, but thats not a good API and this approach comes with caveats too.

I'm really interested in hearing a feedback from @RobinMalfait on this topic.

[adamwathan]

Hey! This isn't a bug, for accessibility reasons all modal dialogs must contain a focusable element. Worth reading:

https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal

If content is large enough that focusing the first interactive element could cause the beginning of content to scroll out of view, it is advisable to add tabindex=-1 to a static element at the top of the dialog, such as the dialog title or first paragraph, and initially focus that element.

If you are showing a loading state or similar, it seems like it would be acceptable to add tabindex="-1" to the loading area and focus that while the content loads in, then imperatively focus the element you want focused after loading is finished.

I don't think we have any plans to change the current behavior as a major goal of this project is to help people build accessible interfaces, but I'll leave this open unless @RobinMalfait has anything he would like to add.

[barnabaskecskes]

Thinking about it (every since), also reading up on this (thanks, @adamwathan), the Dialog might not be appropriate for my use case anyway.

TL;DR I'll leave a simplified example below, just in case somebody tries to abuse the Dialog just as I did.

<TableLayout>
    <TableFilters>
        <FilterConfiguration />
        <Dialog>
            <FilterA />
            <FilterB />
        </Dialog>
    </TableFilters>
</TableLayout>

The TableFilters component deals with managing the URL as the actual filters are applied. The FilterConfiguration is a textual representation of the filters set for the table. It uses the provide/inject API and the filters register themselves on created/mounted, so the FilterConfiguration can display the initial filters set should they come from the URL. For that, the Dialog has to be unmounted=false, which in turn causes the error.

Again, thinking this through, the Dialog is not the solution - the more I think about it, the more it seems that Popover is much more appropriate for this thing. I'll convert to that instead.

Thanks for the help!

[Krystofee]

@adamwathan what you are saying is that "There should be at least one focusable element in the modal." and I agree with that.

What I don't agree with, is the way it's enforced. You cannot throw an error because of accessibility reasons. By doing that, you are putting accessibility before reliability, which means that you will have fully accessible software which will crash all the time or not even boot. I believe software must be fully reliable before one start dealing with accessibility.

I don't understand the reasons behind this decision, and it's also because I haven't seen any UI framework enforcing accessibility via throwing errors. And going even further, if I notice that some of the hundreds of dependencies in our project throws an exception, I just go to the project github and report a bug, because most of the time it's a bug.

And even if it was intentional, it's not documented and users will have to read through the code and will eventually create a Bug report like this one.

[hailwood]

This is also an issue with hmr (react) which I believe is a bug.

When I have a dialog, whenever I have a dialog open and change something so it has to reload (e.g. text in the dialog content) I always get There are no focusable elements inside the <FocusTrap />

It seems to be related to initialFocus e.g. if I have

<Dialog>
    <input />
</Dialog>

Then on the hmr reload it will fail, but if I have

<Dialog initialFocus={inputRef}>
    <input ref={inputRef} />
</Dialog>

Then it will work as expected

[realStandal]

I'm running into this error when using the Dialog. It's occurring while using the component in Storybook. I'm not sure if it's related to the above HMR issue, I can create a new issue if need be. I'm using the React Dialog component.

The error occurs while trying to control the component using Storybooks controls, where I can click a button to toggle a true/fale prop passed to the Dialog. Changing this value will throw the error, the component loads fine at first.

Controlling the Dialog from within the story (having a button setState to show/hide it) also throws.

If I manually pass the ref, it works as expected.

It works as expected in my Redwood app, without the need to explicitly define the initial ref.

Edit: After putting more time towards the bug, and not what I was trying to accomplish at the time, I'm able to use the Dialog within a Story and control it with Storybook's controls by creating an intermediary component that handles the Dialog's state; no explicit passing of a ref involved. I've had to do the same thing before for other components which have a controlled state, so it's possible it's just a hiccup with Storybook/how I've or the framework I'm using has set it up. Again, it's not the first time Storybook hasn't played well with a component, Transition has a hard time being controlled via it and components I create/have used from other libraries.

[Krystofee]

Any news related to this issue? 🤔

[mxswat]

Having an error thrown is terrible, as it blocks the rendering in the app I'm working on.
It's okay to care about accessibly, but accessibility must not interfere with the reliability

[pieterbeulque]

Hey! This isn't a bug, for accessibility reasons all modal dialogs must contain a focusable element. Worth reading:

https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal

If content is large enough that focusing the first interactive element could cause the beginning of content to scroll out of view, it is advisable to add tabindex=-1 to a static element at the top of the dialog, such as the dialog title or first paragraph, and initially focus that element.

If you are showing a loading state or similar, it seems like it would be acceptable to add tabindex="-1" to the loading area and focus that while the content loads in, then imperatively focus the element you want focused after loading is finished.

I don't think we have any plans to change the current behavior as a major goal of this project is to help people build accessible interfaces, but I'll leave this open unless @RobinMalfait has anything he would like to add.

@adamwathan - You're right here, however FocusTrap strips out all elements with tabindex="-1" so what you're saying doesn't work now. My workaround now is to set tabindex="0" on the DialogTitle if there are no focusable elements, i.e. when dialog will be closed or updated on some asynchronous action.

I'm fine personally with FocusTrap throwing an error if there's nothing focusable, but we should be able to set tabindex="-1" on a non-interactive element instead of tabindex="0" to keep the programmatic focus, but keep it out of the keyboard navigation flow. It's what's advised by WAI-ARIA too, for what it's worth.

I'd be happy to take a stab at a PR if I can find the time.

[RobinMalfait]

Hi @pieterbeulque

The FocusTrap only takes tabindex="0" into account when doing the initial focus and while using your Tab and Shift+Tab keys.

What you describe with the tabIndex={-1} should already be possible by using the initialFocus prop (https://headlessui.dev/react/dialog#managing-focus-within-your-dialog)

Here is an example: https://stackblitz.com/edit/react-gvucjx?file=src/App.js

[pieterbeulque]

Thanks for that example! initialFocus doesn't work for my use case. I pass in some buttons using a <slot> which are rendered in a tick after the component mount, if my understanding is correct, so on the moment of initialization these aren't available yet.

Edit: We're using Vue 3, FYI

[barnabaskecskes]

@pieterbeulque I do the same thing. I've got a <footer /> slot to handle buttons - and that has a default value. Vue3 works in quirky ways (in my opinion) with refs but here is my actual implementation as an example. See if that works for you?

<template>
    <TransitionRoot appear leave as="template" :unmount="unmount" :show="show">
        <Dialog as="div" static :open="true" :unmount="unmount" :initial-focus="$refs.footer">
            <div class="fixed inset-0 z-10 overflow-y-auto">
                <div class="min-h-screen px-4 text-center">
                    <TransitionChild
                        as="template"
                        enter="duration-300 ease-out"
                        enter-from="opacity-0"
                        enter-to="opacity-100"
                        leave="duration-200 ease-in"
                        leave-from="opacity-100"
                        leave-to="opacity-0"
                        :unmount="unmount"
                        :show="show"
                    >
                        <DialogOverlay class="fixed inset-0 bg-gray-500 bg-opacity-75" />
                    </TransitionChild>

                    <TransitionChild
                        as="template"
                        enter="duration-300 ease-out"
                        enter-from="opacity-0 scale-95"
                        enter-to="opacity-100 scale-100"
                        leave="duration-200 ease-in"
                        leave-from="opacity-100 scale-100"
                        leave-to="opacity-0 scale-95"
                        :unmount="unmount"
                        :show="show"
                    >
                        <div
                            class="inline-block w-full px-6 py-4 my-8 text-left transition-all transform bg-white shadow-xl"
                            :class="maxWidthClass"
                        >
                            <DialogTitle
                                as="h3"
                                class="text-lg font-medium leading-6 text-gray-900"
                            >
                                <slot name="title" />
                            </DialogTitle>

                            <DialogDescription
                                as="p"
                                class="mt-3 text-gray-700"
                            >
                                <slot name="description" />
                            </DialogDescription>

                            <div class="mt-3 text-gray-700">
                                <slot name="content" />
                            </div>

                            <div class="-mx-6 -mb-4 mt-4 px-6 py-4 bg-gray-100 flex justify-end items-center" ref="footer">
                                <slot name="footer">
                                    <SecondaryButton @click.native.stop="$emit('close')">{{ closeButtonTitle }}</SecondaryButton>
                                </slot>
                            </div>
                        </div>
                    </TransitionChild>
                </div>
            </div>
        </Dialog>
    </TransitionRoot>
</template>

<script>
    import {nextTick, ref, toRefs, watch} from 'vue'
    import SecondaryButton from '~/Compositions/Buttons/SecondaryButton'
    import {Dialog, DialogDescription, DialogOverlay, DialogTitle, TransitionChild, TransitionRoot} from "@headlessui/vue";

    export default {
        components: {
            SecondaryButton,
            TransitionRoot,
            TransitionChild,
            Dialog,
            DialogOverlay,
            DialogTitle,
            DialogDescription,
        },

        props: {
            show: {
                type: Boolean,
                required: true,
            },
            maxWidth: {
                default: '2xl',
            },
            unmount: {
                default: true,
            },
            closeButtonTitle: {
                type: String,
                default: 'Close',
            },
        },

        setup(props, context) {
            const maxWidthClass = {
                'sm': 'sm:max-w-sm',
                'md': 'sm:max-w-md',
                'lg': 'sm:max-w-lg',
                'xl': 'sm:max-w-xl',
                '2xl': 'sm:max-w-2xl',
                '3xl': 'sm:max-w-3xl',
                '4xl': 'sm:max-w-4xl',
                '5xl': 'sm:max-w-5xl',
                '6xl': 'sm:max-w-6xl',
            }[props.maxWidth];

            const {show} = toRefs(props);
            const footer = ref(null)

            watch(show, (value) => {
                if (!value) {
                    return;
                }

                nextTick(() => footer.value.children[0].focus());
            })

            return {
                maxWidthClass,
                footer,
            };
        },
    };
</script>

[Ghirigoro]

This issue is getting muddled. There's a discussion around a feature request (i.e. allow modals that have no focusable elements) and a bug (i.e. as @hailwood pointed out this error is thrown in scenarios where you DO have a focusable element). Most folks probably agree that the bug needs fixing sooner rather than later, and that the feature request requires more discussion (or not - it seems the maintainers don't agree that this feature is required), so this should be split into two issues

[Krystofee]

I would definitely appreciate if we think about the high level design concept of throwing errors in favor of accessibility (which is why I opened this issue) and not deal with other peoples specific issues with it, because the fundamental issue gets only more distorted.

I don't mean to bother you, but these concrete problems and hacks with initialRef are not really the thing I would like to discuss and change.

[trm217]

I would also welcome a decision on what the core purpose of the library is. For me as a developer it would be important to know what the core target of HeadlessUI is. For me at least this would be important to know. Does it offer utility with a11y in mind or offer utility and enforce a11y?

[pieterbeulque]

Fair point, Krystofee, sorry if I contributed to sidetracking this issue.

[cortopy]

I'd like to add another use case where forcing the existence of a focusable element on initial render is troublesome.

I have a Dialog wrapped by a Transition with very complex animations set dynamically for root and children. Also animations may or may not run depending on context.

This is leading to many errors due to the fact that Dialog may try to focus before it should (i.e.: animations are not done and not all elements are mounted yet). This is very painfully when developing the animations, since elements may get removed due to HMR

Like others have said, I think a warning should be enough. Even when React detects something dodgy, it warns the developer rather than throw, panic and make everything failed.

Not having a focusable element is bad for accessibility. In fact, I do know my Dialog is accessible because eventually (a matter of milliseconds) there will be a focusable element. But the Dialog component cannot know that, and a escape hatch would be nice to have

[Gunner245]

Cam we at least consider downgrading this to a warning until the issues with it being thrown when there are focusable elemets is fixed.

It is the wrost of all worlds throwing an exception when the check is known to be flaky.

[etc-tiago]

I agree that the error can be warning instead of throw.

I have a similar case when using headlessui with next.js in a medium sized application, where I use dynamic import for non "important" components during the page initialization period.

In my point of view, it is more "accessible" not to have accessibility than to have an application broken by not having minimum component requirements.

[Krystofee]

@Gunner245 Can we at least consider downgrading this to a warning until the issues with it being thrown when there are focusable elements is fixed.

I totally agree with you.

I think this issue is open for way too long. It would be really nice if @RobinMalfait left here his opinion on that so we can somehow resolve this issue.

[cipriancaba]

Leaving my usecase here. I am using this to display a slide shopping cart content. However, I have 2 different versions of content for mobile and desktop.

I am currently showing/hiding one of the dialogs based on tailwind's breakpoints. Conditional rendering is a bit problematic since I'm using nextjs and want to avoid hydration issues. The hidden Dialog is causing the FocusTrap error

[Krystofee]

Hey guys, I've created a pull request which resolves this issue as discussed above. Feedback is appreciated 🙂

[RobinMalfait]

As a first step, we will now log a warning instead of throwing an actual error. A bit more info can be found in the PR here: #775

This should be fixed, and is available in the next release.

You can try it using:

• npm install @headlessui/react@latest or yarn add @headlessui/react@latest.
• npm install @headlessui/vue@ latest or yarn add @headlessui/vue@latest.