Skip to content

Commit caff840

Browse files
fix: resolve bug where onAction is executed multiple times (#6671)
* fix: menu components to remove onAction prop duplication * refactor: remove unused prop and add tests for onAction in GridList and ListBox * test: remove unnecessary tests for react-spectrum menu test * simplify useMenuItem onAction logic * Remove case covered in unit tests --------- Co-authored-by: Robert Snow <[email protected]>
1 parent 76365df commit caff840

File tree

10 files changed

+112
-31
lines changed

10 files changed

+112
-31
lines changed

packages/@react-aria/menu/src/useMenuItem.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,10 @@ export function useMenuItem<T>(props: AriaMenuItemProps, state: TreeState<T>, re
136136
item.props.onAction();
137137
}
138138

139-
if (props.onAction) {
140-
props.onAction(key);
141-
} else if (data.onAction) {
142-
data.onAction(key);
139+
if (data.onAction) {
140+
// Must reassign to variable otherwise `this` binding gets messed up. Something to do with WeakMap.
141+
let onAction = data.onAction;
142+
onAction(key);
143143
}
144144

145145
if (e.target instanceof HTMLAnchorElement) {

packages/@react-aria/menu/stories/useMenu.stories.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,8 @@ function MenuPopup(props) {
127127
key={item.key}
128128
item={item}
129129
state={state}
130-
onAction={props.onAction}
131-
onClose={props.onClose} />
130+
onClose={props.onClose}
131+
onAction={props.onAction} />
132132
))}
133133
</ul>
134134
<DismissButton onDismiss={props.onClose} />

packages/@react-spectrum/menu/src/Menu.tsx

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,17 +98,15 @@ function Menu<T extends object>(props: SpectrumMenuProps<T>, ref: DOMRef<HTMLDiv
9898
<MenuSection
9999
key={item.key}
100100
item={item}
101-
state={state}
102-
onAction={completeProps.onAction} />
101+
state={state} />
103102
);
104103
}
105104

106105
let menuItem = (
107106
<MenuItem
108107
key={item.key}
109108
item={item}
110-
state={state}
111-
onAction={completeProps.onAction} />
109+
state={state} />
112110
);
113111

114112
if (item.wrapper) {

packages/@react-spectrum/menu/src/MenuItem.tsx

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import CheckmarkMedium from '@spectrum-icons/ui/CheckmarkMedium';
1414
import ChevronLeft from '@spectrum-icons/workflow/ChevronLeft';
1515
import ChevronRight from '@spectrum-icons/workflow/ChevronRight';
1616
import {classNames, ClearSlots, SlotProvider} from '@react-spectrum/utils';
17-
import {DOMAttributes, Key, Node} from '@react-types/shared';
17+
import {DOMAttributes, Node} from '@react-types/shared';
1818
import {FocusRing} from '@react-aria/focus';
1919
import {Grid} from '@react-spectrum/layout';
2020
import InfoOutline from '@spectrum-icons/workflow/InfoOutline';
@@ -32,17 +32,15 @@ import {useMenuItem} from '@react-aria/menu';
3232
interface MenuItemProps<T> {
3333
item: Node<T>,
3434
state: TreeState<T>,
35-
isVirtualized?: boolean,
36-
onAction?: (key: Key) => void
35+
isVirtualized?: boolean
3736
}
3837

3938
/** @private */
4039
export function MenuItem<T>(props: MenuItemProps<T>) {
4140
let {
4241
item,
4342
state,
44-
isVirtualized,
45-
onAction
43+
isVirtualized
4644
} = props;
4745
let {
4846
closeOnSelect
@@ -83,7 +81,6 @@ export function MenuItem<T>(props: MenuItemProps<T>) {
8381
key,
8482
closeOnSelect,
8583
isVirtualized,
86-
onAction,
8784
...submenuTriggerProps
8885
},
8986
state,

packages/@react-spectrum/menu/src/MenuSection.tsx

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212

1313
import {classNames} from '@react-spectrum/utils';
1414
import {getChildNodes} from '@react-stately/collections';
15-
import {Key, Node} from '@react-types/shared';
1615
import {MenuItem} from './MenuItem';
16+
import {Node} from '@react-types/shared';
1717
import React, {Fragment} from 'react';
1818
import styles from '@adobe/spectrum-css-temp/components/menu/vars.css';
1919
import {TreeState} from '@react-stately/tree';
@@ -22,13 +22,12 @@ import {useSeparator} from '@react-aria/separator';
2222

2323
interface MenuSectionProps<T> {
2424
item: Node<T>,
25-
state: TreeState<T>,
26-
onAction?: (key: Key) => void
25+
state: TreeState<T>
2726
}
2827

2928
/** @private */
3029
export function MenuSection<T>(props: MenuSectionProps<T>) {
31-
let {item, state, onAction} = props;
30+
let {item, state} = props;
3231
let {itemProps, headingProps, groupProps} = useMenuSection({
3332
heading: item.rendered,
3433
'aria-label': item['aria-label']
@@ -84,8 +83,7 @@ export function MenuSection<T>(props: MenuSectionProps<T>) {
8483
<MenuItem
8584
key={node.key}
8685
item={node}
87-
state={state}
88-
onAction={onAction} />
86+
state={state} />
8987
);
9088

9189
if (node.wrapper) {

packages/@react-spectrum/menu/test/Menu.test.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -684,6 +684,44 @@ describe('Menu', function () {
684684
expect(onAction).toHaveBeenCalledWith('Three');
685685
expect(onSelectionChange).toHaveBeenCalledTimes(0);
686686
});
687+
688+
it('should support onAction on menu and menu items', async () => {
689+
let user = userEvent.setup({delay: null, pointerMap});
690+
let onAction = jest.fn();
691+
let itemAction = jest.fn();
692+
let {getAllByRole} = render(
693+
<Menu aria-label="Test" onAction={onAction}>
694+
<Item id="cat" onAction={itemAction}>Cat</Item>
695+
<Item id="dog">Dog</Item>
696+
<Item id="kangaroo">Kangaroo</Item>
697+
</Menu>
698+
);
699+
700+
let items = getAllByRole('menuitem');
701+
await user.click(items[0]);
702+
expect(onAction).toHaveBeenCalledTimes(1);
703+
expect(itemAction).toHaveBeenCalledTimes(1);
704+
});
705+
706+
it('should support onAction on menu and menu items in sections', async () => {
707+
let user = userEvent.setup({delay: null, pointerMap});
708+
let onAction = jest.fn();
709+
let itemAction = jest.fn();
710+
let {getAllByRole} = render(
711+
<Menu aria-label="Test" onAction={onAction}>
712+
<Section title="Animals">
713+
<Item id="cat" onAction={itemAction}>Cat</Item>
714+
<Item id="dog">Dog</Item>
715+
<Item id="kangaroo">Kangaroo</Item>
716+
</Section>
717+
</Menu>
718+
);
719+
720+
let items = getAllByRole('menuitem');
721+
await user.click(items[0]);
722+
expect(onAction).toHaveBeenCalledTimes(1);
723+
expect(itemAction).toHaveBeenCalledTimes(1);
724+
});
687725
});
688726

689727
it('supports complex menu items with aria-labelledby and aria-describedby', function () {

packages/react-aria-components/src/Menu.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ export const MenuItem = /*#__PURE__*/ createLeafComponent('item', function MenuI
310310
let id = useSlottedContext(MenuItemContext)?.id as string;
311311
let state = useContext(MenuStateContext)!;
312312
let ref = useObjectRef<any>(forwardedRef);
313+
313314
let {menuItemProps, labelProps, descriptionProps, keyboardShortcutProps, ...states} = useMenuItem({...props, id, key: item.key}, state, ref);
314315

315316
let {isFocusVisible, focusProps} = useFocusRing();

packages/react-aria-components/test/GridList.test.js

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,23 @@ describe('GridList', () => {
268268
);
269269
let items = getAllByRole('row');
270270
await user.click(items[0]);
271-
expect(onAction).toHaveBeenCalled();
271+
expect(onAction).toHaveBeenCalledTimes(1);
272+
});
273+
274+
it('should support onAction on list and list items', async () => {
275+
let onAction = jest.fn();
276+
let itemAction = jest.fn();
277+
let {getAllByRole} = render(
278+
<GridList aria-label="Test" onAction={onAction}>
279+
<GridListItem id="cat" onAction={itemAction}>Cat</GridListItem>
280+
<GridListItem id="dog">Dog</GridListItem>
281+
<GridListItem id="kangaroo">Kangaroo</GridListItem>
282+
</GridList>
283+
);
284+
let items = getAllByRole('row');
285+
await user.click(items[0]);
286+
expect(onAction).toHaveBeenCalledWith('cat');
287+
expect(itemAction).toHaveBeenCalledTimes(1);
272288
});
273289

274290
it('should support empty state', () => {
@@ -343,7 +359,7 @@ describe('GridList', () => {
343359

344360
await user.click(items[0]);
345361
await user.click(items[1]);
346-
362+
347363
expect(items[0]).toHaveAttribute('aria-selected', 'false');
348364
expect(items[1]).toHaveAttribute('aria-selected', 'true');
349365
expect(items[2]).toHaveAttribute('aria-selected', 'false');
@@ -386,7 +402,7 @@ describe('GridList', () => {
386402
let grid = getByRole('grid');
387403
grid.scrollTop = 200;
388404
fireEvent.scroll(grid);
389-
405+
390406
rows = getAllByRole('row');
391407
expect(rows).toHaveLength(8);
392408
expect(rows.map(r => r.textContent)).toEqual(['Item 7', 'Item 8', 'Item 9', 'Item 10', 'Item 11', 'Item 12', 'Item 13', 'Item 14']);

packages/react-aria-components/test/ListBox.test.js

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ import {
1818
Header, Heading,
1919
ListBox,
2020
ListBoxContext,
21-
ListBoxItem,
22-
UNSTABLE_ListLayout as ListLayout,
21+
ListBoxItem,
22+
UNSTABLE_ListLayout as ListLayout,
2323
Modal,
2424
Section,
2525
Text,
@@ -479,7 +479,23 @@ describe('ListBox', () => {
479479
);
480480
let items = getAllByRole('option');
481481
await user.click(items[0]);
482-
expect(onAction).toHaveBeenCalled();
482+
expect(onAction).toHaveBeenCalledTimes(1);
483+
});
484+
485+
it('should support onAction on list ans list items', async () => {
486+
let onAction = jest.fn();
487+
let itemAction = jest.fn();
488+
let {getAllByRole} = render(
489+
<ListBox aria-label="Test" onAction={onAction}>
490+
<ListBoxItem id="cat" onAction={itemAction}>Cat</ListBoxItem>
491+
<ListBoxItem id="dog">Dog</ListBoxItem>
492+
<ListBoxItem id="kangaroo">Kangaroo</ListBoxItem>
493+
</ListBox>
494+
);
495+
let items = getAllByRole('option');
496+
await user.click(items[0]);
497+
expect(onAction).toHaveBeenCalledWith('cat');
498+
expect(itemAction).toHaveBeenCalledTimes(1);
483499
});
484500

485501
it('should support empty state', () => {
@@ -725,11 +741,11 @@ describe('ListBox', () => {
725741
let listbox = getByRole('listbox');
726742
listbox.scrollTop = 200;
727743
fireEvent.scroll(listbox);
728-
744+
729745
options = getAllByRole('option');
730746
expect(options).toHaveLength(8);
731747
expect(options.map(r => r.textContent)).toEqual(['Item 7', 'Item 8', 'Item 9', 'Item 10', 'Item 11', 'Item 12', 'Item 13', 'Item 14']);
732-
748+
733749
await user.tab();
734750
await user.keyboard('{End}');
735751

packages/react-aria-components/test/Menu.test.js

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,24 @@ describe('Menu', () => {
370370
);
371371
let items = getAllByRole('menuitem');
372372
await user.click(items[0]);
373-
expect(onAction).toHaveBeenCalled();
373+
expect(onAction).toHaveBeenCalledTimes(1);
374+
});
375+
376+
it('should support onAction on menu and menu items', async () => {
377+
let onAction = jest.fn();
378+
let itemAction = jest.fn();
379+
let {getAllByRole} = render(
380+
<Menu aria-label="Test" onAction={onAction}>
381+
<MenuItem id="cat" onAction={itemAction}>Cat</MenuItem>
382+
<MenuItem id="dog">Dog</MenuItem>
383+
<MenuItem id="kangaroo">Kangaroo</MenuItem>
384+
</Menu>
385+
);
386+
387+
let items = getAllByRole('menuitem');
388+
await user.click(items[0]);
389+
expect(onAction).toHaveBeenCalledTimes(1);
390+
expect(itemAction).toHaveBeenCalledTimes(1);
374391
});
375392

376393
it('should support menu trigger', async () => {

0 commit comments

Comments
 (0)