From b7ed41cb0b3c8a868a3b48ec4dd09da1f57be0e1 Mon Sep 17 00:00:00 2001
From: Dylan Kilgore <dkilgore@eightfold.ai>
Date: Fri, 31 Mar 2023 17:25:05 -0700
Subject: [PATCH] fix: menu: fixes keyboard focus and screen reader
 functionality

---
 src/components/Dropdown/Dropdown.stories.tsx  |  2 +
 src/components/Dropdown/Dropdown.tsx          | 38 ++++++++++++-------
 src/components/Dropdown/Dropdown.types.ts     |  5 +++
 src/components/Link/Link.tsx                  |  3 +-
 src/components/Link/Link.types.ts             |  4 ++
 src/components/List/List.tsx                  |  2 -
 .../MenuItemButton/MenuItemButton.tsx         |  9 ++---
 .../MenuItem/MenuItemLink/MenuItemLink.tsx    |  3 +-
 .../Menu/__snapshots__/Menu.test.tsx.snap     | 33 ++++++----------
 9 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/src/components/Dropdown/Dropdown.stories.tsx b/src/components/Dropdown/Dropdown.stories.tsx
index b70cb3f42..11f8ea62b 100644
--- a/src/components/Dropdown/Dropdown.stories.tsx
+++ b/src/components/Dropdown/Dropdown.stories.tsx
@@ -121,11 +121,13 @@ const Overlay = () => (
         iconProps={{
           path: item.icon,
         }}
+        role="menuitem"
         style={{
           margin: '4px 0',
         }}
       />
     )}
+    role="menu"
   />
 );
 
