[accordion] Fix trigger behavior bugs

[atomiks]

This is part of the Codex component behavior and test coverage sweep. It fixes Accordion behavior/API issues found during review and adds focused regression and adjacent sweep coverage for the affected paths.

Root cause

Accordion had edge cases where its implementation and tests did not fully cover the expected behavior/API contract.

Changes

• Fix the Accordion behavior covered by the review findings.
• Add regression tests for the failing or under-tested interaction paths.
• Add adjacent test-sweep coverage for Accordion root panel mounting props.

Original findings addressed

• [P2] Accordion.Root reports the wrong change reason for user-triggered changes. AccordionRoot.tsx creates a fresh REASONS.none details object instead of forwarding the trigger details that Accordion.Item receives from Collapsible, so clicking or keyboard-activating a trigger makes onValueChange(_, details).reason always "none" with a synthetic "base-ui" event.
• [P2] Arrow/Home/End navigation can focus non-trigger buttons inside an item. AccordionTrigger.tsx uses section.querySelector('[type="button"], [role="button"]'), so it returns the first matching descendant, not necessarily the Accordion.Trigger.
• [P3] Removing a custom trigger id breaks panel labeling. Accordion.Item initializes a generated fallback, but Accordion.Trigger only sets triggerId when idProp exists and always clears it on cleanup. If id changes from "custom" to undefined, the fallback is not restored, so Accordion.Panel loses aria-labelledby.

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	🔺+91B(+0.02%)	🔺+32B(+0.02%)

Details of bundle changes

Performance

Total duration: 1,045.64 ms -241.37 ms^(-18.8%) | Renders: 50 ⁽⁺⁰⁾ | Paint: 1,585.34 ms -376.52 ms^(-19.2%)

Test	Duration	Renders
Tabs mount (200 instances)	202.26 ms ▼-69.52 ms(-25.6%)	4 (+0)
Menu mount (300 instances)	117.33 ms ▼-35.77 ms(-23.4%)	2 (+0)
Scroll Area mount (300 instances)	73.38 ms ▼-34.15 ms(-31.8%)	3 (+0)
Dialog mount (300 instances)	45.82 ms ▼-16.05 ms(-25.9%)	1 (+0)

8 tests within noise — details

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	b1462a7
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a0bfe138b5bd60008d97808
😎 Deploy Preview	https://deploy-preview-4833--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@4833
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@4833
  

commit: b1462a7

[michaldudak]

Codex Review (GPT-5.5)

Reviewed the full PR 4833 branch against freshly fetched upstream/master. Dominant type: bug-fix, with secondary tests; the main Accordion regressions are covered, but one disabled-state escape hatch remains.

1. Bugs / Issues

1. 🔴 Accordion.Trigger disabled={false} can bypass a disabled root/item (blocking)

packages/react/src/accordion/trigger/AccordionTrigger.tsx

const { panelId, open, handleTrigger, disabled: contextDisabled } = useCollapsibleRootContext();

const disabled = disabledProp ?? contextDisabled;

Using ?? lets an explicit disabled={false} on the trigger override disabled state inherited from Accordion.Root or Accordion.Item. Since the Collapsible trigger handler relies on useButton({ disabled }) to block activation, that trigger can still toggle and fire callbacks inside a disabled root/item. It can also remain in the arrow-navigation set because the item element only has data-disabled, which isElementDisabled(section) does not treat as disabled.

Fix: Inherit disabled state with OR, matching AccordionItem and similar controls:

const disabled = disabledProp || contextDisabled;

Add regression coverage for disabled root and disabled item with <Accordion.Trigger disabled={false}>.

2. 🟡 Event details regression only asserts reason (non-blocking)

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

expect(onValueChange.mock.lastCall?.[1].reason).toBe(REASONS.triggerPress);

The original bug included both the wrong reason and a synthetic "base-ui" event. The implementation now forwards the original details, but the test would still pass if Accordion created a fresh triggerPress details object without forwarding the real native event.

Fix: Also assert that eventDetails.event comes from the real click/keyboard activation, for example by checking event.type is not "base-ui" or by capturing the native event identity where practical.

Test Coverage Assessment

