From f1daa1e52b6e109bd824fde39629fec0a01c4f88 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Thu, 14 Jul 2022 14:20:04 -0400 Subject: [PATCH] Adjust outside click handling (#1667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Don’t close dialog if opened during mouse up event * Don’t close dialog if drag starts inside dialog and ends outside dialog * Handle closing of nested dialogs that are always mounted * Fix focus trap restoration in Vue * Update changelog --- packages/@headlessui-react/CHANGELOG.md | 3 + .../src/components/dialog/dialog.test.tsx | 124 ++++++++++++++-- .../src/components/dialog/dialog.tsx | 1 + .../src/hooks/use-outside-click.ts | 24 +++- .../src/internal/stack-context.tsx | 13 +- .../src/test-utils/interactions.ts | 68 +++++++++ packages/@headlessui-vue/CHANGELOG.md | 3 + .../src/components/dialog/dialog.test.ts | 132 ++++++++++++++++-- .../src/components/dialog/dialog.ts | 1 + .../src/components/focus-trap/focus-trap.ts | 47 +++---- .../src/hooks/use-outside-click.ts | 26 +++- .../src/internal/stack-context.ts | 23 ++- .../src/test-utils/interactions.ts | 68 +++++++++ 13 files changed, 471 insertions(+), 62 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 0c748df..3e94bf5 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed SSR support on Deno ([#1671](https://github.com/tailwindlabs/headlessui/pull/1671)) +- Don’t close dialog when opened during mouse up event ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667)) +- Don’t close dialog when drag ends outside dialog ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667)) +- Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667)) ## [1.6.6] - 2022-07-07 diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index 21971b8..fe12492 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -17,7 +17,7 @@ import { getDialogs, getDialogOverlays, } from '../../test-utils/accessibility-assertions' -import { click, press, Keys } from '../../test-utils/interactions' +import { click, mouseDrag, press, Keys } from '../../test-utils/interactions' import { PropsOf } from '../../types' import { Transition } from '../transitions/transition' import { createPortal } from 'react-dom' @@ -1066,14 +1066,101 @@ describe('Mouse interactions', () => { assertDialog({ state: DialogState.Visible }) }) ) + + it( + 'should not close the dialog if opened during mouse up', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + + + + + + + + + + ) + } + + render() + + await click(document.getElementById('trigger')) + + assertDialog({ state: DialogState.Visible }) + + await click(document.getElementById('inside')) + + assertDialog({ state: DialogState.Visible }) + }) + ) + + it( + 'should not close the dialog if click starts inside the dialog but ends outside', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + +
this thing
+ + + + + + + + + ) + } + + render() + + // Open the dialog + await click(document.getElementById('trigger')) + + assertDialog({ state: DialogState.Visible }) + + // Start a click inside the dialog and end it outside + await mouseDrag(document.getElementById('inside'), document.getElementById('imoutside')) + + // It should not have hidden + assertDialog({ state: DialogState.Visible }) + + await click(document.getElementById('imoutside')) + + // It's gone + assertDialog({ state: DialogState.InvisibleUnmounted }) + }) + ) }) describe('Nesting', () => { - function Nested({ onClose, level = 1 }: { onClose: (value: boolean) => void; level?: number }) { + type RenderStrategy = 'mounted' | 'always' + + function Nested({ + onClose, + open = true, + level = 1, + renderWhen = 'mounted', + }: { + onClose: (value: boolean) => void + open?: boolean + level?: number + renderWhen?: RenderStrategy + }) { let [showChild, setShowChild] = useState(false) return ( - +
@@ -1082,31 +1169,42 @@ describe('Nesting', () => {
- {showChild && } + {renderWhen === 'always' ? ( + + ) : ( + showChild && + )} ) } - function Example() { + function Example({ renderWhen = 'mounted' }: { renderWhen: RenderStrategy }) { let [open, setOpen] = useState(false) return ( <> - {open && } + {open && } ) } it.each` - strategy | action - ${'with `Escape`'} | ${() => press(Keys.Escape)} - ${'with `Outside Click`'} | ${() => click(document.body)} - ${'with `Click on Dialog.Overlay`'} | ${() => click(getDialogOverlays().pop()!)} + strategy | when | action + ${'with `Escape`'} | ${'mounted'} | ${() => press(Keys.Escape)} + ${'with `Outside Click`'} | ${'mounted'} | ${() => click(document.body)} + ${'with `Click on Dialog.Overlay`'} | ${'mounted'} | ${() => click(getDialogOverlays().pop()!)} + ${'with `Escape`'} | ${'always'} | ${() => press(Keys.Escape)} + ${'with `Outside Click`'} | ${'always'} | ${() => click(document.body)} `( - 'should be possible to open nested Dialog components and close them $strategy', - async ({ action }) => { - render() + 'should be possible to open nested Dialog components (visible when $when) and close them $strategy', + async ({ when, action }) => { + render() // Verify we have no open dialogs expect(getDialogs()).toHaveLength(0) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index 351c28a..db16df7 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -305,6 +305,7 @@ let DialogRoot = forwardRefWithAs(function Dialog< return ( { if (type !== 'Dialog') return diff --git a/packages/@headlessui-react/src/hooks/use-outside-click.ts b/packages/@headlessui-react/src/hooks/use-outside-click.ts index d358023..30c5e8d 100644 --- a/packages/@headlessui-react/src/hooks/use-outside-click.ts +++ b/packages/@headlessui-react/src/hooks/use-outside-click.ts @@ -90,9 +90,31 @@ export function useOutsideClick( return cb(event, target) } + let initialClickTarget = useRef(null) + + useWindowEvent( + 'mousedown', + (event) => { + if (enabledRef.current) { + initialClickTarget.current = event.target + } + }, + true + ) + useWindowEvent( 'click', - (event) => handleOutsideClick(event, (event) => event.target as HTMLElement), + (event) => { + if (!initialClickTarget.current) { + return + } + + handleOutsideClick(event, () => { + return initialClickTarget.current as HTMLElement + }) + + initialClickTarget.current = null + }, // 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` diff --git a/packages/@headlessui-react/src/internal/stack-context.tsx b/packages/@headlessui-react/src/internal/stack-context.tsx index 6db45db..e8ff63d 100644 --- a/packages/@headlessui-react/src/internal/stack-context.tsx +++ b/packages/@headlessui-react/src/internal/stack-context.tsx @@ -32,11 +32,13 @@ export function StackProvider({ onUpdate, type, element, + enabled, }: { children: ReactNode onUpdate?: OnUpdate type: string element: MutableRefObject + enabled?: boolean }) { let parentUpdate = useStackContext() @@ -49,9 +51,14 @@ export function StackProvider({ }) useIsoMorphicEffect(() => { - notify(StackMessage.Add, type, element) - return () => notify(StackMessage.Remove, type, element) - }, [notify, type, element]) + let shouldNotify = enabled === undefined || enabled === true + + shouldNotify && notify(StackMessage.Add, type, element) + + return () => { + shouldNotify && notify(StackMessage.Remove, type, element) + } + }, [notify, type, element, enabled]) return {children} } diff --git a/packages/@headlessui-react/src/test-utils/interactions.ts b/packages/@headlessui-react/src/test-utils/interactions.ts index 2b7976d..2a71b96 100644 --- a/packages/@headlessui-react/src/test-utils/interactions.ts +++ b/packages/@headlessui-react/src/test-utils/interactions.ts @@ -344,6 +344,74 @@ export async function mouseLeave(element: Document | Element | Window | null) { } } +export async function mouseDrag( + startingElement: Document | Element | Window | Node | null, + endingElement: Document | Element | Window | Node | null +) { + let button = MouseButton.Left + + try { + if (startingElement === null) return expect(startingElement).not.toBe(null) + if (endingElement === null) return expect(endingElement).not.toBe(null) + if (startingElement instanceof HTMLButtonElement && startingElement.disabled) return + + let options = { button } + + // Cancel in pointerDown cancels mouseDown, mouseUp + let cancelled = !fireEvent.pointerDown(startingElement, options) + + if (!cancelled) { + cancelled = !fireEvent.mouseDown(startingElement, options) + } + + // Ensure to trigger a `focus` event if the element is focusable, or within a focusable element + if (!cancelled) { + let next: HTMLElement | null = startingElement as HTMLElement | null + while (next !== null) { + if (next.matches(focusableSelector)) { + next.focus() + break + } + next = next.parentElement + } + } + + fireEvent.pointerMove(startingElement, options) + if (!cancelled) { + fireEvent.mouseMove(startingElement, options) + } + + fireEvent.pointerOut(startingElement, options) + if (!cancelled) { + fireEvent.mouseOut(startingElement, options) + } + + // crosses over to the ending element + + fireEvent.pointerOver(endingElement, options) + if (!cancelled) { + fireEvent.mouseOver(endingElement, options) + } + + fireEvent.pointerMove(endingElement, options) + if (!cancelled) { + fireEvent.mouseMove(endingElement, options) + } + + fireEvent.pointerUp(endingElement, options) + if (!cancelled) { + fireEvent.mouseUp(endingElement, options) + } + + fireEvent.click(endingElement, options) + + await new Promise(nextFrame) + } catch (err) { + if (err instanceof Error) Error.captureStackTrace(err, click) + throw err + } +} + // --- function focusNext(event: Partial) { diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 7638e9d..67496d5 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed SSR support on Deno ([#1671](https://github.com/tailwindlabs/headlessui/pull/1671)) +- Don’t close dialog when opened during mouse up event ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667)) +- Don’t close dialog when drag ends outside dialog ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667)) +- Fix outside clicks to close dialog when nested, unopened dialogs are present ([#1667](https://github.com/tailwindlabs/headlessui/pull/1667)) ## [1.6.7] - 2022-07-12 diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index c5d9b37..3b76267 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -25,7 +25,7 @@ import { getDialogs, getDialogOverlays, } from '../../test-utils/accessibility-assertions' -import { click, press, Keys } from '../../test-utils/interactions' +import { click, mouseDrag, press, Keys } from '../../test-utils/interactions' import { html } from '../../test-utils/html' // @ts-expect-error @@ -1444,13 +1444,106 @@ describe('Mouse interactions', () => { assertDialog({ state: DialogState.Visible }) }) ) + + it( + 'should not close the dialog if opened during mouse up', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + + + + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + } + }, + }) + + await click(document.getElementById('trigger')) + + assertDialog({ state: DialogState.Visible }) + + await click(document.getElementById('inside')) + + assertDialog({ state: DialogState.Visible }) + }) + ) + + it( + 'should not close the dialog if click starts inside the dialog but ends outside', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ +
this thing
+ + + + + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + } + }, + }) + + // Open the dialog + await click(document.getElementById('trigger')) + + assertDialog({ state: DialogState.Visible }) + + // Start a click inside the dialog and end it outside + await mouseDrag(document.getElementById('inside'), document.getElementById('imoutside')) + + // It should not have hidden + assertDialog({ state: DialogState.Visible }) + + await click(document.getElementById('imoutside')) + + // It's gone + assertDialog({ state: DialogState.InvisibleUnmounted }) + }) + ) }) describe('Nesting', () => { let Nested: ConcreteComponent = defineComponent({ components: { Dialog, DialogOverlay }, emits: ['close'], - props: ['level'], + props: ['open', 'level', 'renderWhen'], setup(props, { emit }) { let showChild = ref(false) function onClose() { @@ -1459,7 +1552,7 @@ describe('Nesting', () => { return () => { let level = props.level ?? 1 - return h(Dialog, { open: true, onClose }, () => [ + return h(Dialog, { open: props.open ?? true, onClose }, () => [ h(DialogOverlay), h('div', [ h('p', `Level: ${level}`), @@ -1467,29 +1560,40 @@ describe('Nesting', () => { h('button', { onClick: () => (showChild.value = true) }, `Open ${level + 1} b`), h('button', { onClick: () => (showChild.value = true) }, `Open ${level + 1} c`), ]), - showChild.value && - h(Nested, { - onClose: () => (showChild.value = false), - level: level + 1, - }), + props.renderWhen === 'always' + ? h(Nested, { + open: showChild.value, + onClose: () => (showChild.value = false), + level: level + 1, + renderWhen: props.renderWhen, + }) + : showChild.value && + h(Nested, { + open: true, + onClose: () => (showChild.value = false), + level: level + 1, + renderWhen: props.renderWhen, + }), ]) } }, }) it.each` - strategy | action - ${'with `Escape`'} | ${() => press(Keys.Escape)} - ${'with `Outside Click`'} | ${() => click(document.body)} - ${'with `Click on Dialog.Overlay`'} | ${() => click(getDialogOverlays().pop()!)} + strategy | when | action + ${'with `Escape`'} | ${'mounted'} | ${() => press(Keys.Escape)} + ${'with `Outside Click`'} | ${'mounted'} | ${() => click(document.body)} + ${'with `Click on Dialog.Overlay`'} | ${'mounted'} | ${() => click(getDialogOverlays().pop()!)} + ${'with `Escape`'} | ${'always'} | ${() => press(Keys.Escape)} + ${'with `Outside Click`'} | ${'always'} | ${() => click(document.body)} `( 'should be possible to open nested Dialog components and close them $strategy', - async ({ action }) => { + async ({ when, action }) => { renderTemplate({ components: { Nested }, template: ` - + `, setup() { let isOpen = ref(false) diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.ts b/packages/@headlessui-vue/src/components/dialog/dialog.ts index 76f82fe..1a60622 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.ts @@ -140,6 +140,7 @@ export let Dialog = defineComponent({ ) useStackProvider({ type: 'Dialog', + enabled: computed(() => dialogState.value === DialogStates.Open), element: internalDialogRef, onUpdate: (message, type, element) => { if (type !== 'Dialog') return diff --git a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts index b6b1ceb..1db7628 100644 --- a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts +++ b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.ts @@ -10,6 +10,7 @@ import { PropType, Fragment, Ref, + onUnmounted, } from 'vue' import { render } from '../../utils/render' import { Hidden, Features as HiddenFeatures } from '../../internal/hidden' @@ -142,44 +143,38 @@ function useRestoreFocus( ) { let restoreElement = ref(null) - // Deliberately not using a ref, we don't want to trigger re-renders. - let mounted = { value: false } + function captureFocus() { + if (restoreElement.value) return + restoreElement.value = ownerDocument.value?.activeElement as HTMLElement + } + + // Restore the focus to the previous element + function restoreFocusIfNeeded() { + if (!restoreElement.value) return + focusElement(restoreElement.value) + restoreElement.value = null + } onMounted(() => { - // Capture the currently focused element, before we try to move the focus inside the FocusTrap. watch( enabled, (newValue, prevValue) => { if (newValue === prevValue) return - if (!enabled.value) return - mounted.value = true - - if (!restoreElement.value) { - restoreElement.value = ownerDocument.value?.activeElement as HTMLElement + if (newValue) { + // The FocusTrap has become enabled which means we're going to move the focus into the trap + // We need to capture the current focus before we do that so we can restore it when done + captureFocus() + } else { + restoreFocusIfNeeded() } }, { immediate: true } ) - - // Restore the focus when we unmount the component. - watch( - enabled, - (newValue, prevValue, onInvalidate) => { - if (newValue === prevValue) return - if (!enabled.value) return - - onInvalidate(() => { - if (mounted.value === false) return - mounted.value = false - - focusElement(restoreElement.value) - restoreElement.value = null - }) - }, - { immediate: true } - ) }) + + // Restore the focus when we unmount the component + onUnmounted(restoreFocusIfNeeded) } function useInitialFocus( diff --git a/packages/@headlessui-vue/src/hooks/use-outside-click.ts b/packages/@headlessui-vue/src/hooks/use-outside-click.ts index 83d8e65..3d2b748 100644 --- a/packages/@headlessui-vue/src/hooks/use-outside-click.ts +++ b/packages/@headlessui-vue/src/hooks/use-outside-click.ts @@ -1,5 +1,5 @@ import { useWindowEvent } from './use-window-event' -import { computed, Ref, ComputedRef } from 'vue' +import { computed, Ref, ComputedRef, ref } from 'vue' import { FocusableMode, isFocusableElement } from '../utils/focus-management' import { dom } from '../utils/dom' @@ -76,9 +76,31 @@ export function useOutsideClick( return cb(event, target) } + let initialClickTarget = ref(null) + + useWindowEvent( + 'mousedown', + (event) => { + if (enabled.value) { + initialClickTarget.value = event.target + } + }, + true + ) + useWindowEvent( 'click', - (event) => handleOutsideClick(event, (event) => event.target as HTMLElement), + (event) => { + if (!initialClickTarget.value) { + return + } + + handleOutsideClick(event, () => { + return initialClickTarget.value as HTMLElement + }) + + initialClickTarget.value = null + }, // 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` diff --git a/packages/@headlessui-vue/src/internal/stack-context.ts b/packages/@headlessui-vue/src/internal/stack-context.ts index ada634e..075bc92 100644 --- a/packages/@headlessui-vue/src/internal/stack-context.ts +++ b/packages/@headlessui-vue/src/internal/stack-context.ts @@ -7,6 +7,9 @@ import { // Types InjectionKey, Ref, + watch, + ref, + onBeforeUnmount, } from 'vue' type OnUpdate = (message: StackMessage, type: string, element: Ref) => void @@ -24,10 +27,12 @@ export function useStackContext() { export function useStackProvider({ type, + enabled, element, onUpdate, }: { type: string + enabled: Ref element: Ref onUpdate?: OnUpdate }) { @@ -42,11 +47,23 @@ export function useStackProvider({ } onMounted(() => { - notify(StackMessage.Add, type, element) + watch( + enabled, + (isEnabled, oldIsEnabled) => { + if (isEnabled) { + notify(StackMessage.Add, type, element) + } else if (oldIsEnabled === true) { + notify(StackMessage.Remove, type, element) + } + }, + { immediate: true, flush: 'sync' } + ) + }) - onUnmounted(() => { + onUnmounted(() => { + if (enabled.value) { notify(StackMessage.Remove, type, element) - }) + } }) provide(StackContext, notify) diff --git a/packages/@headlessui-vue/src/test-utils/interactions.ts b/packages/@headlessui-vue/src/test-utils/interactions.ts index 4f99cb7..01c7a3d 100644 --- a/packages/@headlessui-vue/src/test-utils/interactions.ts +++ b/packages/@headlessui-vue/src/test-utils/interactions.ts @@ -338,6 +338,74 @@ export async function mouseLeave(element: Document | Element | Window | null) { } } +export async function mouseDrag( + startingElement: Document | Element | Window | Node | null, + endingElement: Document | Element | Window | Node | null +) { + let button = MouseButton.Left + + try { + if (startingElement === null) return expect(startingElement).not.toBe(null) + if (endingElement === null) return expect(endingElement).not.toBe(null) + if (startingElement instanceof HTMLButtonElement && startingElement.disabled) return + + let options = { button } + + // Cancel in pointerDown cancels mouseDown, mouseUp + let cancelled = !fireEvent.pointerDown(startingElement, options) + + if (!cancelled) { + cancelled = !fireEvent.mouseDown(startingElement, options) + } + + // Ensure to trigger a `focus` event if the element is focusable, or within a focusable element + if (!cancelled) { + let next: HTMLElement | null = startingElement as HTMLElement | null + while (next !== null) { + if (next.matches(focusableSelector)) { + next.focus() + break + } + next = next.parentElement + } + } + + fireEvent.pointerMove(startingElement, options) + if (!cancelled) { + fireEvent.mouseMove(startingElement, options) + } + + fireEvent.pointerOut(startingElement, options) + if (!cancelled) { + fireEvent.mouseOut(startingElement, options) + } + + // crosses over to the ending element + + fireEvent.pointerOver(endingElement, options) + if (!cancelled) { + fireEvent.mouseOver(endingElement, options) + } + + fireEvent.pointerMove(endingElement, options) + if (!cancelled) { + fireEvent.mouseMove(endingElement, options) + } + + fireEvent.pointerUp(endingElement, options) + if (!cancelled) { + fireEvent.mouseUp(endingElement, options) + } + + fireEvent.click(endingElement, options) + + await new Promise(nextFrame) + } catch (err) { + if (err instanceof Error) Error.captureStackTrace(err, click) + throw err + } +} + // --- function focusNext(event: Partial) {