[menu-button] Fix preventing menu in dialog (#808)

Portaled menus still will not work correctly, but if this is needed users can pass portal={false} to `MenuList` to prevent conflicts with the dialog's focus lock. These changes make it easier to deal with non-portaled popovers by allowing consumers to render the `Menu` component as a DOM node that can be relatively positioned. This change also removes the outdated `data-reach-menu` attribute from the popover and uses it for the wrapping menu *if* a DOM node is rendered. This is a breaking change.
reach · Jul 31, 2021 · 031f2a6 · 031f2a6
1 parent 3404072
commit 031f2a6
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 19 deletions.
diff --git a/package.json b/package.json
@@ -52,8 +52,9 @@
     "@types/jest": "^26.0.24",
     "@types/lodash": "^4.14.171",
     "@types/node": "^14.x",
-    "@types/react": "^17.0.14",
+    "@types/react": "^17.0.15",
     "@types/react-dom": "^17.0.9",
+    "@types/react-is": "^17.0.2",
     "@types/react-router-dom": "^5.1.8",
     "@types/react-test-renderer": "^17.0.1",
     "@types/sinon": "^10.0.2",

diff --git a/packages/menu-button/examples/inside-dialog.example.tsx b/packages/menu-button/examples/inside-dialog.example.tsx
@@ -21,20 +21,37 @@ function Example() {
             borderRadius: "10px",
           }}
         >
-          <p>You can open me only after second click</p>
-          <Menu>
-            <MenuButton>Actions</MenuButton>
-            <MenuList>
+          <Menu
+            // Because of how the focus lock works with @reach/dialog, portaled
+            // nodes, such as the element rendered by the menu button popover,
+            // cannot receive focus without the focus lock immediately stealing
+            // it back and closing the menu. To use the menu inside a dialog,
+            // set the `portal` prop to false.
+            //
+            // This may impact styling since the popover is absolutely
+            // positioned, so the easiest solution is for the entire menu button
+            // and popover to be rendered in a shared, relative-positioned
+            // container. By default `Menu` does not render a DOM node, so you
+            // can either pass an element to its `as` prop or wrap it in your
+            // own container.
+            as="span"
+            style={{ display: "inline-block", position: "relative" }}
+          >
+            <MenuButton>Actions 1</MenuButton>
+            <MenuList portal={false}>
               <MenuItem onSelect={action("Download")}>Download</MenuItem>
               <MenuItem onSelect={action("Copy")}>Create a Copy</MenuItem>
               <MenuItem onSelect={action("Mark as Draft")}>
                 Mark as Draft
               </MenuItem>
             </MenuList>
           </Menu>
-          <Menu>
-            <MenuButton>Actions</MenuButton>
-            <MenuList>
+          <Menu
+            as="span"
+            style={{ display: "inline-block", position: "relative" }}
+          >
+            <MenuButton>Actions 2</MenuButton>
+            <MenuList portal={false}>
               <MenuItem onSelect={action("Download")}>Download</MenuItem>
               <MenuItem onSelect={action("Copy")}>Create a Copy</MenuItem>
               <MenuItem onSelect={action("Mark as Draft")}>

diff --git a/packages/menu-button/package.json b/packages/menu-button/package.json
@@ -22,11 +22,13 @@
   },
   "devDependencies": {
     "react": "^17.0.2",
-    "react-dom": "^17.0.2"
+    "react-dom": "^17.0.2",
+    "react-is": "^17.0.2"
   },
   "peerDependencies": {
     "react": "^16.8.0 || 17.x",
-    "react-dom": "^16.8.0 || 17.x"
+    "react-dom": "^16.8.0 || 17.x",
+    "react-is": "^16.8.0 || 17.x"
   },
   "main": "dist/reach-menu-button.cjs.js",
   "module": "dist/reach-menu-button.esm.js",

diff --git a/packages/menu-button/src/index.tsx b/packages/menu-button/src/index.tsx
@@ -25,6 +25,7 @@ import {
 } from "@reach/dropdown";
 import { noop } from "@reach/utils/noop";
 import { useCheckStyles } from "@reach/utils/dev-utils";
