From b1462a7de5c83fd5252f4c46aa9848419930117e Mon Sep 17 00:00:00 2001 From: atomiks Date: Mon, 18 May 2026 13:42:18 +1000 Subject: [PATCH] [accordion] Fix trigger behavior bugs --- .../src/accordion/item/AccordionItem.tsx | 8 +- .../accordion/panel/AccordionPanel.test.tsx | 39 ++ .../src/accordion/root/AccordionRoot.test.tsx | 533 +++++++++++++++++- .../src/accordion/root/AccordionRoot.tsx | 15 +- .../accordion/root/AccordionRootContext.ts | 7 +- .../accordion/trigger/AccordionTrigger.tsx | 39 +- 6 files changed, 613 insertions(+), 28 deletions(-) diff --git a/packages/react/src/accordion/item/AccordionItem.tsx b/packages/react/src/accordion/item/AccordionItem.tsx index 3730131eb89..c70f1399711 100644 --- a/packages/react/src/accordion/item/AccordionItem.tsx +++ b/packages/react/src/accordion/item/AccordionItem.tsx @@ -77,7 +77,7 @@ export const AccordionItem = React.forwardRef(function AccordionItem( return; } - handleValueChange(value, nextOpen); + handleValueChange(value, nextOpen, eventDetails); }, ); @@ -117,16 +117,16 @@ export const AccordionItem = React.forwardRef(function AccordionItem( ); const defaultTriggerId = useBaseUiId(); - const [triggerId, setTriggerId] = React.useState(defaultTriggerId); + const [triggerId, setTriggerId] = React.useState(); const accordionItemContext: AccordionItemContext = React.useMemo( () => ({ open: isOpen, state, setTriggerId, - triggerId, + triggerId: triggerId ?? defaultTriggerId, }), - [isOpen, state, setTriggerId, triggerId], + [defaultTriggerId, isOpen, state, setTriggerId, triggerId], ); const element = useRenderElement('div', componentProps, { diff --git a/packages/react/src/accordion/panel/AccordionPanel.test.tsx b/packages/react/src/accordion/panel/AccordionPanel.test.tsx index 09422a0a87b..c8ce7e2b708 100644 --- a/packages/react/src/accordion/panel/AccordionPanel.test.tsx +++ b/packages/react/src/accordion/panel/AccordionPanel.test.tsx @@ -71,6 +71,45 @@ describe('', () => { }); }); + it('passes root keepMounted to closed panels', async () => { + await render( + + + + Trigger + + {PANEL_CONTENT} + + , + ); + + expect(screen.getByText(PANEL_CONTENT)).toHaveAttribute('hidden'); + }); + + it('passes root hiddenUntilFound to closed panels and allows panel overrides', async () => { + await render( + + + + Trigger 1 + + {PANEL_CONTENT} + + + + Trigger 2 + + + Overridden panel + + + , + ); + + expect(screen.getByText(PANEL_CONTENT).getAttribute('hidden')).toBe('until-found'); + expect(screen.queryByText('Overridden panel')).toBe(null); + }); + describe.skipIf(isJSDOM)('CSS transitions', () => { it('keeps the closing panel visible until its exit transition completes when switching items', async () => { const { user } = await render( diff --git a/packages/react/src/accordion/root/AccordionRoot.test.tsx b/packages/react/src/accordion/root/AccordionRoot.test.tsx index 3b8b5f2de27..bacee285e4f 100644 --- a/packages/react/src/accordion/root/AccordionRoot.test.tsx +++ b/packages/react/src/accordion/root/AccordionRoot.test.tsx @@ -1,8 +1,10 @@ import { expect, vi } from 'vitest'; -import { fireEvent, screen } from '@mui/internal-test-utils'; +import * as React from 'react'; +import { fireEvent, screen, waitFor } from '@mui/internal-test-utils'; import { DirectionProvider } from '@base-ui/react/direction-provider'; import { Accordion } from '@base-ui/react/accordion'; import { createRenderer, describeConformance, isJSDOM } from '#test-utils'; +import { REASONS } from '../../internals/reasons'; const PANEL_CONTENT_1 = 'Panel contents 1'; const PANEL_CONTENT_2 = 'Panel contents 2'; @@ -57,6 +59,107 @@ describe('', () => { expect(trigger).toHaveAttribute('aria-controls', 'custom-panel-id'); expect(panel).toHaveAttribute('id', 'custom-panel-id'); }); + + it('references manual trigger id in panel aria-labelledby', async () => { + await render( + + + + Trigger 1 + + {PANEL_CONTENT_1} + + , + ); + + const panel = screen.getByText(PANEL_CONTENT_1); + + expect(panel).toHaveAttribute('aria-labelledby', 'custom-trigger-id'); + }); + + it('updates panel labeling when a manual trigger id is added or changed', async () => { + function App() { + const [triggerId, setTriggerId] = React.useState(); + + return ( + + + + + + + Trigger 1 + + {PANEL_CONTENT_1} + + + + ); + } + + const { user } = await render(); + + const trigger = screen.getByRole('button', { name: 'Trigger 1' }); + const panel = screen.getByText(PANEL_CONTENT_1); + + expect(trigger).toHaveAttribute('id'); + expect(panel).toHaveAttribute('aria-labelledby', trigger.id); + + await user.click(screen.getByRole('button', { name: 'Set id 1' })); + + await waitFor(() => { + expect(trigger).toHaveAttribute('id', 'custom-trigger-id-1'); + expect(panel).toHaveAttribute('aria-labelledby', 'custom-trigger-id-1'); + }); + + await user.click(screen.getByRole('button', { name: 'Set id 2' })); + + await waitFor(() => { + expect(trigger).toHaveAttribute('id', 'custom-trigger-id-2'); + expect(panel).toHaveAttribute('aria-labelledby', 'custom-trigger-id-2'); + }); + }); + + it('restores panel labeling when a manual trigger id is removed', async () => { + function App() { + const [triggerId, setTriggerId] = React.useState('custom-trigger-id'); + + return ( + + + + + + Trigger 1 + + {PANEL_CONTENT_1} + + + + ); + } + + const { user } = await render(); + + const trigger = screen.getByRole('button', { name: 'Trigger 1' }); + const panel = screen.getByText(PANEL_CONTENT_1); + + expect(panel).toHaveAttribute('aria-labelledby', 'custom-trigger-id'); + + await user.click(screen.getByRole('button', { name: 'Remove id' })); + + await waitFor(() => { + expect(trigger).toHaveAttribute('id'); + expect(trigger).not.toHaveAttribute('id', 'custom-trigger-id'); + expect(panel).toHaveAttribute('aria-labelledby', trigger.id); + }); + }); }); describe('uncontrolled', () => { @@ -240,6 +343,57 @@ describe('', () => { expect(element).not.toHaveAttribute('data-disabled'); }); }); + + it.each(['root', 'item'] as const)( + 'does not toggle or fire callbacks when the %s is disabled', + async (disabledPart) => { + const onValueChange = vi.fn(); + const onOpenChange = vi.fn(); + + const { user } = await render( + + + + Trigger 1 + + {PANEL_CONTENT_1} + + + + Trigger 2 + + {PANEL_CONTENT_2} + + , + ); + + const [trigger1, trigger2] = screen.getAllByRole('button'); + + await user.pointer({ keys: '[MouseLeft]', target: trigger1 }); + trigger1.focus(); + await user.keyboard('[Space]'); + await user.keyboard('[Enter]'); + + expect(trigger1).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null); + expect(onValueChange.mock.calls.length).toBe(0); + expect(onOpenChange.mock.calls.length).toBe(0); + + if (disabledPart === 'root') { + trigger1.focus(); + await user.keyboard('[ArrowDown]'); + expect(trigger1).toHaveFocus(); + } else { + trigger2.focus(); + await user.keyboard('[ArrowUp]'); + expect(trigger2).toHaveFocus(); + } + }, + ); }); it('allows onMouseUp to call preventBaseUIHandler on the trigger', async () => { @@ -387,6 +541,131 @@ describe('', () => { expect(trigger1).toHaveFocus(); }); + it('navigation keys should only put focus on accordion triggers', async () => { + const { user } = await render( + + + + + } + > + Trigger 1 + + + 1 + + + + Trigger 2 + + 2 + + , + ); + + const trigger1 = screen.getByRole('button', { name: 'Trigger 1' }); + const trigger2 = screen.getByRole('button', { name: 'Trigger 2' }); + const nestedButton = screen.getByRole('button', { name: 'Nested button' }); + + trigger1.focus(); + await user.keyboard('[ArrowDown]'); + + expect(trigger2).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + + await user.keyboard('[Home]'); + expect(trigger1).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + + await user.keyboard('[End]'); + expect(trigger2).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + + await user.keyboard('[ArrowUp]'); + expect(trigger1).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + }); + + it('keeps trigger navigation correct when items are added and removed', async () => { + function App() { + const [showFirst, setShowFirst] = React.useState(true); + const [showMiddle, setShowMiddle] = React.useState(false); + + return ( + + + + + {showFirst && ( + + + Trigger 1 + + 1 + + )} + + + } + > + Trigger 2 + + + 2 + + {showMiddle && ( + + + Trigger middle + + Middle + + )} + + + Trigger 3 + + 3 + + + + ); + } + + const { user } = await render(); + + await user.click(screen.getByRole('button', { name: 'Remove first' })); + + const trigger2 = screen.getByRole('button', { name: 'Trigger 2' }); + const trigger3 = screen.getByRole('button', { name: 'Trigger 3' }); + + trigger2.focus(); + await user.keyboard('[ArrowDown]'); + expect(trigger3).toHaveFocus(); + + await user.keyboard('[ArrowUp]'); + expect(trigger2).toHaveFocus(); + + await user.click(screen.getByRole('button', { name: 'Add middle' })); + + const middleTrigger = screen.getByRole('button', { name: 'Trigger middle' }); + + trigger2.focus(); + await user.keyboard('[ArrowDown]'); + expect(middleTrigger).toHaveFocus(); + + await user.keyboard('[ArrowDown]'); + expect(trigger3).toHaveFocus(); + }); + describe('key: End/Home', () => { it('End key moves focus to the last trigger', async () => { const { user } = await render( @@ -598,6 +877,181 @@ describe('', () => { }); }); + describe('BaseUIChangeEventDetails', () => { + it('onOpenChange cancel() prevents opening while uncontrolled', async () => { + const onValueChange = vi.fn(); + + await render( + + { + if (nextOpen) { + eventDetails.cancel(); + } + }} + > + + Trigger 1 + + {PANEL_CONTENT_1} + + , + ); + + const trigger = screen.getByRole('button'); + + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null); + expect(onValueChange.mock.calls.length).toBe(0); + }); + + it('onValueChange cancel() prevents opening while uncontrolled', async () => { + const onValueChange = vi.fn((_value, eventDetails) => { + eventDetails.cancel(); + }); + + await render( + + + + Trigger 1 + + {PANEL_CONTENT_1} + + , + ); + + const trigger = screen.getByRole('button'); + + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null); + expect(onValueChange.mock.calls.length).toBe(1); + }); + + it('onOpenChange cancel() prevents onValueChange while controlled', async () => { + const onValueChange = vi.fn(); + + await render( + + { + if (nextOpen) { + eventDetails.cancel(); + } + }} + > + + Trigger 1 + + {PANEL_CONTENT_1} + + , + ); + + const trigger = screen.getByRole('button'); + + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null); + expect(onValueChange.mock.calls.length).toBe(0); + }); + + it('onValueChange cancel() prevents opening while controlled', async () => { + const onValueChange = vi.fn(); + + function App() { + const [value, setValue] = React.useState([]); + + return ( + { + onValueChange(nextValue, eventDetails); + eventDetails.cancel(); + if (!eventDetails.isCanceled) { + setValue(nextValue); + } + }} + > + + + Trigger 1 + + {PANEL_CONTENT_1} + + + ); + } + + await render(); + + const trigger = screen.getByRole('button'); + + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null); + expect(onValueChange.mock.calls.length).toBe(1); + }); + + it('onValueChange cancel() prevents opening while multiple', async () => { + const onValueChange = vi.fn((_value, eventDetails) => { + eventDetails.cancel(); + }); + + await render( + + + + Trigger 1 + + {PANEL_CONTENT_1} + + , + ); + + const trigger = screen.getByRole('button'); + + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(PANEL_CONTENT_1)).toBe(null); + expect(onValueChange.mock.calls.length).toBe(1); + }); + + it('onValueChange cancel() prevents closing while multiple', async () => { + const onValueChange = vi.fn((_value, eventDetails) => { + eventDetails.cancel(); + }); + + await render( + + + + Trigger 1 + + {PANEL_CONTENT_1} + + , + ); + + const trigger = screen.getByRole('button'); + + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + expect(screen.queryByText(PANEL_CONTENT_1)).not.toBe(null); + expect(onValueChange.mock.calls.length).toBe(1); + }); + }); + describe.skipIf(isJSDOM)('prop: multiple', () => { it('multiple items can be open when `multiple = true`', async () => { const { user } = await render( @@ -709,6 +1163,40 @@ describe('', () => { expect(trigger1).toHaveFocus(); }); + it('navigation keys should only put focus on accordion triggers', async () => { + const { user } = await render( + + + + + Trigger 1 + + 1 + + + + Trigger 2 + + 2 + + , + ); + + const trigger1 = screen.getByRole('button', { name: 'Trigger 1' }); + const trigger2 = screen.getByRole('button', { name: 'Trigger 2' }); + const nestedButton = screen.getByRole('button', { name: 'Nested button' }); + + trigger1.focus(); + await user.keyboard('[ArrowRight]'); + + expect(trigger2).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + + await user.keyboard('[ArrowLeft]'); + expect(trigger1).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + }); + describe.skipIf(isJSDOM)('RTL', () => { it('ArrowLeft/Right is reversed for horizontal accordions in RTL mode', async () => { const { user } = await render( @@ -747,6 +1235,42 @@ describe('', () => { await user.keyboard('[ArrowLeft]'); expect(trigger1).toHaveFocus(); }); + + it('navigation keys should only put focus on accordion triggers', async () => { + const { user } = await render( + + + + + + Trigger 1 + + 1 + + + + Trigger 2 + + 2 + + + , + ); + + const trigger1 = screen.getByRole('button', { name: 'Trigger 1' }); + const trigger2 = screen.getByRole('button', { name: 'Trigger 2' }); + const nestedButton = screen.getByRole('button', { name: 'Nested button' }); + + trigger1.focus(); + await user.keyboard('[ArrowLeft]'); + + expect(trigger2).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + + await user.keyboard('[ArrowRight]'); + expect(trigger1).toHaveFocus(); + expect(nestedButton).not.toHaveFocus(); + }); }); }); @@ -779,11 +1303,16 @@ describe('', () => { expect(onValueChange.mock.calls.length).toBe(1); expect(onValueChange.mock.lastCall?.[0]).toEqual([0]); + expect(onValueChange.mock.lastCall?.[1].reason).toBe(REASONS.triggerPress); + expect(onValueChange.mock.lastCall?.[1].event.type).not.toBe('base-ui'); - await user.pointer({ keys: '[MouseLeft]', target: trigger2 }); + trigger2.focus(); + await user.keyboard('[Space]'); expect(onValueChange.mock.calls.length).toBe(2); expect(onValueChange.mock.lastCall?.[0]).toEqual([0, 1]); + expect(onValueChange.mock.lastCall?.[1].reason).toBe(REASONS.triggerPress); + expect(onValueChange.mock.lastCall?.[1].event.type).not.toBe('base-ui'); }); it('custom item value', async () => { diff --git a/packages/react/src/accordion/root/AccordionRoot.tsx b/packages/react/src/accordion/root/AccordionRoot.tsx index ebd9eb065ee..e0d8f50fe0f 100644 --- a/packages/react/src/accordion/root/AccordionRoot.tsx +++ b/packages/react/src/accordion/root/AccordionRoot.tsx @@ -9,10 +9,7 @@ import { CompositeList } from '../../internals/composite/list/CompositeList'; import { useDirection } from '../../internals/direction-context/DirectionContext'; import { AccordionRootContext } from './AccordionRootContext'; import { useRenderElement } from '../../internals/useRenderElement'; -import { - createChangeEventDetails, - type BaseUIChangeEventDetails, -} from '../../internals/createBaseUIEventDetails'; +import { type BaseUIChangeEventDetails } from '../../internals/createBaseUIEventDetails'; import { REASONS } from '../../internals/reasons'; const rootStateAttributesMapping = { @@ -69,6 +66,8 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot([]); + // Mirrors `accordionItemRefs` indexes so focus navigation targets only Accordion.Trigger. + const accordionTriggerRefs = React.useRef<(HTMLElement | null)[]>([]); const [value, setValue] = useControlled({ controlled: valueProp, @@ -78,8 +77,11 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot[number], nextOpen: boolean) => { - const details = createChangeEventDetails(REASONS.none); + ( + newValue: AccordionRoot.Value[number], + nextOpen: boolean, + details: AccordionRoot.ChangeEventDetails, + ) => { if (!multiple) { const nextValue = value[0] === newValue ? [] : [newValue]; onValueChange?.(nextValue, details); @@ -118,6 +120,7 @@ export const AccordionRoot = React.forwardRef(function AccordionRoot = React.useMemo( () => ({ accordionItemRefs, + accordionTriggerRefs, direction, disabled, handleValueChange, diff --git a/packages/react/src/accordion/root/AccordionRootContext.ts b/packages/react/src/accordion/root/AccordionRootContext.ts index b6ab273736f..aaf449a948b 100644 --- a/packages/react/src/accordion/root/AccordionRootContext.ts +++ b/packages/react/src/accordion/root/AccordionRootContext.ts @@ -6,9 +6,14 @@ import type { AccordionRoot } from './AccordionRoot'; export interface AccordionRootContext { accordionItemRefs: React.RefObject<(HTMLElement | null)[]>; + accordionTriggerRefs: React.RefObject<(HTMLElement | null)[]>; direction: TextDirection; disabled: boolean; - handleValueChange: (newValue: AccordionRoot.Value[number], nextOpen: boolean) => void; + handleValueChange: ( + newValue: AccordionRoot.Value[number], + nextOpen: boolean, + eventDetails: AccordionRoot.ChangeEventDetails, + ) => void; hiddenUntilFound: boolean; keepMounted: boolean; loopFocus: boolean; diff --git a/packages/react/src/accordion/trigger/AccordionTrigger.tsx b/packages/react/src/accordion/trigger/AccordionTrigger.tsx index f246c1add3c..c848e52ca24 100644 --- a/packages/react/src/accordion/trigger/AccordionTrigger.tsx +++ b/packages/react/src/accordion/trigger/AccordionTrigger.tsx @@ -19,18 +19,17 @@ import type { AccordionItemState } from '../item/AccordionItem'; import { useAccordionItemContext } from '../item/AccordionItemContext'; import { useRenderElement } from '../../internals/useRenderElement'; -function getActiveTriggers(accordionItemRefs: { current: (HTMLElement | null)[] }): HTMLElement[] { - const { current: accordionItemElements } = accordionItemRefs; - +function getActiveTriggers( + accordionItemRefs: { current: (HTMLElement | null)[] }, + accordionTriggerRefs: { current: (HTMLElement | null)[] }, +): HTMLElement[] { const output: HTMLElement[] = []; - for (let i = 0; i < accordionItemElements.length; i += 1) { - const section = accordionItemElements[i]; - if (!isElementDisabled(section)) { - const trigger = section?.querySelector('[type="button"], [role="button"]'); - if (trigger && !isElementDisabled(trigger)) { - output.push(trigger); - } + for (let i = 0; i < accordionItemRefs.current.length; i += 1) { + const section = accordionItemRefs.current[i]; + const trigger = accordionTriggerRefs.current[i]; + if (!isElementDisabled(section) && trigger && !isElementDisabled(trigger)) { + output.push(trigger); } } @@ -60,7 +59,7 @@ export const AccordionTrigger = React.forwardRef(function AccordionTrigger( const { panelId, open, handleTrigger, disabled: contextDisabled } = useCollapsibleRootContext(); - const disabled = disabledProp ?? contextDisabled; + const disabled = disabledProp || contextDisabled; const { getButtonProps, buttonRef } = useButton({ disabled, @@ -69,7 +68,8 @@ export const AccordionTrigger = React.forwardRef(function AccordionTrigger( composite: true, }); - const { accordionItemRefs, direction, loopFocus, orientation } = useAccordionRootContext(); + const { accordionItemRefs, accordionTriggerRefs, direction, loopFocus, orientation } = + useAccordionRootContext(); const isRtl = direction === 'rtl'; const isHorizontal = orientation === 'horizontal'; @@ -85,6 +85,15 @@ export const AccordionTrigger = React.forwardRef(function AccordionTrigger( }; }, [idProp, setTriggerId]); + const triggerRef = React.useCallback( + (element: HTMLElement | null) => { + if (state.index !== -1) { + accordionTriggerRefs.current[state.index] = element; + } + }, + [accordionTriggerRefs, state.index], + ); + const props = { 'aria-controls': open ? panelId : undefined, 'aria-expanded': open, @@ -98,7 +107,7 @@ export const AccordionTrigger = React.forwardRef(function AccordionTrigger( stopEvent(event); - const triggers = getActiveTriggers(accordionItemRefs); + const triggers = getActiveTriggers(accordionItemRefs, accordionTriggerRefs); const numOfEnabledTriggers = triggers.length; const lastIndex = numOfEnabledTriggers - 1; @@ -162,7 +171,7 @@ export const AccordionTrigger = React.forwardRef(function AccordionTrigger( break; } - if (nextIndex > -1) { + if (nextIndex > -1 && triggers[nextIndex]) { triggers[nextIndex].focus(); } }, @@ -170,7 +179,7 @@ export const AccordionTrigger = React.forwardRef(function AccordionTrigger( const element = useRenderElement('button', componentProps, { state, - ref: [forwardedRef, buttonRef], + ref: [forwardedRef, buttonRef, triggerRef], props: [props, elementProps, getButtonProps], stateAttributesMapping: triggerOpenStateMapping, });