fix(dropdown-menu): Remove custom link behavior (#90713)

Currently we have custom logic in the DropdownMenu component which
handles link items. The react-aria library should already be handling
this, but we configured things a bit differently than react-aria
expected which prevented it. This PR removes the custom behavior and
provides a `href` for react-aria to use.

I'm hoping that this change fixes some bugs I've seen on mobile devices.
On iOS especially, taps on dropdown link items have no effect.

Summary of changes:

- Add `href` to the dropdown item state if `to` or `externalHref` is
passed
- Remove custom logic handling keyboard selection of a link item
- Moves the dropdown item props from the container `li` element to the
"inner item" (which is an anchor tag in the case of links)
- Fixes tests that looked for `link` roles instead of `menuitemradio`

Author: malwilley

static/app/components/core/menuListItem/index.chonk.tsx

@@ -71,8 +71,12 @@ export const ChonkInnerWrap = chonkStyled('div', {
     font-size: ${p => p.theme.form[p.size ?? 'md'].fontSize};
 
     &,
-    &:hover {
+    &:hover,
+    &:focus,
+    &:focus-visible {
       color: ${getTextColor};
+      box-shadow: none;
+      outline: none;
     }
     ${p => p.disabled && `cursor: default;`}
 

static/app/components/core/menuListItem/index.tsx

@@ -2,7 +2,7 @@ import {memo, useId, useRef, useState} from 'react';
 import {createPortal} from 'react-dom';
 import {usePopper} from 'react-popper';
 import isPropValid from '@emotion/is-prop-valid';
-import {type Theme, useTheme} from '@emotion/react';
+import {css, type Theme, useTheme} from '@emotion/react';
 import styled from '@emotion/styled';
 import {mergeRefs} from '@react-aria/utils';
 
@@ -123,7 +123,6 @@ function BaseMenuListItem({
 
   return (
     <MenuItemWrap
-      role="menuitem"
       aria-disabled={disabled}
       aria-labelledby={labelId}
       aria-describedby={detailId}
@@ -313,8 +312,12 @@ export const InnerWrap = withChonk(
     font-size: ${p => p.theme.form[p.size ?? 'md'].fontSize};
 
     &,
-    &:hover {
+    &:hover,
+    &:focus,
+    &:focus-visible {
       color: ${getTextColor};
+      box-shadow: none;
+      outline: none;
     }
     ${p => p.disabled && `cursor: default;`}
 
@@ -330,13 +333,13 @@ export const InnerWrap = withChonk(
 
     ${p =>
       p.isFocused &&
-      `
-      z-index: 1;
-      /* Background to hide the previous item's divider */
-      ::before {
-        background: ${p.theme.backgroundElevated};
-      }
-    `}
+      css`
+        z-index: 1;
+        /* Background to hide the previous item's divider */
+        ::before {
+          background: ${p.theme.backgroundElevated};
+        }
+      `}
   `,
   ChonkInnerWrap
 );

static/app/components/dropdownMenu/index.spec.tsx

@@ -1,5 +1,4 @@
 import {Fragment} from 'react';
-import {RouterFixture} from 'sentry-fixture/routerFixture';
 
 import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary';
 
@@ -25,10 +24,7 @@ describe('DropdownMenu', function () {
           },
         ]}
         triggerLabel="This is a Menu"
-      />,
-      {
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     // Open the mneu
@@ -71,10 +67,7 @@ describe('DropdownMenu', function () {
           },
         ]}
         triggerLabel="Menu"
-      />,
-      {
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await userEvent.click(screen.getByRole('button', {name: 'Menu'}));
@@ -95,10 +88,7 @@ describe('DropdownMenu', function () {
       <Fragment>
         <DropdownMenu items={[{key: 'item1', label: 'Item One'}]} triggerLabel="Menu A" />
         <DropdownMenu items={[{key: 'item2', label: 'Item Two'}]} triggerLabel="Menu B" />
-      </Fragment>,
-      {
-        deprecatedRouterMocks: true,
-      }
+      </Fragment>
     );
 
     // Can be dismissed by clicking outside
@@ -164,10 +154,7 @@ describe('DropdownMenu', function () {
           },
         ]}
         triggerLabel="Menu"
-      />,
-      {
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await userEvent.click(screen.getByRole('button', {name: 'Menu'}));
@@ -254,10 +241,7 @@ describe('DropdownMenu', function () {
           },
         ]}
         triggerLabel="Menu"
-      />,
-      {
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await userEvent.click(screen.getByRole('button', {name: 'Menu'}));
@@ -271,10 +255,7 @@ describe('DropdownMenu', function () {
       <DropdownMenu
         items={[{key: 'item1', label: 'Item One', to: '/test'}]}
         triggerLabel="Menu"
-      />,
-      {
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await userEvent.click(screen.getByRole('button', {name: 'Menu'}));
@@ -285,54 +266,48 @@ describe('DropdownMenu', function () {
   });
 
   it('closes after clicking external link', async function () {
+    const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+
     render(
       <DropdownMenu
         items={[{key: 'item1', label: 'Item One', externalHref: 'https://example.com'}]}
         triggerLabel="Menu"
-      />,
-      {
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await userEvent.click(screen.getByRole('button', {name: 'Menu'}));
     await userEvent.click(screen.getByRole('menuitemradio', {name: 'Item One'}));
     await waitFor(() => {
       expect(screen.queryByRole('menuitemradio')).not.toBeInTheDocument();
     });
+
+    // JSDOM throws an error on navigation to random urls
+    expect(errorSpy).toHaveBeenCalledTimes(1);
   });
 
   it('navigates to link on enter', async function () {
     const onAction = jest.fn();
-    const router = RouterFixture();
-    render(
+    const {router} = render(
       <DropdownMenu
         items={[
           {key: 'item1', label: 'Item One', to: '/test'},
           {key: 'item2', label: 'Item Two', to: '/test2', onAction},
         ]}
         triggerLabel="Menu"
-      />,
-      {
-        router,
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await userEvent.click(screen.getByRole('button', {name: 'Menu'}));
     await userEvent.keyboard('{ArrowDown}');
     await userEvent.keyboard('{Enter}');
     await waitFor(() => {
-      expect(router.push).toHaveBeenCalledWith(
-        expect.objectContaining({pathname: '/test2'})
-      );
+      expect(router.location.pathname).toBe('/test2');
     });
     expect(onAction).toHaveBeenCalledTimes(1);
   });
 
   it('navigates to link on meta key', async function () {
     const onAction = jest.fn();
-    const router = RouterFixture();
     const user = userEvent.setup();
 
     const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
@@ -344,11 +319,7 @@ describe('DropdownMenu', function () {
           {key: 'item2', label: 'Item Two', to: '/test2', onAction},
         ]}
         triggerLabel="Menu"
-      />,
-      {
-        router,
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await user.click(screen.getByRole('button', {name: 'Menu'}));
@@ -366,9 +337,10 @@ describe('DropdownMenu', function () {
 
   it('navigates to external link enter', async function () {
     const onAction = jest.fn();
-    const router = RouterFixture();
     const user = userEvent.setup();
 
+    const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+
     render(
       <DropdownMenu
         items={[
@@ -381,17 +353,15 @@ describe('DropdownMenu', function () {
           },
         ]}
         triggerLabel="Menu"
-      />,
-      {
-        router,
-        deprecatedRouterMocks: true,
-      }
+      />
     );
 
     await user.click(screen.getByRole('button', {name: 'Menu'}));
     await user.keyboard('{ArrowDown}');
     await user.keyboard('{Enter}');
 
     expect(onAction).toHaveBeenCalledTimes(1);
+    // JSDOM throws an error on navigation
+    expect(errorSpy).toHaveBeenCalledTimes(1);
   });
 });

static/app/components/dropdownMenu/index.tsx

@@ -218,7 +218,17 @@ function DropdownMenu({
     );
   }
 
-  const activeItems = useMemo(() => removeHiddenItems(items), [items]);
+  const activeItems = useMemo(
+    () =>
+      removeHiddenItems(items).map(item => {
+        return {
+          ...item,
+          // react-aria uses the href prop on item state to determine if the item is a link
+          href: item.to ?? item.externalHref,
+        };
+      }),
+    [items]
+  );
   const defaultDisabledKeys = useMemo(() => getDisabledKeys(activeItems), [activeItems]);
 
   function renderMenu() {