Improve Menu role support (CMEM-7707)

CHANGELOG.md

@@ -6,6 +6,19 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 
 ## [Unreleased]
 
+### Added
+
+- `<Menu />`
+    - `role` property: `menu` (default), `navigation`, `list`, `listbox`, `none` or `presentation` get special support, other ARIA roles are set as they are
+        - `navigation` is rendered as `list`, the `nav` wrapper must be added by yourself
+    - sets `roleStructure` of `<MenuItem />` children automatically according to the `role` if it is not given, otherwise a console warning is emitted if it does not fit
+    - emits a console warning if an explicitly set `role` of a `<MenuItem />` child does not fit to its (explicitly or automatically set) `roleStructure`
+    - sets `role="listitem"` (lists) or `role="none"` (listboxes) on `<MenuDivider />` children, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"`
+- `<MenuDivider />`
+    - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden`
+- `<ContextMenu />`
+    - `role` property: `menu` (default) or `listbox`, forwarded to the included `<Menu />` and used for `aria-haspopup` of the toggler
+
 ### Fixed
 
 - Fix misused menu roles in selection popups and overlays

src/components/ContextOverlay/ContextMenu.stories.tsx

@@ -23,11 +23,12 @@ const Template: StoryFn<typeof ContextMenu> = (args) => (
 
 export const Default = Template.bind({});
 Default.args = {
+    role: "menu",
     children: [
-        <MenuItem key="m0" text={"First option"} />,
-        <MenuItem key="m1" text={"Item two"}>
-            <MenuItem key="m2" text={"First sub option"} />
-            <MenuItem key="m3" text={"Sub item two"} />
+        <MenuItem key="m0" roleStructure="menuitem" text={"First option"} />,
+        <MenuItem key="m1" roleStructure="menuitem" text={"Item two"}>
+            <MenuItem key="m2" roleStructure="menuitem" text={"First sub option"} />
+            <MenuItem key="m3" roleStructure="menuitem" text={"Sub item two"} />
         </MenuItem>,
     ],
 };

src/components/ContextOverlay/ContextMenu.tsx

@@ -5,7 +5,7 @@ import { CLASSPREFIX as eccgui } from "../../configuration/constants";
 import { ValidIconName } from "../Icon/canonicalIconNames";
 import { IconButton, IconButtonProps } from "../Icon/IconButton";
 import { TestableComponent } from "../interfaces";
-import Menu from "../Menu/Menu";
+import Menu, { MenuProps } from "../Menu/Menu";
 
 import ContextOverlay, { ContextOverlayProps } from "./ContextOverlay";
 
@@ -19,6 +19,12 @@ export interface ContextMenuProps extends TestableComponent {
      * They will be wrapped in a `Menu` element automatically.
      */
     children?: React.JSX.Element | React.JSX.Element[];
+    /**
+     * Role of the included `Menu` element.
+     * It defines the `roleStructure` of the `MenuItem` children, e.g. use `listbox` for selectable options.
+     * It is also used for `aria-haspopup` of the toggler element.
+     */
+    role?: "menu" | "listbox";
     /**
      * Toggler that need to be used to display menu.
      * If a valid icon name is used then the icon element is displayed.
@@ -46,6 +52,11 @@ export interface ContextMenuProps extends TestableComponent {
      * Props to spread to `ContextOverlay` that is used to display the dropdown.
      */
     contextOverlayProps?: Partial<Omit<ContextOverlayProps, "content" | "children" | "className">>;
+    /**
+     * Props to spread to the included `Menu` element, e.g. additional `aria-*` attributes.
+     * Use the `role` property of `ContextMenu` to set the role of the menu.
+     */
+    menuProps?: Partial<Omit<MenuProps, "children" | "role">>;
     /**
      * Disables the button to open the menu.
      */
@@ -63,9 +74,11 @@ export interface ContextMenuProps extends TestableComponent {
 export const ContextMenu = ({
     children,
     className = "",
+    role = "menu",
     togglerElement = "item-moremenu",
     togglerText = "Show more options",
     contextOverlayProps,
+    menuProps,
     disabled,
     togglerLarge = false,
     togglerSize,
@@ -94,11 +107,15 @@ export const ContextMenu = ({
 
     return (
         <ContextOverlay
-            popupKind={PopupKind.MENU}
             {...restProps}
             {...contextOverlayProps}
             className={`${eccgui}-contextmenu ` + className}
-            content={<Menu>{children}</Menu>}
+            content={
+                <Menu {...menuProps} role={role}>
+                    {children}
+                </Menu>
+            }
+            popupKind={role === "listbox" ? PopupKind.LISTBOX : PopupKind.MENU}
             disabled={!!disabled}
             usePlaceholder={!preventPlaceholder}
         >

src/components/ContextOverlay/tests/ContextMenu.test.tsx

@@ -4,6 +4,7 @@ import { fireEvent, render, screen } from "@testing-library/react";
 import "@testing-library/jest-dom";
 
 import { CLASSPREFIX as eccgui } from "../../../configuration/constants";
+import MenuItem from "../../Menu/MenuItem";
 
 import ContextMenu from "./../ContextMenu";
 import { Default as ContextMenuStory } from "./../ContextMenu.stories";
@@ -43,4 +44,49 @@ describe("ContextMenu", () => {
         expect(screen.getByRole("button")).toHaveAttribute("aria-haspopup", "menu");
         expect(screen.getByRole("menu")).toBeVisible();
     });
+    it("should use role `menu` for the menu by default", async () => {
+        const { container } = render(<ContextMenu {...ContextMenuStory.args} preventPlaceholder={true} />);
+        fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]);
+        expect(await screen.findByRole("menu")).toBeInTheDocument();
+    });
+    it("should forward `role` to the menu", async () => {
+        const { container } = render(
+            <ContextMenu role="listbox" preventPlaceholder={true}>
+                <MenuItem text="First option" />
+            </ContextMenu>,
+        );
+        fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]);
+        expect(await screen.findByRole("listbox")).toBeInTheDocument();
+        expect(screen.getByRole("option", { name: "First option" })).toBeInTheDocument();
+    });
+    it("should forward `menuProps` to the menu", async () => {
+        const { container } = render(
+            <ContextMenu
+                role="listbox"
+                preventPlaceholder={true}
+                menuProps={{ "aria-multiselectable": true, "aria-label": "Options", className: "custom-menu" }}
+            >
+                <MenuItem text="First option" />
+            </ContextMenu>,
+        );
+        fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]);
+        const listbox = await screen.findByRole("listbox");
+        expect(listbox).toHaveAttribute("aria-multiselectable", "true");
+        expect(listbox).toHaveAttribute("aria-label", "Options");
+        expect(listbox).toHaveClass("custom-menu");
+    });
+    it.each([true, false])(
+        "should set `aria-haspopup` according to role `listbox` (preventPlaceholder: %s)",
+        async (preventPlaceholder) => {
+            const { container } = render(
+                <ContextMenu role="listbox" preventPlaceholder={preventPlaceholder}>
+                    <MenuItem text="First option" />
+                </ContextMenu>,
+            );
+            expect(screen.getByRole("button")).toHaveAttribute("aria-haspopup", "listbox");
+            fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]);
+            expect(await screen.findByRole("listbox")).toBeVisible();
+            expect(screen.getByRole("button")).toHaveAttribute("aria-haspopup", "listbox");
+        },
+    );
 });

src/components/Menu/Menu.tsx

@@ -1,14 +1,160 @@
-import React from "react";
-import { Menu as BlueprintMenu, MenuProps as BlueprintMenuProps } from "@blueprintjs/core";
+import React, { AriaRole } from "react";
+import {
+    Menu as BlueprintMenu,
+    MenuItem as BlueprintMenuItem,
+    MenuProps as BlueprintMenuProps,
+} from "@blueprintjs/core";
 
 import { CLASSPREFIX as eccgui } from "../../configuration/constants";
 
-export type MenuProps = BlueprintMenuProps;
+import MenuDivider, { MenuDividerProps } from "./MenuDivider";
+import MenuItem, { MenuItemProps } from "./MenuItem";
+
+/**
+ * Roles that are supported by the automatic `roleStructure` handling of the `MenuItem` children.
+ */
+type SupportedMenuRole = "menu" | "navigation" | "list" | "listbox" | "none" | "presentation";
+
+export interface MenuProps extends BlueprintMenuProps {
+    /**
+     * Sets the ARIA role of the menu.
+     * It may become mandatory in a future version.
+     * Setting a `role` checks whether `MenuItem` and `MenuDivider` children use a correct `roleStructure`, or sets it automatically.
+     * The automatic checks only work if `MenuItem` and `MenuDivider` are used as direct children or inside a `Fragment`.
+     * The checks only work for `menu`, `navigation`, `list`, `listbox`, `none` and `presentation`.
+     * Other roles are applied as they are, and the children are not processed.
+     */
+    role?: SupportedMenuRole | AriaRole;
+}
+
+const roleMapping: Partial<Record<SupportedMenuRole, AriaRole>> = {
+    navigation: "list",
+};
+
+const allowedRoleStructure: Record<SupportedMenuRole, NonNullable<MenuItemProps["roleStructure"]>[]> = {
+    menu: ["menuitem"],
+    navigation: ["listitem"],
+    list: ["listitem"],
+    listbox: ["listoption"],
+    none: ["none"],
+    presentation: ["none"],
+};
+
+/**
+ * Allowed values for an explicitly set `role` on `MenuItem` children, regarding their (explicitly or automatically set) `roleStructure`.
+ * Blueprint sets this `role` on the inner target element of the item, where it overwrites the role defined by `roleStructure`,
+ * so it must fit to the role of the `li` element that is defined by `roleStructure`.
+ * A `roleStructure` without entry does not restrict the item role.
+ */
+const allowedItemRole: Partial<Record<NonNullable<MenuItemProps["roleStructure"]>, AriaRole[]>> = {
+    menuitem: ["menuitem", "menuitemcheckbox", "menuitemradio"],
+    listitem: ["link", "button", "none", "presentation"],
+    // the `li` element already has the `option` role, so the target element must not have an own role
+    listoption: ["none", "presentation"],
+};
+
+/**
+ * `MenuDivider` children get this role if their `role="separator"` is not allowed inside the (mapped) menu role.
+ */
+const dividerRoleMapping: Partial<Record<AriaRole, AriaRole>> = {
+    list: "listitem",
+    listbox: "none",
+};
+
+/**
+ * Blueprint uses this `roleStructure` by default, so it does not need to be set explicitly.
+ */
+const defaultRoleStructure: MenuItemProps["roleStructure"] = "menuitem";
+
+/**
+ * Represents a list of items, used as menu, navigation, options, or only for presentation.
+ * It sets automatically `roleStructure` for its direct `MenuItem` children (also inside fragments) regarding the set `role`,
+ * or it emits a console warning if it does not fit to an already set `roleStructure`.
+ * It also emits a console warning if an explicitly set `role` of a `MenuItem` does not fit to its `roleStructure`.
+ * On `MenuDivider` children without role or with `role="separator"` it sets `role="listitem"` in lists
+ * (`role="list"` or `role="navigation"`) and `role="none"` in listboxes, because `separator` is not allowed there.
+ * It also sets `aria-hidden` then if the divider has no `title` and no own `aria-hidden` value.
+ * It does not add a `nav` wrapper on `role="navigation"`, you need to do it on your own.
+ */
+export const Menu = ({ children, className = "", role = "menu", ...restProps }: MenuProps) => {
+    const isSupportedRole = Object.prototype.hasOwnProperty.call(allowedRoleStructure, role);
+    const roleMapped = (isSupportedRole && roleMapping[role as SupportedMenuRole]) || role;
+    const subroles = isSupportedRole ? allowedRoleStructure[role as SupportedMenuRole] : undefined;
+    const dividerRole = Object.prototype.hasOwnProperty.call(dividerRoleMapping, roleMapped)
+        ? dividerRoleMapping[roleMapped]
+        : undefined;
+
+    // warnings are collected while rendering and emitted by an effect, so they are not repeated on each re-render
+    const warnings: string[] = [];
+
+    const processItems = (
+        nodes: React.ReactNode,
+        subroles: NonNullable<MenuItemProps["roleStructure"]>[],
+    ): React.ReactNode =>
+        React.Children.map(nodes, (child) => {
+            if (React.isValidElement<{ children?: React.ReactNode }>(child) && child.type === React.Fragment) {
+                return <React.Fragment key={child.key}>{processItems(child.props.children, subroles)}</React.Fragment>;
+            }
+            if (React.isValidElement<MenuDividerProps>(child) && child.type === MenuDivider) {
+                if (!dividerRole) {
+                    return child;
+                }
+                const { role: ownRole, title, "aria-hidden": ariaHidden } = child.props;
+                return React.cloneElement(child, {
+                    // `separator` is not allowed here, other explicitly set roles are kept
+                    ...(!ownRole || ownRole === "separator" ? { role: dividerRole } : {}),
+                    ...(!title && ariaHidden === undefined ? { "aria-hidden": true } : {}),
+                });
+            }
+            if (
+                !React.isValidElement<MenuItemProps>(child) ||
+                (child.type !== MenuItem && child.type !== BlueprintMenuItem)
+            ) {
+                return child;
+            }
+            const { roleStructure, role: itemRole } = child.props;
+            const roleStructureUsed = roleStructure ?? subroles[0];
+            const itemRoles = allowedItemRole[roleStructureUsed];
+            if (itemRole && itemRoles && !itemRoles.includes(itemRole)) {
+                warnings.push(
+                    `Menu: MenuItem with role="${itemRole}" does not fit to its roleStructure="${roleStructureUsed}"${
+                        roleStructure ? "" : ` (set automatically for Menu role="${role}")`
+                    }, allowed: ${itemRoles.map((allowedRole) => `"${allowedRole}"`).join(", ")}.`,
+                );
+            }
+            if (!roleStructure) {
+                // prevent unnecessary cloning if Blueprint's default is used anyway
+                return subroles[0] === defaultRoleStructure
+                    ? child
+                    : React.cloneElement(child, { roleStructure: subroles[0] });
+            }
+            if (!subroles.includes(roleStructure)) {
+                warnings.push(
+                    `Menu: MenuItem with roleStructure="${roleStructure}" does not fit into Menu with role="${role}", allowed: ${subroles
+                        .map((subrole) => `"${subrole}"`)
+                        .join(", ")}.`,
+                );
+            }
+            return child;
+        });
+
+    const items = subroles ? processItems(children, subroles) : children;
+
+    // only emit warnings again if they changed
+    const warningsKey = warnings.join("\n");
+    React.useEffect(() => {
+        if (!warningsKey) {
+            return;
+        }
+        warningsKey.split("\n").forEach((warning) => {
+            // eslint-disable-next-line no-console
+            console.warn(warning);
+        });
+    }, [warningsKey]);
 
-export const Menu = ({ children, className = "", ...restProps }: MenuProps) => {
     return (
-        <BlueprintMenu {...restProps} className={`${eccgui}-menu__list ` + className}>
-            {children}
+        <BlueprintMenu role={roleMapped} {...restProps} className={`${eccgui}-menu__list ` + className}>
+            {items}
         </BlueprintMenu>
     );
 };

src/components/Menu/MenuDivider.tsx

@@ -1,15 +1,40 @@
 import React from "react";
-import { MenuDivider as BlueprintMenuDivider, MenuDividerProps as BlueprintMenuDividerProps } from "@blueprintjs/core";
+import {
+    Classes as BlueprintClasses,
+    H6 as BlueprintSeparatorTitle,
+    MenuDividerProps as BlueprintMenuDividerProps,
+} from "@blueprintjs/core";
+import classNames from "classnames";
 
 import { CLASSPREFIX as eccgui } from "../../configuration/constants";
 
-export type MenuDividerProps = BlueprintMenuDividerProps;
+/**
+ * Properties of the Blueprint `MenuDivider` and all attributes of the `li` element.
+ * Use `role="listitem"` if the parent `Menu` is a list, because `separator` is not allowed there.
+ * `Menu` sets it automatically, and `aria-hidden` for dividers without `title`.
+ */
+export interface MenuDividerProps
+    extends
+        BlueprintMenuDividerProps,
+        Omit<React.LiHTMLAttributes<HTMLLIElement>, "title" | "children" | "className"> {}
 
-export const MenuDivider = ({ children, className = "", ...restProps }: MenuDividerProps) => {
+/**
+ * Divider or section header inside `Menu`.
+ * It uses the same markup and classes like the Blueprint `MenuDivider`, but supports other roles than `separator`.
+ */
+export const MenuDivider = ({ className, role = "separator", title, titleId, ...otherLiProps }: MenuDividerProps) => {
     return (
-        <BlueprintMenuDivider {...restProps} className={`${eccgui}-menu__divider ` + className}>
-            {children}
-        </BlueprintMenuDivider>
+        <li
+            {...otherLiProps}
+            role={role}
+            className={classNames(
+                title ? BlueprintClasses.MENU_HEADER : BlueprintClasses.MENU_DIVIDER,
+                `${eccgui}-menu__divider`,
+                className,
+            )}
+        >
+            {title && <BlueprintSeparatorTitle id={titleId}>{title}</BlueprintSeparatorTitle>}
+        </li>
     );
 };
 

src/components/Menu/MenuItem.tsx

@@ -34,10 +34,18 @@ export interface MenuItemProps
      * Visual intent color to apply to element.
      */
     intent?: BlueprintMenuItemProps["intent"] | "accent";
+    /**
+     * Defines the ARIA role structure that is used for the menu item element and its inner target element.
+     * It is recommended to set it, even if it is optional right now.
+     * The value is checked if a `role` is set on the parent `Menu`.
+     * It may become mandatory in a future version.
+     */
+    roleStructure?: BlueprintMenuItemProps["roleStructure"];
 }
 
 /**
  * Single item, used as child inside `Menu`.
+ * It is recommended to use it as a direct child and not to wrap it inside other elements, except `Fragment`.
  */
 export const MenuItem = React.forwardRef<HTMLLIElement, MenuItemProps>(
     ({ children, className = "", icon, onClick, href, text, tooltip, intent, ...restProps }, ref) => {