From 4b019af83159c9b1d90ae20e55203ad49aa658b5 Mon Sep 17 00:00:00 2001 From: Rebecca Alpert Date: Mon, 17 Aug 2026 14:50:13 -0400 Subject: [PATCH 1/2] fix(Menu): Allow for different MenuItem roles MenuItems should correctly support list behavior, where MenuItems do not have a role. Related to https://github.com/patternfly/chatbot/issues/827 Assisted-by: Cursor --- .../react-core/src/components/Menu/Menu.tsx | 3 +- .../src/components/Menu/MenuContext.ts | 19 +++++ .../src/components/Menu/MenuItem.tsx | 8 +- .../src/components/Menu/MenuItemAction.tsx | 77 ++++++++++--------- .../components/Menu/__tests__/Menu.test.tsx | 56 ++++++++++++++ 5 files changed, 122 insertions(+), 41 deletions(-) diff --git a/packages/react-core/src/components/Menu/Menu.tsx b/packages/react-core/src/components/Menu/Menu.tsx index 73f7d4ac20b..ba91953e1e6 100644 --- a/packages/react-core/src/components/Menu/Menu.tsx +++ b/packages/react-core/src/components/Menu/Menu.tsx @@ -63,7 +63,8 @@ export interface MenuProps extends Omit, 'r /** 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. */ ouiaSafe?: boolean; /** Determines the accessible role of the menu. For a non-checkbox menu that can have - * one or more items selected, pass in "listbox". */ + * one or more items selected, pass in "listbox". For a static list of actions that is not + * a dismissible menu widget, pass in "list". */ role?: string; } diff --git a/packages/react-core/src/components/Menu/MenuContext.ts b/packages/react-core/src/components/Menu/MenuContext.ts index d1c9d626240..9842ff300dd 100644 --- a/packages/react-core/src/components/Menu/MenuContext.ts +++ b/packages/react-core/src/components/Menu/MenuContext.ts @@ -45,3 +45,22 @@ export const MenuItemContext = createContext<{ itemId: null, isDisabled: false }); + +/** Returns the ARIA role for a menu item's interactive element based on the parent menu role. */ +export const getMenuItemInteractiveRole = (menuRole?: string): string | undefined => { + if (menuRole === 'listbox') { + return 'option'; + } + if (menuRole === 'list') { + return undefined; + } + return 'menuitem'; +}; + +/** Returns the ARIA role for a menu item's list item wrapper based on the parent menu role. */ +export const getMenuListItemRole = (menuRole: string | undefined, hasCheckbox: boolean): string | undefined => { + if (menuRole === 'list') { + return undefined; + } + return hasCheckbox ? 'menuitem' : 'none'; +}; diff --git a/packages/react-core/src/components/Menu/MenuItem.tsx b/packages/react-core/src/components/Menu/MenuItem.tsx index 355d77ad0a1..4245e0ea1ef 100644 --- a/packages/react-core/src/components/Menu/MenuItem.tsx +++ b/packages/react-core/src/components/Menu/MenuItem.tsx @@ -9,7 +9,7 @@ import RhMicronsCaretLeftIcon from '@patternfly/react-icons/dist/esm/icons/rh-mi import RhMicronsCaretRightIcon from '@patternfly/react-icons/dist/esm/icons/rh-microns-caret-right-icon'; import RhMicronsCheckmarkIcon from '@patternfly/react-icons/dist/esm/icons/rh-microns-checkmark-icon'; import { Checkbox } from '../Checkbox'; -import { MenuContext, MenuItemContext } from './MenuContext'; +import { getMenuItemInteractiveRole, getMenuListItemRole, MenuContext, MenuItemContext } from './MenuContext'; import { MenuItemAction } from './MenuItemAction'; import { Tooltip, TooltipProps } from '../Tooltip'; import { canUseDOM } from '../../helpers/util'; @@ -341,6 +341,8 @@ const MenuItemBase: React.FunctionComponent = ({ }, [isFocused]); const isSelectMenu = menuRole === 'listbox'; + const interactiveRole = !hasCheckbox && !flyoutMenu ? getMenuItemInteractiveRole(menuRole) : undefined; + const listItemRole = getMenuListItemRole(menuRole, hasCheckbox); const renderItem = ( <> @@ -350,7 +352,7 @@ const MenuItemBase: React.FunctionComponent = ({ className={css(styles.menuItem, getIsSelected() && !hasCheckbox && styles.modifiers.selected, className)} aria-current={getAriaCurrent()} {...(!hasCheckbox && { disabled: isDisabled, 'aria-label': ariaLabel })} - {...(!hasCheckbox && !flyoutMenu && { role: isSelectMenu ? 'option' : 'menuitem' })} + {...(interactiveRole !== undefined && { role: interactiveRole })} {...(!hasCheckbox && !flyoutMenu && isSelectMenu && { 'aria-selected': getIsSelected() })} ref={innerComponentRef} {...(!hasCheckbox && { @@ -452,7 +454,7 @@ const MenuItemBase: React.FunctionComponent = ({ }} {...(flyoutMenu && !isAriaDisabled && { onKeyDown: handleFlyout })} ref={ref} - role={!hasCheckbox ? 'none' : 'menuitem'} + {...(listItemRole !== undefined && { role: listItemRole })} {...(hasCheckbox && { 'aria-label': ariaLabel })} {...props} > diff --git a/packages/react-core/src/components/Menu/MenuItemAction.tsx b/packages/react-core/src/components/Menu/MenuItemAction.tsx index d01ce5d22be..855acd86798 100644 --- a/packages/react-core/src/components/Menu/MenuItemAction.tsx +++ b/packages/react-core/src/components/Menu/MenuItemAction.tsx @@ -1,7 +1,7 @@ import { forwardRef } from 'react'; import styles from '@patternfly/react-styles/css/components/Menu/menu'; import { css } from '@patternfly/react-styles'; -import { MenuContext, MenuItemContext } from './MenuContext'; +import { getMenuItemInteractiveRole, MenuContext, MenuItemContext } from './MenuContext'; import { Button } from '../Button'; export interface MenuItemActionProps extends React.HTMLProps { /** Additional classes added to the action button */ @@ -34,42 +34,45 @@ const MenuItemActionBase: React.FunctionComponent = ({ ...props }: MenuItemActionProps) => ( - {({ onActionClick }) => ( - - {({ itemId, isDisabled: isDisabledContext }) => { - const onClickButton = (event: any) => { - // event specified on the MenuItemAction - onClick && onClick(event); - // event specified on the Menu - onActionClick && onActionClick(event, itemId, actionId); - }; - return ( -
-
- ); - }} - - )} + {({ onActionClick, role: menuRole }) => { + const interactiveRole = getMenuItemInteractiveRole(menuRole); + return ( + + {({ itemId, isDisabled: isDisabledContext }) => { + const onClickButton = (event: any) => { + // event specified on the MenuItemAction + onClick && onClick(event); + // event specified on the Menu + onActionClick && onActionClick(event, itemId, actionId); + }; + return ( +
+
+ ); + }} + + ); + }} ); diff --git a/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx b/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx index 08b9ffcbdec..9dd99c3e92a 100644 --- a/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx +++ b/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx @@ -94,4 +94,60 @@ describe('Menu', () => { expect(screen.getByText('Checkbox 1')).toBeInTheDocument(); }); }); + + describe('with role="list"', () => { + test('should render list semantics on menu items', () => { + render( + + + + Item + + + + ); + + expect(screen.getByRole('list')).toBeInTheDocument(); + + const listItem = screen.getByRole('listitem'); + expect(listItem).not.toHaveAttribute('role'); + + const button = screen.getByRole('button', { name: 'Item' }); + expect(button).not.toHaveAttribute('role'); + }); + }); + + describe('with role="listbox"', () => { + test('should render option semantics on menu items', () => { + render( + + + + Item + + + + ); + + expect(screen.getByRole('listbox')).toBeInTheDocument(); + expect(screen.getByRole('option', { name: 'Item' })).toBeInTheDocument(); + }); + }); + + describe('with default menu role', () => { + test('should render menuitem semantics on menu items', () => { + render( + + + + Item + + + + ); + + expect(screen.getByRole('menu')).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: 'Item' })).toBeInTheDocument(); + }); + }); }); From ae41bf418dd344cd94036d0db2b18d19ed5b1dd3 Mon Sep 17 00:00:00 2001 From: Rebecca Alpert Date: Mon, 17 Aug 2026 15:07:49 -0400 Subject: [PATCH 2/2] Address CodeRabbit feedback --- .../src/components/Menu/MenuContext.ts | 8 +++++++ .../src/components/Menu/MenuItemAction.tsx | 4 ++-- .../components/Menu/__tests__/Menu.test.tsx | 24 +++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/packages/react-core/src/components/Menu/MenuContext.ts b/packages/react-core/src/components/Menu/MenuContext.ts index 9842ff300dd..1fc9fc01c15 100644 --- a/packages/react-core/src/components/Menu/MenuContext.ts +++ b/packages/react-core/src/components/Menu/MenuContext.ts @@ -57,6 +57,14 @@ export const getMenuItemInteractiveRole = (menuRole?: string): string | undefine return 'menuitem'; }; +/** Returns the ARIA role for a menu item action button based on the parent menu role. */ +export const getMenuItemActionInteractiveRole = (menuRole?: string): string | undefined => { + if (menuRole === 'listbox' || menuRole === 'list') { + return undefined; + } + return 'menuitem'; +}; + /** Returns the ARIA role for a menu item's list item wrapper based on the parent menu role. */ export const getMenuListItemRole = (menuRole: string | undefined, hasCheckbox: boolean): string | undefined => { if (menuRole === 'list') { diff --git a/packages/react-core/src/components/Menu/MenuItemAction.tsx b/packages/react-core/src/components/Menu/MenuItemAction.tsx index 855acd86798..f83962e2c00 100644 --- a/packages/react-core/src/components/Menu/MenuItemAction.tsx +++ b/packages/react-core/src/components/Menu/MenuItemAction.tsx @@ -1,7 +1,7 @@ import { forwardRef } from 'react'; import styles from '@patternfly/react-styles/css/components/Menu/menu'; import { css } from '@patternfly/react-styles'; -import { getMenuItemInteractiveRole, MenuContext, MenuItemContext } from './MenuContext'; +import { getMenuItemActionInteractiveRole, MenuContext, MenuItemContext } from './MenuContext'; import { Button } from '../Button'; export interface MenuItemActionProps extends React.HTMLProps { /** Additional classes added to the action button */ @@ -35,7 +35,7 @@ const MenuItemActionBase: React.FunctionComponent = ({ }: MenuItemActionProps) => ( {({ onActionClick, role: menuRole }) => { - const interactiveRole = getMenuItemInteractiveRole(menuRole); + const interactiveRole = getMenuItemActionInteractiveRole(menuRole); return ( {({ itemId, isDisabled: isDisabledContext }) => { diff --git a/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx b/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx index 9dd99c3e92a..bc0af10c129 100644 --- a/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx +++ b/packages/react-core/src/components/Menu/__tests__/Menu.test.tsx @@ -3,6 +3,7 @@ import '@testing-library/jest-dom'; import { Menu } from '../Menu'; import { MenuItem, MenuItemProps } from '../MenuItem'; +import { MenuItemAction } from '../MenuItemAction'; import { MenuList } from '../MenuList'; import { MenuContent } from '../MenuContent'; @@ -132,6 +133,29 @@ describe('Menu', () => { expect(screen.getByRole('listbox')).toBeInTheDocument(); expect(screen.getByRole('option', { name: 'Item' })).toBeInTheDocument(); }); + + test('should not expose menu item actions as options', () => { + render( + + + + } + > + Item + + + + + ); + + expect(screen.getByRole('option', { name: 'Item' })).toBeInTheDocument(); + + const actionButton = screen.getByRole('button', { name: 'Favorite action' }); + expect(actionButton).not.toHaveAttribute('role'); + expect(screen.queryByRole('option', { name: 'Favorite action' })).not.toBeInTheDocument(); + }); }); describe('with default menu role', () => {