headlessui: [Bug]: Dialog: There are no focusable elements inside the
What package within Headless UI are you using?
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!
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Reactions: 1
- Comments: 29 (10 by maintainers)
@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.
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
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.
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
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:
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.
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
Then on the hmr reload it will fail, but if I have
Then it will work as expected
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
oryarn add @headlessui/react@latest
.npm install @headlessui/vue@ latest
oryarn add @headlessui/vue@latest
.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
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 errorI 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.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.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.
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:
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.