Preview: Enforce inert body if manager is focus-trapped #33186
Conversation
Sidnioulz
added
bug
ui
ci:normal
a11y: keyboard
📝 WalkthroughWalkthroughAdds a MANAGER_INERT_ATTRIBUTE_CHANGED core event, a MutationObserver in the manager that emits it when the root element's inert attribute changes, a preview runtime listener that mirrors inert to document.body, and local ariaHideOutside effects in Modal and Select to hide/outside content when mounted/open. Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant Addons as "Addons Channel"
participant Preview
participant DOM as "Preview DOM"
Note over Manager: MutationObserver on manager root
Manager->>DOM: Observe root `inert` attribute
DOM-->>Manager: Notify `inert` attribute change
Manager->>Addons: Emit MANAGER_INERT_ATTRIBUTE_CHANGED { isInert }
Addons->>Preview: Deliver event
Preview->>DOM: Set or remove `document.body.inert` based on isInert
Note over DOM,Preview: Modal/Select call ariaHideOutside when mounted/open
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 d1f9e60
☁️ Nx Cloud last updated this comment at |
Sidnioulz
added
the
patch:yes
|
Found two more holes in the emperor's armour:
|
Sidnioulz
force-pushed
the
sidnioulz/issue-32596-focus-trap
branch
from
085f4e8 to
72cc97e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/components/components/Select/Select.tsx (1)
145-149: Correct implementation of inert behavior for Select popover.The
ariaHideOutsidecall withshouldUseInert: truecorrectly prevents focus from escaping the Select popover, addressing the issue described in the PR objectives. The cleanup function is properly returned to restore state when the popover closes.Optional: Simplify dependency array.
The dependency array
[popoverRef]can be replaced with an empty array[]sincepopoverRefis a stable ref object that doesn't change between renders. This is more idiomatic and clearer about the effect's intent to run once on mount:- }, [popoverRef]); + }, []);Both implementations work correctly, but the empty array is more conventional for effects that should only run once when a component mounts.
code/core/src/preview/runtime.ts (1)
34-50: Consider handling cases wheresetup()runs afterDOMContentLoadedRight now the preview only subscribes to
MANAGER_INERT_ATTRIBUTE_CHANGEDinside aDOMContentLoadedhandler. Ifsetup()ever runs afterDOMContentLoadedhas already fired, the callback won’t run and the listener won’t be registered, so inert sync would silently be skipped.You can defensively handle both before/after DOM-ready cases by extracting a small initializer:
- /** - * Ensure we synchronise the preview runtime's inert state with the manager's. The inert attribute - * used to be propagated into iframes, but this has changed, breaking focus trap implementations - * that rely on inert on the document root. We synchronise inert to ensure end user components - * don't programmatically focus when a focus trap is active in the manager UI. Otherwise, the UI - * could reach a deadlock state and be unusable. - */ - document.addEventListener('DOMContentLoaded', () => { - const channel = global.__STORYBOOK_ADDONS_CHANNEL__; - channel.on(MANAGER_INERT_ATTRIBUTE_CHANGED, (isInert: boolean) => { - if (isInert) { - document.body.setAttribute('inert', 'true'); - } else { - document.body.removeAttribute('inert'); - } - }); - }); + /** + * Ensure we synchronise the preview runtime's inert state with the manager's. The inert attribute + * used to be propagated into iframes, but this has changed, breaking focus trap implementations + * that rely on inert on the document root. We synchronise inert to ensure end user components + * don't programmatically focus when a focus trap is active in the manager UI. Otherwise, the UI + * could reach a deadlock state and be unusable. + */ + const initInertSync = () => { + const channel = global.__STORYBOOK_ADDONS_CHANNEL__; + channel.on(MANAGER_INERT_ATTRIBUTE_CHANGED, (isInert: boolean) => { + if (isInert) { + document.body.setAttribute('inert', 'true'); + } else { + document.body.removeAttribute('inert'); + } + }); + }; + + if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', initInertSync, { once: true }); + } else { + initInertSync(); + }This keeps the behavior the same in current builds but makes the subscription robust to load-order changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/core/src/components/components/Modal/Modal.tsx(2 hunks)code/core/src/components/components/Select/Select.tsx(2 hunks)code/core/src/core-events/index.ts(2 hunks)code/core/src/manager/App.tsx(2 hunks)code/core/src/manager/globals/exports.ts(1 hunks)code/core/src/preview/runtime.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/core-events/index.ts
- code/core/src/manager/globals/exports.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
🧠 Learnings (10)
📓 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/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
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.
📚 Learning: 2025-11-05T09:36:55.944Z
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.
Applied to files:
code/core/src/components/components/Select/Select.tsxcode/core/src/components/components/Modal/Modal.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/components/components/Select/Select.tsxcode/core/src/components/components/Modal/Modal.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/Select/Select.tsxcode/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/components/components/Select/Select.tsxcode/core/src/components/components/Modal/Modal.tsx
📚 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/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsx
📚 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-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-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
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: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
code/core/src/components/components/Modal/Modal.tsx (2)
7-12: Overlay imports updated appropriately for inert handlingAdding
ariaHideOutsidealongside the existing overlay imports is consistent with React Aria usage and matches the new effect below; no concerns here.
177-181: ariaHideOutside effect correctly applies and cleans up inertThis effect scopes
ariaHideOutsideto the dialog container, appliesshouldUseInert: trueonly while the modal is mounted/open, and relies on the returned cleanup function so outside content (and the manager root) are restored when the modal closes/unmounts. This aligns with the PR goal of drivinginertfrom manager modals without leaving stray attributes behind.code/core/src/manager/App.tsx (3)
4-4: Event import wiring looks correctImporting
Eventshere to emitMANAGER_INERT_ATTRIBUTE_CHANGEDfrom the manager is consistent with the new cross-frame sync.
26-33: Keyboard shortcuts documentation is clearThe added comment accurately documents the shortcut flag propagation and improves discoverability without changing behavior.
35-60: Inert observer and channel emit are well-scopedThe MutationObserver on
#rootwithattributeFilter: ['inert']and cleanup on unmount is tight and focused; emittingEvents.MANAGER_INERT_ATTRIBUTE_CHANGEDwithhasInertis the right abstraction for the preview listener.code/core/src/preview/runtime.ts (1)
1-1: Core-events import is aligned with new inert eventPulling in
MANAGER_INERT_ATTRIBUTE_CHANGEDalongsideTELEMETRY_ERRORis the right place for this preview-side wiring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/preview/runtime.ts (1)
34-57: Consider guarding against a missing addons channel ininitInertSyncThe inert sync block and its comment do a good job documenting and implementing the manager↔preview inert behavior to prevent focus‑trap deadlocks. The only potential fragility is that
initInertSyncassumesglobal.__STORYBOOK_ADDONS_CHANNEL__is always defined when it runs; in nonstandard setups (tests, custom embedding) this could throw before the preview is fully usable.You could make this a bit more defensive without affecting normal Storybook usage by early‑exiting if the channel isn’t available yet:
const initInertSync = () => { const channel = global.__STORYBOOK_ADDONS_CHANNEL__; if (!channel) { // No addons channel available (e.g. custom runtime); skip inert sync. return; } channel.on(MANAGER_INERT_ATTRIBUTE_CHANGED, (isInert: boolean) => { if (isInert) { document.body.setAttribute('inert', 'true'); } else { document.body.removeAttribute('inert'); } }); };This keeps the intended behavior intact in regular environments while avoiding a hard runtime error where the channel is absent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/components/components/Select/Select.tsx(2 hunks)code/core/src/preview/runtime.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/components/components/Select/Select.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/preview/runtime.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/preview/runtime.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/preview/runtime.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/preview/runtime.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/preview/runtime.ts
🧠 Learnings (7)
📓 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/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
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.
📚 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/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-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-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/preview/runtime.ts
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/preview/runtime.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
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: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/preview/runtime.ts (1)
1-1: Import wiring for MANAGER_INERT_ATTRIBUTE_CHANGED looks correctThe new event constant is imported from the same internal core-events module as TELEMETRY_ERROR and is used consistently below; nothing to change here.
Sidnioulz
force-pushed
the
sidnioulz/issue-32596-focus-trap
branch
from
2e5948b to
96968eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/core/src/components/components/Modal/Modal.tsx (1)
177-181: Consider usingstate.isOpeninstead ofopen || defaultOpenfor clarity.The effect correctly applies
ariaHideOutsidewhen the modal is mounted and open, and the cleanup function properly removes it. The conditionisMounted && (open || defaultOpen)works correctly becauseisMountedtracks the transition state.However, for clarity and consistency with the component's state management, consider using
state.isOpeninstead ofopen || defaultOpen, sincestate.isOpenrepresents the actual current open state of the modal:useEffect(() => { - if (isMounted && (open || defaultOpen) && overlayRef.current) { + if (isMounted && state.isOpen && overlayRef.current) { return ariaHideOutside([overlayRef.current], { shouldUseInert: true }); } - }, [isMounted, open, defaultOpen, overlayRef]); + }, [isMounted, state.isOpen, overlayRef]);This would make the effect's behavior more directly tied to the actual modal state rather than the input props.
code/core/src/manager/App.tsx (1)
35-60: Consider emitting the initial inert state after setting up the observer.The MutationObserver implementation correctly watches for
inertattribute changes and emits events accordingly. The comprehensive comment explains the rationale well, and the cleanup properly disconnects the observer.However, there's a potential edge case: if the root element already has the
inertattribute set when this component mounts, the preview won't be notified until the attribute changes. While this is unlikely in practice (since React Aria Components typically setsinertafter mount), you might want to emit the initial state for robustness:observer.observe(rootElement, { attributes: true, attributeFilter: ['inert'], }); + + // Emit initial state to ensure preview is synchronized + const hasInert = rootElement.hasAttribute('inert'); + addons.getChannel().emit(Events.MANAGER_INERT_ATTRIBUTE_CHANGED, hasInert); return () => observer.disconnect();This would guarantee the preview is always synchronized, even in edge cases where
inertmight be set before the observer is established.code/core/src/preview/runtime.ts (1)
34-56: Consider using theinertproperty instead of attributes for cleaner code.The inert synchronization implementation is correct and well-documented. The comment clearly explains the rationale, including the HTML spec changes regarding iframe propagation. The DOMContentLoaded check ensures the DOM is ready, and the
{ once: true }option properly prevents duplicate listeners.However, for boolean HTML attributes like
inert, using the DOM property is more idiomatic thansetAttribute/removeAttribute:const initInertSync = () => { const channel = global.__STORYBOOK_ADDONS_CHANNEL__; channel.on(MANAGER_INERT_ATTRIBUTE_CHANGED, (isInert: boolean) => { - if (isInert) { - document.body.setAttribute('inert', 'true'); - } else { - document.body.removeAttribute('inert'); - } + document.body.inert = isInert; }); };This is functionally equivalent but more concise and directly reflects the boolean nature of the property.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/core/src/components/components/Modal/Modal.tsx(2 hunks)code/core/src/components/components/Select/Select.tsx(2 hunks)code/core/src/core-events/index.ts(2 hunks)code/core/src/manager/App.tsx(2 hunks)code/core/src/manager/globals/exports.ts(1 hunks)code/core/src/preview/runtime.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/components/components/Select/Select.tsx
- code/core/src/manager/globals/exports.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/preview/runtime.tscode/core/src/core-events/index.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/preview/runtime.tscode/core/src/core-events/index.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/preview/runtime.tscode/core/src/core-events/index.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/preview/runtime.tscode/core/src/core-events/index.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/preview/runtime.tscode/core/src/core-events/index.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
🧠 Learnings (11)
📓 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/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
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.
📚 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/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
📚 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-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-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/preview/runtime.tscode/core/src/components/components/Modal/Modal.tsxcode/core/src/manager/App.tsx
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
Applied to files:
code/core/src/preview/runtime.ts
📚 Learning: 2025-11-05T09:36:55.944Z
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.
Applied to files:
code/core/src/components/components/Modal/Modal.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
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.
Applied to files:
code/core/src/components/components/Modal/Modal.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/components/components/Modal/Modal.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/components/components/Modal/Modal.tsx
⏰ 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: nx
🔇 Additional comments (5)
code/core/src/components/components/Modal/Modal.tsx (1)
7-12: LGTM!The import of
ariaHideOutsideis correctly structured and aligns with the PR's objective to prevent preview components from stealing focus when manager overlays are active.code/core/src/core-events/index.ts (1)
94-95: LGTM!The new event
MANAGER_INERT_ATTRIBUTE_CHANGEDis well-named, properly documented with an inline comment, and correctly exported. The naming convention is consistent with other events in the codebase.Also applies to: 164-164
code/core/src/manager/App.tsx (2)
4-4: LGTM!The import correctly uses the default export from core-events, which includes the new
MANAGER_INERT_ATTRIBUTE_CHANGEDevent.
26-33: LGTM!The comment clearly explains the purpose of setting the keyboard shortcuts data attribute, and the implementation correctly updates it based on the config.
code/core/src/preview/runtime.ts (1)
1-1: LGTM!The import correctly includes the new
MANAGER_INERT_ATTRIBUTE_CHANGEDevent alongside the existingTELEMETRY_ERRORimport.
Sidnioulz
force-pushed
the
sidnioulz/issue-32596-focus-trap
branch
from
96968eb to
d1f9e60
Compare
…s-trap Preview: Enforce inert body if manager is focus-trapped (cherry picked from commit 18bdbb8)
github-actions
bot
added
the
patch:done
Closes #32596
Caused by whatwg/html#7605
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
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.