fix: interactions with popover & focus issues (#3137)

* fix(aria-utils): handle click on listbox * 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 * chore(utilities): remove ariaShouldCloseOnInteractOutside * chore(deps): pnpm-lock.yaml * fix(popover): remove disableFocusManagement * fix(modal): remove disableFocusManagement * fix(autocomplete): remove custom focus logic and remove ariaShouldCloseOnInteractOutside * fix(popover): rewrite shouldCloseOnInteractOutside logic * chore(utilities): remove ariaShouldCloseOnInteractOutside * chore(deps): bump react-aria dependencies * chore(autocomplete): change back to focus * feat(changeset): update changeset * chore(docs): update type in onSelectionChange * fix(popover): revise popover test case * chore(deps): add @nextui-org/aria-utils * fix(autocomplete): add ariaShouldCloseOnInteractOutside * fix(date-picker): add ariaShouldCloseOnInteractOutside * fix(select): add ariaShouldCloseOnInteractOutside * chore(deps): add @nextui-org/aria-utils * fix(dropdown): add ariaShouldCloseOnInteractOutside * feat(utilities): rewrite ariaShouldCloseOnInteractOutside * fix(popover): use ariaShouldCloseOnInteractOutside * fix(autocomplete): add back shouldFocus * fix(utilities): include shouldFocus logic * chore(utilities): remove ! * refactor(aria-utils): add more comments * chore(changeset): update packages * refactor(aria-utils): add more comments * feat(popover): add test
heroui-inc · Jun 12, 2024 · 465b11a · 465b11a
1 parent 3eabbba
commit 465b11a
Show file tree

Hide file tree

Showing 12 changed files with 564 additions and 162 deletions.
diff --git a/.changeset/clever-gifts-joke.md b/.changeset/clever-gifts-joke.md
@@ -1,5 +1,7 @@
 ---
 "@nextui-org/popover": patch
+"@nextui-org/autocomplete": patch
+"@nextui-org/aria-utils": patch
 ---
 
 Fix popover focus issue (#3171, #2992)
diff --git a/.changeset/cold-peas-dream.md b/.changeset/cold-peas-dream.md
@@ -0,0 +1,6 @@
+---
+"@nextui-org/modal": patch
+"@nextui-org/popover": patch
+---
+
+remove `disableFocusManagement` from Overlay
diff --git a/apps/docs/app/examples/autocomplete/fully-controlled/page.tsx b/apps/docs/app/examples/autocomplete/fully-controlled/page.tsx
@@ -53,7 +53,7 @@ export default function Page() {
 
   // Specify how each of the Autocomplete values should change when an
   // option is selected from the list box
-  const onSelectionChange = (key: React.Key) => {
+  const onSelectionChange = (key: React.Key | null) => {
     setFieldState((prevState) => {
       let selectedItem = prevState.items.find((option) => option.value === key);
 

diff --git a/packages/components/autocomplete/package.json b/packages/components/autocomplete/package.json
@@ -53,14 +53,14 @@
     "@nextui-org/use-aria-button": "workspace:*",
     "@nextui-org/shared-icons": "workspace:*",
     "@nextui-org/use-safe-layout-effect": "workspace:*",
-    "@react-aria/combobox": "3.8.4",
-    "@react-aria/focus": "3.16.2",
+    "@react-aria/combobox": "3.9.1",
+    "@react-aria/focus": "3.17.1",
     "@react-aria/i18n": "3.10.2",
-    "@react-aria/interactions": "3.21.1",
+    "@react-aria/interactions": "3.21.3",
     "@react-aria/utils": "3.24.1",
-    "@react-aria/visually-hidden": "3.8.10",
-    "@react-stately/combobox": "3.8.2",
-    "@react-types/combobox": "3.10.1",
+    "@react-aria/visually-hidden": "3.8.12",
+    "@react-stately/combobox": "3.8.4",
+    "@react-types/combobox": "3.11.1",
     "@react-types/shared": "3.23.1"
   },
   "devDependencies": {

diff --git a/packages/components/modal/src/modal.tsx b/packages/components/modal/src/modal.tsx
@@ -17,11 +17,7 @@ const Modal = forwardRef<"div", ModalProps>((props, ref) => {
   const {children, ...otherProps} = props;
   const context = useModal({...otherProps, ref});
 
-  const overlay = (
-    <Overlay disableFocusManagement portalContainer={context.portalContainer}>
-      {children}
-    </Overlay>
-  );
+  const overlay = <Overlay portalContainer={context.portalContainer}>{children}</Overlay>;
 
   return (
     <ModalProvider value={context}>

diff --git a/packages/components/popover/__tests__/popover.test.tsx b/packages/components/popover/__tests__/popover.test.tsx
@@ -4,6 +4,7 @@ import userEvent from "@testing-library/user-event";
 import {Button} from "@nextui-org/button";
 
 import {Popover, PopoverContent, PopoverTrigger} from "../src";
+import {Select, SelectItem} from "../../select/src";
 
 // e.g. console.error Warning: Function components cannot be given refs.
 // Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
@@ -247,29 +248,70 @@ describe("Popover", () => {
     const wrapper = render(
       <Popover>
         <PopoverTrigger>
-          <Button disableRipple data-testid="trigger-test">
-            Open popover
-          </Button>
+          <Button data-testid="popover-trigger">Open popover</Button>
         </PopoverTrigger>
         <PopoverContent>
           <p>This is the content of the popover.</p>
         </PopoverContent>
       </Popover>,
     );
 
-    const trigger = wrapper.getByTestId("trigger-test");
+    const trigger = wrapper.getByTestId("popover-trigger");
 
-    // open popover
     await act(async () => {
+      // open popover
       await userEvent.click(trigger);
+      // close popover
+      await userEvent.click(trigger);
+      // assert that the focus is restored back to trigger
+      expect(trigger).toHaveFocus();
     });
+  });
+
+  it("should not close popover if nested select is closed", async () => {
+    const wrapper = render(
+      <Popover>
+        <PopoverTrigger>
+          <Button data-testid="popover">Open popover</Button>
+        </PopoverTrigger>
+        <PopoverContent>
+          <Select data-testid="select" label="Select country">
+            <SelectItem key="argentina">Argentina</SelectItem>
+            <SelectItem key="venezuela">Venezuela</SelectItem>
+            <SelectItem key="brazil">Brazil</SelectItem>
+          </Select>
+        </PopoverContent>
+      </Popover>,
+    );
+
+    const popover = wrapper.getByTestId("popover");
 
-    // close popover
     await act(async () => {
-      await userEvent.click(trigger);
+      // open popover
+      await userEvent.click(popover);
+    });
+
+    // assert that the popover is open
+    expect(popover).toHaveAttribute("aria-expanded", "true");
+
+    const select = wrapper.getByTestId("select");
+
+    await act(async () => {
+      // open select
+      await userEvent.click(select);
+    });
+
+    // assert that the select is open
+    expect(select).toHaveAttribute("aria-expanded", "true");
+
+    await act(async () => {
+      await userEvent.click(document.body);
     });
 
-    // assert that the focus is restored back to trigger
-    expect(trigger).toHaveFocus();
+    // assert that the select is closed
+    expect(select).toHaveAttribute("aria-expanded", "false");
+
+    // assert that the popover is still open
+    expect(popover).toHaveAttribute("aria-expanded", "true");
   });
 });
diff --git a/packages/components/popover/package.json b/packages/components/popover/package.json
@@ -48,14 +48,14 @@
     "@nextui-org/shared-utils": "workspace:*",
     "@nextui-org/react-utils": "workspace:*",
     "@nextui-org/use-safe-layout-effect": "workspace:*",
-    "@react-aria/dialog": "3.5.12",
-    "@react-aria/focus": "3.16.2",
-    "@react-aria/interactions": "3.21.1",
-    "@react-aria/overlays": "3.21.1",
+    "@react-aria/dialog": "3.5.14",
+    "@react-aria/focus": "3.17.1",
+    "@react-aria/interactions": "3.21.3",
+    "@react-aria/overlays": "3.22.1",
     "@react-aria/utils": "3.24.1",
-    "@react-stately/overlays": "3.6.5",
-    "@react-types/button": "3.9.2",
-    "@react-types/overlays": "3.8.5",
+    "@react-stately/overlays": "3.6.7",
+    "@react-types/button": "3.9.4",
+    "@react-types/overlays": "3.8.7",
     "react-remove-scroll": "^2.5.6"
   },
   "devDependencies": {

diff --git a/packages/components/popover/src/popover.tsx b/packages/components/popover/src/popover.tsx
@@ -20,11 +20,7 @@ const Popover = forwardRef<"div", PopoverProps>((props, ref) => {
 
   const [trigger, content] = Children.toArray(children);
 
-  const overlay = (
-    <Overlay disableFocusManagement portalContainer={context.portalContainer}>
-      {content}
-    </Overlay>
-  );
+  const overlay = <Overlay portalContainer={context.portalContainer}>{content}</Overlay>;
 
   return (
     <PopoverProvider value={context}>

diff --git a/packages/components/popover/src/use-aria-popover.ts b/packages/components/popover/src/use-aria-popover.ts
@@ -6,11 +6,15 @@ import {
   useOverlayPosition,
   AriaOverlayProps,
 } from "@react-aria/overlays";
-import {OverlayPlacement, ariaHideOutside, toReactAriaPlacement} from "@nextui-org/aria-utils";
+import {
+  OverlayPlacement,
+  ariaHideOutside,
+  toReactAriaPlacement,
+  ariaShouldCloseOnInteractOutside,
+} from "@nextui-org/aria-utils";
 import {OverlayTriggerState} from "@react-stately/overlays";
 import {mergeProps} from "@react-aria/utils";
 import {useSafeLayoutEffect} from "@nextui-org/use-safe-layout-effect";
-import {ariaShouldCloseOnInteractOutside} from "@nextui-org/aria-utils";
 
 export interface Props {
   /**

diff --git a/packages/utilities/aria-utils/package.json b/packages/utilities/aria-utils/package.json
@@ -42,9 +42,9 @@
     "@nextui-org/shared-utils": "workspace:*",
     "@nextui-org/react-rsc-utils": "workspace:*",
     "@react-aria/utils": "3.24.1",
-    "@react-stately/collections": "3.10.5",
-    "@react-stately/overlays": "3.6.5",
-    "@react-types/overlays": "3.8.5",
+    "@react-stately/collections": "3.10.7",
+    "@react-stately/overlays": "3.6.7",
+    "@react-types/overlays": "3.8.7",
     "@react-types/shared": "3.23.1"
   },
   "devDependencies": {

diff --git a/packages/utilities/aria-utils/src/overlays/ariaShouldCloseOnInteractOutside.ts b/packages/utilities/aria-utils/src/overlays/ariaShouldCloseOnInteractOutside.ts
@@ -4,38 +4,52 @@ import {MutableRefObject, RefObject} from "react";
  * Used to handle the outside interaction for popover-based components
  * e.g. dropdown, datepicker, date-range-picker, popover, select, autocomplete etc
  * @param element - the element outside of the popover ref, originally from `shouldCloseOnInteractOutside`
- * @param ref - The popover ref object that will interact outside with
- * @param state - The popover state from the target component
+ * @param triggerRef - The trigger ref object
+ * @param state - The state from the popover component
  * @param shouldFocus - a mutable ref boolean object to control the focus state
  *                      (used in input-based component such as autocomplete)
  * @returns - a boolean value which is same as shouldCloseOnInteractOutside
  */
 export const ariaShouldCloseOnInteractOutside = (
   element: Element,
-  ref: RefObject<Element>,
+  triggerRef: RefObject<Element>,
   state: any,
   shouldFocus?: MutableRefObject<boolean>,
 ) => {
-  let trigger = ref?.current;
+  const trigger = triggerRef?.current;
 
-  // check if the click is on the underlay
-  const clickOnUnderlay = element?.children?.[0]?.getAttribute("role") === "dialog" ?? false;
-
-  // if interacting outside the component
   if (!trigger || !trigger.contains(element)) {
+    // if clicking outside the trigger,
     // blur the component (e.g. autocomplete)
     if (shouldFocus) shouldFocus.current = false;
-    // if the click is not on the underlay,
-    // trigger the state close to prevent from opening multiple popovers at the same time
-    // e.g. open dropdown1 -> click dropdown2 (dropdown1 should be closed and dropdown2 should be open)
-    if (!clickOnUnderlay) state.close();
+
+    // if there is focus scope block, there will be a pair of span[data-focus-scope-start] and span[data-focus-scope-end]
+    // the element with focus trap resides inbetween these two blocks
+    // we push all the elements in focus scope to `focusScopeElements`
+    const startElements = document.querySelectorAll("body > span[data-focus-scope-start]");
+    let focusScopeElements: Element[] = [];
+
+    startElements.forEach((startElement) => {
+      focusScopeElements.push(startElement.nextElementSibling!);
+    });
+
+    // if there is just one focusScopeElement, we close the state
+    // e.g. open a popover A -> click popover B
+    // then popover A should be closed and popover B should be open
+    // TODO: handle cases like modal > popover A -> click modal > popover B
+    // we should close the popover when it is the last opened
+    // however, currently ariaShouldCloseOnInteractOutside is called recursively
+    // and we need a way to check if there is something closed before that (i.e. nested elements)
+    // if so, popover shouldn't be closed in this case
+    if (focusScopeElements.length === 1) {
+      state.close();
+
+      return false;
+    }
   } else {
     // otherwise the component (e.g. autocomplete) should keep focused
     if (shouldFocus) shouldFocus.current = true;
   }
 
-  // if the click is on the underlay,
-  // clicking the overlay should close the popover instead of closing the modal
-  // otherwise, allow interaction with other elements
-  return clickOnUnderlay;
+  return !trigger || !trigger.contains(element);
 };