fix(MenuItem2): improve selected appearance and a11y

packages/core/src/components/menu/menuItem.tsx

@@ -35,15 +35,7 @@ export interface IMenuItemProps extends ActionProps, LinkProps {
     text: React.ReactNode;
 
     /**
-     * Whether this item should render with an active appearance.
-     * This is the same styling as the `:active` CSS element state.
-     *
-     * Note: in Blueprint 3.x, this prop was conflated with a "selected" appearance
-     * when `intent` was undefined. For legacy purposes, we emulate this behavior in
-     * Blueprint 4.x, so setting `active={true} intent={undefined}` is the same as
-     * `selected={true}`. This prop will be removed in a future major version.
-     *
-     * @deprecated use `selected` prop
+     * Whether this item should render with an active appearance. Used to indicate keyboard focus.
      */
     active?: boolean;
 
@@ -117,7 +109,8 @@ export interface IMenuItemProps extends ActionProps, LinkProps {
     popoverProps?: Partial<IPopoverProps>;
 
     /**
-     * Whether this item should appear selected.
+     * Whether this item is selected. Will set `aria-selected` to true, and apply a check icon next to the
+     * item (unless a different icon is specified).
      */
     selected?: boolean;
 

packages/core/test/menu/menuItemTests.tsx

@@ -72,8 +72,9 @@ describe("MenuItem", () => {
     });
 
     it("can set roleStructure to change role prop structure to that of a listbox or select item", () => {
-        const wrapper = mount(<MenuItem text="Roles" roleStructure="listoption" />);
+        const wrapper = mount(<MenuItem text="Roles" roleStructure="listoption" selected={true} />);
         assert.equal(wrapper.find("li").prop("role"), "option");
+        assert.equal(wrapper.find("li").prop("aria-selected"), true);
         assert.equal(wrapper.find("a").prop("role"), undefined);
     });
 

packages/docs-app/src/common/filmSelect.tsx

@@ -18,12 +18,13 @@ import * as React from "react";
 
 import { Button } from "@blueprintjs/core";
 import { MenuItem2 } from "@blueprintjs/popover2";
-import { Select2, Select2Props } from "@blueprintjs/select";
+import { ItemRenderer, Select2, Select2Props } from "@blueprintjs/select";
 
 import {
     areFilmsEqual,
     createFilm,
-    filmSelectProps,
+    filterFilm,
+    getFilmItemProps,
     IFilm,
     maybeAddCreatedFilmToArrays,
     maybeDeleteCreatedFilmFromArrays,
@@ -37,11 +38,12 @@ type Props = Omit<
     Select2Props<IFilm>,
     | "createNewItemFromQuery"
     | "createNewItemRenderer"
+    | "itemPredicate"
+    | "itemRenderer"
     | "items"
     | "itemsEqual"
     | "noResults"
     | "onItemSelect"
-    | keyof typeof filmSelectProps
 > & {
     allowCreate?: boolean;
 };
@@ -51,38 +53,49 @@ export default function ({ allowCreate = false, fill, ...restProps }: Props) {
     const maybeCreateNewItemFromQuery = allowCreate ? createFilm : undefined;
     const maybeCreateNewItemRenderer = allowCreate ? renderCreateFilmOption : null;
 
-    const [items, setItems] = React.useState(filmSelectProps.items);
+    const [items, setItems] = React.useState([...TOP_100_FILMS]);
     const [createdItems, setCreatedItems] = React.useState<IFilm[]>([]);
-    const [film, setFilm] = React.useState(TOP_100_FILMS[0]);
+    const [selectedFilm, setSelectedFilm] = React.useState(TOP_100_FILMS[0]);
     const handleItemSelect = React.useCallback((newFilm: IFilm) => {
         // Delete the old film from the list if it was newly created.
-        const step1Result = maybeDeleteCreatedFilmFromArrays(items, createdItems, film);
+        const step1Result = maybeDeleteCreatedFilmFromArrays(items, createdItems, selectedFilm);
         // Add the new film to the list if it is newly created.
         const step2Result = maybeAddCreatedFilmToArrays(step1Result.items, step1Result.createdItems, newFilm);
         setCreatedItems(step2Result.createdItems);
-        setFilm(newFilm);
+        setSelectedFilm(newFilm);
         setItems(step2Result.items);
     }, []);
 
+    const itemRenderer = React.useCallback<ItemRenderer<IFilm>>(
+        (film, props) => {
+            if (!props.modifiers.matchesPredicate) {
+                return null;
+            }
+            return <MenuItem2 {...getFilmItemProps(film, props)} selected={film === selectedFilm} />;
+        },
+        [selectedFilm],
+    );
+
     return (
         <FilmSelect
-            {...filmSelectProps}
             createNewItemFromQuery={maybeCreateNewItemFromQuery}
             createNewItemRenderer={maybeCreateNewItemRenderer}
+            fill={fill}
+            itemPredicate={filterFilm}
+            itemRenderer={itemRenderer}
+            items={items}
             itemsEqual={areFilmsEqual}
             menuProps={{ "aria-label": "films" }}
             noResults={<MenuItem2 disabled={true} text="No results." roleStructure="listoption" />}
             onItemSelect={handleItemSelect}
-            items={items}
-            fill={fill}
             {...restProps}
         >
             <Button
-                icon="film"
-                rightIcon="caret-down"
-                text={film ? `${film.title} (${film.year})` : "(No selection)"}
                 disabled={restProps.disabled}
                 fill={fill}
+                icon="film"
+                rightIcon="caret-down"
+                text={selectedFilm ? `${selectedFilm.title} (${selectedFilm.year})` : "(No selection)"}
             />
         </FilmSelect>
     );

packages/docs-app/src/common/films.tsx

@@ -16,8 +16,8 @@
 
 import * as React from "react";
 
-import { MenuItem2 } from "@blueprintjs/popover2";
-import { ItemPredicate, ItemRenderer } from "@blueprintjs/select";
+import { MenuItem2, MenuItem2Props } from "@blueprintjs/popover2";
+import { IItemRendererProps, ItemPredicate, ItemRenderer } from "@blueprintjs/select";
 
 export interface IFilm {
     /** Title of film. */
@@ -132,23 +132,35 @@ export const TOP_100_FILMS: IFilm[] = [
     { title: "Monty Python and the Holy Grail", year: 1975 },
 ].map((m, index) => ({ ...m, rank: index + 1 }));
 
-export const renderFilm: ItemRenderer<IFilm> = (film, { handleClick, handleFocus, modifiers, query }) => {
-    if (!modifiers.matchesPredicate) {
+/**
+ * Takes the same arguments as `ItemRenderer<IFilm>`, but returns the common menu item
+ * props for that item instead of the rendered element itself. This is useful for implementing
+ * custom item renderers.
+ */
+export function getFilmItemProps(
+    film: IFilm,
+    { handleClick, handleFocus, modifiers, query }: IItemRendererProps,
+): MenuItem2Props & React.Attributes & React.HTMLAttributes<HTMLAnchorElement> {
+    return {
+        active: modifiers.active,
+        disabled: modifiers.disabled,
+        key: film.rank,
+        label: film.year.toString(),
+        onClick: handleClick,
+        onFocus: handleFocus,
+        roleStructure: "listoption",
+        text: highlightText(`${film.rank}. ${film.title}`, query),
+    };
+}
+
+/**
+ * Simple film item renderer _without_ support for "selected" appearance.
+ */
+export const renderFilm: ItemRenderer<IFilm> = (film, props) => {
+    if (!props.modifiers.matchesPredicate) {
         return null;
     }
-    const text = `${film.rank}. ${film.title}`;
-    return (
-        <MenuItem2
-            active={modifiers.active}
-            disabled={modifiers.disabled}
-            label={film.year.toString()}
-            roleStructure="listoption"
-            key={film.rank}
-            onClick={handleClick}
-            onFocus={handleFocus}
-            text={highlightText(text, query)}
-        />
-    );
+    return <MenuItem2 {...getFilmItemProps(film, props)} />;
 };
 
 export const renderCreateFilmOption = (
@@ -212,12 +224,6 @@ function escapeRegExpChars(text: string) {
     return text.replace(/([.*+?^=!:${}()|\[\]\/\\])/g, "\\$1");
 }
 
-export const filmSelectProps = {
-    itemPredicate: filterFilm,
-    itemRenderer: renderFilm,
-    items: TOP_100_FILMS,
-};
-
 export function createFilm(title: string): IFilm {
     return {
         rank: 100 + Math.floor(Math.random() * 100 + 1),

packages/docs-app/src/examples/popover2-examples/booleanOrUndefinedSelect.tsx

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2022 Palantir Technologies, Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as React from "react";
+
+import { Button, ButtonGroup, Code, Label } from "@blueprintjs/core";
+
+export type BooleanOrUndefined = undefined | false | true;
+
+export interface BooleanOrUndefinedSelectProps {
+    disabled?: boolean;
+    label: string;
+    value: BooleanOrUndefined;
+    onChange: (size: BooleanOrUndefined) => void;
+}
+
+export const BooleanOrUndefinedSelect: React.FC<BooleanOrUndefinedSelectProps> = ({
+    disabled,
+    label,
+    value,
+    onChange,
+}) => {
+    const handleUndefined = React.useCallback(() => onChange(undefined), []);
+    const handleTrue = React.useCallback(() => onChange(true), []);
+    const handleFalse = React.useCallback(() => onChange(false), []);
+
+    return (
+        <Label>
+            {label}
+            <ButtonGroup fill={true} style={{ marginTop: 5 }}>
+                <Button
+                    disabled={disabled}
+                    active={value === undefined}
+                    text={<Code>undefined</Code>}
+                    onClick={handleUndefined}
+                />
+                <Button disabled={disabled} active={value === true} text={<Code>true</Code>} onClick={handleTrue} />
+                <Button disabled={disabled} active={value === false} text={<Code>false</Code>} onClick={handleFalse} />
+            </ButtonGroup>
+        </Label>
+    );
+};

packages/docs-app/src/examples/popover2-examples/menuItem2Example.tsx

@@ -16,29 +16,58 @@
 
 import * as React from "react";
 
-import { Classes, H5, Intent, Menu, Switch } from "@blueprintjs/core";
+import { Classes, Code, H5, HTMLSelect, Intent, Label, Menu, Switch } from "@blueprintjs/core";
 import { Example, handleBooleanChange, handleValueChange, IExampleProps } from "@blueprintjs/docs-theme";
-import { MenuItem2 } from "@blueprintjs/popover2";
+import { MenuItem2, MenuItem2Props } from "@blueprintjs/popover2";
 
+import { PropCodeTooltip } from "../../common/propCodeTooltip";
 import { IntentSelect } from "../core-examples/common/intentSelect";
+import { BooleanOrUndefinedSelect } from "./booleanOrUndefinedSelect";
 
 export function MenuItem2Example(props: IExampleProps) {
     const [large, setLarge] = React.useState(false);
     const [disabled, setDisabled] = React.useState(false);
-    const [selected, setSelected] = React.useState(false);
+    const [selected, setSelected] = React.useState<boolean | undefined>(undefined);
     const [intent, setIntent] = React.useState<Intent>("none");
     const [iconEnabled, setIconEnabled] = React.useState(true);
     const [submenuEnabled, setSubmenuEnabled] = React.useState(true);
+    const [roleStructure, setRoleStructure] = React.useState<MenuItem2Props["roleStructure"]>("menuitem");
+
+    const isSelectedOptionAvailable = roleStructure === "listoption" && !iconEnabled;
 
     const options = (
         <>
             <H5>Props</H5>
             <Switch label="Large" checked={large} onChange={handleBooleanChange(setLarge)} />
             <Switch label="Disabled" checked={disabled} onChange={handleBooleanChange(setDisabled)} />
-            <Switch label="Selected" checked={selected} onChange={handleBooleanChange(setSelected)} />
+            <PropCodeTooltip
+                content={
+                    isSelectedOptionAvailable ? undefined : (
+                        <>
+                            <Code>selected</Code> prop has no effect when <br />
+                            <Code>roleStructure="menuitem"</Code> or when an icon is set
+                        </>
+                    )
+                }
+            >
+                <BooleanOrUndefinedSelect
+                    disabled={!isSelectedOptionAvailable}
+                    label="Selected"
+                    value={selected}
+                    onChange={setSelected}
+                />
+            </PropCodeTooltip>
             <Switch label="Enable icon" checked={iconEnabled} onChange={handleBooleanChange(setIconEnabled)} />
             <Switch label="Enable submenu" checked={submenuEnabled} onChange={handleBooleanChange(setSubmenuEnabled)} />
             <IntentSelect intent={intent} onChange={handleValueChange(setIntent)} />
+            <Label>
+                Role structure
+                <HTMLSelect
+                    options={["menuitem", "listoption"]}
+                    value={roleStructure}
+                    onChange={handleValueChange(setRoleStructure)}
+                />
+            </Label>
         </>
     );
 
@@ -47,11 +76,12 @@ export function MenuItem2Example(props: IExampleProps) {
             <Menu className={Classes.ELEVATION_1} large={large}>
                 <MenuItem2
                     disabled={disabled}
-                    selected={selected}
-                    text="Settings"
                     icon={iconEnabled ? "cog" : undefined}
                     intent={intent}
                     labelElement={submenuEnabled ? undefined : "⌘,"}
+                    roleStructure={roleStructure}
+                    selected={selected}
+                    text="Settings"
                     children={
                         submenuEnabled ? (
                             <>