Fix/sidebar#33513
Conversation
…sk-appropriate fix(stateless-tabs-view): solving accessibility order issue
fix(boolean-controls): solving aria output for true and false
…lights fix(sidebar): solving low contrast highlight issues
📝 WalkthroughWalkthroughThese changes introduce comprehensive accessibility improvements to the sidebar tree navigation component, including ARIA attributes, keyboard navigation handlers (Enter/Space/Arrow keys), focus management enhancements, depth calculation refactoring, and scroll positioning adjustments. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Tree as Tree Component
participant useHighlighted
participant useExpanded
participant DOM as DOM/Scroll
User->>Tree: Press Arrow/Enter/Space key
Tree->>Tree: onKeyDown handler processes event
alt Arrow Up/Down Navigation
Tree->>useHighlighted: Update highlight via focusin listener
useHighlighted->>DOM: Set focus on next element
useHighlighted->>DOM: Scroll into view with topOffset
else Enter/Space on Expandable Node
Tree->>useExpanded: Trigger expansion toggle
alt Has Children
useExpanded->>Tree: Toggle aria-expanded state
useExpanded->>useHighlighted: Select first child
else No Children
useExpanded->>useHighlighted: Select item
end
end
DOM->>User: Visual feedback (focus outline, scroll)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
483-491: UnnecessarypreventDefault()on mouseenter event.The
mouseenterevent has no default browser action to prevent. This call is ineffective and can be removed.🔧 Suggested fix
onMouseEnter={(event) => { - event.preventDefault(); if (item.type === 'component' || item.type === 'story') { api.emit(PRELOAD_ENTRIES, { ids: [children[0]], options: { target: refId }, }); } }}
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/sidebar/Tree.tsx:
- Around line 428-451: The onKeyDown handler creates a local const named
isExpanded that shadows the component prop isExpanded; rename the local variable
(e.g., buttonExpanded or expandedAttr) used to read the button's aria-expanded
attribute inside the ArrowLeft/ArrowRight branch so it no longer conflicts with
the prop, update any uses in that block accordingly, and keep the type cast to
HTMLButtonElement for the queried button.
🧹 Nitpick comments (4)
code/core/src/manager/components/sidebar/useHighlighted.ts (3)
122-131: Consider using a constant for 'storybook_internal' ref ID.The string
'storybook_internal'appears to be a magic value. Consider importing or defining a constant (similar toDEFAULT_REF_IDused elsewhere) for consistency and maintainability.#!/bin/bash # Check if there's an existing constant for 'storybook_internal' rg -n "storybook_internal" --type ts code/core/src/manager
135-140: Potential stale reference in cleanup function.The cleanup function accesses
containerRef.currentwhich may have changed between effect setup and cleanup. Consider capturing the reference at effect setup time.♻️ Proposed fix
// Use focusin (bubbles) instead of focus (doesn't bubble) + const container = containerRef.current; - containerRef.current.addEventListener('focusin', handleFocusIn); + container.addEventListener('focusin', handleFocusIn); return () => { - containerRef.current?.removeEventListener('focusin', handleFocusIn); + container?.removeEventListener('focusin', handleFocusIn); };
218-222: Potential stale reference in cleanup.Same issue as the focusin handler - the cleanup function should capture
containerRef.currentat setup time.♻️ Proposed fix
+ const container = containerRef.current; + const handleKeyDown = (event: KeyboardEvent) => { // ... handler code }; - container.addEventListener('keydown', handleKeyDown, true); + container?.addEventListener('keydown', handleKeyDown, true); return () => { - container.removeEventListener('keydown', handleKeyDown, true); + container?.removeEventListener('keydown', handleKeyDown, true); };Note: The
containervariable on line 166 already captures this, but it's defined inside the effect body. Move it earlier or ensure the cleanup uses the same reference.code/core/src/manager/components/sidebar/Tree.tsx (1)
279-280: Redundant variable assignment.
nodeDepthis a direct copy ofvisualDepthwith no transformation. Consider usingvisualDepthdirectly or renaming the function result.♻️ Suggested simplification
- const visualDepth = calculateVisualDepth(item, collapsedData); - const nodeDepth = visualDepth; + const nodeDepth = calculateVisualDepth(item, collapsedData);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/core/src/manager/components/sidebar/Refs.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/components/sidebar/useExpanded.tscode/core/src/manager/components/sidebar/useHighlighted.tscode/core/src/manager/utils/tree.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/sidebar/useHighlighted.tscode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/utils/tree.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useExpanded.tscode/core/src/manager/components/sidebar/Refs.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/sidebar/useHighlighted.tscode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/utils/tree.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useExpanded.tscode/core/src/manager/components/sidebar/Refs.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/sidebar/useHighlighted.tscode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/utils/tree.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useExpanded.tscode/core/src/manager/components/sidebar/Refs.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/useHighlighted.tscode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/utils/tree.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useExpanded.tscode/core/src/manager/components/sidebar/Refs.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/sidebar/useHighlighted.tscode/core/src/manager/components/sidebar/TreeNode.tsxcode/core/src/manager/utils/tree.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useExpanded.tscode/core/src/manager/components/sidebar/Refs.tsx
🧠 Learnings (5)
📚 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/components/sidebar/useHighlighted.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/manager/components/sidebar/useHighlighted.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/manager/components/sidebar/useHighlighted.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/manager/components/sidebar/useHighlighted.tscode/core/src/manager/components/sidebar/Refs.tsx
📚 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/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useExpanded.ts
🧬 Code graph analysis (2)
code/core/src/manager/components/sidebar/useHighlighted.ts (1)
code/core/src/manager/utils/tree.ts (2)
scrollIntoView(84-105)cycle(71-82)
code/core/src/manager/components/sidebar/Tree.tsx (5)
code/core/src/manager/container/Sidebar.tsx (1)
Item(10-10)code/core/src/types/modules/api-stories.ts (1)
API_HashEntry(67-73)code/core/src/theming/base.ts (1)
color(1-42)code/core/src/manager/utils/tree.ts (1)
getLink(16-18)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Container(96-154)
🔇 Additional comments (12)
code/core/src/manager/components/sidebar/Refs.tsx (1)
123-128: LGTM!The callback is correctly converted to a block-bodied function. Since
onSelectStoryIdis used purely for side effects (triggering story selection), not returning the API call result is appropriate and aligns with the event-driven pattern used elsewhere in this PR.code/core/src/manager/components/sidebar/TreeNode.tsx (1)
40-66: LGTM - Standardized indentation improves consistency.The simplified formula
8 + depth * 24provides uniform indentation across all node types. Note thatisExpandableis still in the function signature (line 43) for the styled component, but it's no longer used in the padding calculation. This is fine since the parameter is still passed toBranchNodefor other purposes.code/core/src/manager/utils/tree.ts (1)
84-105: LGTM - Improved scroll behavior with offset awareness.The refactored logic correctly:
- Centers elements when explicitly requested
- Accounts for fixed UI elements (menu header and bottom wrapper) when calculating visibility bounds
- Only scrolls when the element is actually outside the visible area
The use of
block: 'nearest'for non-centered scrolling provides smooth navigation without jarring jumps.code/core/src/manager/components/sidebar/useExpanded.ts (3)
210-214: Good accessibility improvement: Reading aria-expanded from the inner button.Correctly identifies the button/anchor element inside the wrapper to read
aria-expanded, which aligns with how ARIA attributes are set on interactive elements rather than wrapper divs.
215-219: Good addition: Including 'group' in selectable node types.This enables keyboard activation of group nodes, improving accessibility for users navigating with screen readers or keyboard-only input.
230-238: No action needed - selectStory handles non-leaf node selection correctly.When expanding and selecting the first child via
onSelectStoryId(firstChildId), theselectStoryAPI automatically resolves non-leaf nodes (groups/components) to their first leaf descendant (story/docs) usingfindLeafEntry. This behavior is tested and handles filtered indices correctly, ensuring only visible entries are selected.code/core/src/manager/components/sidebar/useHighlighted.ts (2)
76-83: Good accessibility enhancement: DOM focus management for screen readers.Setting focus with
preventScroll: trueensures NVDA and other screen readers can track the highlighted item while the custom scroll logic handles visibility.
159-222: Good refactor: Container-scoped keyboard navigation.Moving arrow key handling from document-level to container-scoped events improves:
- Screen reader compatibility: NVDA and other assistive technologies can intercept document-level events
- Encapsulation: Events are properly scoped to the sidebar component
- Event cleanup: More predictable cleanup with container reference
The use of
cyclefor wraparound navigation anddidRunAroundfor center-scrolling on wrap provides smooth keyboard navigation.code/core/src/manager/components/sidebar/Tree.tsx (4)
88-102: Well-structured focus and hover styling for accessibility.The separation of hover and focus states improves maintainability, and using
focus-visiblefor keyboard focus indicators follows accessibility best practices. The use oftheme.color.lightestfor the selected state's focus outline provides good contrast against the dark selected background.Also applies to: 125-139
227-252: Depth calculation logic is correct.The
calculateVisualDepthfunction properly walks up the parent chain, correctly excludes root nodes from the depth count, and handles the type guards appropriately for theAPI_HashEntryunion type.
417-426: Excellent accessibility implementation following WAI-ARIA tree pattern.The ARIA attributes are correctly implemented:
role="tree"on the container with a descriptivearia-labelrole="treeitem"on all nodesaria-levelcorrectly uses 1-indexed depth valuesaria-selectedreflects selection statetabIndex={0}enables keyboard navigationThis follows the WAI-ARIA Authoring Practices for tree views.
Also applies to: 529-538, 828-833
463-468: Focus management correctly maintains keyboard navigation context.Moving focus to the wrapper element after click ensures keyboard users can continue navigating from the activated item. The pattern is sound.
Also applies to: 558-563
| onKeyDown={(event) => { | ||
| // Handle keyboard navigation on the focused element (works with NVDA) | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| const button = event.currentTarget.querySelector('button'); | ||
| if (button) { | ||
| button.click(); | ||
| } | ||
| } else if (event.key === 'ArrowLeft' || event.key === 'ArrowRight') { | ||
| event.preventDefault(); | ||
| const button = event.currentTarget.querySelector( | ||
| 'button[aria-expanded]' | ||
| ) as HTMLButtonElement; | ||
| if (button) { | ||
| const isExpanded = button.getAttribute('aria-expanded') === 'true'; | ||
| if ( | ||
| (event.key === 'ArrowRight' && !isExpanded) || | ||
| (event.key === 'ArrowLeft' && isExpanded) | ||
| ) { | ||
| button.click(); | ||
| } | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
Variable shadowing: isExpanded shadows the prop of the same name.
The local isExpanded variable on line 442 shadows the component prop isExpanded from line 267. This can cause confusion during maintenance. Consider renaming the local variable.
🔧 Suggested fix
if (button) {
- const isExpanded = button.getAttribute('aria-expanded') === 'true';
+ const isCurrentlyExpanded = button.getAttribute('aria-expanded') === 'true';
if (
- (event.key === 'ArrowRight' && !isExpanded) ||
- (event.key === 'ArrowLeft' && isExpanded)
+ (event.key === 'ArrowRight' && !isCurrentlyExpanded) ||
+ (event.key === 'ArrowLeft' && isCurrentlyExpanded)
) {
button.click();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onKeyDown={(event) => { | |
| // Handle keyboard navigation on the focused element (works with NVDA) | |
| if (event.key === 'Enter' || event.key === ' ') { | |
| event.preventDefault(); | |
| const button = event.currentTarget.querySelector('button'); | |
| if (button) { | |
| button.click(); | |
| } | |
| } else if (event.key === 'ArrowLeft' || event.key === 'ArrowRight') { | |
| event.preventDefault(); | |
| const button = event.currentTarget.querySelector( | |
| 'button[aria-expanded]' | |
| ) as HTMLButtonElement; | |
| if (button) { | |
| const isExpanded = button.getAttribute('aria-expanded') === 'true'; | |
| if ( | |
| (event.key === 'ArrowRight' && !isExpanded) || | |
| (event.key === 'ArrowLeft' && isExpanded) | |
| ) { | |
| button.click(); | |
| } | |
| } | |
| } | |
| }} | |
| onKeyDown={(event) => { | |
| // Handle keyboard navigation on the focused element (works with NVDA) | |
| if (event.key === 'Enter' || event.key === ' ') { | |
| event.preventDefault(); | |
| const button = event.currentTarget.querySelector('button'); | |
| if (button) { | |
| button.click(); | |
| } | |
| } else if (event.key === 'ArrowLeft' || event.key === 'ArrowRight') { | |
| event.preventDefault(); | |
| const button = event.currentTarget.querySelector( | |
| 'button[aria-expanded]' | |
| ) as HTMLButtonElement; | |
| if (button) { | |
| const isCurrentlyExpanded = button.getAttribute('aria-expanded') === 'true'; | |
| if ( | |
| (event.key === 'ArrowRight' && !isCurrentlyExpanded) || | |
| (event.key === 'ArrowLeft' && isCurrentlyExpanded) | |
| ) { | |
| button.click(); | |
| } | |
| } | |
| } | |
| }} |
🤖 Prompt for AI Agents
In @code/core/src/manager/components/sidebar/Tree.tsx around lines 428 - 451,
The onKeyDown handler creates a local const named isExpanded that shadows the
component prop isExpanded; rename the local variable (e.g., buttonExpanded or
expandedAttr) used to read the button's aria-expanded attribute inside the
ArrowLeft/ArrowRight branch so it no longer conflicts with the prop, update any
uses in that block accordingly, and keep the type cast to HTMLButtonElement for
the queried button.
|
Hi @marvinLaubenstein, could you please give some context as to what issue you're trying to fix on this PR? |
|
@marvinLaubenstein FYI we are working on an entirely new design for the sidebar. I very much appreciate the effort you're going through to improve the existing sidebar, but there's a chance this effort will be wasted as we start working on the next iteration. Please get in touch with me (Steve) on Discord to chat about how you could contribute. I don't want your effort to go to waste! |
|
Hi, sorry — this was supposed to go into the forked project first, that seems to have slipped through. |
2 participants
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
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
Accessibility
Visual Improvements
✏️ Tip: You can customize this high-level summary in your review settings.