diff --git a/src/components/Dropdown/Dropdown.tsx b/src/components/Dropdown/Dropdown.tsx
index 6f3e2c2a3..fd73a45db 100644
--- a/src/components/Dropdown/Dropdown.tsx
+++ b/src/components/Dropdown/Dropdown.tsx
@@ -9,6 +9,7 @@ import React, {
 import {
   autoUpdate,
   flip,
+  FloatingFocusManager,
   FloatingPortal,
   offset as fOffset,
   shift,
@@ -62,6 +63,7 @@ export const Dropdown: FC<DropdownProps> = React.memo(
         placement = 'bottom-start',
         portal = false,
         positionStrategy = 'absolute',
+        role = 'listbox',
         showDropdown,
         style,
         trigger = 'click',
@@ -78,13 +80,12 @@ export const Dropdown: FC<DropdownProps> = React.memo(
       const dropdownId: string = uniqueId('dropdown-');
 
       let timeout: ReturnType<typeof setTimeout>;
-      const { x, y, reference, floating, strategy, update, refs } = useFloating(
-        {
+      const { x, y, reference, floating, strategy, update, refs, context } =
+        useFloating({
           placement,
           strategy: positionStrategy,
           middleware: [fOffset(offset), flip(), shift()],
-        }
-      );
+        });
 
       const toggle: Function =
         (show: boolean, showDropdown = (show: boolean) => show): Function =>
@@ -191,16 +192,27 @@ export const Dropdown: FC<DropdownProps> = React.memo(
 
       const getDropdown = (): JSX.Element =>
         mergedVisible && (
-          <div
-            ref={floating}
-            style={dropdownStyles}
-            className={dropdownClasses}
-            tabIndex={0}
-            onClick={closeOnDropdownClick ? toggle(false, showDropdown) : null}
-            id={dropdownId}
+          <FloatingFocusManager
+            context={context}
+            key={dropdownId}
+            modal={false}
+            order={['reference', 'content']}
+            returnFocus={false}
           >
-            {overlay}
-          </div>
+            <div
+              ref={floating}
+              style={dropdownStyles}
+              className={dropdownClasses}
+              role={role}
+              tabIndex={0}
+              onClick={
+                closeOnDropdownClick ? toggle(false, showDropdown) : null
+              }
+              id={dropdownId}
+            >
+              {overlay}
+            </div>
+          </FloatingFocusManager>
         );
 
       return (
diff --git a/src/components/Dropdown/Dropdown.types.ts b/src/components/Dropdown/Dropdown.types.ts
index b70179aba..dfa54b7ab 100644
--- a/src/components/Dropdown/Dropdown.types.ts
+++ b/src/components/Dropdown/Dropdown.types.ts
@@ -66,6 +66,11 @@ export interface DropdownProps {
    * @default absolute
    */
   positionStrategy?: Strategy;
+  /**
+   * The dropdown aria role.
+   * @default 'listbox'
+   */
+  role?: string;
   /**
    * Callback to control the show/hide behavior of the dropdown.
    * triggered before the visible change
diff --git a/src/components/Link/Link.tsx b/src/components/Link/Link.tsx
index 81d71ed36..440f84b1a 100644
--- a/src/components/Link/Link.tsx
+++ b/src/components/Link/Link.tsx
@@ -14,6 +14,7 @@ export const Link: FC<LinkProps> = React.forwardRef(
       disabled = false,
       fullWidth = true,
       onClick,
+      role = 'link',
       target = '_self',
       underline,
       variant = 'default',
@@ -49,7 +50,7 @@ export const Link: FC<LinkProps> = React.forwardRef(
       <a
         {...rest}
         ref={ref}
-        role="link"
+        role={role}
         aria-disabled={disabled}
         className={linkClassNames}
         href={href}
diff --git a/src/components/Link/Link.types.ts b/src/components/Link/Link.types.ts
index 080716c2f..3b0725d21 100644
--- a/src/components/Link/Link.types.ts
+++ b/src/components/Link/Link.types.ts
@@ -25,6 +25,10 @@ export interface LinkProps
    * The Link onClick event handler.
    */
   onClick?: React.MouseEventHandler<HTMLAnchorElement>;
+  /**
+   * The Link role.
+   */
+  role?: string;
   /**
    * Whether to show the Link underline.
    */
diff --git a/src/components/List/List.tsx b/src/components/List/List.tsx
index 4d568a2e8..41bacc595 100644
--- a/src/components/List/List.tsx
+++ b/src/components/List/List.tsx
@@ -15,7 +15,6 @@ export const List = <T extends any>({
   header,
   classNames,
   style,
-  tabIndex = 0,
   itemClassNames,
   itemStyle,
   listType = 'ul',
@@ -61,7 +60,6 @@ export const List = <T extends any>({
       key={getItemKey(item, index)}
       className={itemClasses}
       style={itemStyle}
-      tabIndex={tabIndex}
     >
       {renderItem(item)}
     </li>
diff --git a/src/components/Menu/MenuItem/MenuItemButton/MenuItemButton.tsx b/src/components/Menu/MenuItem/MenuItemButton/MenuItemButton.tsx
index ece93c951..dfa60d32b 100644
--- a/src/components/Menu/MenuItem/MenuItemButton/MenuItemButton.tsx
+++ b/src/components/Menu/MenuItem/MenuItemButton/MenuItemButton.tsx
@@ -87,6 +87,7 @@ export const MenuItemButton: FC<MenuItemButtonProps> = ({
       tabIndex={tabIndex}
       {...rest}
       onClick={handleOnClick}
+      role={role}
     >
       {iconProps && alignIcon === MenuItemIconAlign.Left && getIcon()}
       <span className={styles.menuItemWrapper}>
@@ -102,7 +103,7 @@ export const MenuItemButton: FC<MenuItemButtonProps> = ({
 
   const secondaryButton = (): JSX.Element => (
     <>
-      <span className={styles.menuSecondaryWrapper}>
+      <span className={styles.menuSecondaryWrapper} role={role}>
         <button
           className={styles.menuOuterButton}
           disabled={disabled}
@@ -153,9 +154,5 @@ export const MenuItemButton: FC<MenuItemButtonProps> = ({
     return dropdownMenuItems ? dropdownMenuButton() : menuButton();
   };
 
-  return (
-    <li role={role} tabIndex={-1} className={menuItemClassNames}>
-      {renderedItem()}
-    </li>
-  );
+  return <li className={menuItemClassNames}>{renderedItem()}</li>;
 };
diff --git a/src/components/Menu/MenuItem/MenuItemLink/MenuItemLink.tsx b/src/components/Menu/MenuItem/MenuItemLink/MenuItemLink.tsx
index 1098bcb1f..3cf7dab04 100644
--- a/src/components/Menu/MenuItem/MenuItemLink/MenuItemLink.tsx
+++ b/src/components/Menu/MenuItem/MenuItemLink/MenuItemLink.tsx
@@ -64,11 +64,12 @@ export const MenuItemLink: FC<MenuItemLinkProps> = ({
   );
 
   return (
-    <li role={role} tabIndex={-1} className={menuItemClassNames}>
+    <li className={menuItemClassNames}>
       <Link
         classNames={styles.menuLink}
         disabled={disabled}
         fullWidth
+        role={role}
         tabIndex={tabIndex}
         {...rest}
       >
diff --git a/src/components/Menu/__snapshots__/Menu.test.tsx.snap b/src/components/Menu/__snapshots__/Menu.test.tsx.snap
index 85ed13089..3188a3655 100644
--- a/src/components/Menu/__snapshots__/Menu.test.tsx.snap
+++ b/src/components/Menu/__snapshots__/Menu.test.tsx.snap
@@ -23,6 +23,7 @@ exports[`Menu Menu is large 1`] = `
     <div
       class="dropdown-wrapper no-padding open"
       id="dropdown-"
+      role="listbox"
       style="position: absolute; top: 4px; left: 0px;"
       tabindex="0"
     >
@@ -36,11 +37,10 @@ exports[`Menu Menu is large 1`] = `
         >
           <li
             class="menu-item large neutral my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <button
               class="menu-item-button"
+              role="menuitem"
               tabindex="0"
             >
               <span
@@ -79,12 +79,11 @@ exports[`Menu Menu is large 1`] = `
           </li>
           <li
             class="menu-item large neutral disabled my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <button
               class="menu-item-button"
               disabled=""
+              role="menuitem"
               tabindex="0"
             >
               <span
@@ -109,14 +108,12 @@ exports[`Menu Menu is large 1`] = `
           </span>
           <li
             class="menu-item large neutral my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <a
               aria-disabled="false"
               class="link-style full-width menu-link"
               href="https://twitter.com"
-              role="link"
+              role="menuitem"
               tabindex="0"
               target="_blank"
             >
@@ -254,6 +251,7 @@ exports[`Menu Menu is medium 1`] = `
     <div
       class="dropdown-wrapper no-padding open"
       id="dropdown-"
+      role="listbox"
       style="position: absolute; top: 4px; left: 0px;"
       tabindex="0"
     >
@@ -267,11 +265,10 @@ exports[`Menu Menu is medium 1`] = `
         >
           <li
             class="menu-item medium neutral my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <button
               class="menu-item-button"
+              role="menuitem"
               tabindex="0"
             >
               <span
@@ -310,12 +307,11 @@ exports[`Menu Menu is medium 1`] = `
           </li>
           <li
             class="menu-item medium neutral disabled my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <button
               class="menu-item-button"
               disabled=""
+              role="menuitem"
               tabindex="0"
             >
               <span
@@ -340,14 +336,12 @@ exports[`Menu Menu is medium 1`] = `
           </span>
           <li
             class="menu-item medium neutral my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <a
               aria-disabled="false"
               class="link-style full-width menu-link"
               href="https://twitter.com"
-              role="link"
+              role="menuitem"
               tabindex="0"
               target="_blank"
             >
@@ -485,6 +479,7 @@ exports[`Menu Menu is small 1`] = `
     <div
       class="dropdown-wrapper no-padding open"
       id="dropdown-"
+      role="listbox"
       style="position: absolute; top: 4px; left: 0px;"
       tabindex="0"
     >
@@ -498,11 +493,10 @@ exports[`Menu Menu is small 1`] = `
         >
           <li
             class="menu-item small neutral my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <button
               class="menu-item-button"
+              role="menuitem"
               tabindex="0"
             >
               <span
@@ -541,12 +535,11 @@ exports[`Menu Menu is small 1`] = `
           </li>
           <li
             class="menu-item small neutral disabled my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <button
               class="menu-item-button"
               disabled=""
+              role="menuitem"
               tabindex="0"
             >
               <span
@@ -571,14 +564,12 @@ exports[`Menu Menu is small 1`] = `
           </span>
           <li
             class="menu-item small neutral my-menu-item-class"
-            role="menuitem"
-            tabindex="-1"
           >
             <a
               aria-disabled="false"
               class="link-style full-width menu-link"
               href="https://twitter.com"
-              role="link"
+              role="menuitem"
               tabindex="0"
               target="_blank"
             >