+import { isFragment } from "react-is";
 
 import type { Position } from "@reach/popover";
 import type * as Polymorphic from "@reach/utils/polymorphic";
@@ -38,10 +39,35 @@ import type * as Polymorphic from "@reach/utils/polymorphic";
  *
  * @see Docs https://reach.tech/menu-button#menu
  */
-const Menu: React.FC<MenuProps> = ({ id, children }) => {
-  useCheckStyles("menu-button");
-  return <DropdownProvider id={id} children={children} />;
-};
+const Menu = React.forwardRef(
+  ({ as: Comp = React.Fragment, id, children, ...rest }, forwardedRef) => {
+    useCheckStyles("menu-button");
+    let parentIsFragment = React.useMemo(() => {
+      try {
+        // To test if the component renders a fragment we need to actually
+        // render it, but this may throw an error since we can't predict what is
+        // actually provided. There's technically a small chance that this could
+        // get it wrong but I don't think it's too likely in practice.
+        return isFragment(<Comp />);
+      } catch (err) {
+        return false;
+      }
+    }, [Comp]);
+    let props = parentIsFragment
+      ? {}
+      : {
+          ref: forwardedRef,
+          id,
+          "data-reach-menu": "",
+          ...rest,
+        };
+    return (
+      <Comp {...props}>
+        <DropdownProvider id={id} children={children} />
+      </Comp>
+    );
+  }
+) as Polymorphic.ForwardRefComponent<any, MenuProps>;
 
 /**
  * @see Docs https://reach.tech/menu-button#menu-props
@@ -386,8 +412,6 @@ const MenuPopover = React.forwardRef(
     } = useDropdownPopover({ ...rest, ref: forwardedRef });
 
     let sharedProps = {
-      // TODO: remove in 1.0
-      "data-reach-menu": "",
       "data-reach-menu-popover": "",
     };
 

diff --git a/packages/menu-button/styles.css b/packages/menu-button/styles.css
@@ -3,13 +3,15 @@
   --reach-menu-button: 1;
 }
 
-[data-reach-menu],
+[data-reach-menu] {
+  position: relative;
+}
+
 [data-reach-menu-popover] {
   display: block;
   position: absolute;
 }
 
-[data-reach-menu][hidden],
 [data-reach-menu-popover][hidden] {
   display: none;
 }

diff --git a/yarn.lock b/yarn.lock
@@ -5338,6 +5338,13 @@
   dependencies:
     "@types/react" "*"
 
+"@types/react-is@^17.0.2":
+  version "17.0.2"
+  resolved "https://registry.yarnpkg.com/@types/react-is/-/react-is-17.0.2.tgz#abc4d910bff5b0bc6b3e1bec57575f6b63fd4e05"
+  integrity sha512-2+L0ilcAEG8udkDnvx8B0upwXFBbNnVwOsSCTxW3SDOkmar9NyEeLG0ZLa3uOEw9zyYf/fQapcnfXAVmDKlyHw==
+  dependencies:
+    "@types/react" "*"
+
 "@types/react-router-dom@^5.1.8":
   version "5.1.8"
   resolved "https://registry.yarnpkg.com/@types/react-router-dom/-/react-router-dom-5.1.8.tgz#bf3e1c8149b3d62eaa206d58599de82df0241192"
@@ -5369,7 +5376,7 @@
   dependencies:
     "@types/react" "*"
 
-"@types/react@*", "@types/react@>=16.9.0", "@types/react@^17.0.14":
+"@types/react@*", "@types/react@>=16.9.0", "@types/react@^17.0.14", "@types/react@^17.0.15":
   version "17.0.14"
   resolved "https://registry.yarnpkg.com/@types/react/-/react-17.0.14.tgz#f0629761ca02945c4e8fea99b8177f4c5c61fb0f"
   integrity sha512-0WwKHUbWuQWOce61UexYuWTGuGY/8JvtUe/dtQ6lR4sZ3UiylHotJeWpf3ArP9+DSGUoLY3wbU59VyMrJps5VQ==