On the PR branch, these passed:

pnpm test:jsdom Accordion --no-watch
pnpm test:chromium Accordion --no-watch

Applying only the added tests to freshly fetched upstream/master confirmed the intended regressions: jsdom fails the trigger-id restoration case, and Chromium fails trigger-id restoration, trigger-only keyboard navigation, and the onValueChange reason case. The new disabled, cancel, and root panel mounting tests pass on upstream, so they are adjacent coverage rather than regression locks for the implementation changes.

Recommendation

Request changes 🔴

The core fixes look good, but disabled root/item behavior is still bypassable through a public trigger prop in the touched trigger path. I’d address that small inheritance bug and add the missing regression before approving.

[flaviendelangle]

PR #4833 Review — [accordion] Fix trigger behavior bugs

Reviewed the PR with three specialized agents (code-reviewer, pr-test-analyzer, silent-failure-hunter), avoiding the two issues Codex already raised:

• ?? vs || on disabled trigger (already fixed in the current PR)
• Missing native event assertion in details test (already added)

Critical Issues

None. The three fixes are architecturally sound. Verified the accordionTriggerRefs lifecycle holds up under add/remove/reorder because getActiveTriggers is bounded by accordionItemRefs.current.length (which CompositeList trims), and reorders work via React's callback-ref cleanup-on-identity-change.

Important Issues (test gaps)

1. Missing dynamic add/remove regression test

AccordionTrigger.tsx:88-95

The parallel-array invariant between accordionItemRefs and accordionTriggerRefs is the whole reason for the new architecture, but no test mounts/unmounts items and asserts arrow navigation still targets the correct remaining triggers. This is the failure mode the new code is most exposed to.

2. Disabled-root test doesn't cover keyboard navigation paths

AccordionRoot.test.tsx:283-316

The new it.each(['root', 'item']) test only fires click, Space, Enter. A regression that flipped || back to ?? would also let a disabled-root trigger participate in Arrow/Home/End navigation — not currently asserted.

3. Trigger-id restoration only covers custom → undefined

AccordionRoot.test.tsx:62-95

Three transitions are uncovered and travel different code paths:

• undefined → custom (effect overrides fallback after mount)
• 'a' → 'b' (effect cleanup setTriggerId(undefined) races the next effect's setTriggerId(idProp) and could leave the fallback briefly visible)
• Initial render with a stable custom id (regression guard for the simple case)

Suggestions

4. cancel() not tested in controlled mode

Both new BaseUIChangeEventDetails tests use uncontrolled mode. A controlled-mode cancel() test would lock down the propagation through both branches.

5. multiple={true} × event-details forwarding

AccordionRoot.tsx:84-99 takes a different branch under multiple. The new reason/event.type assertions exist for that branch's onValueChange, but the parallel cancel() assertion is missing.

6. Nested-button navigation test omits horizontal & RTL

AccordionRoot.test.tsx:461-504. Existing convention in this file covers both orientations; the new test only covers vertical.

7. Dev-warning suggestion

getActiveTriggers silently drops items whose <Accordion.Item> exists in accordionItemRefs but whose accordionTriggerRefs[i] is null (e.g. an item rendered without a trigger). Worth a dev-only console.error since today the misuse produces silent focus weirdness with no diagnostic.

8. Test style nit

AccordionRoot.test.tsx:306-309 mixes fireEvent.click(trigger); trigger.focus(); user.keyboard(...). Existing convention is user.pointer({ keys: '[MouseLeft]', target: trigger }) followed by user.keyboard(...). Minor.

Strengths

• Switching from section.querySelector('[type="button"]') to a dedicated indexed ref array is the right fix; relying on DOM order kept breaking with custom render compositions.
• Lazy fallback (triggerId ?? defaultTriggerId) preserves the labeling invariant across setTriggerId(undefined) cleanups, which was the root of the third bug.
• Threading eventDetails through handleValueChange correctly preserves the cancel signal through both the item → root and the user-callback chains; cancel is checked at both layers.
• The React.useCallback choice for triggerRef is correct — useStableCallback would freeze the callback identity and prevent React from running cleanup that clears the previous state.index slot during reorders.