Skip to content

Core: Fix aria-controls attribute on sidebar nodes to include all children#31491

Merged
ghengeveld merged 7 commits into
storybookjs:nextfrom
candrepa1:candrepa1/fix-ariaControls-accesibility
Jun 25, 2025
Merged

Core: Fix aria-controls attribute on sidebar nodes to include all children#31491
ghengeveld merged 7 commits into
storybookjs:nextfrom
candrepa1:candrepa1/fix-ariaControls-accesibility

Conversation

@candrepa1
Copy link
Copy Markdown
Contributor

@candrepa1 candrepa1 commented May 15, 2025

Closes #31382

What I did

A parent should have the list of all children in aria-controls. Currently, only the first child is being used.

I did not see any testing for accessibility, so I'm unsure if i must add an unit test for this change.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Before
Screenshot 2025-05-15 at 12 02 54 PM

After
Screenshot 2025-05-15 at 11 59 26 AM

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment thread code/core/src/manager/components/sidebar/Tree.tsx Outdated
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Copy Markdown
Contributor

@Sidnioulz Sidnioulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valentinpalkovic @ghengeveld, my understanding of this issue is that:

  • it is an accessibility improvements, as currently only the first child is linked to the section button instead of all children
  • but it does not close the linked issue which hints that some IDs are printed for child buttons that aren't reachable in the DOM, so there could be a separate bug for which we would want a repro repository
  • worth noting that both the existing and proposed code may be challenged by #31267 which seeks to address the navigability of the sidebar more widely; it might be nicer to end users of we use proper list nesting instead of creating detached DOM elements that need an aria-controls attribute

@Sidnioulz
Copy link
Copy Markdown
Contributor

@valentinpalkovic I've circled back with a11y experts and the general consensus is that aria-controls is virtually useless. Most ATs have dropped support for it because it was too often misused and degraded a11y. So, while we can merge this PR for the few ATs that do use it, we shouldn't consider it an a11y improvement worth communicating on.

@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented May 26, 2025

View your CI Pipeline Execution ↗ for commit 8de88ca.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 25s View ↗

☁️ Nx Cloud last updated this comment at 2025-05-26 13:33:41 UTC

@Sidnioulz Sidnioulz force-pushed the candrepa1/fix-ariaControls-accesibility branch from 8de88ca to c2e6661 Compare May 26, 2025 14:48
@Sidnioulz
Copy link
Copy Markdown
Contributor

@candrepa1 don't worry too much about upgrading your branch :) The maintainers will take care of it if needed when they get around to merging :) Things are a bit busy right now with the Storybook 9 release but you haven't been forgotten.

@ghengeveld ghengeveld moved this to Empathy Queue (prioritized) in Core Team Projects Jun 23, 2025
@ghengeveld ghengeveld changed the title fix(accessibility): parents of components/groups should have all children explicitly set in aria-controls Core: Fix aria-controls attribute on sidebar nodes to include all children Jun 25, 2025
@ghengeveld ghengeveld enabled auto-merge June 25, 2025 15:26
@ghengeveld ghengeveld disabled auto-merge June 25, 2025 15:26
@ghengeveld ghengeveld merged commit 3a5a40b into storybookjs:next Jun 25, 2025
51 checks passed
@github-project-automation github-project-automation Bot moved this from Empathy Queue (prioritized) to Done in Core Team Projects Jun 25, 2025
@ghengeveld ghengeveld added the maintenance User-facing maintenance tasks label Jun 25, 2025
@github-actions github-actions Bot mentioned this pull request Jun 25, 2025
71 tasks
@storybook-app-bot
Copy link
Copy Markdown

Package Benchmarks

Commit: 6da6852, ran on 25 June 2025 at 15:36:34 UTC

The following packages have significant changes to their size or dependencies:

@storybook/addon-actions

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 580 B 🚨 +580 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-backgrounds

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 596 B 🚨 +596 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-controls

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 584 B 🚨 +584 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-measure

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 580 B 🚨 +580 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-outline

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 580 B 🚨 +580 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-toolbars

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 584 B 🚨 +584 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-viewport

