Fix Nav Flyout Accessibility (#8279)

* fix(NavItem): disallow flyout and link props to both be defined on NavItem * docs(Navigation): remove links from NavItems with flyouts * fix(NavItem): remove aria-expanded prop * fix(NavItem): use a button for Component if there is a flyout * fix(NavItem): allow flyout to be opened on pressing Enter * fix(MenuItem): disallow flyoutMenu and link props to both be defined * docs(Navigation): remove links from MenuItems with flyouts * fix(SelectOption): make props conditional to align with MenuItemProps * fix(DropdownItem): make props conditional to align with MenuItemProps * fix(NavItem): revert conditional prop changes * fix(NavItem): fix opening nested MenuItem-based flyouts with keyboard * fix(NavItem): add a console.err message if to and flyout are used together
patternfly · Dec 8, 2022 · 09ac7d4 · 09ac7d4
1 parent d0a31d9
commit 09ac7d4
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 35 deletions.
diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/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);

diff --git a/packages/react-core/src/components/Nav/NavItem.tsx b/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}

diff --git a/packages/react-core/src/components/Nav/examples/NavFlyout.tsx b/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'}
         >

diff --git a/packages/react-core/src/demos/Nav.md b/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>

diff --git a/packages/react-integration/demo-app-ts/src/components/demos/NavDemo/NavDemo.tsx b/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}>