Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon A11y: Change default element selector #30253

Merged

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Jan 13, 2025

Closes #

What I did

I have changed the default element selector of @storybook/addon-a11y from #storybook-root to body. I did this mainly to align an a11y test run in Storybook and within an Addon Test run.

In portable stories, we don't have a #storybook-root element; therefore, a11y tests were always running on the document.body element as part of a Vitest run. When a11y tests are running as part of the @storybook/addon-a11y panel in the Storybook UI, the tests were running against #storybook-root. This difference in element selection leads to discrepancies, where, for example, modals or popovers, which are teleported to the document root, were successfully tested as part of a Vitest a11y test run but weren't taken into account when a11y tests were run via the Storybook UI.

#storybook-root was taken as the root element in the past for a11y test runs because it contains other Storybook-specific elements like error-boundary components or addon-docs specific elements. axe, though, doesn't run a11y tests on hidden elements by default and therefore we see it as uncritical to run a11y tests on the document.body instead.

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.8 MB 77.8 MB 0 B 1.1 0%
initSize 131 MB 131 MB 0 B 0.08 0%
diffSize 52.9 MB 52.9 MB 0 B -2 0%
buildSize 7.19 MB 7.19 MB 0 B -1.98 0%
buildSbAddonsSize 1.85 MB 1.85 MB 0 B -2 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.87 MB 1.87 MB 0 B -2 0%
buildSbPreviewSize 0 B 0 B 0 B - -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.91 MB 3.91 MB 0 B -2 0%
buildPreviewSize 3.28 MB 3.28 MB 0 B 2.38 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 6.8s 7.3s 550ms -0.84 7.4%
generateTime 19.3s 20.7s 1.3s -0.08 6.4%
initTime 12.7s 14.3s 1.5s 0.09 10.8%
buildTime 9.3s 8.6s -730ms -1 -8.5%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6s 5s -991ms -0.12 -19.6%
devManagerResponsive 4.3s 3.6s -640ms -0.17 -17.3%
devManagerHeaderVisible 998ms 601ms -397ms -0.52 -66.1%
devManagerIndexVisible 1s 642ms -405ms -0.53 -63.1%
devStoryVisibleUncached 2.2s 2.1s -153ms 0.7 -7.2%
devStoryVisible 1s 680ms -357ms -0.21 -52.5%
devAutodocsVisible 858ms 602ms -256ms 0.23 -42.5%
devMDXVisible 779ms 503ms -276ms -0.74 -54.9%
buildManagerHeaderVisible 801ms 596ms -205ms -0.16 -34.4%
buildManagerIndexVisible 818ms 725ms -93ms 0.31 -12.8%
buildStoryVisible 792ms 567ms -225ms -0.31 -39.7%
buildAutodocsVisible 612ms 478ms -134ms -0.08 -28%
buildMDXVisible 614ms 438ms -176ms -0.67 -40.2%

Greptile Summary

Based on the provided information, I'll create a concise summary of the key changes in this PR:

Changed the default element selector in Storybook's accessibility addon from '#storybook-root' to 'document.body' to ensure consistent testing behavior across environments.

  • Modified code/addons/a11y/src/a11yRunner.ts to use document.body as default element for a11y testing
  • Updated test runner configurations to align with new default element selector
  • Changed documentation examples to reflect new default behavior in accessibility testing
  • Improved testing coverage for teleported elements like modals and popovers
  • Safe change since axe-core ignores hidden elements by default, preventing unwanted testing of Storybook UI elements

Copy link
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.

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

code/addons/a11y/src/a11yRunner.ts Show resolved Hide resolved
Copy link

nx-cloud bot commented Jan 13, 2025

View your CI Pipeline Execution ↗ for commit 6b33750.

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

☁️ Nx Cloud last updated this comment at 2025-01-13 11:24:59 UTC

Copy link

nx-cloud bot commented Jan 13, 2025

View your CI Pipeline Execution ↗ for commit e384651.

Command Status Duration Result
nx run-many -t build -c production --parallel=3 ✅ Succeeded 9s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-13 11:17:00 UTC

@valentinpalkovic valentinpalkovic force-pushed the valentin/run-a11y-tests-on-document-body-per-default branch from e384651 to 7ce27f1 Compare January 13, 2025 11:19
@valentinpalkovic valentinpalkovic force-pushed the valentin/run-a11y-tests-on-document-body-per-default branch from 7ce27f1 to 6b33750 Compare January 13, 2025 11:20
@valentinpalkovic valentinpalkovic merged commit 9057d1f into next Jan 13, 2025
58 of 60 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/run-a11y-tests-on-document-body-per-default branch January 13, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants