Skip to content

Commit 3ea36ec

Browse files
atomiksatomiks
committed
[accordion] Fix trigger behavior bugs
1 parent bbff4e3 commit 3ea36ec

6 files changed

Lines changed: 248 additions & 25 deletions

File tree

‎packages/react/src/accordion/item/AccordionItem.tsx‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ export const AccordionItem = React.forwardRef(function AccordionItem(
7777
return;
7878
}
7979

80-
handleValueChange(value, nextOpen);
80+
handleValueChange(value, nextOpen, eventDetails);
8181
},
8282
);
8383

@@ -117,16 +117,16 @@ export const AccordionItem = React.forwardRef(function AccordionItem(
117117
);
118118

119119
const defaultTriggerId = useBaseUiId();
120-
const [triggerId, setTriggerId] = React.useState<string | undefined>(defaultTriggerId);
120+
const [triggerId, setTriggerId] = React.useState<string | undefined>();
121121

122122
const accordionItemContext: AccordionItemContext = React.useMemo(
123123
() => ({
124124
open: isOpen,
125125
state,
126126
setTriggerId,
127-
triggerId,
127+
triggerId: triggerId ?? defaultTriggerId,
128128
}),
129-
[isOpen, state, setTriggerId, triggerId],
129+
[defaultTriggerId, isOpen, state, setTriggerId, triggerId],
130130
);
131131

132132
const element = useRenderElement('div', componentProps, {

‎packages/react/src/accordion/panel/AccordionPanel.test.tsx‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,47 @@ describe('<Accordion.Panel />', () => {
7171
});
7272
});
7373

74+
describe('root props', () => {
75+
it('passes keepMounted to closed panels', async () => {
76+
await render(
77+
<Accordion.Root keepMounted>
78+
<Accordion.Item value={0}>
79+
<Accordion.Header>
80+
<Accordion.Trigger>Trigger</Accordion.Trigger>
81+
</Accordion.Header>
82+
<Accordion.Panel>{PANEL_CONTENT}</Accordion.Panel>
83+
</Accordion.Item>
84+
</Accordion.Root>,
85+
);
86+
87+
expect(screen.getByText(PANEL_CONTENT)).toHaveAttribute('hidden');
88+
});
89+
90+
it('passes hiddenUntilFound to closed panels and allows panel overrides', async () => {
91+
await render(
92+
<Accordion.Root hiddenUntilFound keepMounted>
93+
<Accordion.Item value={0}>
94+
<Accordion.Header>
95+
<Accordion.Trigger>Trigger 1</Accordion.Trigger>
96+
</Accordion.Header>
97+
<Accordion.Panel>{PANEL_CONTENT}</Accordion.Panel>
98+
</Accordion.Item>
99+
<Accordion.Item value={1}>
100+
<Accordion.Header>
101+
<Accordion.Trigger>Trigger 2</Accordion.Trigger>
102+
</Accordion.Header>
103+
<Accordion.Panel hiddenUntilFound={false} keepMounted={false}>
104+
Overridden panel
105+
</Accordion.Panel>
106+
</Accordion.Item>
107+
</Accordion.Root>,
108+
);
109+
110+
expect(screen.getByText(PANEL_CONTENT).getAttribute('hidden')).toBe('until-found');
111+
expect(screen.queryByText('Overridden panel')).toBe(null);
112+
});
113+
});
114+
74115
describe.skipIf(isJSDOM)('CSS transitions', () => {
75116
it('keeps the closing panel visible until its exit transition completes when switching items', async () => {
76117
const { user } = await render(

‎packages/react/src/accordion/root/AccordionRoot.test.tsx‎

Lines changed: 167 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import { expect, vi } from 'vitest';
2-
import { fireEvent, screen } from '@mui/internal-test-utils';
2+
import * as React from 'react';
3+
import { fireEvent, screen, waitFor } from '@mui/internal-test-utils';
34
import { DirectionProvider } from '@base-ui/react/direction-provider';
45
import { Accordion } from '@base-ui/react/accordion';
56
import { createRenderer, describeConformance, isJSDOM } from '#test-utils';
7+
import { REASONS } from '../../internals/reasons';
68

79
const PANEL_CONTENT_1 = 'Panel contents 1';
810
const PANEL_CONTENT_2 = 'Panel contents 2';
@@ -57,6 +59,43 @@ describe('<Accordion.Root />', () => {
5759
expect(trigger).toHaveAttribute('aria-controls', 'custom-panel-id');
5860
expect(panel).toHaveAttribute('id', 'custom-panel-id');
5961
});
62+
63+
it('restores panel labeling when a manual trigger id is removed', async () => {
64+
function App() {
65+
const [triggerId, setTriggerId] = React.useState<string | undefined>('custom-trigger-id');
66+
67+
return (
68+
<React.Fragment>
69+
<button type="button" onClick={() => setTriggerId(undefined)}>
70+
Remove id
71+
</button>
72+
<Accordion.Root defaultValue={[0]}>
73+
<Accordion.Item value={0}>
74+
<Accordion.Header>
75+
<Accordion.Trigger id={triggerId}>Trigger 1</Accordion.Trigger>
76+
</Accordion.Header>
77+
<Accordion.Panel>{PANEL_CONTENT_1}</Accordion.Panel>
78+
</Accordion.Item>
79+
</Accordion.Root>
80+
</React.Fragment>
81+
);
82+
}
83+
84+
const { user } = await render(<App />);
85+
86+
const trigger = screen.getByRole('button', { name: 'Trigger 1' });
87+
const panel = screen.getByText(PANEL_CONTENT_1);
88+
89+
expect(panel).toHaveAttribute('aria-labelledby', 'custom-trigger-id');
90+
91+
await user.click(screen.getByRole('button', { name: 'Remove id' }));
92+
93+
await waitFor(() => {
94+
expect(trigger).toHaveAttribute('id');
95+
expect(trigger).not.toHaveAttribute('id', 'custom-trigger-id');
96+
expect(panel).toHaveAttribute('aria-labelledby', trigger.id);
97+
});
98+
});
6099
});
61100

62101
describe('uncontrolled', () => {
@@ -240,6 +279,40 @@ describe('<Accordion.Root />', () => {
240279
expect(element).not.toHaveAttribute('data-disabled');
241280
});
242281
});
282+
283+
it.each(['root', 'item'] as const)(
284+
'does not toggle or fire callbacks when the %s is disabled',
285+
async (disabledPart) => {
286+
const onValueChange = vi.fn();
287+
const onOpenChange = vi.fn();
288+
289+
await render(
290+
<Accordion.Root disabled={disabledPart === 'root'} onValueChange={onValueChange}>
291+
<Accordion.Item
292+
value={0}
293+
disabled={disabledPart === 'item'}
294+
onOpenChange={onOpenChange}
295+
>
296+
<Accordion.Header>
297+
<Accordion.Trigger>Trigger 1</Accordion.Trigger>
298+
</Accordion.Header>
299+
<Accordion.Panel>{PANEL_CONTENT_1}</Accordion.Panel>
300+
</Accordion.Item>
301+
</Accordion.Root>,
302+
);
303+
304+
const trigger = screen.getByRole('button');
305+
306+
fireEvent.click(trigger);
307+
trigger.focus();
308+
fireEvent.keyDown(trigger, { key: ' ' });
309+
310+
expect(trigger).toHaveAttribute('aria-expanded', 'false');
311+
expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null);
312+
expect(onValueChange.mock.calls.length).toBe(0);
313+
expect(onOpenChange.mock.calls.length).toBe(0);
314+
},
315+
);
243316
});
244317

245318
it('allows onMouseUp to call preventBaseUIHandler on the trigger', async () => {
@@ -387,6 +460,41 @@ describe('<Accordion.Root />', () => {
387460
expect(trigger1).toHaveFocus();
388461
});
389462

463+
it('Arrow keys should only put focus on accordion triggers', async () => {
464+
const { user } = await render(
465+
<Accordion.Root>
466+
<Accordion.Item>
467+
<button type="button">Nested button</button>
468+
<Accordion.Header>
469+
<Accordion.Trigger
470+
nativeButton={isNativeButton}
471+
render={isNativeButton ? undefined : <span />}
472+
>
473+
Trigger 1
474+
</Accordion.Trigger>
475+
</Accordion.Header>
476+
<Accordion.Panel>1</Accordion.Panel>
477+
</Accordion.Item>
478+
<Accordion.Item>
479+
<Accordion.Header>
480+
<Accordion.Trigger>Trigger 2</Accordion.Trigger>
481+
</Accordion.Header>
482+
<Accordion.Panel>2</Accordion.Panel>
483+
</Accordion.Item>
484+
</Accordion.Root>,
485+
);
486+
487+
const trigger1 = screen.getByRole('button', { name: 'Trigger 1' });
488+
const trigger2 = screen.getByRole('button', { name: 'Trigger 2' });
489+
const nestedButton = screen.getByRole('button', { name: 'Nested button' });
490+
491+
trigger1.focus();
492+
await user.keyboard('[ArrowDown]');
493+
494+
expect(trigger2).toHaveFocus();
495+
expect(nestedButton).not.toHaveFocus();
496+
});
497+
390498
describe('key: End/Home', () => {
391499
it('End key moves focus to the last trigger', async () => {
392500
const { user } = await render(
@@ -598,6 +706,63 @@ describe('<Accordion.Root />', () => {
598706
});
599707
});
600708

709+
describe('BaseUIChangeEventDetails', () => {
710+
it('onOpenChange cancel() prevents opening while uncontrolled', async () => {
711+
const onValueChange = vi.fn();
712+
713+
await render(
714+
<Accordion.Root onValueChange={onValueChange}>
715+
<Accordion.Item
716+
value={0}
717+
onOpenChange={(nextOpen, eventDetails) => {
718+
if (nextOpen) {
719+
eventDetails.cancel();
720+
}
721+
}}
722+
>
723+
<Accordion.Header>
724+
<Accordion.Trigger>Trigger 1</Accordion.Trigger>
725+
</Accordion.Header>
726+
<Accordion.Panel>{PANEL_CONTENT_1}</Accordion.Panel>
727+
</Accordion.Item>
728+
</Accordion.Root>,
729+
);
730+
731+
const trigger = screen.getByRole('button');
732+
733+
fireEvent.click(trigger);
734+
735+
expect(trigger).toHaveAttribute('aria-expanded', 'false');
736+
expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null);
737+
expect(onValueChange.mock.calls.length).toBe(0);
738+
});
739+
740+
it('onValueChange cancel() prevents opening while uncontrolled', async () => {
741+
const onValueChange = vi.fn((_value, eventDetails) => {
742+
eventDetails.cancel();
743+
});
744+
745+
await render(
746+
<Accordion.Root onValueChange={onValueChange}>
747+
<Accordion.Item value={0}>
748+
<Accordion.Header>
749+
<Accordion.Trigger>Trigger 1</Accordion.Trigger>
750+
</Accordion.Header>
751+
<Accordion.Panel>{PANEL_CONTENT_1}</Accordion.Panel>
752+
</Accordion.Item>
753+
</Accordion.Root>,
754+
);
755+
756+
const trigger = screen.getByRole('button');
757+
758+
fireEvent.click(trigger);
759+
760+
expect(trigger).toHaveAttribute('aria-expanded', 'false');
761+
expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null);
762+
expect(onValueChange.mock.calls.length).toBe(1);
763+
});
764+
});
765+
601766
describe.skipIf(isJSDOM)('prop: multiple', () => {
602767
it('multiple items can be open when `multiple = true`', async () => {
603768
const { user } = await render(
@@ -779,6 +944,7 @@ describe('<Accordion.Root />', () => {
779944

780945
expect(onValueChange.mock.calls.length).toBe(1);
781946
expect(onValueChange.mock.lastCall?.[0]).toEqual([0]);
947+
expect(onValueChange.mock.lastCall?.[1].reason).toBe(REASONS.triggerPress);
782948

783949
await user.pointer({ keys: '[MouseLeft]', target: trigger2 });
784950

‎packages/react/src/accordion/root/AccordionRoot.tsx‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,7 @@ import { CompositeList } from '../../internals/composite/list/CompositeList';
99
import { useDirection } from '../../internals/direction-context/DirectionContext';
1010
import { AccordionRootContext } from './AccordionRootContext';
1111
import { useRenderElement } from '../../internals/useRenderElement';
12-
import {
13-
createChangeEventDetails,
14-
type BaseUIChangeEventDetails,
15-
} from '../../internals/createBaseUIEventDetails';
12+
import { type BaseUIChangeEventDetails } from '../../internals/createBaseUIEventDetails';
1613
import { REASONS } from '../../internals/reasons';
1714

1815
const rootStateAttributesMapping = {
@@ -69,6 +66,7 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot<Value = any
6966
}, [valueProp, defaultValueProp]);
7067

7168
const accordionItemRefs = React.useRef<(HTMLElement | null)[]>([]);
69+
const accordionTriggerRefs = React.useRef<(HTMLElement | null)[]>([]);
7270

7371
const [value, setValue] = useControlled({
7472
controlled: valueProp,
@@ -78,8 +76,11 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot<Value = any
7876
});
7977

8078
const handleValueChange = useStableCallback(
81-
(newValue: AccordionRoot.Value<Value>[number], nextOpen: boolean) => {
82-
const details = createChangeEventDetails(REASONS.none);
79+
(
80+
newValue: AccordionRoot.Value<Value>[number],
81+
nextOpen: boolean,
82+
details: AccordionRoot.ChangeEventDetails,
83+
) => {
8384
if (!multiple) {
8485
const nextValue = value[0] === newValue ? [] : [newValue];
8586
onValueChange?.(nextValue, details);
@@ -118,6 +119,7 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot<Value = any
118119
const contextValue: AccordionRootContext<Value> = React.useMemo(
119120
() => ({
120121
accordionItemRefs,
122+
accordionTriggerRefs,
121123
direction,
122124
disabled,
123125
handleValueChange,

‎packages/react/src/accordion/root/AccordionRootContext.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,14 @@ import type { AccordionRoot } from './AccordionRoot';
66

77
export interface AccordionRootContext<Value = any> {
88
accordionItemRefs: React.RefObject<(HTMLElement | null)[]>;
9+
accordionTriggerRefs: React.RefObject<(HTMLElement | null)[]>;
910
direction: TextDirection;
1011
disabled: boolean;
11-
handleValueChange: (newValue: AccordionRoot.Value<Value>[number], nextOpen: boolean) => void;
12+
handleValueChange: (
13+
newValue: AccordionRoot.Value<Value>[number],
14+
nextOpen: boolean,
15+
eventDetails: AccordionRoot.ChangeEventDetails,
16+
) => void;
1217
hiddenUntilFound: boolean;
1318
keepMounted: boolean;
1419
loopFocus: boolean;

0 commit comments

Comments
 (0)