The closed event executes twice for Dialog components #1490

druc · 2022-05-22T22:43:49Z

What package within Headless UI are you using?
@headlessui/vue

What version of that package are you using?
v1.6.2

What browser are you using?
Chrome, Firefox

Reproduction URL
https://github.com/druc/headlessui/pull/1/files

Describe your issue
When using a Dialog without specifying an open prop, the closed event fires twice when clicking outside the DialogPanel.

From my investigations the reason seems to be the outside click handler executing twice; even though the intention is to only do it once: #1175

Replacing queueMicrotask with setTimeout fixes it but I'm not sure it's the best solution.

The text was updated successfully, but these errors were encountered:

Here is a little story. We used to use the `click` event listener on the window to try and detect whether we clicked outside of the main area we are working in. This all worked fine, until we got a bug report that it didn't work properly on Mobile, especially iOS. After a bit of debugging we switched this behaviour to use `pointerdown` instead of the `click` event listener. Worked great! Maybe... The reason the `click` didn't work was because of another bug fix. In React if you render a `<form><Dialog></form>` and your `Dialog` contains a button without a type, (or an input where you press enter) then the form would submit... even though we portalled the `Dialog` to a different location, but it bubbled the event up via the SyntethicEvent System. To fix this, we've added a "simple" `onClick(e) { e.stopPropagation() }` to make sure that click events didn't leak out. Alright no worries, but, now that we switched to `pointerdown` we got another bug report that it didn't work on older iOS devices. Fine, let's add a `mousedown` next to the `pointerdown` event. Now this works all great! Maybe... This doesn't work quite as we expected because it could happen that both events fire and then the `onClose` of the Dialog component would fire twice. In fact, there is an open issue about this: #1490 at the time of writing this commit message. We tried to only call the close function once by checking if those events happen within the same "tick", which is not always the case... Alright, let's ignore that issue for a second, there is another issue that popped up... If you have a Dialog that is scrollable (because it is greater than the current viewport) then a wild scrollbar appears (what a weird Pokémon). The moment you try to click the scrollbar or drag it the Dialog closes. What in the world...? Well... turns out that `pointerdown` gets fired if you happen to "click" (or touch) on the scrollbar. A click event does not get fired. No worries we can fix this! Maybe... (Narrator: ... nope ...) One thing we can try is to measure the scrollbar width, and if you happen to click near the edge then we ignore this click. You can think of it like `let safeArea = viewportWidth - scrollBarWidth`. Everything works great now! Maybe... Well, let me tell you about macOS and "floating" scrollbars... you can't measure those... AAAAAAAARGHHHH Alright, scratch that, let's add an invisible 20px gap all around the viewport without measuring as a safe area. Nobody will click in the 20px gap, right, right?! Everything works great now! Maybe... Mobile devices, yep, Dialogs are used there as well and usually there is not a lot of room around those Dialogs so you almost always hit the "safe area". Should we now try and detect the device people are using...? /me takes a deep breath... Inhales... Exhales... Alright, time to start thinking again... The outside click with a "simple" click worked on Menu and Listbox not on the Dialog so this should be enough right? WAIT A MINUTE Remember this piece of code from earlier: ```js onClick(event) { event.stopPropagation() } ``` The click event never ever reaches the `window` so we can't detect the click outside... Let's move that code to the `Dialog.Panel` instead of on the `Dialog` itself, this will make sure that we stop the click event from leaking if you happen to nest a Dialog in a form and have a submitable button/input in the `Dialog.Panel`. But if you click outside of the `Dialog.Panel` the "click" event will bubble to the `window` so that we can detect a click and check whether it was outside or not. Time to start cleaning: - ☑️ Remove all the scrollbar measuring code... - Closing works on mobile now, no more safe area hack - ☑️ Remove the pointerdown & mousedown event - Outside click doesn't fire twice anymore - ☑️ Use a "simple" click event listener - We can click the scrollbar and the browser ignores it for us All issues have been fixed! (Until the next one of course...)

