Skip to content

fix(Accordion): leaking stale summaryButtons refs on panel removal - #3193

Open
zigzagdev wants to merge 2 commits into
salesforce:masterfrom
zigzagdev:fix/accordion-stale-summary-buttons
Open

fix(Accordion): leaking stale summaryButtons refs on panel removal#3193
zigzagdev wants to merge 2 commits into
salesforce:masterfrom
zigzagdev:fix/accordion-stale-summary-buttons

Conversation

@zigzagdev

@zigzagdev zigzagdev commented Aug 29, 2026

Copy link
Copy Markdown

Fixes #

Button for Accordion's addSummaryButton only pushed into this.summaryButtons and never removed an entry when a panel's button unmounted (its ref callback fires with null, which was ignored). After removing a panel, a stale detached DOM node stayed in the array, so keyboard navigation could focus that stale node instead of the correct remaining panel.
With this fix, keying summaryButtons by the child's render index and clearing the slot when its ref detaches, and added a regression test.

Additional description


CONTRIBUTOR checklist (do not remove)

Please complete for every pull request

  • First-time contributors should sign the Contributor License Agreement. It's a fancy way of saying that you are giving away your contribution to this project. If you haven't before, wait a few minutes and a bot will comment on this pull request with instructions.
  • npm run lint:fix has been run and linting passes.
  • Mocha, Jest (Storyshots), and components/component-docs.json CI tests pass (npm test).
  • Tests have been added for new props to prevent regressions in the future. See readme.
  • Review the appropriate Storybook stories. Open http://localhost:9001/.
  • Review tests are passing in the browser. Open http://localhost:8001/.
  • Review markup conforms to SLDS by looking at DOM snapshot strings.

REVIEWER checklist (do not remove)

  • CircleCI tests pass. This includes linting, Mocha, Jest, Storyshots, and components/component-docs.json tests.
  • Tests have been added for new props to prevent regressions in the future. See readme.
  • Review the appropriate Storybook stories. Open http://localhost:9001/.
  • The Accessibility panel of each Storybook story has 0 violations (aXe). Open http://localhost:9001/.
  • Review tests are passing in the browser. Open http://localhost:8001/.
  • Review markup conforms to SLDS by looking at DOM snapshot strings.
Required only if there are markup / UX changes
  • Add year-first date and commit SHA to last-slds-markup-review in package.json and push.
  • Request a review of the deployed Heroku app by the Salesforce UX Accessibility Team.
  • Add year-first review date, and commit SHA, last-accessibility-review, to package.json and push.
  • While the contributor's branch is checked out, run npm run local-update within locally cloned site repo to confirm the site will function correctly at the next release.

addSummaryButton only ever pushed into this.summaryButtons and never
removed an entry when a panel's button unmounted (its ref callback
fires with null, which was ignored). After removing a panel, stale
detached DOM nodes stayed in the array and index-based lookups in
onKeyDownSummary/componentDidUpdate could reference them, so keyboard
navigation could silently focus a stale node instead of the correct
remaining panel.

Key summaryButtons by the child's render index instead of push-only,
clearing the slot when its ref detaches (button === null), and guard
the id-based lookup and the componentDidUpdate focus call against
holes left by pruned/removed slots.
Covers the fix in the previous commit: removing a panel and then
navigating with the arrow keys focuses the correct remaining button
instead of a stale detached node.
@zigzagdev zigzagdev changed the title Fix Accordion leaking stale summaryButtons refs on panel removal fix(Accordion): leaking stale summaryButtons refs on panel removal Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant