From 084a2497d80d28a0bd76e60b482b9b020b6f405e Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Fri, 7 May 2021 16:29:35 +0200 Subject: [PATCH] Fix incorrect nested Dialogs behaviour (#489) * add tests to verify the nested Dialog behaviour * set mounted to true once rendered once * cache useWindowEvent listener We only care about the very last version of the listener function. This allows us to only change the event listener if the event name (string) and options (boolean | object) change. * add/delete messages when mounting/unmounting We don't require a dedicated hook anymore, so this is a bit of cleanup! * add comments to the FocusResult enum * splitup functionality and make it a bit more clear using feature flags * add getDialogOverlays helper * simplify the Portal component We don't need to add the current element to the Stack. We only want to take care of that in the Dialog component itself. * drop dom-containers Currently it is only used in a single spot, so I inlined it into that file. * simplify the FocusTrap component, use new API * improve Dialog component * update CHANGELOG --- CHANGELOG.md | 1 + .../src/components/dialog/dialog.test.tsx | 273 +++++++++++---- .../src/components/dialog/dialog.tsx | 62 +++- .../src/components/focus-trap/focus-trap.tsx | 11 +- .../src/components/portal/portal.tsx | 19 +- .../src/hooks/use-focus-trap.ts | 139 +++++--- .../src/hooks/use-is-mounted.ts | 4 +- .../src/hooks/use-window-event.ts | 15 +- .../src/internal/dom-containers.ts | 7 - .../src/internal/stack-context.tsx | 40 ++- .../test-utils/accessibility-assertions.ts | 4 + .../src/utils/focus-management.ts | 7 + .../src/components/dialog/dialog.test.ts | 330 +++++++++++++----- .../test-utils/accessibility-assertions.ts | 4 + 14 files changed, 633 insertions(+), 283 deletions(-) delete mode 100644 packages/@headlessui-react/src/internal/dom-containers.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a75b4e..0b61b8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Improve SSR for `Dialog` ([#477](https://github.com/tailwindlabs/headlessui/pull/477)) - Delay focus trap initialization ([#477](https://github.com/tailwindlabs/headlessui/pull/477)) +- Improve incorrect behaviour for nesting `Dialog` components ([#560](https://github.com/tailwindlabs/headlessui/pull/560)) ## [Unreleased - Vue] diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index e49273a..6fa6127 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -14,6 +14,7 @@ import { getByText, assertActiveElement, getDialogs, + getDialogOverlays, } from '../../test-utils/accessibility-assertions' import { click, press, Keys } from '../../test-utils/interactions' import { PropsOf } from '../../types' @@ -637,79 +638,205 @@ describe('Mouse interactions', () => { }) describe('Nesting', () => { - it('should be possible to open nested Dialog components and close them with `Escape`', async () => { - function Nested({ onClose, level = 1 }: { onClose: (value: boolean) => void; level?: number }) { - let [showChild, setShowChild] = useState(false) + function Nested({ onClose, level = 1 }: { onClose: (value: boolean) => void; level?: number }) { + let [showChild, setShowChild] = useState(false) - return ( - <> - -
-

Level: {level}

- -
- {showChild && } - - - ) + return ( + <> + + + +
+

Level: {level}

+ + + +
+ {showChild && } + + + ) + } + + function Example() { + let [open, setOpen] = useState(false) + + return ( + <> + + {open && } + + ) + } + + it.each` + strategy | action + ${'with `Escape`'} | ${() => press(Keys.Escape)} + ${'with `Outside Click`'} | ${() => click(document.body)} + ${'with `Click on Dialog.Overlay`'} | ${() => click(getDialogOverlays().pop()!)} + `( + 'should be possible to open nested Dialog components and close them $strategy', + async ({ action }) => { + render() + + // Verify we have no open dialogs + expect(getDialogs()).toHaveLength(0) + + // Open Dialog 1 + await click(getByText('Open 1')) + + // Verify that we have 1 open dialog + expect(getDialogs()).toHaveLength(1) + + // Verify that the `Open 2 a` has focus + assertActiveElement(getByText('Open 2 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 a')) + + // Open Dialog 2 via the second button + await click(getByText('Open 2 b')) + + // Verify that we have 2 open dialogs + expect(getDialogs()).toHaveLength(2) + + // Verify that the `Open 3 a` has focus + assertActiveElement(getByText('Open 3 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 a')) + + // Close the top most Dialog + await action() + + // Verify that we have 1 open dialog + expect(getDialogs()).toHaveLength(1) + + // Verify that the `Open 2 b` button got focused again + assertActiveElement(getByText('Open 2 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 b')) + + // Open Dialog 2 via button b + await click(getByText('Open 2 b')) + + // Verify that the `Open 3 a` has focus + assertActiveElement(getByText('Open 3 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 a')) + + // Verify that we have 2 open dialogs + expect(getDialogs()).toHaveLength(2) + + // Open Dialog 3 via button c + await click(getByText('Open 3 c')) + + // Verify that the `Open 4 a` has focus + assertActiveElement(getByText('Open 4 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 4 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 4 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 4 a')) + + // Verify that we have 3 open dialogs + expect(getDialogs()).toHaveLength(3) + + // Close the top most Dialog + await action() + + // Verify that the `Open 3 c` button got focused again + assertActiveElement(getByText('Open 3 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 c')) + + // Verify that we have 2 open dialogs + expect(getDialogs()).toHaveLength(2) + + // Close the top most Dialog + await action() + + // Verify that we have 1 open dialog + expect(getDialogs()).toHaveLength(1) + + // Verify that the `Open 2 b` button got focused again + assertActiveElement(getByText('Open 2 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 b')) + + // Close the top most Dialog + await action() + + // Verify that we have 0 open dialogs + expect(getDialogs()).toHaveLength(0) + + // Verify that the `Open 1` button got focused again + assertActiveElement(getByText('Open 1')) } - - function Example() { - let [open, setOpen] = useState(false) - - return ( - <> - - {open && } - - ) - } - - render() - - // Verify we have no open dialogs - expect(getDialogs()).toHaveLength(0) - - // Open Dialog 1 - await click(getByText('Open 1')) - - // Verify that we have 1 open dialog - expect(getDialogs()).toHaveLength(1) - - // Open Dialog 2 - await click(getByText('Open 2')) - - // Verify that we have 2 open dialogs - expect(getDialogs()).toHaveLength(2) - - // Press escape to close the top most Dialog - await press(Keys.Escape) - - // Verify that we have 1 open dialog - expect(getDialogs()).toHaveLength(1) - - // Open Dialog 2 - await click(getByText('Open 2')) - - // Verify that we have 2 open dialogs - expect(getDialogs()).toHaveLength(2) - - // Open Dialog 3 - await click(getByText('Open 3')) - - // Verify that we have 3 open dialogs - expect(getDialogs()).toHaveLength(3) - - // Press escape to close the top most Dialog - await press(Keys.Escape) - - // Verify that we have 2 open dialogs - expect(getDialogs()).toHaveLength(2) - - // Press escape to close the top most Dialog - await press(Keys.Escape) - - // Verify that we have 1 open dialog - expect(getDialogs()).toHaveLength(1) - }) + ) }) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index 7319db6..2055bd4 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -15,6 +15,7 @@ import React, { KeyboardEvent as ReactKeyboardEvent, MutableRefObject, Ref, + useState, } from 'react' import { Props } from '../../types' @@ -24,16 +25,15 @@ import { useSyncRefs } from '../../hooks/use-sync-refs' import { Keys } from '../keyboard' import { isDisabledReactIssue7711 } from '../../utils/bugs' import { useId } from '../../hooks/use-id' -import { useFocusTrap } from '../../hooks/use-focus-trap' +import { useFocusTrap, Features as FocusTrapFeatures } from '../../hooks/use-focus-trap' import { useInertOthers } from '../../hooks/use-inert-others' import { Portal } from '../../components/portal/portal' -import { StackProvider, StackMessage } from '../../internal/stack-context' import { ForcePortalRoot } from '../../internal/portal-force-root' -import { contains } from '../../internal/dom-containers' import { Description, useDescriptions } from '../description/description' import { useWindowEvent } from '../../hooks/use-window-event' import { useOpenClosed, State } from '../../internal/open-closed' import { useServerHandoffComplete } from '../../hooks/use-server-handoff-complete' +import { StackProvider, StackMessage } from '../../internal/stack-context' enum DialogStates { Open, @@ -117,6 +117,7 @@ let DialogRoot = forwardRefWithAs(function Dialog< ref: Ref ) { let { open, onClose, initialFocus, ...rest } = props + let [nestedDialogCount, setNestedDialogCount] = useState(0) let usesOpenClosedState = useOpenClosed() if (open === undefined && usesOpenClosedState !== null) { @@ -127,7 +128,7 @@ let DialogRoot = forwardRefWithAs(function Dialog< }) } - let containers = useRef>(new Set()) + let containers = useRef>>(new Set()) let internalDialogRef = useRef(null) let dialogRef = useSyncRefs(internalDialogRef, ref) @@ -184,13 +185,34 @@ let DialogRoot = forwardRefWithAs(function Dialog< [dispatch] ) + let ready = useServerHandoffComplete() + let enabled = ready && dialogState === DialogStates.Open + let hasNestedDialogs = nestedDialogCount > 1 // 1 is the current dialog + let hasParentDialog = useContext(DialogContext) !== null + + // If there are multiple dialogs, then you can be the root, the leaf or one + // in between. We only care abou whether you are the top most one or not. + let position = !hasNestedDialogs ? 'leaf' : 'parent' + + useFocusTrap( + internalDialogRef, + enabled + ? match(position, { + parent: FocusTrapFeatures.RestoreFocus, + leaf: FocusTrapFeatures.All, + }) + : FocusTrapFeatures.None, + { initialFocus, containers } + ) + useInertOthers(internalDialogRef, hasNestedDialogs ? enabled : false) + // Handle outside click useWindowEvent('mousedown', event => { let target = event.target as HTMLElement if (dialogState !== DialogStates.Open) return - if (containers.current.size !== 1) return - if (contains(containers.current, target)) return + if (hasNestedDialogs) return + if (internalDialogRef.current?.contains(target)) return close() }) @@ -198,6 +220,7 @@ let DialogRoot = forwardRefWithAs(function Dialog< // Scroll lock useEffect(() => { if (dialogState !== DialogStates.Open) return + if (hasParentDialog) return let overflow = document.documentElement.style.overflow let paddingRight = document.documentElement.style.paddingRight @@ -211,7 +234,7 @@ let DialogRoot = forwardRefWithAs(function Dialog< document.documentElement.style.overflow = overflow document.documentElement.style.paddingRight = paddingRight } - }, [dialogState]) + }, [dialogState, hasParentDialog]) // Trigger close when the FocusTrap gets hidden useEffect(() => { @@ -236,11 +259,6 @@ let DialogRoot = forwardRefWithAs(function Dialog< return () => observer.disconnect() }, [dialogState, internalDialogRef, close]) - let ready = useServerHandoffComplete() - let enabled = ready && dialogState === DialogStates.Open - - useFocusTrap(containers, enabled, { initialFocus }) - useInertOthers(internalDialogRef, enabled) let [describedby, DescriptionProvider] = useDescriptions() let id = `headlessui-dialog-${useId()}` @@ -269,7 +287,7 @@ let DialogRoot = forwardRefWithAs(function Dialog< onKeyDown(event: ReactKeyboardEvent) { if (event.key !== Keys.Escape) return if (dialogState !== DialogStates.Open) return - if (containers.current.size > 1) return // 1 is myself, otherwise other elements in the Stack + if (hasNestedDialogs) return event.preventDefault() event.stopPropagation() close() @@ -279,16 +297,22 @@ let DialogRoot = forwardRefWithAs(function Dialog< return ( { - return match(message, { - [StackMessage.AddElement]() { + type="Dialog" + element={internalDialogRef} + onUpdate={useCallback((message, type, element) => { + if (type !== 'Dialog') return + + match(message, { + [StackMessage.Add]() { containers.current.add(element) + setNestedDialogCount(count => count + 1) }, - [StackMessage.RemoveElement]() { - containers.current.delete(element) + [StackMessage.Remove]() { + containers.current.add(element) + setNestedDialogCount(count => count - 1) }, }) - }} + }, [])} > 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 ee587a7..360fb4b 100644 --- a/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx +++ b/packages/@headlessui-react/src/components/focus-trap/focus-trap.tsx @@ -8,7 +8,7 @@ import { import { Props } from '../../types' import { render } from '../../utils/render' -import { useFocusTrap } from '../../hooks/use-focus-trap' +import { useFocusTrap, Features as FocusTrapFeatures } from '../../hooks/use-focus-trap' import { useServerHandoffComplete } from '../../hooks/use-server-handoff-complete' let DEFAULT_FOCUS_TRAP_TAG = 'div' as const @@ -16,17 +16,14 @@ let DEFAULT_FOCUS_TRAP_TAG = 'div' as const export function FocusTrap( props: Props & { initialFocus?: MutableRefObject } ) { - let containers = useRef>(new Set()) + let container = useRef(null) let { initialFocus, ...passthroughProps } = props let ready = useServerHandoffComplete() - useFocusTrap(containers, ready, { initialFocus }) + useFocusTrap(container, ready ? FocusTrapFeatures.All : FocusTrapFeatures.None, { initialFocus }) let propsWeControl = { - ref(element: HTMLElement | null) { - if (!element) return - containers.current.add(element) - }, + ref: container, } return render({ diff --git a/packages/@headlessui-react/src/components/portal/portal.tsx b/packages/@headlessui-react/src/components/portal/portal.tsx index 93d55b3..24cd314 100644 --- a/packages/@headlessui-react/src/components/portal/portal.tsx +++ b/packages/@headlessui-react/src/components/portal/portal.tsx @@ -14,7 +14,6 @@ import { createPortal } from 'react-dom' import { Props } from '../../types' import { render } from '../../utils/render' import { useIsoMorphicEffect } from '../../hooks/use-iso-morphic-effect' -import { useElementStack, StackProvider } from '../../internal/stack-context' import { usePortalRoot } from '../../internal/portal-force-root' import { useServerHandoffComplete } from '../../hooks/use-server-handoff-complete' @@ -60,8 +59,6 @@ export function Portal( let ready = useServerHandoffComplete() - useElementStack(element) - useIsoMorphicEffect(() => { if (!target) return if (!element) return @@ -82,16 +79,12 @@ export function Portal( if (!ready) return null - return ( - - {!target || !element - ? null - : createPortal( - render({ props: passthroughProps, defaultTag: DEFAULT_PORTAL_TAG, name: 'Portal' }), - element - )} - - ) + return !target || !element + ? null + : createPortal( + render({ props: passthroughProps, defaultTag: DEFAULT_PORTAL_TAG, name: 'Portal' }), + element + ) } // --- diff --git a/packages/@headlessui-react/src/hooks/use-focus-trap.ts b/packages/@headlessui-react/src/hooks/use-focus-trap.ts index 600039b..2b7eb90 100644 --- a/packages/@headlessui-react/src/hooks/use-focus-trap.ts +++ b/packages/@headlessui-react/src/hooks/use-focus-trap.ts @@ -7,93 +7,124 @@ import { import { Keys } from '../components/keyboard' import { focusElement, focusIn, Focus, FocusResult } from '../utils/focus-management' -import { contains } from '../internal/dom-containers' import { useWindowEvent } from './use-window-event' +import { useIsMounted } from './use-is-mounted' + +export enum Features { + /** No features enabled for the `useFocusTrap` hook. */ + None = 1 << 0, + + /** Ensure that we move focus initially into the container. */ + InitialFocus = 1 << 1, + + /** Ensure that pressing `Tab` and `Shift+Tab` is trapped within the container. */ + TabLock = 1 << 2, + + /** Ensure that programmatically moving focus outside of the container is disallowed. */ + FocusLock = 1 << 3, + + /** Ensure that we restore the focus when unmounting the component that uses this `useFocusTrap` hook. */ + RestoreFocus = 1 << 4, + + /** Enable all features. */ + All = InitialFocus | TabLock | FocusLock | RestoreFocus, +} export function useFocusTrap( - containers: MutableRefObject>, - enabled: boolean = true, - options: { initialFocus?: MutableRefObject } = {} + container: MutableRefObject, + features: Features = Features.All, + { + initialFocus, + containers, + }: { + initialFocus?: MutableRefObject + containers?: MutableRefObject>> + } = {} ) { let restoreElement = useRef( typeof window !== 'undefined' ? (document.activeElement as HTMLElement) : null ) let previousActiveElement = useRef(null) - let mounted = useRef(false) + let mounted = useIsMounted() + + let featuresRestoreFocus = Boolean(features & Features.RestoreFocus) + let featuresInitialFocus = Boolean(features & Features.InitialFocus) + + // Capture the currently focused element, before we enable the focus trap. + useEffect(() => { + if (!featuresRestoreFocus) return + + restoreElement.current = document.activeElement as HTMLElement + }, [featuresRestoreFocus]) + + // Restore the focus when we unmount the component. + useEffect(() => { + if (!featuresRestoreFocus) return + + return () => { + focusElement(restoreElement.current) + restoreElement.current = null + } + }, [featuresRestoreFocus]) // Handle initial focus useEffect(() => { - if (!enabled) return - if (containers.current.size !== 1) return - - mounted.current = true + if (!featuresInitialFocus) return + if (!container.current) return let activeElement = document.activeElement as HTMLElement - if (options.initialFocus?.current) { - if (options.initialFocus?.current === activeElement) { + if (initialFocus?.current) { + if (initialFocus?.current === activeElement) { + previousActiveElement.current = activeElement return // Initial focus ref is already the active element } - } else if (contains(containers.current, activeElement)) { + } else if (container.current.contains(activeElement)) { + previousActiveElement.current = activeElement return // Already focused within Dialog } - restoreElement.current = activeElement - // Try to focus the initialFocus ref - if (options.initialFocus?.current) { - focusElement(options.initialFocus.current) + if (initialFocus?.current) { + focusElement(initialFocus.current) } else { - let couldFocus = false - for (let container of containers.current) { - let result = focusIn(container, Focus.First) - if (result === FocusResult.Success) { - couldFocus = true - break - } + if (focusIn(container.current, Focus.First) === FocusResult.Error) { + throw new Error('There are no focusable elements inside the ') } - - if (!couldFocus) throw new Error('There are no focusable elements inside the ') } previousActiveElement.current = document.activeElement as HTMLElement + }, [container, initialFocus, featuresInitialFocus]) - return () => { - mounted.current = false - focusElement(restoreElement.current) - restoreElement.current = null - previousActiveElement.current = null - } - }, [enabled, containers, mounted, options.initialFocus]) - - // Handle Tab & Shift+Tab keyboard events + // Handle `Tab` & `Shift+Tab` keyboard events useWindowEvent('keydown', event => { - if (!enabled) return + if (!(features & Features.TabLock)) return + + if (!container.current) return if (event.key !== Keys.Tab) return - if (!document.activeElement) return - if (containers.current.size !== 1) return event.preventDefault() - for (let element of containers.current) { - let result = focusIn( - element, + if ( + focusIn( + container.current, (event.shiftKey ? Focus.Previous : Focus.Next) | Focus.WrapAround - ) - - if (result === FocusResult.Success) { - previousActiveElement.current = document.activeElement as HTMLElement - break - } + ) === FocusResult.Success + ) { + previousActiveElement.current = document.activeElement as HTMLElement } }) - // Prevent programmatically escaping + // Prevent programmatically escaping the container useWindowEvent( 'focus', event => { - if (!enabled) return - if (containers.current.size !== 1) return + if (!(features & Features.FocusLock)) return + + let allContainers = new Set(containers?.current) + allContainers.add(container) + + if (!allContainers.size) return let previous = previousActiveElement.current if (!previous) return @@ -102,7 +133,7 @@ export function useFocusTrap( let toElement = event.target as HTMLElement | null if (toElement && toElement instanceof HTMLElement) { - if (!contains(containers.current, toElement)) { + if (!contains(allContainers, toElement)) { event.preventDefault() event.stopPropagation() focusElement(previous) @@ -117,3 +148,11 @@ export function useFocusTrap( true ) } + +function contains(containers: Set>, element: HTMLElement) { + for (let container of containers) { + if (container.current?.contains(element)) return true + } + + return false +} diff --git a/packages/@headlessui-react/src/hooks/use-is-mounted.ts b/packages/@headlessui-react/src/hooks/use-is-mounted.ts index 66d6c0a..8f890a6 100644 --- a/packages/@headlessui-react/src/hooks/use-is-mounted.ts +++ b/packages/@headlessui-react/src/hooks/use-is-mounted.ts @@ -1,9 +1,11 @@ import { useRef, useEffect } from 'react' export function useIsMounted() { - let mounted = useRef(true) + let mounted = useRef(false) useEffect(() => { + mounted.current = true + return () => { mounted.current = false } diff --git a/packages/@headlessui-react/src/hooks/use-window-event.ts b/packages/@headlessui-react/src/hooks/use-window-event.ts index afbce92..f3dda15 100644 --- a/packages/@headlessui-react/src/hooks/use-window-event.ts +++ b/packages/@headlessui-react/src/hooks/use-window-event.ts @@ -1,12 +1,19 @@ -import { useEffect } from 'react' +import { useEffect, useRef } from 'react' export function useWindowEvent( type: TType, listener: (this: Window, ev: WindowEventMap[TType]) => any, options?: boolean | AddEventListenerOptions ) { + let listenerRef = useRef(listener) + listenerRef.current = listener + useEffect(() => { - window.addEventListener(type, listener, options) - return () => window.removeEventListener(type, listener, options) - }, [type, listener, options]) + function handler(event: WindowEventMap[TType]) { + listenerRef.current.call(window, event) + } + + window.addEventListener(type, handler, options) + return () => window.removeEventListener(type, handler, options) + }, [type, options]) } diff --git a/packages/@headlessui-react/src/internal/dom-containers.ts b/packages/@headlessui-react/src/internal/dom-containers.ts deleted file mode 100644 index caa0c72..0000000 --- a/packages/@headlessui-react/src/internal/dom-containers.ts +++ /dev/null @@ -1,7 +0,0 @@ -export function contains(containers: Set, element: HTMLElement) { - for (let container of containers) { - if (container.contains(element)) return true - } - - return false -} diff --git a/packages/@headlessui-react/src/internal/stack-context.tsx b/packages/@headlessui-react/src/internal/stack-context.tsx index 8501120..525470f 100644 --- a/packages/@headlessui-react/src/internal/stack-context.tsx +++ b/packages/@headlessui-react/src/internal/stack-context.tsx @@ -1,37 +1,42 @@ -import React, { ReactNode, createContext, useContext, useCallback } from 'react' +import React, { + createContext, + useCallback, + useContext, + + // Types + MutableRefObject, + ReactNode, +} from 'react' import { useIsoMorphicEffect } from '../hooks/use-iso-morphic-effect' -type OnUpdate = (message: StackMessage, element: HTMLElement) => void +type OnUpdate = ( + message: StackMessage, + type: string, + element: MutableRefObject +) => void let StackContext = createContext(() => {}) StackContext.displayName = 'StackContext' export enum StackMessage { - AddElement, - RemoveElement, + Add, + Remove, } export function useStackContext() { return useContext(StackContext) } -export function useElementStack(element: HTMLElement | null) { - let notify = useStackContext() - - useIsoMorphicEffect(() => { - if (!element) return - - notify(StackMessage.AddElement, element) - return () => notify(StackMessage.RemoveElement, element) - }, [element]) -} - export function StackProvider({ children, onUpdate, + type, + element, }: { children: ReactNode onUpdate?: OnUpdate + type: string + element: MutableRefObject }) { let parentUpdate = useStackContext() @@ -46,5 +51,10 @@ export function StackProvider({ [parentUpdate, onUpdate] ) + useIsoMorphicEffect(() => { + notify(StackMessage.Add, type, element) + return () => notify(StackMessage.Remove, type, element) + }, [notify, type, element]) + return {children} } diff --git a/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts b/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts index 1d34c57..828d460 100644 --- a/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts +++ b/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts @@ -880,6 +880,10 @@ export function getDialogOverlay(): HTMLElement | null { return document.querySelector('[id^="headlessui-dialog-overlay-"]') } +export function getDialogOverlays(): HTMLElement[] { + return Array.from(document.querySelectorAll('[id^="headlessui-dialog-overlay-"]')) +} + // --- export enum DialogState { diff --git a/packages/@headlessui-react/src/utils/focus-management.ts b/packages/@headlessui-react/src/utils/focus-management.ts index f873373..b3d1104 100644 --- a/packages/@headlessui-react/src/utils/focus-management.ts +++ b/packages/@headlessui-react/src/utils/focus-management.ts @@ -44,9 +44,16 @@ export enum Focus { } export enum FocusResult { + /** Something went wrong while trying to focus. */ Error, + + /** When `Focus.WrapAround` is enabled, going from position `N` to `N+1` where `N` is the last index in the array, then we overflow. */ Overflow, + + /** Focus was successful. */ Success, + + /** When `Focus.WrapAround` is enabled, going from position `N` to `N-1` where `N` is the first index in the array, then we underflow. */ Underflow, } diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index cdfafec..e01d98e 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -15,6 +15,7 @@ import { getByText, assertActiveElement, getDialogs, + getDialogOverlays, } from '../../test-utils/accessibility-assertions' import { click, press, Keys } from '../../test-utils/interactions' import { html } from '../../test-utils/html' @@ -781,102 +782,243 @@ describe('Mouse interactions', () => { }) describe('Nesting', () => { - it('should be possible to open nested Dialog components and close them with `Escape`', async () => { - let Nested = defineComponent({ - components: { Dialog }, - emits: ['close'], - props: ['level'], - render() { - let level = this.$props.level ?? 1 - return h(Dialog, { open: true, onClose: this.onClose }, () => [ - h('div', [ - h('p', `Level: ${level}`), - h( - 'button', - { - onClick: () => { - this.showChild = true - }, + let Nested = defineComponent({ + components: { Dialog, DialogOverlay }, + emits: ['close'], + props: ['level'], + render() { + let level = this.$props.level ?? 1 + return h(Dialog, { open: true, onClose: this.onClose }, () => [ + h(DialogOverlay), + h('div', [ + h('p', `Level: ${level}`), + h( + 'button', + { + onClick: () => { + this.showChild = true }, - `Open ${level + 1}` - ), - ]), - this.showChild && - h(Nested, { - onClose: () => { - this.showChild = false + }, + `Open ${level + 1} a` + ), + h( + 'button', + { + onClick: () => { + this.showChild = true }, - level: level + 1, - }), - ]) - }, - setup(_props, { emit }) { - let showChild = ref(false) + }, + `Open ${level + 1} b` + ), + h( + 'button', + { + onClick: () => { + this.showChild = true + }, + }, + `Open ${level + 1} c` + ), + ]), + this.showChild && + h(Nested, { + onClose: () => { + this.showChild = false + }, + level: level + 1, + }), + ]) + }, + setup(_props, { emit }) { + let showChild = ref(false) - return { - showChild, - onClose() { - emit('close', false) - }, - } - }, - }) - - renderTemplate({ - components: { Nested }, - template: ` - - - `, - setup() { - let isOpen = ref(false) - return { isOpen } - }, - }) - - // Verify we have no open dialogs - expect(getDialogs()).toHaveLength(0) - - // Open Dialog 1 - await click(getByText('Open 1')) - - // Verify that we have 1 open dialog - expect(getDialogs()).toHaveLength(1) - - // Open Dialog 2 - await click(getByText('Open 2')) - - // Verify that we have 2 open dialogs - expect(getDialogs()).toHaveLength(2) - - // Press escape to close the top most Dialog - await press(Keys.Escape) - - // Verify that we have 1 open dialog - expect(getDialogs()).toHaveLength(1) - - // Open Dialog 2 - await click(getByText('Open 2')) - - // Verify that we have 2 open dialogs - expect(getDialogs()).toHaveLength(2) - - // Open Dialog 3 - await click(getByText('Open 3')) - - // Verify that we have 3 open dialogs - expect(getDialogs()).toHaveLength(3) - - // Press escape to close the top most Dialog - await press(Keys.Escape) - - // Verify that we have 2 open dialogs - expect(getDialogs()).toHaveLength(2) - - // Press escape to close the top most Dialog - await press(Keys.Escape) - - // Verify that we have 1 open dialog - expect(getDialogs()).toHaveLength(1) + return { + showChild, + onClose() { + emit('close', false) + }, + } + }, }) + + it.each` + strategy | action + ${'with `Escape`'} | ${() => press(Keys.Escape)} + ${'with `Outside Click`'} | ${() => click(document.body)} + ${'with `Click on Dialog.Overlay`'} | ${() => click(getDialogOverlays().pop()!)} + `( + 'should be possible to open nested Dialog components and close them $strategy', + async ({ action }) => { + renderTemplate({ + components: { Nested }, + template: ` + + + `, + setup() { + let isOpen = ref(false) + return { isOpen } + }, + }) + + // Verify we have no open dialogs + expect(getDialogs()).toHaveLength(0) + + // Open Dialog 1 + await click(getByText('Open 1')) + + // Verify that we have 1 open dialog + expect(getDialogs()).toHaveLength(1) + + // Verify that the `Open 2 a` has focus + assertActiveElement(getByText('Open 2 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 a')) + + // Open Dialog 2 via the second button + await click(getByText('Open 2 b')) + + // Verify that we have 2 open dialogs + expect(getDialogs()).toHaveLength(2) + + // Verify that the `Open 3 a` has focus + assertActiveElement(getByText('Open 3 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 a')) + + // Close the top most Dialog + await action() + + // Verify that we have 1 open dialog + expect(getDialogs()).toHaveLength(1) + + // Verify that the `Open 2 b` button got focused again + assertActiveElement(getByText('Open 2 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 b')) + + // Open Dialog 2 via button b + await click(getByText('Open 2 b')) + + // Verify that the `Open 3 a` has focus + assertActiveElement(getByText('Open 3 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 a')) + + // Verify that we have 2 open dialogs + expect(getDialogs()).toHaveLength(2) + + // Open Dialog 3 via button c + await click(getByText('Open 3 c')) + + // Verify that the `Open 4 a` has focus + assertActiveElement(getByText('Open 4 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 4 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 4 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 4 a')) + + // Verify that we have 3 open dialogs + expect(getDialogs()).toHaveLength(3) + + // Close the top most Dialog + await action() + + // Verify that the `Open 3 c` button got focused again + assertActiveElement(getByText('Open 3 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 3 c')) + + // Verify that we have 2 open dialogs + expect(getDialogs()).toHaveLength(2) + + // Close the top most Dialog + await action() + + // Verify that we have 1 open dialog + expect(getDialogs()).toHaveLength(1) + + // Verify that the `Open 2 b` button got focused again + assertActiveElement(getByText('Open 2 b')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 c')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 a')) + + // Verify that we can tab around + await press(Keys.Tab) + assertActiveElement(getByText('Open 2 b')) + + // Close the top most Dialog + await action() + + // Verify that we have 0 open dialogs + expect(getDialogs()).toHaveLength(0) + + // Verify that the `Open 1` button got focused again + assertActiveElement(getByText('Open 1')) + } + ) }) diff --git a/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts b/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts index 1d34c57..828d460 100644 --- a/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts +++ b/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts @@ -880,6 +880,10 @@ export function getDialogOverlay(): HTMLElement | null { return document.querySelector('[id^="headlessui-dialog-overlay-"]') } +export function getDialogOverlays(): HTMLElement[] { + return Array.from(document.querySelectorAll('[id^="headlessui-dialog-overlay-"]')) +} + // --- export enum DialogState {