* convert dialog in playground to use Dialog.Panel * convert `tabs-in-dialog` example to use `Dialog.Panel` * add scrollable dialog example to the playground * simplify `outside click` behaviour Here is a little story. We used to use the `click` event listener on the window to try and detect whether we clicked outside of the main area we are working in. This all worked fine, until we got a bug report that it didn't work properly on Mobile, especially iOS. After a bit of debugging we switched this behaviour to use `pointerdown` instead of the `click` event listener. Worked great! Maybe... The reason the `click` didn't work was because of another bug fix. In React if you render a `<form><Dialog></form>` and your `Dialog` contains a button without a type, (or an input where you press enter) then the form would submit... even though we portalled the `Dialog` to a different location, but it bubbled the event up via the SyntethicEvent System. To fix this, we've added a "simple" `onClick(e) { e.stopPropagation() }` to make sure that click events didn't leak out. Alright no worries, but, now that we switched to `pointerdown` we got another bug report that it didn't work on older iOS devices. Fine, let's add a `mousedown` next to the `pointerdown` event. Now this works all great! Maybe... This doesn't work quite as we expected because it could happen that both events fire and then the `onClose` of the Dialog component would fire twice. In fact, there is an open issue about this: #1490 at the time of writing this commit message. We tried to only call the close function once by checking if those events happen within the same "tick", which is not always the case... Alright, let's ignore that issue for a second, there is another issue that popped up... If you have a Dialog that is scrollable (because it is greater than the current viewport) then a wild scrollbar appears (what a weird Pokémon). The moment you try to click the scrollbar or drag it the Dialog closes. What in the world...? Well... turns out that `pointerdown` gets fired if you happen to "click" (or touch) on the scrollbar. A click event does not get fired. No worries we can fix this! Maybe... (Narrator: ... nope ...) One thing we can try is to measure the scrollbar width, and if you happen to click near the edge then we ignore this click. You can think of it like `let safeArea = viewportWidth - scrollBarWidth`. Everything works great now! Maybe... Well, let me tell you about macOS and "floating" scrollbars... you can't measure those... AAAAAAAARGHHHH Alright, scratch that, let's add an invisible 20px gap all around the viewport without measuring as a safe area. Nobody will click in the 20px gap, right, right?! Everything works great now! Maybe... Mobile devices, yep, Dialogs are used there as well and usually there is not a lot of room around those Dialogs so you almost always hit the "safe area". Should we now try and detect the device people are using...? /me takes a deep breath... Inhales... Exhales... Alright, time to start thinking again... The outside click with a "simple" click worked on Menu and Listbox not on the Dialog so this should be enough right? WAIT A MINUTE Remember this piece of code from earlier: ```js onClick(event) { event.stopPropagation() } ``` The click event never ever reaches the `window` so we can't detect the click outside... Let's move that code to the `Dialog.Panel` instead of on the `Dialog` itself, this will make sure that we stop the click event from leaking if you happen to nest a Dialog in a form and have a submitable button/input in the `Dialog.Panel`. But if you click outside of the `Dialog.Panel` the "click" event will bubble to the `window` so that we can detect a click and check whether it was outside or not. Time to start cleaning: - ☑️ Remove all the scrollbar measuring code... - Closing works on mobile now, no more safe area hack - ☑️ Remove the pointerdown & mousedown event - Outside click doesn't fire twice anymore - ☑️ Use a "simple" click event listener - We can click the scrollbar and the browser ignores it for us All issues have been fixed! (Until the next one of course...) * ensure a `Dialog.Panel` exists * cleanup unnecessary code * use capture phase for outside click behaviour * further improve outside click We added event.preventDefault() & event.defaultPrevented checks to make sure that we only handle 1 layer at a time. E.g.: ```js <Dialog> <Menu> <Menu.Button>Button</Menu.Button> <Menu.Items>...</Menu.Items> </Menu> </Dialog> ``` If you open the Dialog, then open the Menu, pressing `Escape` will close the Menu but not the Dialog, pressing `Escape` again will close the Dialog. Now this is also applied to the outside click behaviour. If you open the Dialog, then open the Menu, clicking outside will close the Menu but not the Dialog, outside again will close the Dialog. * add explicit `enabled` value to the `useOutsideClick` hook * ensure outside click properly works with Poratl components Usually this works out of the box, however our Portal components will render inside the Dialog component "root" to ensure that it is inside the non-inert tree and is inside the Dialog visually. This means that the Portal is not in a separate container and technically outside of the `Dialog.Panel` which means that it will close when you click on a non-interactive item inside that Portal... This fixes that and allows all Portal components. * update changelog

RobinMalfait · 2022-06-03T14:21:31Z

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

This should be fixed by #1546, and will be available in the next release.

You can already try it using npm install @headlessui/react@insiders.

chiuwah · 2023-12-10T21:11:27Z

Hi, I am having the same problem with @headlessui/vue v1.7.16 using TransitionRoot in combination with Dialog
Was this issue solved already?

RobinMalfait mentioned this issue May 29, 2022

Dialog.Panel outside click onClose execute twice . #1520

Closed

RobinMalfait mentioned this issue Jun 2, 2022

Improve outside click of Dialog component #1546

Merged

RobinMalfait closed this as completed in #1546 Jun 3, 2022

aldabil21 mentioned this issue Aug 14, 2023

Dialog onClose executes twice in mobile #2671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The closed event executes twice for Dialog components #1490

The closed event executes twice for Dialog components #1490

druc commented May 22, 2022

RobinMalfait commented Jun 3, 2022

chiuwah commented Dec 10, 2023 •

edited

Loading

The closed event executes twice for Dialog components #1490

The closed event executes twice for Dialog components #1490

Comments

druc commented May 22, 2022

RobinMalfait commented Jun 3, 2022

chiuwah commented Dec 10, 2023 • edited Loading

chiuwah commented Dec 10, 2023 •

edited

Loading