diff --git a/CHANGELOG.md b/CHANGELOG.md index dc06004fe..d0b8de9f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,27 @@ 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` 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 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` +- `` + - `role` property: `menu` (default) or `listbox`, forwarded to the included `` and used for `aria-haspopup` of the toggler + +### Fixed + +- 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 + ## [26.2.1] - 2026-11-02 ### Fixed @@ -115,6 +136,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - 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 +### Deprecated + +- `preventReactFlowActionsClasses`: use `ClassNames.ReactFlow.preventAllActions` + ## [26.1.0] - 2026-08-20 ### Added diff --git a/src/cmem/ActivityControl/SilkActivityControl.tsx b/src/cmem/ActivityControl/SilkActivityControl.tsx index 3f9e54ce0..b328e840c 100644 --- a/src/cmem/ActivityControl/SilkActivityControl.tsx +++ b/src/cmem/ActivityControl/SilkActivityControl.tsx @@ -1,9 +1,10 @@ import React, { useEffect, useRef, useState } from "react"; import { Intent } from "@blueprintjs/core/src/common/intent"; -import { Icon, Spacing } from "../../"; import { IntentTypes } from "../../common/Intent"; +import { Icon } from "../../components/Icon/Icon"; import { TestableComponent } from "../../components/interfaces"; +import { Spacing } from "../../components/Separation/Spacing"; import { ElapsedDateTimeDisplay, ElapsedDateTimeDisplayUnits } from "../DateTimeDisplay/ElapsedDateTimeDisplay"; import { SilkActivityStatusConcrete, SilkActivityStatusProps } from "./ActivityControlTypes"; diff --git a/src/cmem/react-flow/StickyNoteModal/StickyNoteModal.tsx b/src/cmem/react-flow/StickyNoteModal/StickyNoteModal.tsx index f04857829..64676b3c9 100644 --- a/src/cmem/react-flow/StickyNoteModal/StickyNoteModal.tsx +++ b/src/cmem/react-flow/StickyNoteModal/StickyNoteModal.tsx @@ -1,20 +1,15 @@ import React from "react"; import getColorConfiguration from "../../../common/utils/getColorConfiguration"; -import { CodeEditor } from "../../../extensions"; +import { Button } from "../../../components/Button/Button"; +import { SimpleDialog, type SimpleDialogProps } from "../../../components/Dialog/SimpleDialog"; +import { FieldItem } from "../../../components/Form/FieldItem"; +import { Icon } from "../../../components/Icon/Icon"; +import Tag from "../../../components/Tag/Tag"; +import TagList from "../../../components/Tag/TagList"; +import { CodeEditor, type CodeEditorProps } from "../../../extensions/codemirror/CodeMirror"; import { ReactFlowHotkeyContext } from "../extensions/ReactFlowHotkeyContext"; -import { - Button, - CodeEditorProps, - FieldItem, - Icon, - SimpleDialog, - SimpleDialogProps, - Tag, - TagList, -} from "./../../../index"; - export type StickyNoteModalTranslationKeys = "modalTitle" | "noteLabel" | "colorLabel" | "saveButton" | "cancelButton"; export type StickyNoteMetadataType = { note: string; color: string }; diff --git a/src/components/AutoSuggestion/AutoSuggestion.tsx b/src/components/AutoSuggestion/AutoSuggestion.tsx index 89b76b4d7..23f610958 100644 --- a/src/components/AutoSuggestion/AutoSuggestion.tsx +++ b/src/components/AutoSuggestion/AutoSuggestion.tsx @@ -1,14 +1,19 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import React, { useEffect, useMemo, useState } from "react"; -import { Classes as BlueprintClassNames } from "@blueprintjs/core"; +import { Classes as BlueprintClassNames, PopupKind } from "@blueprintjs/core"; import { EditorView, Rect } from "@codemirror/view"; import { debounce } from "lodash"; import { IntentTypes } from "../../common/Intent"; import { CLASSPREFIX as eccgui } from "../../configuration/constants"; import { SupportedCodeEditorModes } from "../../extensions/codemirror/hooks/useCodemirrorModeExtension.hooks"; +import { ContextOverlay } from "../ContextOverlay/ContextOverlay"; +import { FieldItem } from "../Form/FieldItem"; +import { IconButton } from "../Icon/IconButton"; +import { Spinner } from "../Spinner/Spinner"; +import { Toolbar } from "../Toolbar/Toolbar"; +import { ToolbarSection } from "../Toolbar/ToolbarSection"; -import { ContextOverlay, FieldItem, IconButton, Spinner, Toolbar, ToolbarSection } from "./../../"; import { markText, removeMarkFromText } from "./extensions/markText"; import { AutoSuggestionList } from "./AutoSuggestionList"; //custom components @@ -218,6 +223,7 @@ export const CodeAutocompleteField = ({ CodeAutocompleteFieldSuggestionWithReplacementInfo | undefined >(undefined); const [cm, setCM] = React.useState(); + const suggestionListId = React.useId(); const currentCm = React.useRef(undefined); currentCm.current = cm; const isFocused = React.useRef(false); @@ -241,6 +247,36 @@ export const CodeAutocompleteField = ({ /** This is for the AutoSuggestionList component in order to re-render. */ const [focusedIndex, setFocusedIndex] = React.useState(0); + + React.useEffect(() => { + if (!cm) return; + const editor = cm.contentDOM; + editor.setAttribute("role", "combobox"); + editor.setAttribute("aria-autocomplete", "list"); + editor.setAttribute("aria-haspopup", "listbox"); + return () => { + editor.setAttribute("role", "textbox"); + editor.removeAttribute("aria-autocomplete"); + editor.removeAttribute("aria-haspopup"); + editor.removeAttribute("aria-controls"); + editor.removeAttribute("aria-expanded"); + editor.removeAttribute("aria-activedescendant"); + }; + }, [cm]); + + React.useEffect(() => { + if (!cm) return; + const editor = cm.contentDOM; + const suggestionsVisible = shouldShowDropdown && !suggestionsPending && suggestions.length > 0; + editor.setAttribute("aria-expanded", String(suggestionsVisible)); + if (suggestionsVisible) { + editor.setAttribute("aria-controls", suggestionListId); + editor.setAttribute("aria-activedescendant", `${suggestionListId}-option-${focusedIndex}`); + } else { + editor.removeAttribute("aria-controls"); + editor.removeAttribute("aria-activedescendant"); + } + }, [cm, shouldShowDropdown, suggestionsPending, suggestions.length, focusedIndex, suggestionListId]); const selectedTextRanges = React.useRef([]); const pathIsValid = validationResponse?.valid ?? true; @@ -709,6 +745,7 @@ export const CodeAutocompleteField = ({ } ${blueprintIntent ? BlueprintClassNames.intentClass(blueprintIntent as any) : ""}${inputIntentClass}`} > , "children"> { + listboxId?: string; // The options of the drop down options: Array; // Called when an item has been selected from the drop down @@ -76,6 +74,7 @@ const Item = React.forwardRef(ListItem); /** A drop-down-like list that can be used in combination with other components to show and select items. */ export const AutoSuggestionList = ({ + listboxId, isOpen, options, loading, @@ -86,6 +85,8 @@ export const AutoSuggestionList = ({ offsetValues, ...otherDivProps }: AutoSuggestionListProps) => { + const generatedListboxId = React.useId(); + const optionListId = listboxId ?? generatedListboxId; const [hoveredItem, setHoveredItem] = React.useState< CodeAutocompleteFieldSuggestionWithReplacementInfo | undefined >(undefined); @@ -141,10 +142,18 @@ export const AutoSuggestionList = ({ {loading ? ( Loader ) : ( - + {options.map((item, index) => ( { + if (element) { + element.id = `${optionListId}-option-${index}`; + element.setAttribute("aria-selected", String(currentlyFocusedIndex === index)); + } + }} active={currentlyFocusedIndex === index} onMouseDown={(e: any) => e.preventDefault()} onClick={() => onItemSelectionChange(item)} diff --git a/src/components/AutoSuggestion/tests/AutoSuggestion.test.tsx b/src/components/AutoSuggestion/tests/AutoSuggestion.test.tsx index 2a097fa8e..8577079ec 100644 --- a/src/components/AutoSuggestion/tests/AutoSuggestion.test.tsx +++ b/src/components/AutoSuggestion/tests/AutoSuggestion.test.tsx @@ -51,6 +51,39 @@ describe("CodeAutocompleteField", () => { expect(getByText(props.label!)).toBeTruthy(); }); + it("connects the focused editor to the active suggestion", async () => { + const { container } = render( + ({ + inputString, + cursorPosition, + replacementResults: [ + { + replacementInterval: { from: 0, length: 5 }, + extractedQuery: "", + replacements: [{ value: "first" }, { value: "second" }], + }, + ], + })} + />, + ); + const editor = screen.getByRole("combobox"); + act(() => editor.focus()); + const options = await screen.findAllByRole("option"); + expect(container.querySelector('[aria-haspopup="menu"]')).toBeNull(); + expect(editor).toHaveAttribute("aria-haspopup", "listbox"); + expect(editor).toHaveAttribute("aria-controls", options[0].parentElement?.id); + expect(editor).toHaveAttribute("aria-activedescendant", options[0].id); + fireEvent.keyDown(editor, { key: "ArrowDown", code: "ArrowDown", keyCode: 40 }); + await waitFor(() => expect(editor).toHaveAttribute("aria-activedescendant", options[1].id)); + fireEvent.keyDown(editor, { key: "Escape", code: "Escape", keyCode: 27 }); + await waitFor(() => expect(editor).toHaveAttribute("aria-expanded", "false")); + expect(editor).not.toHaveAttribute("aria-activedescendant"); + }); + it.each([false, true])("updates the container height (multiline: %s)", (multiline) => { const { container, rerender } = render(); const editorContainer = container.querySelector(".eccgui-codeeditor"); @@ -92,7 +125,7 @@ describe("CodeAutocompleteField", () => { })} />, ); - const editor = screen.getByRole("textbox"); + const editor = screen.getByRole("combobox"); act(() => editor.focus()); expect(await screen.findByText("completion")).toBeVisible(); const view = EditorView.findFromDOM(editor); diff --git a/src/components/AutoSuggestion/tests/AutoSuggestionList.test.tsx b/src/components/AutoSuggestion/tests/AutoSuggestionList.test.tsx index ee9d7e740..43967d967 100644 --- a/src/components/AutoSuggestion/tests/AutoSuggestionList.test.tsx +++ b/src/components/AutoSuggestion/tests/AutoSuggestionList.test.tsx @@ -1,6 +1,6 @@ import React from "react"; import { Classes } from "@blueprintjs/core"; -import { fireEvent, render, waitFor } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import "@testing-library/jest-dom"; @@ -65,6 +65,9 @@ describe("Dropdown list", () => { const dropdownItems = container.getElementsByTagName("li"); expect(container).not.toBeEmptyDOMElement(); expect(dropdownItems.length).toBe(2); + expect(screen.getByRole("listbox")).toBeInTheDocument(); + expect(screen.getAllByRole("option")).toHaveLength(2); + expect(screen.queryByRole("menu")).not.toBeInTheDocument(); }); it("should move horizontally for every cursor movement", async () => { diff --git a/src/components/AutocompleteField/AutoCompleteField.test.tsx b/src/components/AutocompleteField/AutoCompleteField.test.tsx new file mode 100644 index 000000000..709a8e781 --- /dev/null +++ b/src/components/AutocompleteField/AutoCompleteField.test.tsx @@ -0,0 +1,27 @@ +import React from "react"; +import { fireEvent, render, screen } from "@testing-library/react"; + +import "@testing-library/jest-dom"; + +import { SuggestField } from "./AutoCompleteField"; + +describe("SuggestField", () => { + it("renders suggestions as listbox options", async () => { + render( + + onSearch={() => ["Alpha"]} + itemRenderer={(item) => item} + itemValueRenderer={(item) => item} + itemValueSelector={(item) => item} + itemValueString={(item) => item} + noResultText="No results" + />, + ); + + fireEvent.focus(screen.getByRole("combobox")); + + const listbox = await screen.findByRole("listbox"); + expect(listbox.querySelectorAll('[role="option"]')).toHaveLength(1); + expect(listbox.querySelectorAll('[role="menuitem"]')).toHaveLength(0); + }); +}); diff --git a/src/components/AutocompleteField/AutoCompleteField.tsx b/src/components/AutocompleteField/AutoCompleteField.tsx index 770fc222e..e8027a481 100644 --- a/src/components/AutocompleteField/AutoCompleteField.tsx +++ b/src/components/AutocompleteField/AutoCompleteField.tsx @@ -7,16 +7,14 @@ import { Suggest as BlueprintSuggest } from "@blueprintjs/select"; import { preventReactFlowDragClass } from "../../cmem/react-flow/ReactFlow/constants"; import { CLASSPREFIX as eccgui } from "../../configuration/constants"; -import { - ContextOverlayProps, - Highlighter, - IconButton, - Menu, - MenuItem, - Notification, - OverflowText, - Spinner, -} from "../../index"; +import type { ContextOverlayProps } from "../ContextOverlay/ContextOverlay"; +import { IconButton } from "../Icon/IconButton"; +import { Menu } from "../Menu/Menu"; +import { MenuItem } from "../Menu/MenuItem"; +import { Notification } from "../Notification/Notification"; +import { Spinner } from "../Spinner/Spinner"; +import { Highlighter } from "../Typography/Highlighter"; +import { OverflowText } from "../Typography/OverflowText"; import { SuggestFieldItemRendererModifierProps } from "./interfaces"; @@ -341,6 +339,7 @@ export function SuggestField(props: SuggestFieldProps(props: SuggestFieldProps ""} itemRenderer={requestError ? requestErrorRenderer : optionRenderer} itemsEqual={areEqualItems} - noResults={} + noResults={} onItemSelect={onSelectionChange} onQueryChange={(q) => setQuery(q)} resetOnQuery={false} @@ -495,8 +494,12 @@ export function SuggestField(props: SuggestFieldProps ( - - } /> + + } + /> ) : undefined diff --git a/src/components/AutocompleteField/autoCompleteFieldUtils.tsx b/src/components/AutocompleteField/autoCompleteFieldUtils.tsx index 24e9945e3..9b7d8ca7e 100644 --- a/src/components/AutocompleteField/autoCompleteFieldUtils.tsx +++ b/src/components/AutocompleteField/autoCompleteFieldUtils.tsx @@ -30,7 +30,14 @@ export const createNewItemRendererFactory = ( ); } return ( - + ); }; }; 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/ContextOverlay/ContextMenu.tsx b/src/components/ContextOverlay/ContextMenu.tsx index 1515b0fd5..3bfcd39a7 100644 --- a/src/components/ContextOverlay/ContextMenu.tsx +++ b/src/components/ContextOverlay/ContextMenu.tsx @@ -1,10 +1,11 @@ import React, { ReactElement } from "react"; +import { PopupKind } from "@blueprintjs/core"; 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 +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. @@ -45,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. */ @@ -62,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, @@ -96,7 +110,12 @@ 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/ContextOverlay.tsx b/src/components/ContextOverlay/ContextOverlay.tsx index 2b4949f98..27caf2dfe 100644 --- a/src/components/ContextOverlay/ContextOverlay.tsx +++ b/src/components/ContextOverlay/ContextOverlay.tsx @@ -11,6 +11,11 @@ import { CLASSPREFIX as eccgui } from "../../configuration/constants"; import { WhiteSpaceContainer, WhiteSpaceContainerProps } from "../Typography"; export interface ContextOverlayProps extends Omit { + /** + * Semantic type of the popup, matching its content role. + * When omitted, the target uses aria-haspopup="false" for generic attached content. + */ + popupKind?: BlueprintPopoverProps["popupKind"]; /** * `target` element to use as toggler for the overlay display. */ @@ -44,6 +49,7 @@ export const ContextOverlay = ({ usePlaceholder = false, paddingSize, content, + popupKind, ...otherPopoverProps }: ContextOverlayProps) => { const placeholderRef = React.useRef(null); @@ -159,6 +165,10 @@ export const ContextOverlay = ({ }, React.cloneElement(childTarget, { ...childTarget.props, + "aria-haspopup": + otherPopoverProps.interactionKind === InteractionKind.HOVER_TARGET_ONLY + ? undefined + : (popupKind ?? false), className: childTarget.props.className ?? "" + (otherPopoverProps.fill ? ` ${BlueprintClasses.FILL}` : ""), tabIndex: @@ -194,6 +204,8 @@ export const ContextOverlay = ({ ) : undefined } {...otherPopoverProps} + // Blueprint forwards this value directly to aria-haspopup, but its type excludes the valid false value. + popupKind={(popupKind ?? false) as BlueprintPopoverProps["popupKind"]} className={targetClassName} portalClassName={portalClassNameFinal.trim() ?? undefined} ref={refocus} diff --git a/src/components/ContextOverlay/tests/ContextMenu.test.tsx b/src/components/ContextOverlay/tests/ContextMenu.test.tsx index e2c909521..021b9ccc1 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"; @@ -37,7 +38,55 @@ describe("ContextMenu", () => { it("if placeholder is used the menu should be displayed on click", async () => { const { container } = render(); checkForPlaceholderClass(container, 1); + expect(screen.getByRole("button")).toHaveAttribute("aria-haspopup", "menu"); fireEvent.click(container.getElementsByClassName(overlayWrapper)[0]); expect(await screen.findByText("First option")).toBeVisible(); + 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(); + 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(); + }); + 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) => { + 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"); + }, + ); }); diff --git a/src/components/ContextOverlay/tests/ContextOverlay.test.tsx b/src/components/ContextOverlay/tests/ContextOverlay.test.tsx index 5c417dc33..2d349cbce 100644 --- a/src/components/ContextOverlay/tests/ContextOverlay.test.tsx +++ b/src/components/ContextOverlay/tests/ContextOverlay.test.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { PopoverInteractionKind } from "@blueprintjs/core"; +import { PopoverInteractionKind, PopupKind } from "@blueprintjs/core"; import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import "@testing-library/jest-dom"; @@ -17,6 +17,40 @@ const checkForPlaceholderClass = (container: HTMLElement, tobe: number) => { }; describe("ContextOverlay", () => { + it("does not announce generic attached content as a menu", () => { + render( + Attached information

}> + + , + ); + expect(screen.getByRole("button", { name: "Show information" })).toHaveAttribute("aria-haspopup", "false"); + }); + + it.each([PopupKind.MENU, PopupKind.LISTBOX, PopupKind.DIALOG])( + "announces an explicit %s before and after replacing the placeholder", + async (popupKind) => { + render( + Popup}> + + , + ); + expect(screen.getByRole("button", { name: "Open popup" })).toHaveAttribute("aria-haspopup", popupKind); + fireEvent.click(screen.getByRole("button", { name: "Open popup" })); + expect(await screen.findByText("Popup")).toBeVisible(); + expect(screen.getByRole("button", { name: "Open popup" })).toHaveAttribute("aria-haspopup", popupKind); + }, + ); + + it("does not advertise a menu to a custom target renderer", () => { + render( + Attached information

} + renderTarget={({ isOpen, ...targetProps }) => } + />, + ); + expect(screen.getByRole("button", { name: "Show information" })).toHaveAttribute("aria-haspopup", "false"); + }); + it("should not render placeholder automatically", () => { const { container } = render(); checkForPlaceholderClass(container, 0); diff --git a/src/components/Menu/Menu.tsx b/src/components/Menu/Menu.tsx index 5f6f8563c..e9935ea6e 100644 --- a/src/components/Menu/Menu.tsx +++ b/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> = { + navigation: "list", +}; + +const allowedRoleStructure: Record[]> = { + 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, 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> = { + 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[], + ): React.ReactNode => + React.Children.map(nodes, (child) => { + if (React.isValidElement<{ children?: React.ReactNode }>(child) && child.type === React.Fragment) { + return {processItems(child.props.children, subroles)}; + } + if (React.isValidElement(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(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 ( - - {children} + + {items} ); }; 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/MenuItem.tsx b/src/components/Menu/MenuItem.tsx index bed6fa6a9..74a415ac1 100644 --- a/src/components/Menu/MenuItem.tsx +++ b/src/components/Menu/MenuItem.tsx @@ -34,47 +34,50 @@ 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 = ({ - children, - className = "", - icon, - onClick, - href, - text, - tooltip, - intent, - ...restProps -}: MenuItemProps) => { - return ( - - {text} - - ) : ( - text - ) - } - href={href} - onClick={(e: React.MouseEvent) => - openInNewTab(e as React.MouseEvent, onClick, href) - } - className={classNames(`${eccgui}-menu__item`, className, { - // control blueprint intent classes to enhance it by new options - [`${BlueprintClasses.getClassNamespace()}-intent-${intent}`]: intent, - })} - icon={icon ? typeof icon === "string" || Array.isArray(icon) ? : icon : false} - > - {children ?? null} - - ); -}; +export const MenuItem = React.forwardRef( + ({ children, className = "", icon, onClick, href, text, tooltip, intent, ...restProps }, ref) => { + return ( + + {text} + + ) : ( + text + ) + } + href={href} + onClick={(e: React.MouseEvent) => + openInNewTab(e as React.MouseEvent, onClick, href) + } + className={classNames(`${eccgui}-menu__item`, className, { + // control blueprint intent classes to enhance it by new options + [`${BlueprintClasses.getClassNamespace()}-intent-${intent}`]: intent, + })} + icon={icon ? typeof icon === "string" || Array.isArray(icon) ? : icon : false} + > + {children ?? null} + + ); + }, +); + +MenuItem.displayName = "MenuItem"; export default MenuItem; diff --git a/src/components/Menu/Stories/Menu.stories.tsx b/src/components/Menu/Stories/Menu.stories.tsx index 2c2d4d4b7..699b9ec87 100644 --- a/src/components/Menu/Stories/Menu.stories.tsx +++ b/src/components/Menu/Stories/Menu.stories.tsx @@ -25,11 +25,47 @@ const MenuExample: StoryFn = (args) => ( export const Default = MenuExample.bind({}); Default.args = { + role: "menu", children: ( <> {ContentMenuStory.args.children} - + ), }; + +/** + * Use `role="navigation"` for a list of links, e.g. as main or sidebar navigation. + * 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) => ( + + + +); + +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", }; diff --git a/src/components/Menu/tests/Menu.test.tsx b/src/components/Menu/tests/Menu.test.tsx new file mode 100644 index 000000000..8fad19c2a --- /dev/null +++ b/src/components/Menu/tests/Menu.test.tsx @@ -0,0 +1,634 @@ +import React from "react"; +import { Classes as BlueprintClasses, 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.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`); + 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 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( + + + , + ); + 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("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("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("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( + + + + , + ); + 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("MenuItem role check", () => { + 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( + + + , + ); + // 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<[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( + + + , + ); + 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("should check the item role against an explicitly set roleStructure, not against the menu role", () => { + render( + + + , + ); + 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( + + + , + ); + 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 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"], + ["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 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( + + + , + ); + 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(); + }); + }); +}); diff --git a/src/components/MultiSelect/MultiSelect.tsx b/src/components/MultiSelect/MultiSelect.tsx index 497151d59..3713ae9b2 100644 --- a/src/components/MultiSelect/MultiSelect.tsx +++ b/src/components/MultiSelect/MultiSelect.tsx @@ -1,26 +1,23 @@ import React, { useRef } from "react"; -import { HTMLInputProps as BlueprintHTMLInputProps } from "@blueprintjs/core"; +import { Classes as BlueprintClasses, HTMLInputProps as BlueprintHTMLInputProps, mergeRefs } from "@blueprintjs/core"; import { ItemRendererProps as BlueprintItemRendererProps, MultiSelect as BlueprintMultiSelect, MultiSelectProps as BlueprintMultiSelectProps, } from "@blueprintjs/select"; import classNames from "classnames"; +import { uniqBy } from "lodash"; import { IntentBlueprint } from "../../common/Intent"; import { removeExtraSpaces } from "../../common/utils/stringUtils"; import { CLASSPREFIX as eccgui } from "../../configuration/constants"; +import type { ContextOverlayProps } from "../ContextOverlay/ContextOverlay"; +import { IconButton } from "../Icon/IconButton"; import { TestableComponent } from "../interfaces"; - -import { - ContextOverlayProps, - Highlighter, - highlighterUtils, - IconButton, - MenuItem, - OverflowText, - Spinner, -} from "./../../index"; +import { MenuItem, type MenuItemProps } from "../Menu/MenuItem"; +import { Spinner } from "../Spinner/Spinner"; +import { Highlighter, highlighterUtils } from "../Typography/Highlighter"; +import { OverflowText } from "../Typography/OverflowText"; export interface MultiSuggestFieldSelectionProps { newlySelected?: T; @@ -110,8 +107,8 @@ export interface MultiSuggestFieldCommonProps requestDelay?: number; /** - * Clear query when an option is selected or unselected. - * The query is empty then and the user need to enter a new query. + * Clear query when an existing option is selected or unselected. Defaults to false to preserve the filter. + * Creating a new item always clears the query. */ clearQueryOnSelection?: boolean; /** @@ -206,7 +203,8 @@ export function MultiSuggestField({ // Options created by a user const createdItems = useRef([]); // Options passed outside (f.e. from the backend) - const [externalItems, setExternalItems] = React.useState([...items]); + const externalItems = useRef(items); + externalItems.current = items; // All options (created and passed) that match the query const [filteredItems, setFilteredItems] = React.useState([]); // All options (created and passed) selected by a user @@ -216,22 +214,46 @@ export function MultiSuggestField({ // Max height of the menu const [calculatedMaxHeight, setCalculatedMaxHeight] = React.useState(null); - // The active popover item is only needed for keyboard interaction and should not trigger rerenders. - const focusedItemRef = React.useRef(null); const [showSpinner, setShowSpinner] = React.useState(false); + const [selectionAnnouncement, setSelectionAnnouncement] = React.useState(""); + const [keepOpenAfterSelection, setKeepOpenAfterSelection] = React.useState(false); + const optionIds = React.useRef(new Map()); + const createdOptionId = React.useId(); const inputRef = React.useRef(null); const requestState = useRef<{ query?: string; timeoutId?: number; + queryItems?: T[]; }>({}); const selectionChange = useRef({ type: "none" }); + const availableItems = (options: T[] = externalItems.current): T[] => + uniqBy([...options, ...createdItems.current], itemId); + + React.useLayoutEffect(() => { + const input = inputRef.current; + const target = input?.closest(`.${eccgui}-multiselect__target`); + if (!input || !target) return; + + // Blueprint puts the combobox role on a wrapper; active-descendant navigation needs it on the focused input. + target.removeAttribute("role"); + const updateExpandedState = () => { + const expanded = target.getAttribute("aria-expanded") ?? "false"; + input.setAttribute("aria-expanded", expanded); + if (expanded === "false") input.removeAttribute("aria-activedescendant"); + }; + updateExpandedState(); + const observer = new MutationObserver(updateExpandedState); + observer.observe(target, { attributes: true, attributeFilter: ["aria-expanded"] }); + return () => observer.disconnect(); + }, []); /** Update external items when they change * e.g for auto-complete when query change */ React.useEffect(() => { - setExternalItems(items); - setFilteredItems([...items, ...createdItems.current]); + setFilteredItems( + filterItems(availableItems(requestState.current.queryItems ?? items), requestState.current.query ?? ""), + ); }, [items.map((item) => itemId(item)).join("|")]); React.useEffect(() => { @@ -305,6 +327,10 @@ export function MultiSuggestField({ * @param matcher */ const removeItemSelection = (matcher: string) => { + const removedItem = selectedItems.find((item) => itemId(item) === matcher); + if (removedItem) { + setSelectionAnnouncement(`Removed ${itemLabel(removedItem)}`); + } setSelectedItems((items) => { const removedItem = items.find((item) => itemId(item) === matcher); @@ -323,21 +349,51 @@ export function MultiSuggestField({ }); }; + const filterItems = (items: T[], query: string): T[] => { + if (!query) return items; + if (searchListPredicate) return searchListPredicate(items, query); + if (searchPredicate) return items.filter((item) => searchPredicate(item, query)); + return defaultSearchListPredicate(items, query); + }; + + const resetQuery = () => { + requestState.current.query = ""; + requestState.current.queryItems = undefined; + if (requestState.current.timeoutId) { + clearTimeout(requestState.current.timeoutId); + requestState.current.timeoutId = undefined; + } + setShowSpinner(false); + setFilteredItems(availableItems()); + }; + /** * selects and deselects an item from selection list * if the item exists it removes it instead * @param item */ const onItemSelect = (item: T) => { + const isNewItem = + !!createNewItemFromQuery && + ![...availableItems(), ...filteredItems, ...selectedItems].some( + (option) => itemId(option) === itemId(item), + ); + if (isNewItem) { + createdItems.current = [...createdItems.current, item]; + } + inputRef.current?.removeAttribute("aria-activedescendant"); if (itemHasBeenSelectedAlready(itemId(item))) { removeItemSelection(itemId(item)); } else { + setSelectionAnnouncement(`Selected ${itemLabel(item)}`); selectionChange.current = { type: "selected", item }; setSelectedItems((items) => [...items, item]); } - if (clearQueryOnSelection) { - requestState.current.query = ""; + if (isNewItem || clearQueryOnSelection) { + // Blueprint otherwise closes a keyboard-opened popup when the cleared query reaches its QueryList. + setKeepOpenAfterSelection(true); + resetQuery(); inputRef.current?.focus(); } else { inputRef.current?.select(); @@ -351,6 +407,7 @@ export function MultiSuggestField({ const onQueryChange = (query: string) => { if (query.length && query !== requestState.current.query) { requestState.current.query = query; + requestState.current.queryItems = undefined; if (requestState.current.timeoutId) { clearTimeout(requestState.current.timeoutId); } @@ -360,25 +417,15 @@ export function MultiSuggestField({ const resultFromQuery = runOnQueryChange && (await runOnQueryChange(removeExtraSpaces(query))); if (requestState.current.query === query) { // Only use most recent request - const outsideOptions = [...(resultFromQuery ?? externalItems)]; - let itemFilter = defaultSearchListPredicate; - if (searchListPredicate) { - itemFilter = searchListPredicate; - } else if (searchPredicate) { - itemFilter = (items, query) => { - return items.filter((item) => searchPredicate(item, query)); - }; - } - - setFilteredItems(itemFilter([...outsideOptions, ...createdItems.current], query)); + requestState.current.queryItems = resultFromQuery; + setFilteredItems(filterItems(availableItems(resultFromQuery ?? externalItems.current), query)); setShowSpinner(false); } }; requestState.current.timeoutId = window.setTimeout(fn, requestDelay && requestDelay > 0 ? requestDelay : 0); } else if (!query.length) { // if the query is empty we need to show all options and reset current query - requestState.current.query = ""; - setFilteredItems(() => [...externalItems, ...createdItems.current]); + resetQuery(); } }; @@ -400,6 +447,7 @@ export function MultiSuggestField({ } return ( ({ * clear all selected items in the multi-select input */ const handleClear = () => { - requestState.current.query = ""; + resetQuery(); + setSelectionAnnouncement("Selection cleared"); selectionChange.current = { type: "none" }; setSelectedItems([]); - setFilteredItems([...externalItems, ...createdItems.current]); }; /** @@ -427,6 +475,10 @@ export function MultiSuggestField({ * @param index */ const removeTagFromSelectionViaIndex = (_label: React.ReactNode, index: number) => { + const removedItem = selectedItems[index]; + if (removedItem) { + setSelectionAnnouncement(`Removed ${itemLabel(removedItem)}`); + } setSelectedItems((items) => { const removedItem = items[index]; @@ -436,53 +488,6 @@ export function MultiSuggestField({ }); }; - /** - * Utility function to create a new Item. createNewItemFromQuery is assumed to be defined! - */ - const createNewItem = (query: string): T => { - const newItem = createNewItemFromQuery!(query); - //set new items - createdItems.current = [...createdItems.current, newItem]; - setFilteredItems((items) => [...items, newItem]); - requestState.current.query = ""; - return newItem; - }; - - /** - * added functionality to create new item when there are no matching items on enter keypress - * @param event - */ - const handleOnKeyUp = (event: React.KeyboardEvent) => { - if (event.key === "Enter" && !filteredItems.length && !!requestState.current.query && createNewItemFromQuery) { - if (!isValidNewOption || isValidNewOption(requestState.current.query)) { - createNewItem(requestState.current.query); - } - } - inputRef.current?.focus(); - }; - - /** - * added functionality to either create new item - * when there are no matching items or select an item on tab keypress - * @param event - */ - const handleOnKeyDown = (event: React.KeyboardEvent) => { - if (event.key === "Tab" && !!requestState.current.query) { - event.preventDefault(); - if (focusedItemRef.current) { - onItemSelect(focusedItemRef.current); - } else { - if (!isValidNewOption || isValidNewOption(requestState.current.query)) { - onItemSelect(createNewItem(requestState.current.query)); - } else { - return; - } - } - requestState.current.query = ""; - setTimeout(() => inputRef.current?.focus()); - } - }; - /** * create new item handler, displays the new item selector and creates a new item when selected * @param label ' @@ -492,28 +497,130 @@ export function MultiSuggestField({ */ const newItemRenderer = (label: string, active: boolean, handleClick: React.MouseEventHandler) => { if (!createNewItemFromQuery || (isValidNewOption && !isValidNewOption(label))) return undefined; - const clickHandler = (e: React.MouseEvent) => { - createNewItem(label); - handleClick(e); - }; return ( {`${newItemCreationText} '${label}'`}} /> ); }; + const handleOptionKeyDown = (event: React.KeyboardEvent) => { + if (!event.defaultPrevented && !event.nativeEvent.isComposing && ["Enter", " "].includes(event.key)) { + event.preventDefault(); + // Clicking focuses the input; prevent QueryList from selecting again on the ensuing input keyup. + event.stopPropagation(); + event.currentTarget.click(); + } + }; + + const renderOption = (item: T, props: BlueprintItemRendererProps) => { + const rendered = otherMultiSelectProps.itemRenderer + ? otherMultiSelectProps.itemRenderer(item, props) + : onItemRenderer(item, props); + if (!React.isValidElement(rendered) || rendered.type !== MenuItem) { + return rendered; + } + const optionId = props.id ?? `${createdOptionId}-item-${props.index}`; + const isSelected = itemHasBeenSelectedAlready(itemId(item)); + const hasSelectionIcon = rendered.props.icon === "state-checked" || rendered.props.icon === "state-unchecked"; + optionIds.current.set(itemId(item), optionId); + return React.cloneElement(rendered, { + roleStructure: "listoption", + onKeyDown: (event) => { + rendered.props.onKeyDown?.(event); + handleOptionKeyDown(event); + }, + // Blueprint adds its own tick for selected options, which duplicates our checkbox icon. + selected: isSelected && !hasSelectionIcon, + ref: mergeRefs(props.ref, (element: HTMLLIElement | null) => { + if (element) { + element.id = optionId; + if (!otherMultiSelectProps.itemRenderer) { + element.setAttribute("aria-label", itemLabel(item)); + } + element.setAttribute("aria-selected", String(isSelected)); + if (element.parentElement?.id) { + inputRef.current?.setAttribute("aria-controls", element.parentElement.id); + } + if (props.modifiers.active) { + inputRef.current?.setAttribute("aria-activedescendant", optionId); + } + } + }), + }); + }; + + const renderCreateOption = (label: string, active: boolean, handleClick: React.MouseEventHandler) => { + const rendered = otherMultiSelectProps.createNewItemRenderer + ? otherMultiSelectProps.createNewItemRenderer(label, active, handleClick) + : newItemRenderer(label, active, handleClick); + return rendered && React.isValidElement(rendered) && rendered.type === MenuItem + ? React.cloneElement(rendered, { + roleStructure: "listoption", + onKeyDown: (event) => { + rendered.props.onKeyDown?.(event); + handleOptionKeyDown(event); + }, + selected: false, + ref: (element: HTMLLIElement | null) => { + if (element) { + element.id = createdOptionId; + if (element.parentElement?.id) { + inputRef.current?.setAttribute("aria-controls", element.parentElement.id); + } + if (active) { + inputRef.current?.setAttribute("aria-activedescendant", createdOptionId); + } + } + }, + }) + : rendered; + }; + + const providedTagProps = tagInputProps?.tagProps; + const getTagProps: NonNullable["tagInputProps"]>["tagProps"] = (tag, index) => { + const props = typeof providedTagProps === "function" ? providedTagProps(tag, index) : (providedTagProps ?? {}); + return { + minimal: true, + ...props, + onKeyDownCapture: (event: React.KeyboardEvent) => { + props.onKeyDownCapture?.(event); + if ((event.target as HTMLElement).matches(`.${BlueprintClasses.TAG_REMOVE}`)) { + event.stopPropagation(); + } + }, + onKeyUpCapture: (event: React.KeyboardEvent) => { + props.onKeyUpCapture?.(event); + if ((event.target as HTMLElement).matches(`.${BlueprintClasses.TAG_REMOVE}`)) { + event.stopPropagation(); + } + }, + ref: mergeRefs(typeof props.ref === "string" ? undefined : props.ref, (element: HTMLSpanElement | null) => { + // Blueprint hardcodes the same accessible name for every tag's remove button. + const removeButton = element?.querySelector(`.${BlueprintClasses.TAG_REMOVE}`); + if (removeButton && selectedItems[index]) { + removeButton.setAttribute("aria-label", `Removing tag '${itemLabel(selectedItems[index])}'`); + } + }), + }; + }; + // Clear button and spinner are both shown as "right element" const clearButton = selectedItems.length > 0 ? ( ({ onQueryChange={onQueryChange} items={filteredItems} onItemSelect={onItemSelect} - itemRenderer={onItemRenderer} itemsEqual={(a: T, b: T) => itemId(a) === itemId(b)} selectedItems={selectedItems} - noResults={} + noResults={} tagRenderer={(item) => itemLabel(item)} - createNewItemRenderer={newItemRenderer} - onActiveItemChange={(activeItem) => { - focusedItemRef.current = activeItem; - }} fill={fullWidth} createNewItemFromQuery={createNewItemFromQuery} disabled={disabled} @@ -553,9 +655,16 @@ export function MultiSuggestField({ inputProps: { id: "item", autoComplete: "off", + role: "combobox", + "aria-haspopup": "listbox", + "aria-autocomplete": "list", "data-test-id": dataTestId ? dataTestId + "_searchinput" : undefined, "data-testid": dataTestid ? dataTestid + "_searchinput" : undefined, ...inputProps, + onBlur: (event) => { + event.currentTarget.removeAttribute("aria-activedescendant"); + inputProps?.onBlur?.(event); + }, } as React.InputHTMLAttributes, className: classNames(`${eccgui}-multisuggestfield`, `${eccgui}-multiselect`, className, { [`${eccgui}-intent--${intent}`]: intent === "accent", @@ -564,8 +673,6 @@ export function MultiSuggestField({ inputRef: inputRef, intent: intent && intent !== "accent" ? intent : undefined, addOnBlur: true, - onKeyDown: handleOnKeyDown, - onKeyUp: handleOnKeyUp, onRemove: removeTagFromSelectionViaIndex, rightElement: ( <> @@ -574,25 +681,34 @@ export function MultiSuggestField({ )} ), - tagProps: { minimal: true }, disabled, ...tagInputProps, + tagProps: getTagProps, ...spinnerProps, }} - popoverTargetProps={{ - className: `${eccgui}-multiselect__target`, - }} popoverProps={{ minimal: true, placement: "bottom-start", matchTargetWidth: fullWidth, ...contextOverlayProps, + onClosing: (node) => { + setKeepOpenAfterSelection(false); + contextOverlayProps?.onClosing?.(node); + }, + onOpening: (node) => { + // Blueprint makes the whole overlay live, which announces every option when it opens. + node.closest(`.${BlueprintClasses.OVERLAY}`)?.setAttribute("aria-live", "off"); + contextOverlayProps?.onOpening?.(node); + }, }} popoverContentProps={ { @@ -606,6 +722,28 @@ export function MultiSuggestField({ } as BlueprintMultiSelectProps["popoverContentProps"] } {...otherMultiSelectProps} + openOnKeyDown={keepOpenAfterSelection ? false : otherMultiSelectProps.openOnKeyDown} + popoverTargetProps={{ + className: `${eccgui}-multiselect__target`, + onKeyDown: (event) => { + // Blueprint explicitly blurs on Escape before invoking this handler. + if (event.key === "Escape") inputRef.current?.focus(); + }, + }} + itemRenderer={renderOption} + createNewItemRenderer={renderCreateOption} + onActiveItemChange={(activeItem, isCreateNewItem) => { + const optionId = isCreateNewItem + ? createdOptionId + : activeItem + ? optionIds.current.get(itemId(activeItem)) + : undefined; + if (optionId) { + inputRef.current?.setAttribute("aria-activedescendant", optionId); + } else { + inputRef.current?.removeAttribute("aria-activedescendant"); + } + }} /> ); @@ -616,8 +754,16 @@ export function MultiSuggestField({ {...{ "data-test-id": dataTestId, "data-testid": dataTestid }} > {contentMultiSelect} + + {selectionAnnouncement} + ) : ( - <>{contentMultiSelect} + <> + {contentMultiSelect} + + {selectionAnnouncement} + + ); } diff --git a/src/components/MultiSuggestField/tests/MultiSuggestField.regressions.test.tsx b/src/components/MultiSuggestField/tests/MultiSuggestField.regressions.test.tsx new file mode 100644 index 000000000..cb94adb38 --- /dev/null +++ b/src/components/MultiSuggestField/tests/MultiSuggestField.regressions.test.tsx @@ -0,0 +1,298 @@ +import React from "react"; +import { act, fireEvent, render, screen, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; + +import "@testing-library/jest-dom"; + +import { MenuItem, MultiSuggestField, MultiSuggestFieldSelectionProps } from "../../../../index"; + +interface Option { + id: string; + label: string; +} + +const itemId = (item: Option) => item.id; +const itemLabel = (item: Option) => item.label; +const createItem = (query: string): Option => ({ id: query, label: query }); +const requestDelay = 200; + +const deferredOptions = () => { + let resolve!: (items: Option[]) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +}; + +const advanceTime = async (milliseconds = requestDelay) => { + await act(async () => { + jest.advanceTimersByTime(milliseconds); + }); +}; + +describe("MultiSuggestField regressions", () => { + beforeEach(() => jest.useFakeTimers()); + + afterEach(() => { + act(() => jest.runOnlyPendingTimers()); + jest.useRealTimers(); + }); + + it("shows created options once when the parent feeds selected values back as items", async () => { + const SelectionField = () => { + const [selected, setSelected] = React.useState([]); + const onSelection = React.useCallback(({ selectedItems }: MultiSuggestFieldSelectionProps