Fix Nav Flyout Accessibility

packages/react-core/src/components/Menu/MenuItem.tsx

@@ -22,7 +22,7 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
   className?: string;
   /** Identifies the component in the Menu onSelect or onActionClick callback */
   itemId?: any;
-  /** Target navigation link */
+  /** Target navigation link. Should not be used if the flyout prop is defined. */
   to?: string;
   /** @beta Flag indicating the item has a checkbox */
   hasCheck?: boolean;
@@ -54,7 +54,7 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
   isFocused?: boolean;
   /** Flag indicating the item is in danger state */
   isDanger?: boolean;
-  /** @beta Flyout menu */
+  /** @beta Flyout menu. Should not be used if the to prop is defined. */
   flyoutMenu?: React.ReactElement;
   /** @beta Callback function when mouse leaves trigger */
   onShowFlyout?: (event?: any) => void;
@@ -202,6 +202,7 @@ const MenuItemBase: React.FunctionComponent<MenuItemProps> = ({
 
     if (key === ' ' || key === 'Enter' || key === 'ArrowRight' || type === 'click') {
       event.stopPropagation();
+      event.preventDefault();
       if (!flyoutVisible) {
         showFlyout(true);
         setFlyoutTarget(target as HTMLElement);

packages/react-core/src/components/Nav/NavItem.tsx

@@ -14,7 +14,7 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
   styleChildren?: boolean;
   /** Additional classes added to the nav item */
   className?: string;
-  /** Target navigation link */
+  /** Target navigation link. Should not be used if the flyout prop is defined. */
   to?: string;
   /** Flag indicating whether the item is active */
   isActive?: boolean;
@@ -28,7 +28,7 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
   onClick?: NavSelectClickHandler;
   /** Component used to render NavItems if  React.isValidElement(children) is false */
   component?: React.ReactNode;
-  /** Flyout of a nav item. This should be a Menu component. */
+  /** Flyout of a nav item. This should be a Menu component. Should not be used if the to prop is defined. */
   flyout?: React.ReactElement;
   /** Callback when flyout is opened or closed */
   onShowFlyout?: () => void;
@@ -68,8 +68,14 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
   const ref = React.useRef<HTMLLIElement>();
   const flyoutVisible = ref === flyoutRef;
   const popperRef = React.useRef<HTMLDivElement>();
-  const Component = component as any;
   const hasFlyout = flyout !== undefined;
+  const Component = hasFlyout ? 'button' : (component as any);
+
+  // A NavItem should not be both a link and a flyout
+  if (to && hasFlyout) {
+    // eslint-disable-next-line no-console
+    console.error('NavItem cannot have both "to" and "flyout" props.');
+  }
 
   const showFlyout = (show: boolean, override?: boolean) => {
     if ((!flyoutVisible || override) && show) {
@@ -106,11 +112,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
     const key = event.key;
     const target = event.target as HTMLElement;
 
-    if (!(popperRef?.current?.contains(target) || (hasFlyout && ref?.current?.contains(target)))) {
-      return;
-    }
-
-    if (key === ' ' || key === 'ArrowRight') {
+    if ((key === ' ' || key === 'Enter' || key === 'ArrowRight') && hasFlyout && ref?.current?.contains(target)) {
       event.stopPropagation();
       event.preventDefault();
       if (!flyoutVisible) {
@@ -119,7 +121,9 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
       }
     }
 
-    if (key === 'Escape' || key === 'ArrowLeft') {
+    // We only want the NavItem to handle closing a flyout menu if only the first level flyout is open.
+    // Otherwise, MenuItem should handle closing its flyouts
+    if ((key === 'Escape' || key === 'ArrowLeft') && popperRef?.current?.querySelectorAll('.pf-c-menu').length === 1) {
       if (flyoutVisible) {
         event.stopPropagation();
         event.preventDefault();
@@ -165,6 +169,8 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
     'aria-expanded': flyoutVisible
   };
 
+  const tabIndex = isNavOpen ? null : -1;
+
   const renderDefaultLink = (context: any): React.ReactNode => {
     const preventLinkDefault = preventDefault || !to;
     return (
@@ -178,7 +184,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
           className
         )}
         aria-current={isActive ? 'page' : null}
-        tabIndex={isNavOpen ? null : '-1'}
+        tabIndex={tabIndex}
         {...(hasFlyout && { ...ariaFlyoutProps })}
         {...props}
       >
@@ -195,7 +201,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
       ...(styleChildren && {
         className: css(styles.navLink, isActive && styles.modifiers.current, child.props && child.props.className)
       }),
-      tabIndex: child.props.tabIndex || isNavOpen ? null : -1,
+      tabIndex: child.props.tabIndex || tabIndex,
       children: hasFlyout ? (
         <React.Fragment>
           {child.props.children}

packages/react-core/src/components/Nav/examples/NavFlyout.tsx

@@ -19,12 +19,7 @@ export const NavFlyout: React.FunctionComponent = () => {
     <Menu key={depth} containsFlyout isNavFlyout id={`nav-flyout-menu-${depth}`} onSelect={onMenuSelect}>
       <MenuContent>
         <MenuList>
-          <MenuItem
-            onClick={onMenuItemClick}
-            flyoutMenu={children}
-            itemId={`nav-flyout-next-menu-${depth}`}
-            to={`#next-menu-link-${depth}`}
-          >
+          <MenuItem onClick={onMenuItemClick} flyoutMenu={children} itemId={`nav-flyout-next-menu-${depth}`}>
             Next menu
           </MenuItem>
           {Array.apply(0, Array(numFlyouts - depth)).map((_item, index: number) => (
@@ -38,12 +33,7 @@ export const NavFlyout: React.FunctionComponent = () => {
               Menu {depth} item {index}
             </MenuItem>
           ))}
-          <MenuItem
-            onClick={onMenuItemClick}
-            flyoutMenu={children}
-            itemId={`nav-flyout-next-menu-2-${depth}`}
-            to={`#next-menu-2-link-${depth}`}
-          >
+          <MenuItem onClick={onMenuItemClick} flyoutMenu={children} itemId={`nav-flyout-next-menu-2-${depth}`}>
             Next menu
           </MenuItem>
         </MenuList>
@@ -81,7 +71,6 @@ export const NavFlyout: React.FunctionComponent = () => {
           preventDefault
           flyout={curFlyout}
           id="nav-flyout-default-link-3"
-          to="#nav-flyout-default-link-3"
           itemId="nav-flyout-default-link-3"
           isActive={activeItem === 'nav-flyout-default-link-3'}
         >

packages/react-core/src/demos/Nav.md

@@ -1463,15 +1463,15 @@ class VerticalPage extends React.Component {
       <Menu key={depth} containsFlyout isNavFlyout id={`menu-${depth}`} onSelect={this.onMenuSelect}>
         <MenuContent>
           <MenuList>
-            <MenuItem flyoutMenu={children} itemId={`next-menu-${depth}`} to={`#menu-link-${depth}`}>
+            <MenuItem flyoutMenu={children} itemId={`next-menu-${depth}`}>
               Additional settings
             </MenuItem>
             {[...Array(numFlyouts - depth).keys()].map(j => (
               <MenuItem key={`${depth}-${j}`} itemId={`${depth}-${j}`} to={`#menu-link-${depth}-${j}`}>
                 Settings menu {depth} item {j}
               </MenuItem>
             ))}
-            <MenuItem flyoutMenu={children} itemId={`next-menu-2-${depth}`} to={`#second-menu-link-${depth}`}>
+            <MenuItem flyoutMenu={children} itemId={`next-menu-2-${depth}`}>
               Additional settings
             </MenuItem>
           </MenuList>

packages/react-core/src/next/components/Dropdown/DropdownItem.tsx

@@ -10,6 +10,8 @@ export interface DropdownItemProps extends Omit<MenuItemProps, 'ref'>, OUIAProps
   className?: string;
   /** Description of the dropdown item */
   description?: React.ReactNode;
+  /** Identifies the component in the dropdown onSelect callback */
+  itemId?: any;
   /** Value to overwrite the randomly generated data-ouia-component-id.*/
   ouiaId?: number | string;
   /** Set the value of data-ouia-safe. Only set to true when the component is in a static state, i.e. no animations are occurring. At all other times, this value must be false. */
@@ -20,13 +22,14 @@ export const DropdownItem: React.FunctionComponent<MenuItemProps> = ({
   children,
   className,
   description,
+  itemId,
   ouiaId,
   ouiaSafe,
   ...props
 }: DropdownItemProps) => {
   const ouiaProps = useOUIAProps(DropdownItem.displayName, ouiaId, ouiaSafe);
   return (
-    <MenuItem className={css(className)} description={description} {...ouiaProps} {...props}>
+    <MenuItem className={css(className)} description={description} itemId={itemId} {...ouiaProps} {...props}>
       {children}
     </MenuItem>
   );

packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx

@@ -384,13 +384,7 @@ export class NavDemo extends Component {
               <NavItem id="flyout-link2" to="#flyout-link2" itemId={1} isActive={flyoutActiveItem === 1}>
                 Link 2
               </NavItem>
-              <NavItem
-                flyout={curFlyout}
-                id="flyout-link3"
-                to="#flyout-link3"
-                itemId={2}
-                isActive={flyoutActiveItem === 2}
-              >
+              <NavItem flyout={curFlyout} id="flyout-link3" itemId={2} isActive={flyoutActiveItem === 2}>
                 Link 3
               </NavItem>
               <NavItem id="flyout-link4" to="#flyout-link4" itemId={3} isActive={flyoutActiveItem === 3}>