From 3eabbbabe53fa9bd168c218e92078321ff137605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D5=A1=D3=84=D5=A1?= Date: Wed, 12 Jun 2024 16:11:36 +0800 Subject: [PATCH] fix(popover): popover focus issue (#3187) * fix(popover): move useDialog to popover-content * fix(popover): move useDialog to free-solo-popover * refactor(popover): use const instead * feat(changset): add changeset * feat(popover): popover focus test * refactor(popover): getDialogProps --- .changeset/clever-gifts-joke.md | 5 ++ .../popover/__tests__/popover.test.tsx | 59 +++++++++++++++++++ .../popover/src/free-solo-popover.tsx | 11 +++- .../popover/src/popover-content.tsx | 15 +++-- .../components/popover/src/use-popover.ts | 11 ++-- 5 files changed, 86 insertions(+), 15 deletions(-) create mode 100644 .changeset/clever-gifts-joke.md diff --git a/.changeset/clever-gifts-joke.md b/.changeset/clever-gifts-joke.md new file mode 100644 index 0000000000..20effb69ba --- /dev/null +++ b/.changeset/clever-gifts-joke.md @@ -0,0 +1,5 @@ +--- +"@nextui-org/popover": patch +--- + +Fix popover focus issue (#3171, #2992) diff --git a/packages/components/popover/__tests__/popover.test.tsx b/packages/components/popover/__tests__/popover.test.tsx index 2ab4e75da9..9b6c449bdc 100644 --- a/packages/components/popover/__tests__/popover.test.tsx +++ b/packages/components/popover/__tests__/popover.test.tsx @@ -213,4 +213,63 @@ describe("Popover", () => { // assert that the second popover is open expect(popover2).toHaveAttribute("aria-expanded", "true"); }); + + it("should focus on dialog when opened", async () => { + const wrapper = render( + + + + + +

This is the content of the popover.

+ + , + ); + + const trigger = wrapper.getByTestId("trigger-test"); + + // open popover + await act(async () => { + await userEvent.click(trigger); + }); + + const {getByRole} = wrapper; + + let dialog = getByRole("dialog"); + + // assert that the focus is on the dialog + expect(dialog).toHaveFocus(); + }); + + it("should restore focus on trigger when closed", async () => { + const wrapper = render( + + + + + +

This is the content of the popover.

