From f3f106744432d4682ce6d79e11621457fbf95029 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 13:40:34 +0100 Subject: [PATCH 01/15] add process to fulfil role attribute of Menu element --- src/components/Menu/Menu.tsx | 73 +++++- src/components/Menu/Stories/Menu.stories.tsx | 24 ++ src/components/Menu/tests/Menu.test.tsx | 252 +++++++++++++++++++ 3 files changed, 344 insertions(+), 5 deletions(-) create mode 100644 src/components/Menu/tests/Menu.test.tsx diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 5f6f8563c..944b78a3a 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -1,14 +1,77 @@ import React from "react"; -import { Menu as BlueprintMenu, MenuProps as BlueprintMenuProps } from "@blueprintjs/core"; +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 MenuItem, { MenuItemProps } from "./MenuItem"; + +export interface MenuProps extends BlueprintMenuProps { + /** + * Sets a role according to an allowed subset of ARIA roles. + * This will check if the `MenuItems` using a correct `roleStructure`, + * or set it automatically. + */ + role?: "menu" | "navigation" | "list" | "listbox" | "none" | "presentation"; +} + +type MenuRole = NonNullable; + +const roleMapping: Partial> = { + navigation: "list", +}; + +const allowedSubroles: Record[]> = { + menu: ["menuitem"], + navigation: ["listitem"], + list: ["listitem"], + listbox: ["listoption"], + none: ["none"], + presentation: ["none"], +}; + +/** + * 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 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 roleMapped = roleMapping[role] ?? role; + const subroles = allowedSubroles[role]; + + const processItems = (nodes: React.ReactNode): React.ReactNode => + React.Children.map(nodes, (child) => { + if (React.isValidElement<{ children?: React.ReactNode }>(child) && child.type === React.Fragment) { + return {processItems(child.props.children)}; + } + if ( + !React.isValidElement(child) || + (child.type !== MenuItem && child.type !== BlueprintMenuItem) + ) { + return child; + } + const { roleStructure } = child.props; + if (!roleStructure) { + return React.cloneElement(child, { roleStructure: subroles[0] }); + } + if (!subroles.includes(roleStructure)) { + // eslint-disable-next-line no-console + console.warn( + `Menu: MenuItem with roleStructure="${roleStructure}" does not fit into Menu with role="${role}", allowed: ${subroles + .map((subrole) => `"${subrole}"`) + .join(", ")}.`, + ); + } + return child; + }); -export const Menu = ({ children, className = "", ...restProps }: MenuProps) => { return ( - - {children} + + {processItems(children)} ); }; diff --git a/src/components/Menu/Stories/Menu.stories.tsx b/src/components/Menu/Stories/Menu.stories.tsx index 2c2d4d4b7..4c61f7ad3 100644 --- a/src/components/Menu/Stories/Menu.stories.tsx +++ b/src/components/Menu/Stories/Menu.stories.tsx @@ -33,3 +33,27 @@ Default.args = { ), }; + +/** + * Use `role="navigation"` for a list of links, e.g. as main or sidebar navigation. + * The `MenuItem` elements get `roleStructure="listitem"` automatically. + * `Menu` does not add a `nav` element, so you need to wrap it on your own. + */ +const NavigationExample: StoryFn = (args) => ( + + + +); + +export const Navigation = NavigationExample.bind({}); +Navigation.args = { + role: "navigation", + children: [ + , + , + , + , + ], +}; diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx new file mode 100644 index 000000000..b02438744 --- /dev/null +++ b/src/components/Menu/tests/Menu.test.tsx @@ -0,0 +1,252 @@ +import React from "react"; +import { MenuItem as BlueprintMenuItem } from "@blueprintjs/core"; +import { render } from "@testing-library/react"; + +import "@testing-library/jest-dom"; + +import { CLASSPREFIX as eccgui } from "../../../configuration/constants"; +import Menu, { MenuProps } from "../Menu"; +import MenuDivider from "../MenuDivider"; +import MenuItem, { MenuItemProps } from "../MenuItem"; + +type MenuRole = NonNullable; +type RoleStructure = NonNullable; + +/** + * Blueprint renders `roleStructure` as roles on the `li` and the inner target element. + * Each `roleStructure` value results in an unique combination. + */ +const expectedDomRoles: Record = { + menuitem: { li: "none", target: "menuitem" }, + listitem: { li: null, target: null }, + listoption: { li: "option", target: null }, + none: { li: "none", target: null }, +}; + +const getDomRoles = (container: HTMLElement) => + Array.from(container.querySelectorAll(`.${eccgui}-menu__item`)).map((target) => ({ + li: target.closest("li")!.getAttribute("role"), + target: target.getAttribute("role"), + })); + +describe("Menu", () => { + let warnSpy: jest.SpyInstance; + + beforeEach(() => { + warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); + }); + + afterEach(() => { + warnSpy.mockRestore(); + }); + + describe("role", () => { + it("should use role 'menu' by default", () => { + const { container } = render( + + + , + ); + const list = container.querySelector(`.${eccgui}-menu__list`); + expect(list).toHaveAttribute("role", "menu"); + }); + + it("should map role 'navigation' to 'list'", () => { + const { container } = render( + + + , + ); + const list = container.querySelector(`.${eccgui}-menu__list`); + expect(list).toHaveAttribute("role", "list"); + }); + + it.each(["menu", "list", "listbox", "none", "presentation"])( + "should keep role '%s' unchanged", + (role) => { + const { container } = render( + + + , + ); + const list = container.querySelector(`.${eccgui}-menu__list`); + expect(list).toHaveAttribute("role", role); + }, + ); + + it("should keep custom class names", () => { + const { container } = render(); + const list = container.querySelector(`.${eccgui}-menu__list`); + expect(list).toHaveClass("custom-class"); + }); + }); + + describe("roleStructure autoset", () => { + it.each<[MenuRole, RoleStructure]>([ + ["menu", "menuitem"], + ["navigation", "listitem"], + ["list", "listitem"], + ["listbox", "listoption"], + ["none", "none"], + ["presentation", "none"], + ])("should set roleStructure for role '%s' to '%s' if not set", (role, expectedRoleStructure) => { + const { container } = render( + + + + , + ); + expect(getDomRoles(container)).toEqual([ + expectedDomRoles[expectedRoleStructure], + expectedDomRoles[expectedRoleStructure], + ]); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("should set roleStructure also for Blueprint MenuItem children", () => { + const { container } = render( + + + , + ); + expect(getDomRoles(container)).toEqual([expectedDomRoles.listoption]); + }); + + it("should not overwrite an allowed roleStructure", () => { + const { container } = render( + + + , + ); + expect(getDomRoles(container)).toEqual([expectedDomRoles.listitem]); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("should only set roleStructure on items without one", () => { + const { container } = render( + + + + , + ); + expect(getDomRoles(container)).toEqual([expectedDomRoles.listitem, expectedDomRoles.listitem]); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("should set roleStructure for MenuItems nested in fragments", () => { + const { container } = render( + + <> + + <> + + + {[, ]} + + , + ); + expect(getDomRoles(container)).toEqual(Array(4).fill(expectedDomRoles.listoption)); + }); + + it("should leave other children untouched", () => { + const { container } = render( + + + +
  • custom
    • + {"text"} + {null} +     , + ); + expect(getDomRoles(container)).toEqual([expectedDomRoles.listitem]); + expect(container.querySelector(".custom-child")).toBeInTheDocument(); + expect(container).toHaveTextContent("text"); + expect(warnSpy).not.toHaveBeenCalled(); + }); + }); + + describe("roleStructure check", () => { + it.each<[MenuRole, RoleStructure]>([ + ["menu", "listitem"], + ["menu", "listoption"], + ["navigation", "menuitem"], + ["list", "listoption"], + ["listbox", "menuitem"], + ["none", "listitem"], + ["presentation", "menuitem"], + ])("should warn for role '%s' with not allowed roleStructure '%s'", (role, roleStructure) => { + render( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toContain(`roleStructure="${roleStructure}"`); + expect(warnSpy.mock.calls[0][0]).toContain(`role="${role}"`); + }); + + it("should keep the not allowed roleStructure value", () => { + const { container } = render( + + + , + ); + expect(getDomRoles(container)).toEqual([expectedDomRoles.listoption]); + }); + + it("should list the allowed values in the warning", () => { + render( + + + , + ); + expect(warnSpy.mock.calls[0][0]).toContain('allowed: "listoption"'); + }); + + it("should warn once per not allowed item", () => { + render( + + + + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(2); + }); + + it("should check Blueprint MenuItem children too", () => { + render( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + it("should check MenuItems nested in fragments", () => { + render( + + <> + + <> + + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(2); + }); + + it("should not check MenuItems in submenus", () => { + render( + + + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }); + }); +}); From 7bf3bd3f03e40b759fa5bed28166fa3ff2dafef0 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 14:12:31 +0100 Subject: [PATCH 02/15] improve MenuDivider, so that it is compatible to role=list parents --- src/components/Menu/Menu.tsx | 12 +++ src/components/Menu/MenuDivider.tsx | 37 +++++++-- src/components/Menu/tests/Menu.test.tsx | 101 +++++++++++++++++++++++- 3 files changed, 143 insertions(+), 7 deletions(-) diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 944b78a3a..1dd9bb0b9 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -7,6 +7,7 @@ import { import { CLASSPREFIX as eccgui } from "../../configuration/constants"; +import MenuDivider, { MenuDividerProps } from "./MenuDivider"; import MenuItem, { MenuItemProps } from "./MenuItem"; export interface MenuProps extends BlueprintMenuProps { @@ -37,6 +38,8 @@ const allowedSubroles: Record { @@ -48,6 +51,15 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: if (React.isValidElement<{ children?: React.ReactNode }>(child) && child.type === React.Fragment) { return {processItems(child.props.children)}; } + if (React.isValidElement(child) && child.type === MenuDivider) { + if (roleMapped !== "list") { + return child; + } + return React.cloneElement(child, { + role: "listitem", + ...(child.props.title ? {} : { "aria-hidden": true }), + }); + } if ( !React.isValidElement(child) || (child.type !== MenuItem && child.type !== BlueprintMenuItem) diff --git a/src/components/Menu/MenuDivider.tsx b/src/components/Menu/MenuDivider.tsx index 27385dccc..17a900e6b 100644 --- a/src/components/Menu/MenuDivider.tsx +++ b/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, "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 ( - - {children} - +
  • + {title && {title}} +
    • ); }; diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index b02438744..a57a551e3 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { MenuItem as BlueprintMenuItem } from "@blueprintjs/core"; +import { Classes as BlueprintClasses, MenuItem as BlueprintMenuItem } from "@blueprintjs/core"; import { render } from "@testing-library/react"; import "@testing-library/jest-dom"; @@ -165,6 +165,105 @@ describe("Menu", () => { }); }); + describe("MenuDivider", () => { + const getDividers = (container: HTMLElement) => + Array.from(container.querySelectorAll(`.${eccgui}-menu__divider`)); + + it.each(["list", "navigation"])( + "should set role 'listitem' and aria-hidden on divider without title for role '%s'", + (role) => { + const { container } = render( + + + + + , + ); + const [divider] = getDividers(container); + expect(divider.tagName).toBe("LI"); + expect(divider).toHaveAttribute("role", "listitem"); + expect(divider).toHaveAttribute("aria-hidden", "true"); + expect(divider).toHaveClass(BlueprintClasses.MENU_DIVIDER); + }, + ); + + it.each(["list", "navigation"])( + "should set role 'listitem' but no aria-hidden on divider with title for role '%s'", + (role) => { + const { container } = render( + + + + + , + ); + const [divider] = getDividers(container); + expect(divider).toHaveAttribute("role", "listitem"); + expect(divider).not.toHaveAttribute("aria-hidden"); + expect(divider).toHaveClass(BlueprintClasses.MENU_HEADER); + expect(divider.querySelector("h6")).toHaveTextContent("Section"); + expect(divider.querySelector("h6")).toHaveAttribute("id", "section-title"); + }, + ); + + it("should set role 'listitem' also on dividers inside fragments", () => { + const { container } = render( + + <> + + + + + , + ); + const [divider] = getDividers(container); + expect(divider).toHaveAttribute("role", "listitem"); + expect(divider).toHaveAttribute("aria-hidden", "true"); + }); + + it("should overwrite an explicit separator role in lists", () => { + const { container } = render( + + + , + ); + expect(getDividers(container)[0]).toHaveAttribute("role", "listitem"); + }); + + it.each(["menu", "listbox", "none", "presentation"])( + "should keep role 'separator' on divider for role '%s'", + (role) => { + const { container } = render( + + + + , + ); + getDividers(container).forEach((divider) => { + expect(divider).toHaveAttribute("role", "separator"); + expect(divider).not.toHaveAttribute("aria-hidden"); + }); + }, + ); + + it("should render all children of the list as listitems", () => { + const { container } = render( + + + + + + + , + ); + const list = container.querySelector(`.${eccgui}-menu__list`)!; + Array.from(list.children).forEach((child) => { + expect(child.tagName).toBe("LI"); + expect([null, "listitem"]).toContain(child.getAttribute("role")); + }); + }); + }); + describe("roleStructure check", () => { it.each<[MenuRole, RoleStructure]>([ ["menu", "listitem"], From c9691df557ab9c8a9f2583e153ecfd1c8c7cc92a Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 14:15:11 +0100 Subject: [PATCH 03/15] update changelog --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc06004fe..4c93e8629 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ## [Unreleased] +### Added + +- `` + - `role` property: `menu` (default), `navigation`, `list`, `listbox`, `none` or `presentation` + - `navigation` is rendered as `list`, the `nav` wrapper must be added by yourself + - sets `roleStructure` of `` children automatically according to the `role` if it is not given, otherwise a console warning is emitted if it does not fit + - sets `role="listitem"` on `` children in lists, and `aria-hidden` if the divider has no `title` +- `` + - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` + ## [26.2.1] - 2026-11-02 ### Fixed From d53e3fdddfb92ff02630d4282c959d2ffebeb276 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 14:26:25 +0100 Subject: [PATCH 04/15] fix intern Menu usage in ContextMenu --- CHANGELOG.md | 6 ++++++ src/components/ContextOverlay/ContextMenu.tsx | 10 ++++++++-- .../ContextOverlay/tests/ContextMenu.test.tsx | 16 ++++++++++++++++ .../toolbars/EditorAppearanceConfigMenu.tsx | 1 + 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c93e8629..0d34f490a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - sets `role="listitem"` on `` children in lists, and `aria-hidden` if the divider has no `title` - `` - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` +- `` + - `role` property: forwarded to the included `` + +### Fixed + +- `` uses `role="listbox"` for its menu because its items are selectable options ## [26.2.1] - 2026-11-02 diff --git a/src/components/ContextOverlay/ContextMenu.tsx b/src/components/ContextOverlay/ContextMenu.tsx index 1515b0fd5..a3545fd45 100644 --- a/src/components/ContextOverlay/ContextMenu.tsx +++ b/src/components/ContextOverlay/ContextMenu.tsx @@ -4,7 +4,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"; @@ -18,6 +18,11 @@ 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. + */ + role?: MenuProps["role"]; /** * Toggler that need to be used to display menu. * If a valid icon name is used then the icon element is displayed. @@ -62,6 +67,7 @@ export interface ContextMenuProps extends TestableComponent { export const ContextMenu = ({ children, className = "", + role, togglerElement = "item-moremenu", togglerText = "Show more options", contextOverlayProps, @@ -96,7 +102,7 @@ export const ContextMenu = ({ {...restProps} {...contextOverlayProps} className={`${eccgui}-contextmenu ` + className} - content={{children}} + content={{children}} disabled={!!disabled} usePlaceholder={!preventPlaceholder} > diff --git a/src/components/ContextOverlay/tests/ContextMenu.test.tsx b/src/components/ContextOverlay/tests/ContextMenu.test.tsx index e2c909521..3847e3c7e 100644 --- a/src/components/ContextOverlay/tests/ContextMenu.test.tsx +++ b/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"; @@ -40,4 +41,19 @@ describe("ContextMenu", () => { fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]); expect(await screen.findByText("First option")).toBeVisible(); }); + it("should use role `menu` for the menu by default", async () => { + const { container } = render(); + fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]); + expect(await screen.findByRole("menu")).toBeInTheDocument(); + }); + it("should forward `role` to the menu", async () => { + const { container } = render( + + + , + ); + fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]); + expect(await screen.findByRole("listbox")).toBeInTheDocument(); + expect(screen.getByRole("option", { name: "First option" })).toBeInTheDocument(); + }); }); diff --git a/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx b/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx index 413242a33..c7c279b79 100644 --- a/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx +++ b/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx @@ -28,6 +28,7 @@ export function EditorAppearanceConfigMenu({ return ( Date: Wed, 4 Nov 2026 14:31:21 +0100 Subject: [PATCH 05/15] fix usage of role and aria-hidden in MenuDivider --- CHANGELOG.md | 2 +- src/components/Menu/Menu.tsx | 10 ++++++---- src/components/Menu/tests/Menu.test.tsx | 22 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d34f490a..5cf041c2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - `role` property: `menu` (default), `navigation`, `list`, `listbox`, `none` or `presentation` - `navigation` is rendered as `list`, the `nav` wrapper must be added by yourself - sets `roleStructure` of `` children automatically according to the `role` if it is not given, otherwise a console warning is emitted if it does not fit - - sets `role="listitem"` on `` children in lists, and `aria-hidden` if the divider has no `title` + - sets `role="listitem"` on `` children in lists, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"` - `` - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` - `` diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 1dd9bb0b9..dc4619796 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -38,8 +38,8 @@ const allowedSubroles: Record { @@ -55,9 +55,11 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: if (roleMapped !== "list") { return child; } + const { role: dividerRole, title, "aria-hidden": ariaHidden } = child.props; return React.cloneElement(child, { - role: "listitem", - ...(child.props.title ? {} : { "aria-hidden": true }), + // `separator` is not allowed in lists, other explicitly set roles are kept + ...(!dividerRole || dividerRole === "separator" ? { role: "listitem" } : {}), + ...(!title && ariaHidden === undefined ? { "aria-hidden": true } : {}), }); } if ( diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index a57a551e3..5b4a81a70 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -230,6 +230,28 @@ describe("Menu", () => { expect(getDividers(container)[0]).toHaveAttribute("role", "listitem"); }); + it("should keep other explicitly set roles in lists", () => { + const { container } = render( + + + , + ); + expect(getDividers(container)[0]).toHaveAttribute("role", "presentation"); + }); + + it("should keep an explicitly set aria-hidden value in lists", () => { + const { container } = render( + + + + , + ); + const [withoutTitle, withTitle] = getDividers(container); + expect(withoutTitle).toHaveAttribute("aria-hidden", "false"); + expect(withoutTitle).toHaveAttribute("role", "listitem"); + expect(withTitle).toHaveAttribute("aria-hidden", "true"); + }); + it.each(["menu", "listbox", "none", "presentation"])( "should keep role 'separator' on divider for role '%s'", (role) => { From b0ce61a87f0ac76cf20c91f06f9651f3c0375a3f Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 14:35:24 +0100 Subject: [PATCH 06/15] fix role support limitations --- CHANGELOG.md | 2 +- src/components/Menu/Menu.tsx | 34 +++++++++++++++---------- src/components/Menu/tests/Menu.test.tsx | 19 ++++++++++++++ 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cf041c2b..4858309ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ### Added - `` - - `role` property: `menu` (default), `navigation`, `list`, `listbox`, `none` or `presentation` + - `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 `` children automatically according to the `role` if it is not given, otherwise a console warning is emitted if it does not fit - sets `role="listitem"` on `` children in lists, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"` diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index dc4619796..1d0a0c148 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -1,4 +1,4 @@ -import React from "react"; +import React, { AriaRole } from "react"; import { Menu as BlueprintMenu, MenuItem as BlueprintMenuItem, @@ -10,22 +10,26 @@ import { CLASSPREFIX as eccgui } from "../../configuration/constants"; 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 a role according to an allowed subset of ARIA roles. - * This will check if the `MenuItems` using a correct `roleStructure`, + * Sets the ARIA role of the menu. + * For `menu`, `navigation`, `list`, `listbox`, `none` and `presentation` it checks if the `MenuItems` using a correct `roleStructure`, * or set it automatically. + * Other roles are set as they are, the children are not processed then. */ - role?: "menu" | "navigation" | "list" | "listbox" | "none" | "presentation"; + role?: SupportedMenuRole | AriaRole; } -type MenuRole = NonNullable; - -const roleMapping: Partial> = { +const roleMapping: Partial> = { navigation: "list", }; -const allowedSubroles: Record[]> = { +const allowedSubroles: Record[]> = { menu: ["menuitem"], navigation: ["listitem"], list: ["listitem"], @@ -43,13 +47,17 @@ const allowedSubroles: Record { - const roleMapped = roleMapping[role] ?? role; - const subroles = allowedSubroles[role]; + const isSupportedRole = Object.prototype.hasOwnProperty.call(allowedSubroles, role); + const roleMapped = (isSupportedRole && roleMapping[role as SupportedMenuRole]) || role; + const subroles = isSupportedRole ? allowedSubroles[role as SupportedMenuRole] : undefined; - const processItems = (nodes: React.ReactNode): React.ReactNode => + const processItems = ( + nodes: React.ReactNode, + subroles: NonNullable[], + ): React.ReactNode => React.Children.map(nodes, (child) => { if (React.isValidElement<{ children?: React.ReactNode }>(child) && child.type === React.Fragment) { - return {processItems(child.props.children)}; + return {processItems(child.props.children, subroles)}; } if (React.isValidElement(child) && child.type === MenuDivider) { if (roleMapped !== "list") { @@ -85,7 +93,7 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: return ( - {processItems(children)} + {subroles ? processItems(children, subroles) : children} ); }; diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index 5b4a81a70..4eec16b2e 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -74,6 +74,25 @@ describe("Menu", () => { }, ); + it.each(["group", "tree", "toString"])( + "should render unsupported role '%s' as it is without processing the children", + (role) => { + const { container } = render( + + + + + , + ); + const list = container.querySelector(`.${eccgui}-menu__list`); + expect(list).toHaveAttribute("role", role); + // children keep their own or Blueprint's default `roleStructure` + expect(getDomRoles(container)).toEqual([expectedDomRoles.listoption, expectedDomRoles.menuitem]); + expect(container.querySelector(`.${eccgui}-menu__divider`)).toHaveAttribute("role", "separator"); + expect(warnSpy).not.toHaveBeenCalled(); + }, + ); + it("should keep custom class names", () => { const { container } = render(); const list = container.querySelector(`.${eccgui}-menu__list`); From f414faee1231944137b823a2a50406414b2cc17d Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 14:46:43 +0100 Subject: [PATCH 07/15] imporve MenuDivider usage for role=listbox and prevent unncessary renderings --- CHANGELOG.md | 2 +- src/components/Menu/Menu.tsx | 34 ++++++++++++++---- src/components/Menu/tests/Menu.test.tsx | 48 ++++++++++++++++++++++++- 3 files changed, 75 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4858309ec..cd8ab7ec1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - `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 `` children automatically according to the `role` if it is not given, otherwise a console warning is emitted if it does not fit - - sets `role="listitem"` on `` children in lists, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"` + - sets `role="listitem"` (lists) or `role="none"` (listboxes) on `` children, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"` - `` - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` - `` diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 1d0a0c148..cc90db5d5 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -38,18 +38,35 @@ const allowedSubroles: Record> = { + 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`. - * In lists (`role="list"` or `role="navigation"`) it sets `role="listitem"` on `MenuDivider` children - * without role or with `role="separator"`, and `aria-hidden` if the divider has no `title` and no own `aria-hidden` value. + * 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(allowedSubroles, role); const roleMapped = (isSupportedRole && roleMapping[role as SupportedMenuRole]) || role; const subroles = isSupportedRole ? allowedSubroles[role as SupportedMenuRole] : undefined; + const dividerRole = Object.prototype.hasOwnProperty.call(dividerRoleMapping, roleMapped) + ? dividerRoleMapping[roleMapped] + : undefined; const processItems = ( nodes: React.ReactNode, @@ -60,13 +77,13 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: return {processItems(child.props.children, subroles)}; } if (React.isValidElement(child) && child.type === MenuDivider) { - if (roleMapped !== "list") { + if (!dividerRole) { return child; } - const { role: dividerRole, title, "aria-hidden": ariaHidden } = child.props; + const { role: ownRole, title, "aria-hidden": ariaHidden } = child.props; return React.cloneElement(child, { - // `separator` is not allowed in lists, other explicitly set roles are kept - ...(!dividerRole || dividerRole === "separator" ? { role: "listitem" } : {}), + // `separator` is not allowed here, other explicitly set roles are kept + ...(!ownRole || ownRole === "separator" ? { role: dividerRole } : {}), ...(!title && ariaHidden === undefined ? { "aria-hidden": true } : {}), }); } @@ -78,7 +95,10 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: } const { roleStructure } = child.props; if (!roleStructure) { - return React.cloneElement(child, { roleStructure: subroles[0] }); + // 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)) { // eslint-disable-next-line no-console diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index 4eec16b2e..71c42355b 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -122,6 +122,22 @@ describe("Menu", () => { expect(warnSpy).not.toHaveBeenCalled(); }); + it("should not clone MenuItems if Blueprint's default roleStructure 'menuitem' is used anyway", () => { + const cloneSpy = jest.spyOn(React, "cloneElement"); + try { + const { container } = render( + + + + , + ); + expect(getDomRoles(container)).toEqual([expectedDomRoles.menuitem, expectedDomRoles.menuitem]); + expect(cloneSpy).not.toHaveBeenCalled(); + } finally { + cloneSpy.mockRestore(); + } + }); + it("should set roleStructure also for Blueprint MenuItem children", () => { const { container } = render( @@ -271,7 +287,37 @@ describe("Menu", () => { expect(withTitle).toHaveAttribute("aria-hidden", "true"); }); - it.each(["menu", "listbox", "none", "presentation"])( + it("should set role 'none' and aria-hidden on divider without title for role 'listbox'", () => { + const { container } = render( + + + + + + + , + ); + const [withoutRole, withSeparator, withTitle] = getDividers(container); + expect(withoutRole).toHaveAttribute("role", "none"); + expect(withoutRole).toHaveAttribute("aria-hidden", "true"); + expect(withSeparator).toHaveAttribute("role", "none"); + expect(withSeparator).toHaveAttribute("aria-hidden", "true"); + expect(withTitle).toHaveAttribute("role", "none"); + expect(withTitle).not.toHaveAttribute("aria-hidden"); + }); + + it("should keep explicitly set divider values for role 'listbox'", () => { + const { container } = render( + + + , + ); + const [divider] = getDividers(container); + expect(divider).toHaveAttribute("role", "presentation"); + expect(divider).toHaveAttribute("aria-hidden", "false"); + }); + + it.each(["menu", "none", "presentation"])( "should keep role 'separator' on divider for role '%s'", (role) => { const { container } = render( From a158737102c30c695d223f8f9362d64f86f23f25 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Wed, 4 Nov 2026 14:49:02 +0100 Subject: [PATCH 08/15] prevent too many same warnings --- src/components/Menu/Menu.tsx | 22 ++++++++-- src/components/Menu/tests/Menu.test.tsx | 53 +++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index cc90db5d5..6ba78b012 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -68,6 +68,9 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: ? 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[], @@ -101,8 +104,7 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: : React.cloneElement(child, { roleStructure: subroles[0] }); } if (!subroles.includes(roleStructure)) { - // eslint-disable-next-line no-console - console.warn( + warnings.push( `Menu: MenuItem with roleStructure="${roleStructure}" does not fit into Menu with role="${role}", allowed: ${subroles .map((subrole) => `"${subrole}"`) .join(", ")}.`, @@ -111,9 +113,23 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: 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]); + return ( - {subroles ? processItems(children, subroles) : children} + {items} ); }; diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index 71c42355b..4e49e9463 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -401,6 +401,59 @@ describe("Menu", () => { expect(warnSpy).toHaveBeenCalledTimes(2); }); + it("should not repeat warnings on re-render", () => { + const { rerender } = render( + + + , + ); + rerender( + + + , + ); + rerender( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + it("should warn again if the mismatches changed", () => { + const { rerender } = render( + + + , + ); + rerender( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy.mock.calls[1][0]).toContain('roleStructure="listitem"'); + }); + + it("should not warn after the mismatch was fixed", () => { + const { rerender } = render( + + + , + ); + rerender( + + + , + ); + rerender( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + it("should check Blueprint MenuItem children too", () => { render( From b403cac166870982aa967b655c0be0652475aaa2 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Thu, 5 Nov 2026 12:05:56 +0100 Subject: [PATCH 09/15] add check for role values set manually --- CHANGELOG.md | 1 + src/components/Menu/Menu.tsx | 30 ++++++- src/components/Menu/tests/Menu.test.tsx | 111 ++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd8ab7ec1..7ba8c7f4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - `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 `` 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 `` child does not fit to the `role` of the menu - sets `role="listitem"` (lists) or `role="none"` (listboxes) on `` children, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"` - `` - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 6ba78b012..15fbdaf07 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -29,7 +29,7 @@ const roleMapping: Partial> = { navigation: "list", }; -const allowedSubroles: Record[]> = { +const allowedRoleStructure: Record[]> = { menu: ["menuitem"], navigation: ["listitem"], list: ["listitem"], @@ -38,6 +38,19 @@ const allowedSubroles: Record> = { + menu: ["menuitem", "menuitemcheckbox", "menuitemradio"], + navigation: ["link", "none", "presentation"], + list: ["link", "button", "none", "presentation"], + // the `li` element already has the `option` role, so the target element must not have an own role + listbox: ["none", "presentation"], +}; + /** * `MenuDivider` children get this role if their `role="separator"` is not allowed inside the (mapped) menu role. */ @@ -55,15 +68,17 @@ 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 the `role` of the menu. * 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(allowedSubroles, role); + const isSupportedRole = Object.prototype.hasOwnProperty.call(allowedRoleStructure, role); const roleMapped = (isSupportedRole && roleMapping[role as SupportedMenuRole]) || role; - const subroles = isSupportedRole ? allowedSubroles[role as SupportedMenuRole] : undefined; + const subroles = isSupportedRole ? allowedRoleStructure[role as SupportedMenuRole] : undefined; + const itemRoles = isSupportedRole ? allowedItemRole[role as SupportedMenuRole] : undefined; const dividerRole = Object.prototype.hasOwnProperty.call(dividerRoleMapping, roleMapped) ? dividerRoleMapping[roleMapped] : undefined; @@ -96,7 +111,14 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: ) { return child; } - const { roleStructure } = child.props; + const { roleStructure, role: itemRole } = child.props; + if (itemRole && itemRoles && !itemRoles.includes(itemRole)) { + warnings.push( + `Menu: MenuItem with role="${itemRole}" does not fit into Menu with role="${role}", allowed: ${itemRoles + .map((allowedRole) => `"${allowedRole}"`) + .join(", ")}.`, + ); + } if (!roleStructure) { // prevent unnecessary cloning if Blueprint's default is used anyway return subroles[0] === defaultRoleStructure diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index 4e49e9463..38cac08e1 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -351,6 +351,117 @@ describe("Menu", () => { }); }); + describe("MenuItem role check", () => { + it.each<[MenuRole, string]>([ + ["menu", "menuitem"], + ["menu", "menuitemcheckbox"], + ["menu", "menuitemradio"], + ["navigation", "link"], + ["navigation", "none"], + ["list", "link"], + ["list", "button"], + ["list", "presentation"], + ["listbox", "none"], + ["listbox", "presentation"], + ])("should not warn for role '%s' with allowed item role '%s'", (role, itemRole) => { + render( + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it.each<[MenuRole, string]>([ + ["menu", "link"], + ["menu", "option"], + ["navigation", "menuitem"], + ["navigation", "listitem"], + ["list", "listitem"], + ["list", "option"], + ["listbox", "option"], + ["listbox", "menuitem"], + ])("should warn for role '%s' with not allowed item role '%s'", (role, itemRole) => { + render( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toContain(`MenuItem with role="${itemRole}"`); + expect(warnSpy.mock.calls[0][0]).toContain(`Menu with role="${role}"`); + }); + + it.each(["none", "presentation"])("should not restrict the item role for role '%s'", (role) => { + render( + + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("should not check the item role for unsupported menu roles", () => { + render( + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("should not warn if no item role is set", () => { + render( + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("should list the allowed item roles in the warning", () => { + render( + + + , + ); + expect(warnSpy.mock.calls[0][0]).toContain('allowed: "none", "presentation"'); + }); + + it("should keep the item role and the automatically set roleStructure", () => { + const { container } = render( + + + , + ); + expect(getDomRoles(container)).toEqual([{ li: "option", target: "option" }]); + }); + + it("should warn separately for not allowed role and roleStructure", () => { + render( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy.mock.calls.map((call) => call[0]).join("\n")).toContain('role="menuitem"'); + expect(warnSpy.mock.calls.map((call) => call[0]).join("\n")).toContain('roleStructure="menuitem"'); + }); + + it("should check the item role of Blueprint MenuItem children and inside fragments too", () => { + render( + + + <> + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(2); + }); + }); + describe("roleStructure check", () => { it.each<[MenuRole, RoleStructure]>([ ["menu", "listitem"], From a885fcc79eb68d2cfd6385420ff2556612378fe7 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Thu, 5 Nov 2026 12:33:47 +0100 Subject: [PATCH 10/15] align role and popupKind for ContextMenu --- CHANGELOG.md | 2 +- src/components/ContextOverlay/ContextMenu.tsx | 9 +++++---- .../ContextOverlay/tests/ContextMenu.test.tsx | 14 ++++++++++++++ 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c5602583..f1d3fda04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - `` - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` - `` - - `role` property: forwarded to the included `` + - `role` property: `menu` (default) or `listbox`, forwarded to the included `` and used for `aria-haspopup` of the toggler ### Fixed diff --git a/src/components/ContextOverlay/ContextMenu.tsx b/src/components/ContextOverlay/ContextMenu.tsx index 1a3e06157..97d626c10 100644 --- a/src/components/ContextOverlay/ContextMenu.tsx +++ b/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, { MenuProps } from "../Menu/Menu"; +import Menu from "../Menu/Menu"; import ContextOverlay, { ContextOverlayProps } from "./ContextOverlay"; @@ -22,8 +22,9 @@ export interface ContextMenuProps extends TestableComponent { /** * 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?: MenuProps["role"]; + 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. @@ -68,7 +69,7 @@ export interface ContextMenuProps extends TestableComponent { export const ContextMenu = ({ children, className = "", - role, + role = "menu", togglerElement = "item-moremenu", togglerText = "Show more options", contextOverlayProps, @@ -100,11 +101,11 @@ export const ContextMenu = ({ return ( {children}} + popupKind={role === "listbox" ? PopupKind.LISTBOX : PopupKind.MENU} disabled={!!disabled} usePlaceholder={!preventPlaceholder} > diff --git a/src/components/ContextOverlay/tests/ContextMenu.test.tsx b/src/components/ContextOverlay/tests/ContextMenu.test.tsx index 9b5a825a6..7e4c8db9e 100644 --- a/src/components/ContextOverlay/tests/ContextMenu.test.tsx +++ b/src/components/ContextOverlay/tests/ContextMenu.test.tsx @@ -59,4 +59,18 @@ describe("ContextMenu", () => { expect(await screen.findByRole("listbox")).toBeInTheDocument(); expect(screen.getByRole("option", { name: "First option" })).toBeInTheDocument(); }); + it.each([true, false])( + "should set `aria-haspopup` according to role `listbox` (preventPlaceholder: %s)", + async (preventPlaceholder) => { + const { container } = render( + + + , + ); + 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"); + }, + ); }); From 562bd047dd0e27d00610ae41a14872eb0435cb3a Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Thu, 5 Nov 2026 13:05:49 +0100 Subject: [PATCH 11/15] allow Menu properties via ContextMenu --- src/components/ContextOverlay/ContextMenu.tsx | 14 ++++++++++++-- .../ContextOverlay/tests/ContextMenu.test.tsx | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/components/ContextOverlay/ContextMenu.tsx b/src/components/ContextOverlay/ContextMenu.tsx index 97d626c10..3bfcd39a7 100644 --- a/src/components/ContextOverlay/ContextMenu.tsx +++ b/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"; @@ -52,6 +52,11 @@ export interface ContextMenuProps extends TestableComponent { * Props to spread to `ContextOverlay` that is used to display the dropdown. */ contextOverlayProps?: Partial>; + /** + * 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>; /** * Disables the button to open the menu. */ @@ -73,6 +78,7 @@ export const ContextMenu = ({ togglerElement = "item-moremenu", togglerText = "Show more options", contextOverlayProps, + menuProps, disabled, togglerLarge = false, togglerSize, @@ -104,7 +110,11 @@ export const ContextMenu = ({ {...restProps} {...contextOverlayProps} className={`${eccgui}-contextmenu ` + className} - content={{children}} + content={ + + {children} + + } popupKind={role === "listbox" ? PopupKind.LISTBOX : PopupKind.MENU} disabled={!!disabled} usePlaceholder={!preventPlaceholder} diff --git a/src/components/ContextOverlay/tests/ContextMenu.test.tsx b/src/components/ContextOverlay/tests/ContextMenu.test.tsx index 7e4c8db9e..021b9ccc1 100644 --- a/src/components/ContextOverlay/tests/ContextMenu.test.tsx +++ b/src/components/ContextOverlay/tests/ContextMenu.test.tsx @@ -59,6 +59,22 @@ describe("ContextMenu", () => { 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( + + + , + ); + 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) => { From 2f9249f23040dd1a55574621e7645b021e839269 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Thu, 5 Nov 2026 13:22:00 +0100 Subject: [PATCH 12/15] validate Menu.role against roleStructure --- CHANGELOG.md | 2 +- src/components/Menu/Menu.tsx | 27 ++--- src/components/Menu/tests/Menu.test.tsx | 130 +++++++++++++++--------- 3 files changed, 96 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1d3fda04..2c186a993 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - `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 `` 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 `` child does not fit to the `role` of the menu + - emits a console warning if an explicitly set `role` of a `` child does not fit to its (explicitly or automatically set) `roleStructure` - sets `role="listitem"` (lists) or `role="none"` (listboxes) on `` children, and `aria-hidden` if the divider has no `title`; explicitly set values are kept, except `role="separator"` - `` - accepts all attributes of the `li` element, e.g. `role` and `aria-hidden` diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 15fbdaf07..b0a8a2116 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -39,16 +39,16 @@ const allowedRoleStructure: Record> = { - menu: ["menuitem", "menuitemcheckbox", "menuitemradio"], - navigation: ["link", "none", "presentation"], - list: ["link", "button", "none", "presentation"], +const allowedItemRole: Partial, 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 - listbox: ["none", "presentation"], + listoption: ["none", "presentation"], }; /** @@ -68,7 +68,7 @@ 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 the `role` of the menu. + * 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. @@ -78,7 +78,6 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: 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 itemRoles = isSupportedRole ? allowedItemRole[role as SupportedMenuRole] : undefined; const dividerRole = Object.prototype.hasOwnProperty.call(dividerRoleMapping, roleMapped) ? dividerRoleMapping[roleMapped] : undefined; @@ -112,11 +111,13 @@ export const Menu = ({ children, className = "", role = "menu", ...restProps }: 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 into Menu with role="${role}", allowed: ${itemRoles - .map((allowedRole) => `"${allowedRole}"`) - .join(", ")}.`, + `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) { diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx index 38cac08e1..8fad19c2a 100644 --- a/src/components/Menu/tests/Menu.test.tsx +++ b/src/components/Menu/tests/Menu.test.tsx @@ -352,56 +352,99 @@ describe("Menu", () => { }); describe("MenuItem role check", () => { - it.each<[MenuRole, string]>([ - ["menu", "menuitem"], - ["menu", "menuitemcheckbox"], - ["menu", "menuitemradio"], - ["navigation", "link"], - ["navigation", "none"], - ["list", "link"], - ["list", "button"], - ["list", "presentation"], - ["listbox", "none"], - ["listbox", "presentation"], - ])("should not warn for role '%s' with allowed item role '%s'", (role, itemRole) => { + it.each<[RoleStructure, string]>([ + ["menuitem", "menuitem"], + ["menuitem", "menuitemcheckbox"], + ["menuitem", "menuitemradio"], + ["listitem", "link"], + ["listitem", "button"], + ["listitem", "none"], + ["listitem", "presentation"], + ["listoption", "none"], + ["listoption", "presentation"], + ])("should not warn for roleStructure '%s' with allowed item role '%s'", (roleStructure, itemRole) => { render( - - + + , ); - expect(warnSpy).not.toHaveBeenCalled(); + // only the roleStructure warning is expected because it does not fit into `role="none"` + expect( + warnSpy.mock.calls.filter((call) => call[0].includes(`MenuItem with role="${itemRole}"`)), + ).toHaveLength(0); }); - it.each<[MenuRole, string]>([ - ["menu", "link"], - ["menu", "option"], - ["navigation", "menuitem"], - ["navigation", "listitem"], - ["list", "listitem"], - ["list", "option"], - ["listbox", "option"], - ["listbox", "menuitem"], - ])("should warn for role '%s' with not allowed item role '%s'", (role, itemRole) => { + it.each<[RoleStructure, string]>([ + ["menuitem", "link"], + ["menuitem", "option"], + ["listitem", "menuitem"], + ["listitem", "listitem"], + ["listitem", "option"], + ["listoption", "option"], + ["listoption", "menuitemcheckbox"], + ])("should warn for roleStructure '%s' with not allowed item role '%s'", (roleStructure, itemRole) => { render( - - - , - ); - expect(warnSpy).toHaveBeenCalledTimes(1); - expect(warnSpy.mock.calls[0][0]).toContain(`MenuItem with role="${itemRole}"`); - expect(warnSpy.mock.calls[0][0]).toContain(`Menu with role="${role}"`); - }); + + + , + ); + const roleWarnings = warnSpy.mock.calls + .map((call) => call[0] as string) + .filter((warning) => warning.includes(`MenuItem with role="${itemRole}"`)); + expect(roleWarnings).toHaveLength(1); + expect(roleWarnings[0]).toContain(`does not fit to its roleStructure="${roleStructure}",`); + expect(roleWarnings[0]).not.toContain("set automatically"); + }); + + it.each<[MenuRole, RoleStructure, string]>([ + ["menu", "menuitem", "link"], + ["navigation", "listitem", "menuitem"], + ["list", "listitem", "option"], + ["listbox", "listoption", "option"], + ])( + "should check the item role against the automatically set roleStructure for role '%s'", + (role, roleStructure, itemRole) => { + render( + + + , + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toContain( + `MenuItem with role="${itemRole}" does not fit to its roleStructure="${roleStructure}" (set automatically for Menu role="${role}")`, + ); + }, + ); - it.each(["none", "presentation"])("should not restrict the item role for role '%s'", (role) => { + it("should check the item role against an explicitly set roleStructure, not against the menu role", () => { render( - - - + + , ); - expect(warnSpy).not.toHaveBeenCalled(); + const warnings = warnSpy.mock.calls.map((call) => call[0] as string); + expect(warnings).toHaveLength(2); + expect(warnings.some((warning) => warning.includes('roleStructure="listoption" does not fit'))).toBe(true); + expect( + warnings.some((warning) => + warning.includes('role="menuitemcheckbox" does not fit to its roleStructure="listoption"'), + ), + ).toBe(true); }); + it.each(["none", "presentation"])( + "should not restrict the item role for roleStructure 'none' (role '%s')", + (role) => { + render( + + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }, + ); + it("should not check the item role for unsupported menu roles", () => { render( @@ -438,17 +481,6 @@ describe("Menu", () => { expect(getDomRoles(container)).toEqual([{ li: "option", target: "option" }]); }); - it("should warn separately for not allowed role and roleStructure", () => { - render( - - - , - ); - expect(warnSpy).toHaveBeenCalledTimes(2); - expect(warnSpy.mock.calls.map((call) => call[0]).join("\n")).toContain('role="menuitem"'); - expect(warnSpy.mock.calls.map((call) => call[0]).join("\n")).toContain('roleStructure="menuitem"'); - }); - it("should check the item role of Blueprint MenuItem children and inside fragments too", () => { render( From 82dc89a936d852ec85c32ac795c8f8cfc20098f6 Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Thu, 5 Nov 2026 13:23:26 +0100 Subject: [PATCH 13/15] editor appearance button is a menu now --- .../codemirror/toolbars/EditorAppearanceConfigMenu.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx b/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx index 7180accb0..bba429242 100644 --- a/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx +++ b/src/extensions/codemirror/toolbars/EditorAppearanceConfigMenu.tsx @@ -28,7 +28,6 @@ export function EditorAppearanceConfigMenu({ return ( Date: Thu, 5 Nov 2026 13:32:05 +0100 Subject: [PATCH 14/15] remove double content from wrong section --- CHANGELOG.md | 9 --------- 1 file changed, 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ad14e003..d0b8de9f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,15 +135,6 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - the webpack configuration excludes `dist/` from module resolution now, so the sources are always used even if a story references the package root - Explicitly added `assert` and `util` dependencies - the linter of `` uses `jshint`, which imports `console-browserify`, and this package requires the node core modules `assert` and `util` without declaring them; bundlers based on webpack 5 do not provide shims for node core modules anymore, so both polyfills are part of the delivery now -- Fix misused menu roles in selection popups and overlays -- `` - - improve keyboard navigation, tag removal and focus handling - - improve screen reader labels and selection announcements - - preserve filters and keep the dropdown open when toggling existing options - -### Deprecated - -- `preventReactFlowActionsClasses`: use `ClassNames.ReactFlow.preventAllActions` ### Deprecated From 792f7b984dc99cff82c1f3e45decfd2dfe5c072b Mon Sep 17 00:00:00 2001 From: Michael Haschke Date: Tue, 10 Nov 2026 10:17:49 +0100 Subject: [PATCH 15/15] improve documentation --- .../ContextOverlay/ContextMenu.stories.tsx | 9 +++---- src/components/Menu/Menu.tsx | 8 ++++--- src/components/Menu/MenuItem.tsx | 8 +++++++ src/components/Menu/Stories/Menu.stories.tsx | 24 ++++++++++++++----- .../Menu/Stories/MenuItem.stories.tsx | 7 +++--- 5 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/components/ContextOverlay/ContextMenu.stories.tsx b/src/components/ContextOverlay/ContextMenu.stories.tsx index 98b59a751..b04b66e13 100644 --- a/src/components/ContextOverlay/ContextMenu.stories.tsx +++ b/src/components/ContextOverlay/ContextMenu.stories.tsx @@ -23,11 +23,12 @@ const Template: StoryFn = (args) => ( export const Default = Template.bind({}); Default.args = { + role: "menu", children: [ - , - - - + , + + + , ], }; diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index b0a8a2116..e9935ea6e 100644 --- a/src/components/Menu/Menu.tsx +++ b/src/components/Menu/Menu.tsx @@ -18,9 +18,11 @@ type SupportedMenuRole = "menu" | "navigation" | "list" | "listbox" | "none" | " export interface MenuProps extends BlueprintMenuProps { /** * Sets the ARIA role of the menu. - * For `menu`, `navigation`, `list`, `listbox`, `none` and `presentation` it checks if the `MenuItems` using a correct `roleStructure`, - * or set it automatically. - * Other roles are set as they are, the children are not processed then. + * 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; } diff --git a/src/components/Menu/MenuItem.tsx b/src/components/Menu/MenuItem.tsx index e34b2a4e6..74a415ac1 100644 --- a/src/components/Menu/MenuItem.tsx +++ b/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( ({ children, className = "", icon, onClick, href, text, tooltip, intent, ...restProps }, ref) => { diff --git a/src/components/Menu/Stories/Menu.stories.tsx b/src/components/Menu/Stories/Menu.stories.tsx index 4c61f7ad3..699b9ec87 100644 --- a/src/components/Menu/Stories/Menu.stories.tsx +++ b/src/components/Menu/Stories/Menu.stories.tsx @@ -25,10 +25,11 @@ const MenuExample: StoryFn = (args) => ( export const Default = MenuExample.bind({}); Default.args = { + role: "menu", children: ( <> {ContentMenuStory.args.children} - + ), @@ -36,7 +37,9 @@ Default.args = { /** * Use `role="navigation"` for a list of links, e.g. as main or sidebar navigation. - * The `MenuItem` elements get `roleStructure="listitem"` automatically. + * The `MenuItem` elements need `roleStructure="listitem"`, it is set automatically if missing. + * `MenuDivider` elements need `role="listitem"` because `separator` is not allowed inside a list, + * it is set automatically if missing. Dividers without `title` get `aria-hidden` automatically. * `Menu` does not add a `nav` element, so you need to wrap it on your own. */ const NavigationExample: StoryFn = (args) => ( @@ -51,9 +54,18 @@ export const Navigation = NavigationExample.bind({}); Navigation.args = { role: "navigation", children: [ - , - , - , - , + , + , + , + , + , ], }; diff --git a/src/components/Menu/Stories/MenuItem.stories.tsx b/src/components/Menu/Stories/MenuItem.stories.tsx index f7e9e121b..024e8fc4b 100644 --- a/src/components/Menu/Stories/MenuItem.stories.tsx +++ b/src/components/Menu/Stories/MenuItem.stories.tsx @@ -29,7 +29,7 @@ export default { const MenuExample: StoryFn = (args) => ( - + @@ -39,10 +39,11 @@ export const Default = MenuExample.bind({}); Default.args = { children: ( <> - - + + ), text: "Parent option", + roleStructure: "menuitem", key: "m4", };