Preview: Enforce inert body if manager is focus-trapped #33186
Conversation
Sidnioulz
added
bug
ui
ci:normal
a11y: keyboard
📝 WalkthroughWalkthroughA focus trap management event system is introduced across the manager and preview layers. A new MANAGER_FOCUS_TRAP_CHANGE event is defined, exported through public APIs, observed in the manager when the inert attribute changes on the root element, and handled in the preview runtime to synchronize the document body's inert state. Changes
Sequence DiagramsequenceDiagram
participant Manager
participant Channel as Addons Channel
participant Preview
participant DOM
Manager->>DOM: Observe inert attribute via MutationObserver
DOM-->>Manager: inert attribute changes
Manager->>Channel: Emit MANAGER_FOCUS_TRAP_CHANGE event
Channel->>Preview: Deliver event
Preview->>DOM: Set or remove document.body.inert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/core/src/manager/App.tsx (1)
30-51: Use core-events constant instead of string literal and consider emitting initial stateThe observer logic and cleanup look good, but two small robustness improvements would help:
- Import and use the
MANAGER_FOCUS_TRAP_CHANGEconstant fromstorybook/internal/core-eventsinstead of the hard‑coded'managerFocusTrapChange'string to keep the event name single‑sourced and avoid drift withcore-events/index.ts.- After wiring the observer, consider emitting the current inert state once (e.g.,
addons.getChannel().emit(MANAGER_FOCUS_TRAP_CHANGE, rootElement.hasAttribute('inert'))) so the preview is correct even if#rootalready hadinertset before this effect starts (for example, if a focus trap usesuseLayoutEffect).A possible sketch:
-import { addons } from 'storybook/manager-api'; +import { addons } from 'storybook/manager-api'; +import { MANAGER_FOCUS_TRAP_CHANGE } from 'storybook/internal/core-events'; @@ - useEffect(() => { - const rootElement = document.getElementById('root'); + useEffect(() => { + const rootElement = document.getElementById('root'); if (!rootElement) { return; } - const observer = new MutationObserver((mutations) => { + const channel = addons.getChannel(); + const emitState = () => { + const hasInert = rootElement.hasAttribute('inert'); + channel.emit(MANAGER_FOCUS_TRAP_CHANGE, hasInert); + }; + + const observer = new MutationObserver((mutations) => { mutations.forEach((mutation) => { if (mutation.type === 'attributes' && mutation.attributeName === 'inert') { - const hasInert = rootElement.hasAttribute('inert'); - addons.getChannel().emit('managerFocusTrapChange', hasInert); + emitState(); } }); }); @@ - observer.observe(rootElement, { + observer.observe(rootElement, { attributes: true, attributeFilter: ['inert'], }); - return () => observer.disconnect(); + emitState(); + + return () => observer.disconnect(); }, []);code/core/src/core-events/index.ts (1)
94-96: Clarify event comment to describe state changes and payloadThe enum entry and re‑export look correct and consistent with usage. To make the contract clearer for consumers, consider tightening the comment to reflect that this event is fired on state changes and carries a boolean payload:
- // Emitted when the manager UI sets up a focus trap + // Emitted whenever the manager UI's focus trap state changes; payload: boolean isActive MANAGER_FOCUS_TRAP_CHANGE = 'managerFocusTrapChange',This makes it obvious to listeners (like
preview/runtime.ts) what they should expect.Also applies to: 164-165
code/core/src/preview/runtime.ts (1)
1-1: Register focus-trap listener without depending solely on DOMContentLoaded timingThe event wiring and inert toggling look aligned with the new manager event, but relying on
DOMContentLoadedintroduces a timing edge case: ifsetup()runs afterDOMContentLoadedhas already fired (e.g., depending on how the bundle is loaded), this listener never runs and theMANAGER_FOCUS_TRAP_CHANGEsubscription is never installed.You can avoid this by subscribing immediately and guarding
document.body, which should be safe given that focus traps are only triggered on user interaction:-import { MANAGER_FOCUS_TRAP_CHANGE, TELEMETRY_ERROR } from 'storybook/internal/core-events'; +import { MANAGER_FOCUS_TRAP_CHANGE, TELEMETRY_ERROR } from 'storybook/internal/core-events'; @@ - document.addEventListener('DOMContentLoaded', () => { - const channel = global.__STORYBOOK_ADDONS_CHANNEL__; - channel.on(MANAGER_FOCUS_TRAP_CHANGE, (isActive: boolean) => { - if (isActive) { - document.body.setAttribute('inert', 'true'); - } else { - document.body.removeAttribute('inert'); - } - }); - }); + const channel = global.__STORYBOOK_ADDONS_CHANNEL__; + channel.on(MANAGER_FOCUS_TRAP_CHANGE, (isActive: boolean) => { + const body = document.body; + if (!body) return; + + if (isActive) { + body.setAttribute('inert', 'true'); + } else { + body.removeAttribute('inert'); + } + });If you prefer to keep the DOMContentLoaded guard, consider a
document.readyStatecheck:const subscribe = () => { const channel = global.__STORYBOOK_ADDONS_CHANNEL__; channel.on(MANAGER_FOCUS_TRAP_CHANGE, /* handler as above */); }; if (document.readyState === 'loading') { document.addEventListener('DOMContentLoaded', subscribe, { once: true }); } else { subscribe(); }This ensures the preview always reacts to the manager’s focus‑trap state.
Also applies to: 34-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/core/src/core-events/index.ts(2 hunks)code/core/src/manager/App.tsx(1 hunks)code/core/src/manager/globals/exports.ts(1 hunks)code/core/src/preview/runtime.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/manager/App.tsxcode/core/src/core-events/index.tscode/core/src/manager/globals/exports.tscode/core/src/preview/runtime.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/manager/App.tsxcode/core/src/core-events/index.tscode/core/src/manager/globals/exports.tscode/core/src/preview/runtime.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/manager/App.tsxcode/core/src/core-events/index.tscode/core/src/manager/globals/exports.tscode/core/src/preview/runtime.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/App.tsxcode/core/src/core-events/index.tscode/core/src/manager/globals/exports.tscode/core/src/preview/runtime.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/globals/exports.tscode/core/src/preview/runtime.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/preview/runtime.ts
📚 Learning: 2025-11-24T17:49:47.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: Applies to code/**/!(*.test).{ts,tsx,js,mjs} : Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Applied to files:
code/core/src/preview/runtime.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/preview/runtime.ts
📚 Learning: 2025-11-24T17:49:47.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: Applies to code/**/!(*.test).{ts,tsx,js,mjs} : Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Applied to files:
code/core/src/preview/runtime.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: get-branch
🔇 Additional comments (1)
code/core/src/manager/globals/exports.ts (1)
574-636: LGTM on adding MANAGER_FOCUS_TRAP_CHANGE to globalsIncluding
'MANAGER_FOCUS_TRAP_CHANGE'in thestorybook/internal/core-eventsexport list is consistent with other core events and keeps the globals surface aligned withcore-events/index.ts.
|
View your CI Pipeline Execution ↗ for commit 085f4e8
☁️ Nx Cloud last updated this comment at |
Sidnioulz
added
the
patch:yes
Closes #32596
What I did
inertattr set by RAC on#rootwhen it traps focus)inerton preview body to prevent any programmatic focus from happening)Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
ø
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.