+ + , + ); + + const trigger = wrapper.getByTestId("trigger-test"); + + // open popover + await act(async () => { + await userEvent.click(trigger); + }); + + // close popover + await act(async () => { + await userEvent.click(trigger); + }); + + // assert that the focus is restored back to trigger + expect(trigger).toHaveFocus(); + }); }); diff --git a/packages/components/popover/src/free-solo-popover.tsx b/packages/components/popover/src/free-solo-popover.tsx index edab8f19ec..036a0ce348 100644 --- a/packages/components/popover/src/free-solo-popover.tsx +++ b/packages/components/popover/src/free-solo-popover.tsx @@ -14,6 +14,7 @@ import {domAnimation, HTMLMotionProps, LazyMotion, m} from "framer-motion"; import {mergeProps} from "@react-aria/utils"; import {getTransformOrigins} from "@nextui-org/aria-utils"; import {TRANSITION_VARIANTS} from "@nextui-org/framer-utils"; +import {useDialog} from "@react-aria/dialog"; import {usePopover, UsePopoverProps, UsePopoverReturn} from "./use-popover"; @@ -92,7 +93,6 @@ const FreeSoloPopover = forwardRef<"div", FreeSoloPopoverProps>( state, placement, backdrop, - titleProps, portalContainer, disableAnimation, motionProps, @@ -106,6 +106,13 @@ const FreeSoloPopover = forwardRef<"div", FreeSoloPopoverProps>( ref, }); + const dialogRef = React.useRef(null); + const {dialogProps: ariaDialogProps, titleProps} = useDialog({}, dialogRef); + const dialogProps = getDialogProps({ + ref: dialogRef, + ...ariaDialogProps, + }); + const backdropContent = React.useMemo(() => { if (backdrop === "transparent") { return null; @@ -138,7 +145,7 @@ const FreeSoloPopover = forwardRef<"div", FreeSoloPopoverProps>( placement={placement} tabIndex={-1} transformOrigin={transformOrigin} - {...getDialogProps()} + {...dialogProps} > {!isNonModal && }
diff --git a/packages/components/popover/src/popover-content.tsx b/packages/components/popover/src/popover-content.tsx index 0c22513c8e..05566fcc30 100644 --- a/packages/components/popover/src/popover-content.tsx +++ b/packages/components/popover/src/popover-content.tsx @@ -1,7 +1,7 @@ import type {AriaDialogProps} from "@react-aria/dialog"; import type {HTMLMotionProps} from "framer-motion"; -import {DOMAttributes, ReactNode, useMemo, useCallback, ReactElement} from "react"; +import {DOMAttributes, ReactNode, useMemo, useCallback, ReactElement, useRef} from "react"; import {forwardRef} from "@nextui-org/system"; import {DismissButton} from "@react-aria/overlays"; import {TRANSITION_VARIANTS} from "@nextui-org/framer-utils"; @@ -9,6 +9,7 @@ import {m, domAnimation, LazyMotion} from "framer-motion"; import {HTMLNextUIProps} from "@nextui-org/system"; import {RemoveScroll} from "react-remove-scroll"; import {getTransformOrigins} from "@nextui-org/aria-utils"; +import {useDialog} from "@react-aria/dialog"; import {usePopoverContext} from "./popover-context"; @@ -27,7 +28,6 @@ const PopoverContent = forwardRef<"div", PopoverContentProps>((props, _) => { placement, backdrop, motionProps, - titleProps, disableAnimation, shouldBlockScroll, getPopoverProps, @@ -38,10 +38,13 @@ const PopoverContent = forwardRef<"div", PopoverContentProps>((props, _) => { onClose, } = usePopoverContext(); - const dialogProps = getDialogProps(otherProps); - - // Not needed in the popover context, the popover role comes from getPopoverProps - delete dialogProps.role; + const dialogRef = useRef(null); + const {dialogProps: ariaDialogProps, titleProps} = useDialog({}, dialogRef); + const dialogProps = getDialogProps({ + ref: dialogRef, + ...ariaDialogProps, + ...otherProps, + }); const Component = as || OverlayComponent || "div"; diff --git a/packages/components/popover/src/use-popover.ts b/packages/components/popover/src/use-popover.ts index b1ea300088..66321c6968 100644 --- a/packages/components/popover/src/use-popover.ts +++ b/packages/components/popover/src/use-popover.ts @@ -19,7 +19,7 @@ import {popover} from "@nextui-org/theme"; import {mergeProps, mergeRefs} from "@react-aria/utils"; import {clsx, dataAttr, objectToDeps} from "@nextui-org/shared-utils"; import {useMemo, useCallback, useRef} from "react"; -import {AriaDialogProps, useDialog} from "@react-aria/dialog"; +import {AriaDialogProps} from "@react-aria/dialog"; import {useReactAriaPopover, ReactAriaPopoverProps} from "./use-aria-popover"; @@ -131,7 +131,6 @@ export function usePopover(originalProps: UsePopoverProps) { const domTriggerRef = useRef(null); const wasTriggerPressedRef = useRef(false); - const dialogRef = useRef(null); const triggerRef = triggerRefProp || domTriggerRef; const disableAnimation = @@ -179,8 +178,6 @@ export function usePopover(originalProps: UsePopoverProps) { const {isFocusVisible, isFocused, focusProps} = useFocusRing(); - const {dialogProps, titleProps} = useDialog({}, dialogRef); - const slots = useMemo( () => popover({ @@ -198,14 +195,15 @@ export function usePopover(originalProps: UsePopoverProps) { }); const getDialogProps: PropGetter = (props = {}) => ({ - ref: dialogRef, + // `ref` and `dialogProps` from `useDialog` are passed from props + // if we use `useDialog` here, dialogRef won't be focused on mount "data-slot": "base", "data-open": dataAttr(state.isOpen), "data-focus": dataAttr(isFocused), "data-arrow": dataAttr(showArrow), "data-focus-visible": dataAttr(isFocusVisible), "data-placement": getArrowPlacement(ariaPlacement, placementProp), - ...mergeProps(focusProps, dialogProps, dialogPropsProp, props), + ...mergeProps(focusProps, dialogPropsProp, props), className: slots.base({class: clsx(baseStyles)}), style: { // this prevent the dialog to have a default outline @@ -316,7 +314,6 @@ export function usePopover(originalProps: UsePopoverProps) { triggerRef, placement, isNonModal, - titleProps, popoverRef: domRef, portalContainer, isOpen: state.isOpen,