Skip to content

Commit

Permalink
[Menu] Do not select an item when space is pressed during typeahead (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
michaldudak authored Aug 22, 2024
1 parent 919d9ca commit 7154397
Show file tree
Hide file tree
Showing 13 changed files with 97 additions and 8 deletions.
2 changes: 1 addition & 1 deletion docs/data/base/components/menu/menu.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ Menus are implemented using a collection of related components:
- `<Menu.Positioner />` renders the element responsible for positioning the popup.
- `<Menu.Popup />` is the menu popup.
- `<Menu.Item />` is the menu item.
- `<Popover.Arrow />` renders an optional pointing arrow, placed inside the popup.
- `<Menu.Arrow />` renders an optional pointing arrow, placed inside the popup.
- `<Menu.SubmenuTrigger />` is a menu item that opens a submenu. See [Nested menu](#nested-menu) for more details.

```tsx
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/Menu/Item/MenuItem.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const testRootContext: MenuRootContext = {
popupRef: { current: null },
mounted: true,
transitionStatus: undefined,
typingRef: { current: false },
};

describe('<Menu.Item />', () => {
Expand Down
6 changes: 5 additions & 1 deletion packages/mui-base/src/Menu/Item/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const InnerMenuItem = React.memo(
propGetter,
render,
treatMouseupAsClick,
typingRef,
...other
} = props;

Expand All @@ -35,6 +36,7 @@ const InnerMenuItem = React.memo(
menuEvents,
ref: forwardedRef,
treatMouseupAsClick,
typingRef,
});

const ownerState: MenuItem.OwnerState = { disabled, highlighted };
Expand Down Expand Up @@ -72,7 +74,7 @@ const MenuItem = React.forwardRef(function MenuItem(
const listItem = useListItem({ label: label ?? itemRef.current?.innerText });
const mergedRef = useForkRef(forwardedRef, listItem.ref, itemRef);

const { getItemProps, activeIndex, clickAndDragEnabled } = useMenuRootContext();
const { getItemProps, activeIndex, clickAndDragEnabled, typingRef } = useMenuRootContext();
const id = useId(idProp);

const highlighted = listItem.index === activeIndex;
Expand All @@ -91,6 +93,7 @@ const MenuItem = React.forwardRef(function MenuItem(
menuEvents={menuEvents}
propGetter={getItemProps}
treatMouseupAsClick={clickAndDragEnabled}
typingRef={typingRef}
/>
);
});
Expand All @@ -100,6 +103,7 @@ interface InnerMenuItemProps extends MenuItem.Props {
propGetter: (externalProps?: GenericHTMLProps) => GenericHTMLProps;
menuEvents: FloatingEvents;
treatMouseupAsClick: boolean;
typingRef: React.RefObject<boolean>;
}

namespace MenuItem {
Expand Down
13 changes: 12 additions & 1 deletion packages/mui-base/src/Menu/Item/useMenuItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { FloatingEvents } from '@floating-ui/react';
import { useButton } from '../../useButton';
import { mergeReactProps } from '../../utils/mergeReactProps';
import { GenericHTMLProps } from '../../utils/types';
import { MuiCancellableEvent } from '../../utils/MuiCancellableEvent';

/**
*
Expand All @@ -20,6 +21,7 @@ export function useMenuItem(params: useMenuItem.Parameters): useMenuItem.ReturnV
menuEvents,
ref: externalRef,
treatMouseupAsClick,
typingRef,
} = params;

const { getRootProps: getButtonProps, rootRef: mergedRef } = useButton({
Expand All @@ -36,6 +38,11 @@ export function useMenuItem(params: useMenuItem.Parameters): useMenuItem.ReturnV
id,
role: 'menuitem',
tabIndex: highlighted ? 0 : -1,
onKeyUp: (event: React.KeyboardEvent & MuiCancellableEvent) => {
if (event.key === ' ' && typingRef.current) {
event.defaultMuiPrevented = true;
}
},
onClick: (event: React.MouseEvent) => {
if (closeOnClick) {
menuEvents.emit('close', event);
Expand All @@ -44,7 +51,7 @@ export function useMenuItem(params: useMenuItem.Parameters): useMenuItem.ReturnV
}),
);
},
[closeOnClick, getButtonProps, highlighted, id, menuEvents, treatMouseupAsClick],
[closeOnClick, getButtonProps, highlighted, id, menuEvents, treatMouseupAsClick, typingRef],
);

return React.useMemo(
Expand Down Expand Up @@ -86,6 +93,10 @@ export namespace useMenuItem {
* If `true`, the menu item will listen for mouseup events and treat them as clicks.
*/
treatMouseupAsClick: boolean;
/**
* A ref that is set to `true` when the user is using the typeahead feature.
*/
typingRef: React.RefObject<boolean>;
}

export interface ReturnValue {
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/Menu/Popup/MenuPopup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const testRootContext: MenuRootContext = {
popupRef: { current: null },
mounted: true,
transitionStatus: undefined,
typingRef: { current: false },
};

const testPositionerContext: MenuPositionerContext = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const testRootContext: MenuRootContext = {
popupRef: { current: null },
mounted: true,
transitionStatus: undefined,
typingRef: { current: false },
};

describe('<Menu.Positioner />', () => {
Expand Down
49 changes: 46 additions & 3 deletions packages/mui-base/src/Menu/Root/MenuRoot.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { expect } from 'chai';
import { act, flushMicrotasks, waitFor } from '@mui/internal-test-utils';
import * as Menu from '@base_ui/react/Menu';
import userEvent from '@testing-library/user-event';
import { spy } from 'sinon';
import { createRenderer } from '#test-utils';

describe('<Menu.Root />', () => {
Expand Down Expand Up @@ -135,7 +136,7 @@ describe('<Menu.Root />', () => {
it('changes the highlighted item', async function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
// useMenuPopup Text navigation match menu items using HTMLElement.innerText
// innerText is not supported by JsDom
// innerText is not supported by JSDOM
this.skip();
}

Expand Down Expand Up @@ -209,13 +210,19 @@ describe('<Menu.Root />', () => {
await waitFor(() => {
expect(items[2]).toHaveFocus();
});
expect(items[2]).to.have.attribute('tabindex', '0');

await waitFor(() => {
expect(items[2]).to.have.attribute('tabindex', '0');
});

await user.keyboard('b');
await waitFor(() => {
expect(items[2]).toHaveFocus();
});
expect(items[2]).to.have.attribute('tabindex', '0');

await waitFor(() => {
expect(items[2]).to.have.attribute('tabindex', '0');
});
});

it('skips the non-stringifiable items', async function test() {
Expand Down Expand Up @@ -333,6 +340,42 @@ describe('<Menu.Root />', () => {
});
expect(getByText('ąa')).to.have.attribute('tabindex', '0');
});

it('does not trigger the onClick event when Space is pressed during text navigation', async function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
// useMenuPopup Text navigation match menu items using HTMLElement.innerText
// innerText is not supported by JSDOM
this.skip();
}

const handleClick = spy();

const { getAllByRole } = await render(
<Menu.Root open animated={false}>
<Menu.Positioner>
<Menu.Popup>
<Menu.Item onClick={() => handleClick()}>Item One</Menu.Item>
<Menu.Item onClick={() => handleClick()}>Item Two</Menu.Item>
<Menu.Item onClick={() => handleClick()}>Item Three</Menu.Item>
</Menu.Popup>
</Menu.Positioner>
</Menu.Root>,
);

const items = getAllByRole('menuitem');

await act(() => {
items[0].focus();
});

await user.keyboard('Item T');

expect(handleClick.called).to.equal(false);

await waitFor(() => {
expect(items[1]).toHaveFocus();
});
});
});
});

Expand Down
7 changes: 7 additions & 0 deletions packages/mui-base/src/Menu/Root/MenuRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ function MenuRoot(props: MenuRoot.Props) {
const nested = parentContext != null;

const openOnHover = openOnHoverProp ?? nested;
const typingRef = React.useRef(false);

const onTypingChange = React.useCallback((nextTyping: boolean) => {
typingRef.current = nextTyping;
}, []);

const menuRoot = useMenuRoot({
animated,
Expand All @@ -39,6 +44,7 @@ function MenuRoot(props: MenuRoot.Props) {
nested,
openOnHover,
delay,
onTypingChange,
});

const [localClickAndDragEnabled, setLocalClickAndDragEnabled] = React.useState(false);
Expand All @@ -58,6 +64,7 @@ function MenuRoot(props: MenuRoot.Props) {
disabled,
clickAndDragEnabled,
setClickAndDragEnabled,
typingRef,
}),
[menuRoot, nested, parentContext, disabled, clickAndDragEnabled, setClickAndDragEnabled],
);
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/Menu/Root/MenuRootContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export interface MenuRootContext extends useMenuRoot.ReturnValue {
nested: boolean;
parentContext: MenuRootContext | null;
setClickAndDragEnabled: React.Dispatch<React.SetStateAction<boolean>>;
typingRef: React.RefObject<boolean>;
}

export const MenuRootContext = React.createContext<MenuRootContext | null>(null);
Expand Down
6 changes: 6 additions & 0 deletions packages/mui-base/src/Menu/Root/useMenuRoot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export function useMenuRoot(parameters: useMenuRoot.Parameters): useMenuRoot.Ret
loop,
delay,
openOnHover,
onTypingChange,
} = parameters;

const [triggerElement, setTriggerElement] = React.useState<HTMLElement | null>(null);
Expand Down Expand Up @@ -126,6 +127,7 @@ export function useMenuRoot(parameters: useMenuRoot.Parameters): useMenuRoot.Ret
setActiveIndex(index);
}
},
onTypingChange,
});

const { getReferenceProps, getFloatingProps, getItemProps } = useInteractions([
Expand Down Expand Up @@ -255,6 +257,10 @@ export namespace useMenuRoot {
* Whether the menu popup opens when the trigger is hovered after the provided `delay`.
*/
openOnHover: boolean;
/**
* Callback fired when the user begins or finishes typing (for typeahead search).
*/
onTypingChange: (typing: boolean) => void;
}

export interface ReturnValue {
Expand Down
11 changes: 9 additions & 2 deletions packages/mui-base/src/Menu/SubmenuTrigger/SubmenuTrigger.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ const SubmenuTrigger = React.forwardRef(function SubmenuTriggerComponent(
const { render, className, disabled = false, label, id: idProp, ...other } = props;
const id = useId(idProp);

const { getTriggerProps, parentContext, setTriggerElement, clickAndDragEnabled, open } =
useMenuRootContext();
const {
getTriggerProps,
parentContext,
setTriggerElement,
clickAndDragEnabled,
open,
typingRef,
} = useMenuRootContext();

if (parentContext === null) {
throw new Error('Base UI: ItemTrigger must be placed in a nested Menu.');
Expand All @@ -40,6 +46,7 @@ const SubmenuTrigger = React.forwardRef(function SubmenuTriggerComponent(
menuEvents,
setTriggerElement,
treatMouseupAsClick: clickAndDragEnabled,
typingRef,
});

const ownerState: SubmenuTrigger.OwnerState = { disabled, highlighted, open };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export function useSubmenuTrigger(
menuEvents,
setTriggerElement,
treatMouseupAsClick,
typingRef,
} = parameters;

const { getRootProps: getMenuItemProps, rootRef: menuItemRef } = useMenuItem({
Expand All @@ -31,6 +32,7 @@ export function useSubmenuTrigger(
menuEvents,
ref: externalRef,
treatMouseupAsClick,
typingRef,
});

const menuTriggerRef = useForkRef(menuItemRef, setTriggerElement);
Expand Down Expand Up @@ -81,6 +83,10 @@ export namespace useSubmenuTrigger {
* If `true`, the menu item will listen for mouseup events and treat them as clicks.
*/
treatMouseupAsClick: boolean;
/**
* A ref that is set to `true` when the user is using the typeahead feature.
*/
typingRef: React.RefObject<boolean>;
}

export interface ReturnValue {
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/Menu/Trigger/MenuTrigger.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const testRootContext: MenuRootContext = {
popupRef: { current: null },
mounted: true,
transitionStatus: undefined,
typingRef: { current: false },
};

describe('<Menu.Trigger />', () => {
Expand Down

0 comments on commit 7154397

Please sign in to comment.