Improve outside click of Dialog component (#1546)

* 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

Author: RobinMalfait

packages/@headlessui-react/CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix incorrect transitionend/transitioncancel events for the Transition component ([#1537](https://github.com/tailwindlabs/headlessui/pull/1537))
+- Improve outside click of `Dialog` component ([#1546](https://github.com/tailwindlabs/headlessui/pull/1546))
 
 ## [1.6.4] - 2022-05-29
 

packages/@headlessui-react/src/components/combobox/combobox.tsx

@@ -34,7 +34,7 @@ import { forwardRefWithAs, render, compact, PropsForFeatures, Features } from '.
 import { isDisabledReactIssue7711 } from '../../utils/bugs'
 import { match } from '../../utils/match'
 import { objectToFormEntries } from '../../utils/form'
-import { sortByDomNode } from '../../utils/focus-management'
+import { FocusableMode, isFocusableElement, sortByDomNode } from '../../utils/focus-management'
 
 import { Hidden, Features as HiddenFeatures } from '../../internal/hidden'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
@@ -415,11 +415,11 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
   }, [data])
 
   // Handle outside click
-  useOutsideClick([data.buttonRef, data.inputRef, data.optionsRef], () => {
-    if (data.comboboxState !== ComboboxState.Open) return
-
-    dispatch({ type: ActionTypes.CloseCombobox })
-  })
+  useOutsideClick(
+    [data.buttonRef, data.inputRef, data.optionsRef],
+    () => dispatch({ type: ActionTypes.CloseCombobox }),
+    data.comboboxState === ComboboxState.Open
+  )
 
   let slot = useMemo<ComboboxRenderPropArg<TType>>(
     () => ({

packages/@headlessui-react/src/components/dialog/dialog.test.tsx

@@ -889,9 +889,11 @@ describe('Mouse interactions', () => {
         return (
           <div onClick={wrapperFn}>
             <Dialog open={isOpen} onClose={setIsOpen}>
-              Contents
-              <button onClick={() => setIsOpen(false)}>Inside</button>
-              <TabSentinel />
+              <Dialog.Panel>
+                Contents
+                <button onClick={() => setIsOpen(false)}>Inside</button>
+                <TabSentinel />
+              </Dialog.Panel>
             </Dialog>
           </div>
         )

packages/@headlessui-react/src/components/dialog/dialog.tsx

@@ -32,7 +32,7 @@ import { Description, useDescriptions } from '../description/description'
 import { useOpenClosed, State } from '../../internal/open-closed'
 import { useServerHandoffComplete } from '../../hooks/use-server-handoff-complete'
 import { StackProvider, StackMessage } from '../../internal/stack-context'
-import { useOutsideClick, Features as OutsideClickFeatures } from '../../hooks/use-outside-click'
+import { useOutsideClick } from '../../hooks/use-outside-click'
 import { getOwnerDocument } from '../../utils/owner'
 import { useOwnerDocument } from '../../hooks/use-owner'
 import { useEventListener } from '../../hooks/use-event-listener'
@@ -100,13 +100,7 @@ let DEFAULT_DIALOG_TAG = 'div' as const
 interface DialogRenderPropArg {
   open: boolean
 }
-type DialogPropsWeControl =
-  | 'id'
-  | 'role'
-  | 'aria-modal'
-  | 'aria-describedby'
-  | 'aria-labelledby'
-  | 'onClick'
+type DialogPropsWeControl = 'id' | 'role' | 'aria-modal' | 'aria-describedby' | 'aria-labelledby'
 
 let DialogRenderFeatures = Features.RenderStrategy | Features.Static
 
@@ -204,27 +198,22 @@ let DialogRoot = forwardRefWithAs(function Dialog<
   useOutsideClick(
     () => {
       // Third party roots
-      let rootContainers = Array.from(ownerDocument?.querySelectorAll('body > *') ?? []).filter(
-        (container) => {
-          if (!(container instanceof HTMLElement)) return false // Skip non-HTMLElements
-          if (container.contains(mainTreeNode.current)) return false // Skip if it is the main app
-          if (state.panelRef.current && container.contains(state.panelRef.current)) return false
-          return true // Keep
-        }
-      )
+      let rootContainers = Array.from(
+        ownerDocument?.querySelectorAll('body > *, [data-headlessui-portal]') ?? []
+      ).filter((container) => {
+        if (!(container instanceof HTMLElement)) return false // Skip non-HTMLElements
+        if (container.contains(mainTreeNode.current)) return false // Skip if it is the main app
+        if (state.panelRef.current && container.contains(state.panelRef.current)) return false
+        return true // Keep
+      })
 
       return [
         ...rootContainers,
         state.panelRef.current ?? internalDialogRef.current,
       ] as HTMLElement[]
     },
-    () => {
-      if (dialogState !== DialogStates.Open) return
-      if (hasNestedDialogs) return
-
-      close()
-    },
-    OutsideClickFeatures.IgnoreScrollbars
+    close,
+    enabled && !hasNestedDialogs
   )
 
   // Handle `Escape` to close
@@ -311,9 +300,6 @@ let DialogRoot = forwardRefWithAs(function Dialog<
     'aria-modal': dialogState === DialogStates.Open ? true : undefined,
     'aria-labelledby': state.titleId,
     'aria-describedby': describedby,
-    onClick(event: ReactMouseEvent) {
-      event.stopPropagation()
-    },
   }
 
   return (
@@ -492,10 +478,17 @@ let Panel = forwardRefWithAs(function Panel<TTag extends ElementType = typeof DE
     [dialogState]
   )
 
+  // Prevent the click events inside the Dialog.Panel from bubbling through the React Tree which
+  // could submit wrapping <form> elements even if we portalled the Dialog.
+  let handleClick = useEvent((event: ReactMouseEvent) => {
+    event.stopPropagation()
+  })
+
   let theirProps = props
   let ourProps = {
     ref: panelRef,
     id,
+    onClick: handleClick,
   }
 
   return render({

packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx

@@ -147,7 +147,10 @@ function useRestoreFocus({ ownerDocument }: { ownerDocument: Document | null },
   useWatch(() => {
     if (enabled) return
 
-    focusElement(restoreElement.current)
+    if (ownerDocument?.activeElement === ownerDocument?.body) {
+      focusElement(restoreElement.current)
+    }
+
     restoreElement.current = null
   }, [enabled])
 

packages/@headlessui-react/src/components/listbox/listbox.tsx

@@ -396,16 +396,18 @@ let ListboxRoot = forwardRefWithAs(function Listbox<
   )
 
   // Handle outside click
-  useOutsideClick([buttonRef, optionsRef], (event, target) => {
-    if (listboxState !== ListboxStates.Open) return
-
-    dispatch({ type: ActionTypes.CloseListbox })
+  useOutsideClick(
+    [buttonRef, optionsRef],
+    (event, target) => {
+      dispatch({ type: ActionTypes.CloseListbox })
 
-    if (!isFocusableElement(target, FocusableMode.Loose)) {
-      event.preventDefault()
-      buttonRef.current?.focus()
-    }
-  })
+      if (!isFocusableElement(target, FocusableMode.Loose)) {
+        event.preventDefault()
+        buttonRef.current?.focus()
+      }
+    },
+    listboxState === ListboxStates.Open
+  )
 
   let slot = useMemo<ListboxRenderPropArg>(
     () => ({ open: listboxState === ListboxStates.Open, disabled }),

packages/@headlessui-react/src/components/menu/menu.tsx

@@ -239,16 +239,18 @@ let MenuRoot = forwardRefWithAs(function Menu<TTag extends ElementType = typeof
   let menuRef = useSyncRefs(ref)
 
   // Handle outside click
-  useOutsideClick([buttonRef, itemsRef], (event, target) => {
-    if (menuState !== MenuStates.Open) return
-
-    dispatch({ type: ActionTypes.CloseMenu })
+  useOutsideClick(
+    [buttonRef, itemsRef],
+    (event, target) => {
+      dispatch({ type: ActionTypes.CloseMenu })
 
-    if (!isFocusableElement(target, FocusableMode.Loose)) {
-      event.preventDefault()
-      buttonRef.current?.focus()
-    }
-  })
+      if (!isFocusableElement(target, FocusableMode.Loose)) {
+        event.preventDefault()
+        buttonRef.current?.focus()
+      }
+    },
+    menuState === MenuStates.Open
+  )
 
   let slot = useMemo<MenuRenderPropArg>(
     () => ({ open: menuState === MenuStates.Open }),
@@ -344,7 +346,6 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
       d.nextFrame(() => state.buttonRef.current?.focus({ preventScroll: true }))
     } else {
       event.preventDefault()
-      event.stopPropagation()
       dispatch({ type: ActionTypes.OpenMenu })
     }
   })

packages/@headlessui-react/src/components/popover/popover.tsx

@@ -258,16 +258,18 @@ let PopoverRoot = forwardRefWithAs(function Popover<
   )
 
   // Handle outside click
-  useOutsideClick([button, panel], (event, target) => {
-    if (popoverState !== PopoverStates.Open) return
-
-    dispatch({ type: ActionTypes.ClosePopover })
+  useOutsideClick(
+    [button, panel],
+    (event, target) => {
+      dispatch({ type: ActionTypes.ClosePopover })
 
-    if (!isFocusableElement(target, FocusableMode.Loose)) {
-      event.preventDefault()
-      button?.focus()
-    }
-  })
+      if (!isFocusableElement(target, FocusableMode.Loose)) {
+        event.preventDefault()
+        button?.focus()
+      }
+    },
+    popoverState === PopoverStates.Open
+  )
 
   let close = useEvent((focusableElement?: HTMLElement | MutableRefObject<HTMLElement | null>) => {
     dispatch({ type: ActionTypes.ClosePopover })

packages/@headlessui-react/src/components/portal/portal.test.tsx

@@ -299,6 +299,6 @@ it('should be possible to force the Portal into a specific element using Portal.
   render(<Example />)
 
   expect(document.body.innerHTML).toMatchInlineSnapshot(
-    `"<div><main><aside id=\\"group-1\\">A<div>Next to A</div></aside><section id=\\"group-2\\"><span>B</span></section></main></div><div id=\\"headlessui-portal-root\\"><div>I am in the portal root</div></div>"`
+    `"<div><main><aside id=\\"group-1\\">A<div data-headlessui-portal=\\"\\">Next to A</div></aside><section id=\\"group-2\\"><span>B</span></section></main></div><div id=\\"headlessui-portal-root\\"><div data-headlessui-portal=\\"\\">I am in the portal root</div></div>"`
   )
 })

packages/@headlessui-react/src/components/portal/portal.tsx

@@ -95,6 +95,7 @@ let PortalRoot = forwardRefWithAs(function Portal<
     // Element already exists in target, always calling target.appendChild(element) will cause a
     // brief unmount/remount.
     if (!target.contains(element)) {
+      element.setAttribute('data-headlessui-portal', '')
       target.appendChild(element)
     }