Fix incorrect closing while interacting with third party libraries in Dialog component
#1268
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,7 +129,7 @@ export let Dialog = defineComponent({ | |
| // in between. We only care abou whether you are the top most one or not. | ||
| let position = computed(() => (!hasNestedDialogs.value ? 'leaf' : 'parent')) | ||
|
|
||
| let previousElement = useFocusTrap( | ||
| internalDialogRef, | ||
| computed(() => { | ||
| return enabled.value | ||
|
|
@@ -191,13 +191,28 @@ export let Dialog = defineComponent({ | |
| provide(DialogContext, api) | ||
|
|
||
| // Handle outside click | ||
| useOutsideClick( | ||
| () => { | ||
| // Third party roots | ||
| let rootContainers = Array.from( | ||
| ownerDocument.value?.querySelectorAll('body > *') ?? [] | ||
|
There was a problem hiding this comment. I don't completely understand what's happening here but I'm curious whether this fix works with elements that are not direct children of the There was a problem hiding this comment. Alright so the idea with this code is to allow for 3rd party plugins. We don't allow to interact with elements behind the Dialog component for accessibility reasons and what not. We still don't allow that, but there are a few exceptions that technically violate some of the rules but you can think of it as progressive enhancement. We already allowed our <body>
<div id="app"><!-- Your main application --></div>
<div id="headlessui-portal-root">
<div><!-- The main Dialog --></div>
<div><!-- ... other Portal components --></div>
</div>
</body>What you will notice is that the other portal components live outside the main Dialog, so technically we should close the Dialog if you click on any of those items because they are "outside" the Dialog component. The reason this is already possible with our provided The issue this PR fixes is with 3rd party plugins. Often 3rd party plugins will render the popup elements in a portal as well. The problem is that we don't know when this happens and we also can't get a DOM reference easily to those elements. We also can't ask every library on planet earth to expose some of the information we need. So this fix is definitely not perfect, but I think it will solve a lot of the issues people experience today. How does it work? Let's imagine you have this structure again: <body>
<div id="app">
<div>
<!--
This is an example button in the App that is **not** inside the Dialog.
Trying to click this button will close the Dialog because this is
"outside" of the Dialog which is not allowed.
-->
<button>in app</button>
</div>
</div>
<div id="headlessui-portal-root">
<div>
<!-- This is a button in the Dialog, interacting with this is allowed. -->
<button>in dialog</button>
</div>
</div>
<div id="third-party-library-portal">
<!--
Interacting with this button is technically not allowed since it lives
outside of the Dialog. However we make an exception that you _can_
interact with this one because it lives in another "parent" than the main
application. This means that we are currently making an assumption that
interacteable elements that live in a parent outside of the main app are allowed.
This trade-off is necessary since we don't know when 3rd party libraries
will render certain elements in the DOM and we don't get a stable
reference to those elements.
-->
<button>Deeply nested button inside the 3rd party library</button>
</div>
</body>How it works is that we collect all the direct "root" containers, that's what the
Does that answer your question? There was a problem hiding this comment. Wow yeah that does answer my question, thanks! I really appreciate the detailed explanation. At one point I had thought it would be easier to just listen for clicks directly on the backdrop, but now I'm realizing there doesn't even necessarily have to be a backdrop... not to mention all the other possibilities. This is way more complicated than I thought and your solution looks great. Thanks for breaking it down 👍🏻 |
||
| ).filter((container) => { | ||
| if (!(container instanceof HTMLElement)) return false // Skip non-HTMLElements | ||
| if (container.contains(previousElement.value)) return false // Skip if it is the main app | ||
| return true // Keep | ||
| }) | ||
|
|
||
| return [...rootContainers, internalDialogRef.value] as HTMLElement[] | ||
| }, | ||
|
|
||
| (_event, target) => { | ||
| if (dialogState.value !== DialogStates.Open) return | ||
| if (hasNestedDialogs.value) return | ||
|
|
||
| api.close() | ||
| nextTick(() => target?.focus()) | ||
| } | ||
| ) | ||
|
|
||
| // Handle `Escape` to close | ||
| useEventListener(ownerDocument.value?.defaultView, 'keydown', (event) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My issue in #432 was with third-party components inside a
Dialog, it seems like this test doesn't do that? Maybe I'm misunderstanding. I would have expected something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep I should probably add a test for that case as well. But the way it is implemented it doesn't really matter where the 3rd party gets initiated from. I'll explain the process in the other question you asked 👍