From bdd1b3b7856c30c59a77b22f18f7227f0c0499fa Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Fri, 3 Jun 2022 16:20:56 +0200 Subject: [PATCH] Improve outside click of `Dialog` component (#1546) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 `
` 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 Button ... ``` 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 --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/combobox/combobox.tsx | 12 +- .../src/components/dialog/dialog.test.tsx | 8 +- .../src/components/dialog/dialog.tsx | 45 +++--- .../src/components/focus-trap/focus-trap.tsx | 5 +- .../src/components/listbox/listbox.tsx | 20 +-- .../src/components/menu/menu.tsx | 21 +-- .../src/components/popover/popover.tsx | 20 +-- .../src/components/portal/portal.test.tsx | 2 +- .../src/components/portal/portal.tsx | 1 + .../src/hooks/use-outside-click.ts | 129 ++++++++++-------- packages/@headlessui-vue/CHANGELOG.md | 1 + .../src/components/combobox/combobox.ts | 9 +- .../src/components/dialog/dialog.ts | 19 ++- .../src/components/listbox/listbox.ts | 20 +-- .../src/components/menu/menu.ts | 21 +-- .../src/components/popover/popover.ts | 20 +-- .../src/components/portal/portal.test.ts | 2 +- .../src/components/portal/portal.ts | 1 + .../src/hooks/use-outside-click.ts | 115 ++++++++-------- packages/playground-react/package.json | 1 + .../pages/combinations/tabs-in-dialog.tsx | 6 +- .../playground-react/pages/dialog/dialog.tsx | 13 +- .../pages/dialog/scrollable-dialog.tsx | 98 +++++++++++++ yarn.lock | 5 + 25 files changed, 361 insertions(+), 234 deletions(-) create mode 100644 packages/playground-react/pages/dialog/scrollable-dialog.tsx diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index e40f5d50a..970b37e4c 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/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 diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 1e68c36cb..e00d55fdd 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/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>( () => ({ diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index c59e3041c..21971b840 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -889,9 +889,11 @@ describe('Mouse interactions', () => { return (
- Contents - - + + Contents + + +
) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index 8d4b42cec..351c28ad2 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/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 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({ diff --git a/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx b/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx index 0cdf662b6..bbab04b21 100644 --- a/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx +++ b/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]) diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index b6643312d..1c1c6472f 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/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( () => ({ open: listboxState === ListboxStates.Open, disabled }), diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 1dae520d4..2c7e3d1ac 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/packages/@headlessui-react/src/components/menu/menu.tsx @@ -239,16 +239,18 @@ let MenuRoot = forwardRefWithAs(function Menu { - 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( () => ({ open: menuState === MenuStates.Open }), @@ -344,7 +346,6 @@ let Button = forwardRefWithAs(function Button state.buttonRef.current?.focus({ preventScroll: true })) } else { event.preventDefault() - event.stopPropagation() dispatch({ type: ActionTypes.OpenMenu }) } }) diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx index a1fa990ee..9d9314fb1 100644 --- a/packages/@headlessui-react/src/components/popover/popover.tsx +++ b/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) => { dispatch({ type: ActionTypes.ClosePopover }) diff --git a/packages/@headlessui-react/src/components/portal/portal.test.tsx b/packages/@headlessui-react/src/components/portal/portal.test.tsx index 6b21ea7a5..d434dd608 100644 --- a/packages/@headlessui-react/src/components/portal/portal.test.tsx +++ b/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() expect(document.body.innerHTML).toMatchInlineSnapshot( - `"
B
I am in the portal root
"` + `"
B
I am in the portal root
"` ) }) diff --git a/packages/@headlessui-react/src/components/portal/portal.tsx b/packages/@headlessui-react/src/components/portal/portal.tsx index c6b55d817..86bdba171 100644 --- a/packages/@headlessui-react/src/components/portal/portal.tsx +++ b/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) } diff --git a/packages/@headlessui-react/src/hooks/use-outside-click.ts b/packages/@headlessui-react/src/hooks/use-outside-click.ts index 8cef785e0..3b48fc40f 100644 --- a/packages/@headlessui-react/src/hooks/use-outside-click.ts +++ b/packages/@headlessui-react/src/hooks/use-outside-click.ts @@ -1,82 +1,93 @@ -import { MutableRefObject, useRef } from 'react' -import { microTask } from '../utils/micro-task' -import { useEvent } from './use-event' +import { MutableRefObject, useEffect, useRef } from 'react' +import { FocusableMode, isFocusableElement } from '../utils/focus-management' import { useWindowEvent } from './use-window-event' type Container = MutableRefObject | HTMLElement | null type ContainerCollection = Container[] | Set type ContainerInput = Container | ContainerCollection -export enum Features { - None = 1 << 0, - IgnoreScrollbars = 1 << 1, -} - export function useOutsideClick( containers: ContainerInput | (() => ContainerInput), cb: (event: MouseEvent | PointerEvent, target: HTMLElement) => void, - features: Features = Features.None + enabled: boolean = true ) { - let called = useRef(false) - let handler = useEvent((event: MouseEvent | PointerEvent) => { - if (called.current) return - called.current = true - microTask(() => { - called.current = false - }) + // TODO: remove this once the React bug has been fixed: https://github.com/facebook/react/issues/24657 + let enabledRef = useRef(false) + useEffect( + process.env.NODE_ENV === 'test' + ? () => { + enabledRef.current = enabled + } + : () => { + requestAnimationFrame(() => { + enabledRef.current = enabled + }) + }, + [enabled] + ) - let _containers = (function resolve(containers): ContainerCollection { - if (typeof containers === 'function') { - return resolve(containers()) - } + useWindowEvent( + 'click', + (event) => { + if (!enabledRef.current) return - if (Array.isArray(containers)) { - return containers - } + // Check whether the event got prevented already. This can happen if you use the + // useOutsideClick hook in both a Dialog and a Menu and the inner Menu "cancels" the default + // behaviour so that only the Menu closes and not the Dialog (yet) + if (event.defaultPrevented) return - if (containers instanceof Set) { - return containers - } + let _containers = (function resolve(containers): ContainerCollection { + if (typeof containers === 'function') { + return resolve(containers()) + } - return [containers] - })(containers) + if (Array.isArray(containers)) { + return containers + } - let target = event.target as HTMLElement + if (containers instanceof Set) { + return containers + } - // Ignore if the target doesn't exist in the DOM anymore - if (!target.ownerDocument.documentElement.contains(target)) return + return [containers] + })(containers) - // Ignore scrollbars: - // This is a bit hacky, and is only necessary because we are checking for `pointerdown` and - // `mousedown` events. They _are_ being called if you click on a scrollbar. The `click` event - // is not called when clicking on a scrollbar, but we can't use that otherwise it won't work - // on mobile devices where only pointer events are being used. - if ((features & Features.IgnoreScrollbars) === Features.IgnoreScrollbars) { - // TODO: We can calculate this dynamically~is. On macOS if you have the "Automatically based - // on mouse or trackpad" setting enabled, then the scrollbar will float on top and therefore - // you can't calculate its with by checking the clientWidth and scrollWidth of the element. - // Therefore we are currently hardcoding this to be 20px. - let scrollbarWidth = 20 + let target = event.target as HTMLElement - let viewport = target.ownerDocument.documentElement - if (event.clientX > viewport.clientWidth - scrollbarWidth) return - if (event.clientX < scrollbarWidth) return - if (event.clientY > viewport.clientHeight - scrollbarWidth) return - if (event.clientY < scrollbarWidth) return - } + // Ignore if the target doesn't exist in the DOM anymore + if (!target.ownerDocument.documentElement.contains(target)) return - // Ignore if the target exists in one of the containers - for (let container of _containers) { - if (container === null) continue - let domNode = container instanceof HTMLElement ? container : container.current - if (domNode?.contains(target)) { - return + // Ignore if the target exists in one of the containers + for (let container of _containers) { + if (container === null) continue + let domNode = container instanceof HTMLElement ? container : container.current + if (domNode?.contains(target)) { + return + } } - } - return cb(event, target) - }) + // This allows us to check whether the event was defaultPrevented when you are nesting this + // inside a `` for example. + if ( + // This check alllows us to know whether or not we clicked on a "focusable" element like a + // button or an input. This is a backwards compatibility check so that you can open a and click on another which should close Menu A and open Menu B. We might + // revisit that so that you will require 2 clicks instead. + !isFocusableElement(target, FocusableMode.Loose) && + // This could be improved, but the `Combobox.Button` adds tabIndex={-1} to make it + // unfocusable via the keyboard so that tabbing to the next item from the input doesn't + // first go to the button. + target.tabIndex !== -1 + ) { + event.preventDefault() + } - useWindowEvent('pointerdown', handler) - useWindowEvent('mousedown', handler) + return cb(event, target) + }, + // We will use the `capture` phase so that layers in between with `event.stopPropagation()` + // don't "cancel" this outside click check. E.g.: A `Menu` inside a `DialogPanel` if the `Menu` + // is open, and you click outside of it in the `DialogPanel` the `Menu` should close. However, + // the `DialogPanel` has a `onClick(e) { e.stopPropagation() }` which would cancel this. + true + ) } diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 4c6b3d963..3eb8a3064 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Support `` children when using `as="template"` ([#1548](https://github.com/tailwindlabs/headlessui/pull/1548)) +- Improve outside click of `Dialog` component ([#1546](https://github.com/tailwindlabs/headlessui/pull/1546)) ## [1.6.4] - 2022-05-29 diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index 76bb0c425..4bdb9c193 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -396,10 +396,11 @@ export let Combobox = defineComponent({ } // Handle outside click - useOutsideClick([inputRef, buttonRef, optionsRef], () => { - if (comboboxState.value !== ComboboxStates.Open) return - api.closeCombobox() - }) + useOutsideClick( + [inputRef, buttonRef, optionsRef], + () => api.closeCombobox(), + computed(() => comboboxState.value === ComboboxStates.Open) + ) watch([api.value, api.inputRef], () => api.syncInputValue(), { immediate: true, diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.ts b/packages/@headlessui-vue/src/components/dialog/dialog.ts index 0845d073f..dd1b24c7f 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.ts @@ -29,7 +29,7 @@ import { ForcePortalRoot } from '../../internal/portal-force-root' import { Description, useDescriptions } from '../description/description' import { dom } from '../../utils/dom' import { useOpenClosed, State } from '../../internal/open-closed' -import { useOutsideClick, Features as OutsideClickFeatures } from '../../hooks/use-outside-click' +import { useOutsideClick } from '../../hooks/use-outside-click' import { getOwnerDocument } from '../../utils/owner' import { useEventListener } from '../../hooks/use-event-listener' import { Hidden, Features as HiddenFeatures } from '../../internal/hidden' @@ -179,7 +179,7 @@ export let Dialog = defineComponent({ () => { // Third party roots let rootContainers = Array.from( - ownerDocument.value?.querySelectorAll('body > *') ?? [] + ownerDocument.value?.querySelectorAll('body > *, [data-headlessui-portal]') ?? [] ).filter((container) => { if (!(container instanceof HTMLElement)) return false // Skip non-HTMLElements if (container.contains(dom(mainTreeNode))) return false // Skip if it is the main app @@ -191,13 +191,10 @@ export let Dialog = defineComponent({ }, (_event, target) => { - if (dialogState.value !== DialogStates.Open) return - if (hasNestedDialogs.value) return - api.close() nextTick(() => target?.focus()) }, - OutsideClickFeatures.IgnoreScrollbars + computed(() => dialogState.value === DialogStates.Open && !hasNestedDialogs.value) ) // Handle `Escape` to close @@ -264,10 +261,6 @@ export let Dialog = defineComponent({ onInvalidate(() => observer.disconnect()) }) - function handleClick(event: MouseEvent) { - event.stopPropagation() - } - return () => { let ourProps = { // Manually passthrough the attributes, because Vue can't automatically pass @@ -279,7 +272,6 @@ export let Dialog = defineComponent({ 'aria-modal': dialogState.value === DialogStates.Open ? true : undefined, 'aria-labelledby': titleId.value, 'aria-describedby': describedby.value, - onClick: handleClick, } let { open: _, initialFocus, ...incomingProps } = props @@ -417,10 +409,15 @@ export let DialogPanel = defineComponent({ expose({ el: api.panelRef, $el: api.panelRef }) + function handleClick(event: MouseEvent) { + event.stopPropagation() + } + return () => { let ourProps = { id, ref: api.panelRef, + onClick: handleClick, } let incomingProps = props diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.ts b/packages/@headlessui-vue/src/components/listbox/listbox.ts index b3a123ab3..5bd9062a1 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.ts +++ b/packages/@headlessui-vue/src/components/listbox/listbox.ts @@ -298,16 +298,18 @@ export let Listbox = defineComponent({ } // Handle outside click - useOutsideClick([buttonRef, optionsRef], (event, target) => { - if (listboxState.value !== ListboxStates.Open) return - - api.closeListbox() + useOutsideClick( + [buttonRef, optionsRef], + (event, target) => { + api.closeListbox() - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - dom(buttonRef)?.focus() - } - }) + if (!isFocusableElement(target, FocusableMode.Loose)) { + event.preventDefault() + dom(buttonRef)?.focus() + } + }, + computed(() => listboxState.value === ListboxStates.Open) + ) // @ts-expect-error Types of property 'dataRef' are incompatible. provide(ListboxContext, api) diff --git a/packages/@headlessui-vue/src/components/menu/menu.ts b/packages/@headlessui-vue/src/components/menu/menu.ts index e34a83146..4a4a54322 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.ts +++ b/packages/@headlessui-vue/src/components/menu/menu.ts @@ -201,16 +201,18 @@ export let Menu = defineComponent({ } // Handle outside click - useOutsideClick([buttonRef, itemsRef], (event, target) => { - if (menuState.value !== MenuStates.Open) return - - api.closeMenu() + useOutsideClick( + [buttonRef, itemsRef], + (event, target) => { + api.closeMenu() - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - dom(buttonRef)?.focus() - } - }) + if (!isFocusableElement(target, FocusableMode.Loose)) { + event.preventDefault() + dom(buttonRef)?.focus() + } + }, + computed(() => menuState.value === MenuStates.Open) + ) // @ts-expect-error Types of property 'dataRef' are incompatible. provide(MenuContext, api) @@ -289,7 +291,6 @@ export let MenuButton = defineComponent({ nextTick(() => dom(api.buttonRef)?.focus({ preventScroll: true })) } else { event.preventDefault() - event.stopPropagation() api.openMenu() nextFrame(() => dom(api.itemsRef)?.focus({ preventScroll: true })) } diff --git a/packages/@headlessui-vue/src/components/popover/popover.ts b/packages/@headlessui-vue/src/components/popover/popover.ts index fc9a26f21..d79f64587 100644 --- a/packages/@headlessui-vue/src/components/popover/popover.ts +++ b/packages/@headlessui-vue/src/components/popover/popover.ts @@ -211,16 +211,18 @@ export let Popover = defineComponent({ ) // Handle outside click - useOutsideClick([button, panel], (event, target) => { - if (popoverState.value !== PopoverStates.Open) return - - api.closePopover() + useOutsideClick( + [button, panel], + (event, target) => { + api.closePopover() - if (!isFocusableElement(target, FocusableMode.Loose)) { - event.preventDefault() - dom(button)?.focus() - } - }) + if (!isFocusableElement(target, FocusableMode.Loose)) { + event.preventDefault() + dom(button)?.focus() + } + }, + computed(() => popoverState.value === PopoverStates.Open) + ) return () => { let slot = { open: popoverState.value === PopoverStates.Open, close: api.close } diff --git a/packages/@headlessui-vue/src/components/portal/portal.test.ts b/packages/@headlessui-vue/src/components/portal/portal.test.ts index ff8f2c942..0425e4588 100644 --- a/packages/@headlessui-vue/src/components/portal/portal.test.ts +++ b/packages/@headlessui-vue/src/components/portal/portal.test.ts @@ -389,6 +389,6 @@ it('should be possible to force the Portal into a specific element using PortalG await new Promise(nextTick) expect(document.body.innerHTML).toMatchInlineSnapshot( - `"
B
"` + `"
B
"` ) }) diff --git a/packages/@headlessui-vue/src/components/portal/portal.ts b/packages/@headlessui-vue/src/components/portal/portal.ts index 3ee164d0b..fc8e1aabe 100644 --- a/packages/@headlessui-vue/src/components/portal/portal.ts +++ b/packages/@headlessui-vue/src/components/portal/portal.ts @@ -79,6 +79,7 @@ export let Portal = defineComponent({ let ourProps = { ref: element, + 'data-headlessui-portal': '', } return h( diff --git a/packages/@headlessui-vue/src/hooks/use-outside-click.ts b/packages/@headlessui-vue/src/hooks/use-outside-click.ts index fc2891b7a..562e81d69 100644 --- a/packages/@headlessui-vue/src/hooks/use-outside-click.ts +++ b/packages/@headlessui-vue/src/hooks/use-outside-click.ts @@ -1,82 +1,79 @@ import { useWindowEvent } from './use-window-event' -import { Ref } from 'vue' +import { computed, Ref, ComputedRef } from 'vue' +import { FocusableMode, isFocusableElement } from '../utils/focus-management' import { dom } from '../utils/dom' -import { microTask } from '../utils/micro-task' type Container = Ref | HTMLElement | null type ContainerCollection = Container[] | Set type ContainerInput = Container | ContainerCollection -export enum Features { - None = 1 << 0, - IgnoreScrollbars = 1 << 1, -} - export function useOutsideClick( containers: ContainerInput | (() => ContainerInput), cb: (event: MouseEvent | PointerEvent, target: HTMLElement) => void, - features: Features = Features.None + enabled: ComputedRef = computed(() => true) ) { - let called = false - function handle(event: MouseEvent | PointerEvent) { - if (called) return - called = true - microTask(() => { - called = false - }) + useWindowEvent( + 'click', + (event) => { + if (!enabled.value) return - let target = event.target as HTMLElement + // Check whether the event got prevented already. This can happen if you use the + // useOutsideClick hook in both a Dialog and a Menu and the inner Menu "cancels" the default + // behaviour so that only the Menu closes and not the Dialog (yet) + if (event.defaultPrevented) return - // Ignore if the target doesn't exist in the DOM anymore - if (!target.ownerDocument.documentElement.contains(target)) return + let target = event.target as HTMLElement - let _containers = (function resolve(containers): ContainerCollection { - if (typeof containers === 'function') { - return resolve(containers()) - } + // Ignore if the target doesn't exist in the DOM anymore + if (!target.ownerDocument.documentElement.contains(target)) return - if (Array.isArray(containers)) { - return containers - } + let _containers = (function resolve(containers): ContainerCollection { + if (typeof containers === 'function') { + return resolve(containers()) + } - if (containers instanceof Set) { - return containers - } - - return [containers] - })(containers) + if (Array.isArray(containers)) { + return containers + } - // Ignore scrollbars: - // This is a bit hacky, and is only necessary because we are checking for `pointerdown` and - // `mousedown` events. They _are_ being called if you click on a scrollbar. The `click` event - // is not called when clicking on a scrollbar, but we can't use that otherwise it won't work - // on mobile devices where only pointer events are being used. - if ((features & Features.IgnoreScrollbars) === Features.IgnoreScrollbars) { - // TODO: We can calculate this dynamically~is. On macOS if you have the "Automatically based - // on mouse or trackpad" setting enabled, then the scrollbar will float on top and therefore - // you can't calculate its with by checking the clientWidth and scrollWidth of the element. - // Therefore we are currently hardcoding this to be 20px. - let scrollbarWidth = 20 + if (containers instanceof Set) { + return containers + } - let viewport = target.ownerDocument.documentElement - if (event.clientX > viewport.clientWidth - scrollbarWidth) return - if (event.clientX < scrollbarWidth) return - if (event.clientY > viewport.clientHeight - scrollbarWidth) return - if (event.clientY < scrollbarWidth) return - } + return [containers] + })(containers) - // Ignore if the target exists in one of the containers - for (let container of _containers) { - if (container === null) continue - let domNode = container instanceof HTMLElement ? container : dom(container) - if (domNode?.contains(target)) { - return + // Ignore if the target exists in one of the containers + for (let container of _containers) { + if (container === null) continue + let domNode = container instanceof HTMLElement ? container : dom(container) + if (domNode?.contains(target)) { + return + } } - } - cb(event, target) - } + // This allows us to check whether the event was defaultPrevented when you are nesting this + // inside a `` for example. + if ( + // This check alllows us to know whether or not we clicked on a "focusable" element like a + // button or an input. This is a backwards compatibility check so that you can open a and click on another which should close Menu A and open Menu B. We might + // revisit that so that you will require 2 clicks instead. + !isFocusableElement(target, FocusableMode.Loose) && + // This could be improved, but the `Combobox.Button` adds tabIndex={-1} to make it + // unfocusable via the keyboard so that tabbing to the next item from the input doesn't + // first go to the button. + target.tabIndex !== -1 + ) { + event.preventDefault() + } - useWindowEvent('pointerdown', handle) - useWindowEvent('mousedown', handle) + cb(event, target) + }, + // We will use the `capture` phase so that layers in between with `event.stopPropagation()` + // don't "cancel" this outside click check. E.g.: A `Menu` inside a `DialogPanel` if the `Menu` + // is open, and you click outside of it in the `DialogPanel` the `Menu` should close. However, + // the `DialogPanel` has a `onClick(e) { e.stopPropagation() }` which would cancel this. + true + ) } diff --git a/packages/playground-react/package.json b/packages/playground-react/package.json index bd8d99a1b..bcd4afebc 100644 --- a/packages/playground-react/package.json +++ b/packages/playground-react/package.json @@ -16,6 +16,7 @@ "dependencies": { "@headlessui/react": "*", "@headlessui/tailwindcss": "*", + "@heroicons/react": "^1.0.6", "@popperjs/core": "^2.6.0", "@tailwindcss/forms": "^0.5.2", "@tailwindcss/typography": "^0.5.2", diff --git a/packages/playground-react/pages/combinations/tabs-in-dialog.tsx b/packages/playground-react/pages/combinations/tabs-in-dialog.tsx index bca77d568..1bf057c9c 100644 --- a/packages/playground-react/pages/combinations/tabs-in-dialog.tsx +++ b/packages/playground-react/pages/combinations/tabs-in-dialog.tsx @@ -8,8 +8,8 @@ export default function App() { <> - -
+
+
@@ -24,7 +24,7 @@ export default function App() {
-
+
) diff --git a/packages/playground-react/pages/dialog/dialog.tsx b/packages/playground-react/pages/dialog/dialog.tsx index 9e4f25f32..527484f2f 100644 --- a/packages/playground-react/pages/dialog/dialog.tsx +++ b/packages/playground-react/pages/dialog/dialog.tsx @@ -85,7 +85,12 @@ export default function Home() { beforeLeave={() => console.log('[Transition] Before leave')} afterLeave={() => console.log('[Transition] After leave')} > - + { + console.log('close') + setIsOpen(false) + }} + >
console.log('[Transition.Child] [Overlay] Before leave')} afterLeave={() => console.log('[Transition.Child] [Overlay] After leave')} > - +
​ -
+
@@ -254,7 +259,7 @@ export default function Home() { Cancel
-
+
diff --git a/packages/playground-react/pages/dialog/scrollable-dialog.tsx b/packages/playground-react/pages/dialog/scrollable-dialog.tsx new file mode 100644 index 000000000..aa382e673 --- /dev/null +++ b/packages/playground-react/pages/dialog/scrollable-dialog.tsx @@ -0,0 +1,98 @@ +import { Fragment, useRef, useState } from 'react' +import { Dialog, Transition } from '@headlessui/react' +import { ExclamationIcon } from '@heroicons/react/outline' + +export default function Example() { + const [open, setOpen] = useState(false) + + const cancelButtonRef = useRef(null) + + return ( + <> +
+ +
+ + + +
+ + +
+
+ + +
+
+
+
+ + Deactivate account + +
+

+ Are you sure you want to deactivate your account? All of your data will be + permanently removed from our servers forever. This action cannot be + undone. +

+ {Array(20) + .fill(null) + .map((_, i) => ( +

+ Are you sure you want to deactivate your account? All of your data + will be permanently removed from our servers forever. This action + cannot be undone. +

+ ))} +
+
+
+
+ + +
+ + +
+
+
+ + + ) +} diff --git a/yarn.lock b/yarn.lock index 7aa92138f..5bf11e631 100644 --- a/yarn.lock +++ b/yarn.lock @@ -326,6 +326,11 @@ resolved "https://registry.yarnpkg.com/@emotion/memoize/-/memoize-0.7.4.tgz#19bf0f5af19149111c40d98bb0cf82119f5d9eeb" integrity sha512-Ja/Vfqe3HpuzRsG1oBtWTHk2PGZ7GR+2Vz5iYGelAw8dx32K0y7PjVuxK6z1nMpZOqAFsRUPCkK1YjJ56qJlgw== +"@heroicons/react@^1.0.6": + version "1.0.6" + resolved "https://registry.yarnpkg.com/@heroicons/react/-/react-1.0.6.tgz#35dd26987228b39ef2316db3b1245c42eb19e324" + integrity sha512-JJCXydOFWMDpCP4q13iEplA503MQO3xLoZiKum+955ZCtHINWnx26CUxVxxFQu/uLb4LW3ge15ZpzIkXKkJ8oQ== + "@heroicons/vue@^1.0.6": version "1.0.6" resolved "https://registry.yarnpkg.com/@heroicons/vue/-/vue-1.0.6.tgz#d8b90734b436eb5a87f40cc300b64a0fb0031f7f"