Skip to content

fix(alert): only restore focus that this Alert stored - #3255

Merged
interactivellama merged 2 commits into
salesforce:masterfrom
zigzagdev:fix/alert-focus-restore
Oct 26, 2026
Merged

interactivellama merged 2 commits into
salesforce:masterfrom
zigzagdev:fix/alert-focus-restore

Conversation

@zigzagdev

@zigzagdev zigzagdev commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #

Additional description

Symptom

When an Alert without dismissible unmounts while a Popover is open, focus jumps from inside the Popover back to the Popover's trigger.
A user typing in an input inside the Popover suddenly stops typing into it, while the Popover stays open.

Root cause

Alert's unmount effect called DOMElementFocus.returnFocusToStoredElement() unconditionally.
The stored element in DOMElementFocus is a single module level value shared with Popover (utilities/dialog) and Toast. A non dismissible Alert never calls storeActiveElement(), so on unmount it focused and cleared the element another component had stored.
This is not a regression from #3195.

Change

Alert now remembers in a ref whether it stored focus, which only happens when the close button takes focus on mount. Focus is restored on unmount only in that case.
Dismissible Alerts behave exactly as before.

before

2026-10-25.13.44.40.mov

after

2026-10-25.13.53.07.mov

Tests

  • Added two cases to alert.test.jsx.
    • A non dismissible Alert leaves focus stored by another component alone on unmount.
    • A dismissible Alert still returns focus to the previously focused element.
    • The first case fails without the fix.
  • Reproduced the bug in Storybook with a temporary story where a Popover is open and an Alert unmounts after a few seconds.
    • Before the fix, focus moved to the Popover trigger.
    • After the fix, focus stayed in the Popover input.

CONTRIBUTOR checklist (do not remove)

Please complete for every pull request.

Green gate — all of these must pass locally (CI runs the same):

  • npm run lint passes with 0 errors (npm run lint:fix to auto-fix). Warnings are the tracked gradual-migration backlog; do not add new errors.
  • npm run typecheck is clean (tsc --noEmit).
  • npm test -- --run passes — this runs both Vitest projects (jsdom unit + real-browser browser) and the story HTML snapshots. If you changed rendered markup on purpose, update snapshots with npm test -- --run -u and review the diff.
  • npm run build succeeds (type-check + Vite library build + .d.ts + entry shims).

Behavior & docs:

  • First-time contributors: sign the CLA (a bot will comment with instructions).
  • Added/updated Vitest + React Testing Library tests for the change (template: components/button/__tests__). New props and bug fixes should get a regression test. Tests that genuinely need a real browser (focus traversal, layout/measurement, canvas) go in a *.browser.test.jsx file (the browser project); everything else is jsdom.
  • Reviewed the component's Storybook stories: npm run storybook → http://localhost:6007/. Confirm the story actually renders and behaves (deprecated Storyshots/Karma are gone; there is no separate browser-tests server).
  • Story/example code uses the component's real prop API (e.g. render-props like onRenderContent / onRenderVisualPicker, icon props, events), not assumptions — a common source of "component looks broken" reports has been stories authored against the wrong API. Cross-check against components/<name>/__examples__.
  • Storybook Accessibility (a11y) panel shows no new violations for the affected stories.
  • If you added or changed a type, remember runtime PropTypes were removed in the TS migration — types are the contract. Ship accurate .d.ts (they're generated by the build); JS consumers rely on them via editor type-acquisition.

REVIEWER checklist (do not remove)

  • CI is green (GitHub Actions CI workflow: lint, typecheck, test, build). Chromatic visual review, if configured, has been checked.
  • Change has adequate test coverage; bug fixes include a regression test.
  • Reviewed the affected Storybook stories and their a11y panels.
  • Rendered markup still conforms to SLDS — check the story HTML snapshot diff (components/__tests__/__snapshots__/story-snapshots.test.jsx.snap).
  • Public API / package surface: if exports changed, the deep-import contract still holds (@salesforce/design-system-react/components/<name> and nested paths resolve for ESM, CJS, and types). Removing/renaming a public component or export is a breaking change → feat!:/BREAKING CHANGE: and a major version bump.

Alert's unmount effect called DOMElementFocus.returnFocusToStoredElement()
unconditionally. The stored element is a module level singleton shared
with Popover (utilities/dialog) and Toast. A non dismissible Alert never
calls storeActiveElement(), but on unmount it still focused and cleared
whatever another component had stored. For example, if an inline Alert
unmounts while a Popover is open, focus jumps to the Popover's trigger
and the Popover loses the element it meant to restore.

Track whether this Alert stored focus and only restore in that case.
Dismissible Alerts keep returning focus to the previously focused
element after the close button took it.
A non dismissible Alert leaves focus stored by another component alone,
and a dismissible Alert still returns focus to the previously focused
element on unmount.
@interactivellama

Copy link
Copy Markdown
Contributor

Great find! @zigzagdev

@interactivellama
interactivellama merged commit a1a4780 into salesforce:master Oct 26, 2026
7 checks passed
@zigzagdev
zigzagdev deleted the fix/alert-focus-restore branch October 27, 2026 00:17
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.

2 participants