Before After Difference
Dependency count 0 0 0
Self size 0 B 3 KB 🚨 +3 KB 🚨
Dependency size 0 B 584 B 🚨 +584 B 🚨
Bundle Size Analyzer Link Link

@storybook/addon-vitest

Before After Difference
Dependency count 6 6 0
Self size 550 KB 630 KB 🚨 +80 KB 🚨
Dependency size 1.49 MB 1.49 MB 0 B
Bundle Size Analyzer Link Link

storybook

Before After Difference
Dependency count 51 49 🎉 -2 🎉
Self size 31.72 MB 31.38 MB 🎉 -341 KB 🎉
Dependency size 17.43 MB 17.41 MB 🎉 -22 KB 🎉
Bundle Size Analyzer Link Link

@storybook/angular

Before After Difference
Dependency count 199 208 🚨 +9 🚨
Self size 191 KB 606 KB 🚨 +415 KB 🚨
Dependency size 29.72 MB 29.81 MB 🚨 +83 KB 🚨
Bundle Size Analyzer Link Link

@storybook/ember

Before After Difference
Dependency count 205 208 🚨 +3 🚨
Self size 28 KB 28 KB 🚨 +227 B 🚨
Dependency size 28.95 MB 28.97 MB 🚨 +21 KB 🚨
Bundle Size Analyzer Link Link

@storybook/nextjs

Before After Difference
Dependency count 537 540 🚨 +3 🚨
Self size 903 KB 216 KB 🎉 -687 KB 🎉
Dependency size 59.04 MB 59.06 MB 🚨 +17 KB 🚨
Bundle Size Analyzer Link Link

@storybook/nextjs-vite

Before After Difference
Dependency count 131 127 🎉 -4 🎉
Self size 3.08 MB 2.39 MB 🎉 -685 KB 🎉
Dependency size 22.31 MB 22.14 MB 🎉 -171 KB 🎉
Bundle Size Analyzer Link Link

@storybook/react-native-web-vite

Before After Difference
Dependency count 162 161 🎉 -1 🎉
Self size 35 KB 35 KB 🎉 -139 B 🎉
Dependency size 23.32 MB 23.30 MB 🎉 -16 KB 🎉
Bundle Size Analyzer Link Link

@storybook/react-vite

Before After Difference
Dependency count 121 120 🎉 -1 🎉
Self size 32 KB 32 KB 🎉 -107 B 🎉
Dependency size 20.25 MB 20.24 MB 🎉 -15 KB 🎉
Bundle Size Analyzer Link Link

@storybook/react-webpack5

Before After Difference
Dependency count 286 284 🎉 -2 🎉
Self size 25 KB 25 KB 0 B
Dependency size 43.71 MB 43.69 MB 🎉 -20 KB 🎉
Bundle Size Analyzer Link Link

sb

Before After Difference
Dependency count 52 50 🎉 -2 🎉
Self size 1 KB 1 KB 🎉 -20 B 🎉
Dependency size 49.15 MB 48.78 MB 🎉 -363 KB 🎉
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 218 324 🚨 +106 🚨
Self size 582 KB 244 KB 🎉 -338 KB 🎉
Dependency size 94.80 MB 98.17 MB 🚨 +3.37 MB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 187 267 🚨 +80 🚨
Self size 31 KB 31 KB 🚨 +152 B 🚨
Dependency size 78.90 MB 82.57 MB 🚨 +3.67 MB 🚨
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 1 1 0
Self size 12.50 MB 11.02 MB 🎉 -1.49 MB 🎉
Dependency size 98 KB 98 KB 0 B
Bundle Size Analyzer Link Link

@storybook/preset-react-webpack

Before After Difference
Dependency count 177 175 🎉 -2 🎉
Self size 24 KB 24 KB 🎉 -171 B 🎉
Dependency size 30.53 MB 30.52 MB 🎉 -15 KB 🎉
Bundle Size Analyzer Link Link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility ci:normal Run our default set of CI jobs (choose this for most PRs). maintenance User-facing maintenance tasks ui

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: Accessibility Issue - aria-controls in sidebar not connected to any HTML element

5 participants