fix: Fix Menu preventing default behavior when pressing ASCII keys …

…on elements other than `MenuItem` If we have a Dialog inside of a Menu, for example, and there's an input in the Dialog, trying to type in the input doesn't work since Menu was calling preventDefault().
ariakit · Jun 30, 2019 · cacb978 · cacb978
1 parent 65dcef7
commit cacb978
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 49 deletions.
diff --git a/packages/reakit/src/Menu/StaticMenu.tsx b/packages/reakit/src/Menu/StaticMenu.tsx
@@ -3,8 +3,8 @@ import { warning } from "reakit-utils/warning";
 import { createComponent } from "reakit-system/createComponent";
 import { useCreateElement } from "reakit-system/useCreateElement";
 import { createHook } from "reakit-system/createHook";
-import { useAllCallbacks } from "reakit-utils/useAllCallbacks";
 import { usePipe } from "reakit-utils/usePipe";
+import { mergeRefs } from "reakit-utils/mergeRefs";
 import { BoxOptions, BoxHTMLProps, useBox } from "../Box/Box";
 import { useShortcuts } from "./__utils/useShortcuts";
 import { MenuContext } from "./__utils/MenuContext";
@@ -24,10 +24,8 @@ export const useStaticMenu = createHook<StaticMenuOptions, StaticMenuHTMLProps>(
     compose: useBox,
     useState: useMenuState,
 
-    useProps(
-      options,
-      { onKeyDown: htmlOnKeyDown, unstable_wrap: htmlWrap, ...htmlProps }
-    ) {
+    useProps(options, { ref: htmlRef, unstable_wrap: htmlWrap, ...htmlProps }) {
+      const ref = React.useRef<HTMLElement>(null);
       const parent = React.useContext(MenuContext);
 
       const providerValue = React.useMemo(
@@ -40,7 +38,7 @@ export const useStaticMenu = createHook<StaticMenuOptions, StaticMenuHTMLProps>(
         [options.orientation, options.next, options.previous, parent]
       );
 
-      const onKeyDown = useShortcuts(options);
+      useShortcuts(ref, options);
 
       const wrap = React.useCallback(
         (children: React.ReactNode) => (
@@ -52,9 +50,9 @@ export const useStaticMenu = createHook<StaticMenuOptions, StaticMenuHTMLProps>(
       );
 
       return {
+        ref: mergeRefs(ref, htmlRef),
         role: options.orientation === "horizontal" ? "menubar" : "menu",
         "aria-orientation": options.orientation,
-        onKeyDown: useAllCallbacks(onKeyDown, htmlOnKeyDown),
         unstable_wrap: usePipe(wrap, htmlWrap),
         ...htmlProps
       };

diff --git a/packages/reakit/src/Menu/__utils/useShortcuts.ts b/packages/reakit/src/Menu/__utils/useShortcuts.ts
@@ -2,6 +2,7 @@ import * as React from "react";
 import { MenuStateReturn } from "../MenuState";
 
 export function useShortcuts(
+  menuRef: React.RefObject<HTMLElement>,
   { stops, move }: Pick<MenuStateReturn, "stops" | "move">,
   timeout = 500
 ) {
@@ -27,19 +28,32 @@ export function useShortcuts(
     return () => clearTimeout(timeoutId);
   }, [keys, stops, move, timeout]);
 
-  const onKeyDown = React.useCallback(
-    (event: React.KeyboardEvent) => {
+  React.useEffect(() => {
+    const menu = menuRef.current;
+    if (!menu) return undefined;
+
+    const onKeyDown = (event: KeyboardEvent) => {
       if (event.metaKey || event.altKey || event.shiftKey || event.ctrlKey) {
         return;
       }
+      const target = event.target as HTMLElement;
+      const role = target.getAttribute("role");
+      const targetIsMenu = target === menu;
+      const targetIsMenuItem =
+        role &&
+        role.indexOf("menuitem") >= 0 &&
+        target.closest("[role=menu],[role=menubar]") === menu;
+
+      if (!targetIsMenu && !targetIsMenuItem) return;
+
       if (/^[a-z0-9_-]$/i.test(event.key)) {
         event.stopPropagation();
         event.preventDefault();
-        setKeys(`${keys}${event.key}`);
+        setKeys(k => `${k}${event.key}`);
       }
-    },
-    [keys, setKeys]
-  );
+    };
 
-  return onKeyDown;
+    menu.addEventListener("keydown", onKeyDown);
+    return () => menu.removeEventListener("keydown", onKeyDown);
+  }, [menuRef, setKeys]);
 }
diff --git a/stories/index.stories.tsx b/stories/index.stories.tsx
@@ -59,40 +59,77 @@ storiesOf("Menu", module).add("Animated menu", () => {
   return <Example />;
 });
 
-storiesOf("Menu", module).add("Animated menu (react-spring)", () => {
-  function Example() {
-    const menu = useMenuState({ unstable_animated: true });
-    const { opacity, scale } = useSpring({
-      opacity: menu.visible ? 1 : 0,
-      scale: menu.visible ? 1 : 0,
-      onRest: menu.unstable_stopAnimation,
-      config: name => ({
-        tension: name === "opacity" ? 250 : 300,
-        friction: 25
-      })
-    });
+storiesOf("Menu", module)
+  .add("Animated menu (react-spring)", () => {
+    function Example() {
+      const menu = useMenuState({ unstable_animated: true });
+      const { opacity, scale } = useSpring({
+        opacity: menu.visible ? 1 : 0,
+        scale: menu.visible ? 1 : 0,
+        onRest: menu.unstable_stopAnimation,
+        config: name => ({
+          tension: name === "opacity" ? 250 : 300,
+          friction: 25
+        })
+      });
 
-    return (
-      <Provider unstable_system={system}>
-        <MenuDisclosure {...menu}>Menu</MenuDisclosure>
-        <Menu
-          {...menu}
-          as={animated.div}
-          aria-label="Menu"
-          style={{
-            opacity,
-            transformOrigin: "top center",
-            transform: scale.interpolate(
-              s => `${menu.unstable_popoverStyles.transform} scaleY(${s})`
-            )
-          }}
-        >
-          <MenuItem {...menu}>Item 1</MenuItem>
-          <MenuItem {...menu}>Item 2</MenuItem>
-          <MenuItem {...menu}>Item 3</MenuItem>
-        </Menu>
-      </Provider>
+      return (
+        <Provider unstable_system={system}>
+          <MenuDisclosure {...menu}>Menu</MenuDisclosure>
+          <Menu
+            {...menu}
+            as={animated.div}
+            aria-label="Menu"
+            style={{
+              opacity,
+              transformOrigin: "top center",
+              transform: scale.interpolate(
+                s => `${menu.unstable_popoverStyles.transform} scaleY(${s})`
+              )
+            }}
+          >
+            <MenuItem {...menu}>Item 1</MenuItem>
+            <MenuItem {...menu}>Item 2</MenuItem>
+            <MenuItem {...menu}>Item 3</MenuItem>
+          </Menu>
+        </Provider>
+      );
+    }
+    return <Example />;
+  })
+  .add("Menu with Dialog with input", () => {
+    const DialogWithInput = React.forwardRef(
+      (props, ref: React.RefObject<any>) => {
+        const dialog = useDialogState();
+        const [value, setValue] = React.useState("");
+        return (
+          <>
+            <DialogDisclosure {...dialog} {...props} ref={ref}>
+              Open dialog...
+            </DialogDisclosure>
+            <Dialog {...dialog} aria-label="Dialog with input">
+              <input
+                type="text"
+                value={value}
+                onChange={event => setValue(event.target.value)}
+              />
+            </Dialog>
+          </>
+        );
+      }
     );
-  }
-  return <Example />;
-});
+    function Example() {
+      const menu = useMenuState();
+      return (
+        <Provider unstable_system={system}>
+          <MenuDisclosure {...menu}>Menu</MenuDisclosure>
+          <Menu {...menu} aria-label="Menu">
+            <MenuItem {...menu}>a</MenuItem>
+            <MenuItem {...menu}>b</MenuItem>
+            <MenuItem {...menu} as={DialogWithInput} />
+          </Menu>
+        </Provider>
+      );
+    }
+    return <Example />;
+  });