fix(menu): correctly identify menu item link with same URL as web page

packages/menu/demo/stories/MenuStory.tsx

@@ -54,16 +54,22 @@ const Item = ({ item, getItemProps, focusedValue, isSelected }: MenuItemProps) =
         'cursor-pointer': !item.disabled,
         'cursor-default': item.disabled
       })}
-      role={itemProps.href ? 'none' : undefined}
-      {...(!itemProps.href && (itemProps as LiHTMLAttributes<HTMLLIElement>))}
+      role={item.href ? 'none' : undefined}
+      {...(!item.href && (itemProps as LiHTMLAttributes<HTMLLIElement>))}
     >
-      {itemProps.href ? (
+      {item.href ? (
         <a
           {...(itemProps as AnchorHTMLAttributes<HTMLAnchorElement>)}
-          className="w-full rounded-sm outline-offset-0 transition-none border-width-none"
+          className={classNames(
+            ' w-full rounded-sm outline-offset-0 transition-none border-width-none',
+            {
+              'text-grey-400': item.disabled,
+              'cursor-default': item.disabled
+            }
+          )}
         >
           {itemChildren}
-          {!!item.isExternal && (
+          {!!item.external && (
             <>
               <span aria-hidden="true"> ↗</span>
               <span className="sr-only">(opens in new window)</span>

packages/menu/demo/stories/data.ts

@@ -16,9 +16,16 @@ export const ITEMS: MenuItem[] = [
     value: 'plant-04',
     label: 'Aloe Vera',
     href: 'https://en.wikipedia.org/wiki/Aloe_vera',
-    isExternal: false
+    external: false
   },
-  { value: 'plant-05', label: 'Succulent' },
+  {
+    value: 'plant-05',
+    label: 'Palm tree',
+    href: 'https://en.wikipedia.org/wiki/Palm_tree',
+    external: true,
+    disabled: true
+  },
+  { value: 'plant-06', label: 'Succulent' },
   {
     label: 'Choose favorites',
     items: [

packages/menu/src/MenuContainer.spec.tsx

@@ -6,7 +6,7 @@
  */
 
 import React, { useCallback, useRef, useState } from 'react';
-import { RenderResult, render, act, waitFor } from '@testing-library/react';
+import { act, createEvent, fireEvent, render, RenderResult, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { MenuItem, IMenuItemBase, IUseMenuProps, IUseMenuReturnValue } from './types';
 import { MenuContainer } from './';
@@ -283,14 +283,93 @@ describe('MenuContainer', () => {
       expect(menu).not.toBeVisible();
     });
 
-    it('applies external anchor attributes', () => {
-      const { getByTestId } = render(
-        <TestMenu items={[{ value: 'item', href: '#0', isExternal: true }]} />
-      );
-      const menu = getByTestId('menu');
+    describe('navigational menu items (links)', () => {
+      it('applies href and external anchor attributes, only when not disabled', () => {
+        const { getByTestId } = render(
+          <TestMenu
+            items={[
+              { value: 'link-1', href: '#1', external: true },
+              { value: 'link-2', href: '#2', external: true, disabled: true }
+            ]}
+          />
+        );
+        const menu = getByTestId('menu');
+
+        expect(menu.firstChild).toHaveAttribute('href', '#1');
+        expect(menu.firstChild).toHaveAttribute('target', '_blank');
+        expect(menu.firstChild).toHaveAttribute('rel', 'noopener noreferrer');
+
+        expect(menu.childNodes[1]).not.toHaveAttribute('href');
+        expect(menu.childNodes[1]).not.toHaveAttribute('target', '_blank');
+        expect(menu.childNodes[1]).not.toHaveAttribute('rel', 'noopener noreferrer');
+      });
+
+      it('tracks click events on links and alls the onChange function with the correct parameters', async () => {
+        const onChangeSpy = jest.fn();
+
+        const { getByTestId, getByText } = render(
+          <TestMenu
+            items={[
+              { value: 'link-1', href: '#1' },
+              { value: 'link-2', href: '#2' }
+            ]}
+            onChange={onChangeSpy}
+          />
+        );
+        const trigger = getByTestId('trigger');
+        const link = getByText('link-2');
+
+        await waitFor(async () => {
+          await user.click(trigger);
+          await user.click(link);
+        });
+
+        expect(onChangeSpy.mock.calls[2][0]).toStrictEqual({
+          type: 'menuItem:click',
+          value: 'link-2',
+          isExpanded: false,
+          selectedItems: []
+        });
+
+        expect(link).not.toHaveAttribute('aria-current', 'page');
+      });
+
+      it('supports setting the selected link via item.selected and ignore link selection changes', async () => {
+        const onChangeSpy = jest.fn();
+        const { getByTestId, getByText } = render(
+          <TestMenu
+            items={[
+              { value: 'link-1', href: '#1' },
+              { value: 'link-2', href: '#2', selected: true }
+            ]}
+            onChange={onChangeSpy}
+          />
+        );
+        const trigger = getByTestId('trigger');
+        const secondLink = getByText('link-2');
+
+        await waitFor(async () => {
+          await user.click(trigger);
+        });
+
+        expect(secondLink).toHaveAttribute('aria-current', 'page');
 
-      expect(menu.firstChild).toHaveAttribute('target', '_blank');
-      expect(menu.firstChild).toHaveAttribute('rel', 'noopener noreferrer');
+        const firstLink = getByText('link-1');
+
+        await waitFor(async () => {
+          await user.click(trigger);
+          await user.click(firstLink);
+        });
+
+        expect(onChangeSpy.mock.calls[3][0]).toStrictEqual({
+          type: 'menuItem:click',
+          value: 'link-1',
+          isExpanded: false,
+          selectedItems: [{ href: '#2', selected: true, value: 'link-2' }]
+        });
+
+        expect(firstLink).not.toHaveAttribute('aria-current');
+      });
     });
 
     describe('focus', () => {
@@ -1265,6 +1344,66 @@ describe('MenuContainer', () => {
         expect(changeTypes).toContain(StateChangeTypes.MenuItemKeyDownPrevious);
       });
     });
+
+    describe('navigational menu items (links)', () => {
+      it('applies the correct aria-current attribute to user-selected link', async () => {
+        const { getByTestId, getByText } = render(
+          <TestMenu
+            items={[
+              { value: 'link-1', href: '#1' },
+              { value: 'link-2', href: '#2' }
+            ]}
+            selectedItems={[{ value: 'link-2' }]}
+          />
+        );
+        const trigger = getByTestId('trigger');
+        const link = getByText('link-2');
+
+        await waitFor(async () => {
+          await user.click(trigger);
+        });
+
+        expect(link).toHaveAttribute('aria-current', 'page');
+      });
+
+      it('prevents default only when clicking on user-selected link', async () => {
+        const { getByTestId, getByText } = render(
+          <TestMenu
+            items={[
+              { value: 'link-1', href: '#1' },
+              { value: 'link-2', href: '#2' }
+            ]}
+            selectedItems={[{ value: 'link-2' }]}
+          />
+        );
+        const trigger = getByTestId('trigger');
+        const firstLink = getByText('link-1');
+        const secondLink = getByText('link-2');
+
+        await waitFor(async () => {
+          await user.click(trigger);
+          await user.click(firstLink);
+        });
+
+        expect(firstLink).not.toHaveAttribute('aria-current');
+
+        const firstLinkClickEvent = createEvent.click(firstLink);
+        firstLinkClickEvent.preventDefault = jest.fn();
+        fireEvent(firstLink, firstLinkClickEvent);
+
+        // fire click event one more time to test behavior on previously clicked anchor
+        fireEvent(firstLink, firstLinkClickEvent);
+
+        expect(firstLinkClickEvent.preventDefault).toHaveBeenCalledTimes(0);
+
+        const secondLinkClickEvent = createEvent.click(secondLink);
+        secondLinkClickEvent.preventDefault = jest.fn();
+        fireEvent(secondLink, secondLinkClickEvent);
+
+        expect(secondLinkClickEvent.preventDefault).toHaveBeenCalledTimes(1);
+        expect(secondLink).toHaveAttribute('aria-current', 'page');
+      });
+    });
   });
 
   describe('error handling', () => {

packages/menu/src/types.ts

@@ -14,7 +14,7 @@ export interface ISelectedItem {
   type?: 'radio' | 'checkbox';
   disabled?: boolean;
   href?: string;
-  isExternal?: boolean;
+  external?: boolean;
 }
 
 export interface IMenuItemBase extends ISelectedItem {
@@ -42,8 +42,10 @@ export interface IUseMenuProps<T = HTMLButtonElement, M = HTMLElement> {
    * @param {string} item.value Unique item value
    * @param {string} item.label Optional human-readable text value (defaults to `item.value`)
    * @param {string} item.name A shared name corresponding to an item radio group
+   * @param {string} item.href The URL to navigate to when the link item is clicked
+   * @param {boolean} item.external Indicates that link item is an external link
    * @param {boolean} item.disabled Indicates the item is not interactive
-   * @param {boolean} item.selected Sets initial selection for the option
+   * @param {boolean} item.selected Sets initial selection for the option. The initial selection persists for link items.
    * @param {boolean} item.isNext - Indicates the item transitions to a nested menu
    * @param {boolean} item.isPrevious - Indicates the item will transition back from a nested menu
    * @param {boolean} item.separator Indicates the item is a placeholder for a separator

packages/menu/src/useMenu.ts

@@ -73,9 +73,12 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
   );
   const initialSelectedItems = useMemo(
     () =>
-      menuItems.filter(
-        item => !!(item.type && ['radio', 'checkbox'].includes(item.type) && item.selected)
-      ),
+      menuItems.filter(item => {
+        return !!(
+          (item.href || item.type === 'radio' || item.type === 'checkbox') &&
+          item.selected
+        );
+      }),
     [menuItems]
   );
   const values = useMemo(() => menuItems.map(item => item.value), [menuItems]);
@@ -155,12 +158,12 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
   );
 
   const isItemSelected = useCallback(
-    (value: string, type?: string, name?: string): boolean | undefined => {
+    (value: string, type?: string, name?: string, href?: string): boolean | undefined => {
       let isSelected;
 
       if (type === 'checkbox') {
         isSelected = !!controlledSelectedItems.find(item => item.value === value);
-      } else if (type === 'radio') {
+      } else if (type === 'radio' || href) {
         const match = controlledSelectedItems.filter(item => item.name === name)[0];
 
         isSelected = match?.value === value;
@@ -218,11 +221,12 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
   );
 
   const getSelectedItems = useCallback(
-    ({ value, type, name, label, selected }: IMenuItemBase) => {
-      let changes: ISelectedItem[] | null = [...controlledSelectedItems];
-
+    ({ value, type, name, label, selected, href }: IMenuItemBase) => {
+      if (href) return controlledSelectedItems;
       if (!type) return null;
 
+      let changes: ISelectedItem[] | null = [...controlledSelectedItems];
+
       const selectedItem = {
         value,
         type,
@@ -428,15 +432,17 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
   }, [onChange]);
 
   const handleItemClick = useCallback(
-    (item: IMenuItemBase) => {
+    (event: React.MouseEvent, item: IMenuItemBase) => {
       let changeType = StateChangeTypes.MenuItemClick;
-      const { isNext, isPrevious } = item;
+      const { isNext, isPrevious, href, selected } = item;
       const isTransitionItem = isNext || isPrevious;
 
       if (isNext) {
         changeType = StateChangeTypes.MenuItemClickNext;
       } else if (isPrevious) {
         changeType = StateChangeTypes.MenuItemClickPrevious;
+      } else if (href && selected) {
+        event.preventDefault();
       }
 
       const nextSelection = getSelectedItems(item);
@@ -802,7 +808,7 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
         name,
         value,
         href,
-        isExternal,
+        external,
         isNext = false,
         isPrevious = false,
         label = value
@@ -815,7 +821,7 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
         itemRole = 'menuitemcheckbox';
       }
 
-      const selected = isItemSelected(value, type, name);
+      const selected = isItemSelected(value, type, name, href);
 
       /**
        * The "select" of useSelection isn't
@@ -829,27 +835,44 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
         'data-garden-container-id': 'containers.menu.item',
         'data-garden-container-version': PACKAGE_VERSION,
         'aria-selected': undefined,
-        'aria-checked': selected,
         'aria-disabled': itemDisabled,
         role: itemRole === null ? undefined : itemRole,
-        href,
         onClick,
         onKeyDown,
         onMouseEnter,
         ...other
       };
 
-      if (href && isExternal) {
-        elementProps.target = '_blank';
-        elementProps.rel = 'noopener noreferrer';
-      }
+      if (href) {
+        /**
+         * Validation
+         */
+        if (isNext || isPrevious || type) {
+          anchorItemError(item);
+        }
 
-      /**
-       * Validation
-       */
+        /**
+         * For a disabled link to be valid, it must have:
+         *  - `aria-disabled="true"`
+         *  - `role="link"`, or `role="menuitem"` if within a menu
+         *  - an `undefined` `href`
+         *
+         * @see https://www.scottohara.me/blog/2021/05/28/disabled-links.html
+         */
+        if (!itemDisabled) {
+          elementProps.href = href;
+
+          if (external) {
+            elementProps.target = '_blank';
+            elementProps.rel = 'noopener noreferrer';
+          }
 
-      if (href && (isNext || isPrevious || type)) {
-        anchorItemError(item);
+          if (selected) {
+            elementProps['aria-current'] = 'page';
+          }
+        }
+      } else {
+        elementProps['aria-checked'] = selected;
       }
 
       if (itemDisabled) {
@@ -859,8 +882,8 @@ export const useMenu = <T extends HTMLElement = HTMLElement, M extends HTMLEleme
       const itemProps = getElementProps({
         value: value as any,
         ...elementProps,
-        onClick: composeEventHandlers(onClick, () =>
-          handleItemClick({ ...item, label, selected, isNext, isPrevious })
+        onClick: composeEventHandlers(onClick, (e: React.MouseEvent) =>
+          handleItemClick(e, { ...item, label, selected, isNext, isPrevious })
         ),
         onKeyDown: composeEventHandlers(onKeyDown, (e: React.KeyboardEvent) =>
           handleItemKeyDown(e, { ...item, label, selected, isNext, isPrevious })