UI: Make disabled Buttons keyboard-focusable#34166
Conversation
|
View your CI Pipeline Execution ↗ for commit 609f377
☁️ Nx Cloud last updated this comment at |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced native disabled checks with ARIA-based checks: Button now uses an internal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
📝 Coding Plan
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
🧹 Nitpick comments (2)
code/core/src/components/components/Button/Button.stories.tsx (1)
289-321: Well-structured play function covering key accessibility requirements.The test steps verify:
aria-disabledattribute is set correctly- Click handler is not invoked when disabled
- Button remains focusable for keyboard accessibility
Minor note: Line 316 redeclares
buttonwhich shadows the outer variable from line 304. This works but is redundant.♻️ Optional cleanup
await step('Disabled button should be focusable for accessibility', async () => { - const button = canvas.getByRole('button', { name: 'Disabled Button' }); button.focus(); expect(button).toHaveFocus(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/Button/Button.stories.tsx` around lines 289 - 321, In the Disabled story's play function remove the redundant redeclaration of the button variable that shadows the outer one: use the existing const button = canvas.getByRole('button', { name: 'Disabled Button' }) declared earlier in play rather than declaring another button inside the final step; update the final step to reuse the outer button (and keep the focus/assertion logic unchanged) in the play function of the Disabled story to avoid shadowing.code/core/src/components/components/Button/Button.tsx (1)
263-317: Consider disabling hover/active styles for disabled buttons.The hover/active/focus-visible styles are applied when
!readOnlybut don't account for$disabled. Disabled buttons will show hover background changes, though they do havecursor: not-allowedandopacity: 0.5as visual indicators.This is likely acceptable since the visual indicators are sufficient, but for consistency with
readOnlybehavior, you could extend the condition:♻️ Optional: Disable hover styles for disabled buttons
- ...(!readOnly && { + ...(!readOnly && !$disabled && { '&:hover': {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/Button/Button.tsx` around lines 263 - 317, The hover/active/focus-visible block currently gated only by !readOnly should also skip when the button is disabled; update the conditional around that style block in Button.tsx to check both !readOnly and that the disabled flag/prop (e.g., $disabled or disabled) is false, and ensure the same gating applies to the '&:hover', '&:active' and '&:focus-visible' entries (and the '.sb-bar &:focus-visible, .sb-list &:focus-visible' override) so disabled buttons do not receive hover/active/focus-visible visual changes while keeping cursor: not-allowed and opacity intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts`:
- Around line 241-242: The assertion for the test button is currently written
without awaiting the Playwright assertion and ends with a comma, so it never
executes; update the assertion that checks watchModeButton to prepend await to
the expect(...) call and replace the trailing comma with a semicolon so the
async assertion runs (i.e., await
expect(watchModeButton).toHaveAttribute("aria-disabled", "true");). Ensure this
change is applied where watchModeButton is asserted in the
component-testing.spec.ts test.
---
Nitpick comments:
In `@code/core/src/components/components/Button/Button.stories.tsx`:
- Around line 289-321: In the Disabled story's play function remove the
redundant redeclaration of the button variable that shadows the outer one: use
the existing const button = canvas.getByRole('button', { name: 'Disabled Button'
}) declared earlier in play rather than declaring another button inside the
final step; update the final step to reuse the outer button (and keep the
focus/assertion logic unchanged) in the play function of the Disabled story to
avoid shadowing.
In `@code/core/src/components/components/Button/Button.tsx`:
- Around line 263-317: The hover/active/focus-visible block currently gated only
by !readOnly should also skip when the button is disabled; update the
conditional around that style block in Button.tsx to check both !readOnly and
that the disabled flag/prop (e.g., $disabled or disabled) is false, and ensure
the same gating applies to the '&:hover', '&:active' and '&:focus-visible'
entries (and the '.sb-bar &:focus-visible, .sb-list &:focus-visible' override)
so disabled buttons do not receive hover/active/focus-visible visual changes
while keeping cursor: not-allowed and opacity intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbd3405a-234b-4fb2-93cd-baaf1251aafc
📒 Files selected for processing (6)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsxcode/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
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)
test-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.ts (1)
47-54:⚠️ Potential issue | 🟠 MajorHarden restore flow to avoid cross-test contamination on partial restore failure.
If
fs.writeFilefails for one entry (Line 49), the loop aborts andmodifiedFiles.clear()never runs (Line 51). That can leak modified files into subsequent tests.Suggested fix
const restoreAllFiles = async () => { - for (const [filePath, originalContent] of modifiedFiles.entries()) { - await fs.writeFile(filePath, originalContent); - } - modifiedFiles.clear(); + const errors: Array<{ filePath: string; error: unknown }> = []; + try { + for (const [filePath, originalContent] of modifiedFiles.entries()) { + try { + await fs.writeFile(filePath, originalContent); + } catch (error) { + errors.push({ filePath, error }); + } + } + } finally { + modifiedFiles.clear(); + } + + if (errors.length) { + throw new Error(`Failed to restore ${errors.length} modified file(s)`); + } // the file change causes a HMR event, which causes a browser reload,and that can take a few seconds await new Promise((resolve) => setTimeout(resolve, 2000)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.ts` around lines 47 - 54, The restoreAllFiles function can abort on a single fs.writeFile failure and never clear modifiedFiles, leaking state; change its implementation to attempt writing all entries even if some fail (either by iterating with per-file try/catch or using Promise.allSettled over write promises), collect or aggregate any errors, and ensure modifiedFiles.clear() always runs (put clear and the HMR delay in a finally block) so subsequent tests aren’t contaminated; if writes failed, surface an aggregated error or log the failures after clearing the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.ts`:
- Around line 138-144: The click that expands the story isn’t awaited, causing
race conditions; update the expansion step by awaiting the interaction on the
testStoryElement (i.e., replace the unawaited testStoryElement.click() with an
awaited interaction) and optionally ensure the element’s expanded state by
awaiting getAttribute('aria-expanded') or using a wait-for condition after the
awaited click to guarantee expansion before proceeding.
---
Outside diff comments:
In
`@test-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.ts`:
- Around line 47-54: The restoreAllFiles function can abort on a single
fs.writeFile failure and never clear modifiedFiles, leaking state; change its
implementation to attempt writing all entries even if some fail (either by
iterating with per-file try/catch or using Promise.allSettled over write
promises), collect or aggregate any errors, and ensure modifiedFiles.clear()
always runs (put clear and the HMR delay in a finally block) so subsequent tests
aren’t contaminated; if writes failed, surface an aggregated error or log the
failures after clearing the map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97208389-92f5-46c1-ae4d-726939741f8f
📒 Files selected for processing (2)
code/core/src/manager/components/sidebar/TagsFilter.stories.tsxtest-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.ts
| const testStoryElement = await page.getByRole('button', { | ||
| name: 'Test', | ||
| exact: true, | ||
| }); | ||
| if ((await testStoryElement.getAttribute("aria-expanded")) !== "true") { | ||
| if ((await testStoryElement.getAttribute('aria-expanded')) !== 'true') { | ||
| testStoryElement.click(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify un-awaited Playwright action calls in spec files.
rg -nP --type=ts -C2 '(?<!await\s)\b\w+\.(click|check|uncheck|fill|hover|press)\(' -g '*.spec.ts'Repository: storybookjs/storybook
Length of output: 9135
🏁 Script executed:
sed -n '130,150p' test-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.tsRepository: storybookjs/storybook
Length of output: 880
Await the expansion click to prevent flaky ordering.
Line 144: testStoryElement.click() is not awaited. The next operations can run before the expansion completes.
Suggested fix
if ((await testStoryElement.getAttribute('aria-expanded')) !== 'true') {
- testStoryElement.click();
+ await testStoryElement.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.
| const testStoryElement = await page.getByRole('button', { | |
| name: 'Test', | |
| exact: true, | |
| }); | |
| if ((await testStoryElement.getAttribute("aria-expanded")) !== "true") { | |
| if ((await testStoryElement.getAttribute('aria-expanded')) !== 'true') { | |
| testStoryElement.click(); | |
| } | |
| const testStoryElement = await page.getByRole('button', { | |
| name: 'Test', | |
| exact: true, | |
| }); | |
| if ((await testStoryElement.getAttribute('aria-expanded')) !== 'true') { | |
| await testStoryElement.click(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.ts`
around lines 138 - 144, The click that expands the story isn’t awaited, causing
race conditions; update the expansion step by awaiting the interaction on the
testStoryElement (i.e., replace the unawaited testStoryElement.click() with an
awaited interaction) and optionally ensure the element’s expanded state by
awaiting getAttribute('aria-expanded') or using a wait-for condition after the
awaited click to guarantee expansion before proceeding.
Sidnioulz
force-pushed
the
sidnioulz/issue-31678-aria-disabled-finish
branch
from
a604b3d to
92c9867
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts (1)
241-242:⚠️ Potential issue | 🔴 CriticalAwait this assertion and remove the comma operator.
Line 242 is currently not awaited and is comma-chained, so the disabled-state check can be bypassed/flaky.
Suggested patch
- // The test button will be disabled as tests are running - expect(watchModeButton).toHaveAttribute("aria-disabled", "true"), + // The test button will be disabled as tests are running + await expect(watchModeButton).toHaveAttribute("aria-disabled", "true");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts` around lines 241 - 242, The assertion for the disabled state is currently comma-chained and not awaited, which can make the check flaky; change the expression so you await the assertion on watchModeButton (await expect(watchModeButton).toHaveAttribute("aria-disabled","true")) and remove the trailing comma operator so the test actually waits for the attribute check to complete.
🧹 Nitpick comments (1)
code/renderers/react/template/stories/test-fn.stories.tsx (1)
81-82: UsetoBeEnabled()for this native button test path.Line 81 and Line 91 only check that
aria-disabledis not"true", which can still pass if the nativedisabledattribute is set. Since this file uses a plain<button>,toBeEnabled()is the stronger assertion.Suggested patch
- await expect(button).not.toHaveAttribute('aria-disabled', 'true'); + await expect(button).toBeEnabled(); ... - await expect(button).not.toHaveAttribute('aria-disabled', 'true'); + await expect(button).toBeEnabled();Also applies to: 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/template/stories/test-fn.stories.tsx` around lines 81 - 82, Replace the weak attribute assertion with the stronger DOM-enabled assertion in the test: change the two occurrences where the test does await expect(button).not.toHaveAttribute('aria-disabled', 'true') to await expect(button).toBeEnabled() so the plain <button> is verified by its actual enabled/disabled state; update both occurrences in test-fn.stories.tsx (the assertions referencing the button element) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/component-testing/components/test-fn.stories.tsx`:
- Around line 99-100: Replace the three assertions that check aria-disabled on
the native <button> element with the proper enabled-state matcher: locate the
assertions using the button variable (the lines containing await
expect(button).not.toHaveAttribute('aria-disabled', 'true')) and change them to
use await expect(button).toBeEnabled() so the test verifies the native disabled
behavior (respecting the disabled attribute and disabled fieldset ancestors)
rather than the aria-disabled attribute.
---
Duplicate comments:
In
`@test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts`:
- Around line 241-242: The assertion for the disabled state is currently
comma-chained and not awaited, which can make the check flaky; change the
expression so you await the assertion on watchModeButton (await
expect(watchModeButton).toHaveAttribute("aria-disabled","true")) and remove the
trailing comma operator so the test actually waits for the attribute check to
complete.
---
Nitpick comments:
In `@code/renderers/react/template/stories/test-fn.stories.tsx`:
- Around line 81-82: Replace the weak attribute assertion with the stronger
DOM-enabled assertion in the test: change the two occurrences where the test
does await expect(button).not.toHaveAttribute('aria-disabled', 'true') to await
expect(button).toBeEnabled() so the plain <button> is verified by its actual
enabled/disabled state; update both occurrences in test-fn.stories.tsx (the
assertions referencing the button element) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c45a6947-e854-4cf4-919e-9669f81a49cf
📒 Files selected for processing (8)
code/core/src/component-testing/components/test-fn.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/shared/checklist-store/checklistData.tsxcode/renderers/react/template/stories/test-fn.stories.tsxdocs/api/csf/csf-next.mdxdocs/writing-tests/interaction-testing.mdxtest-storybooks/portable-stories-kitchen-sink/react-vitest-3/e2e-tests/component-testing.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
| await expect(button).not.toHaveAttribute('aria-disabled', 'true'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the target file
cat -n code/core/src/component-testing/components/test-fn.stories.tsx | head -130 | tail -40Repository: storybookjs/storybook
Length of output: 1609
🏁 Script executed:
# Read the beginning of the file to understand the component being tested
head -90 code/core/src/component-testing/components/test-fn.stories.tsx | tail -50Repository: storybookjs/storybook
Length of output: 1293
🏁 Script executed:
# Read from the very top of the file to find imports and component definitions
head -60 code/core/src/component-testing/components/test-fn.stories.tsxRepository: storybookjs/storybook
Length of output: 1474
🏁 Script executed:
# Check the test framework setup - look for jest-dom or testing-library matchers
grep -r "toBeEnabled\|jest-dom\|@testing-library" code/core/src/component-testing --include="*.ts" --include="*.tsx" -A 2 -B 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check what's exported from storybook/test
find code -name "*.ts" -o -name "*.tsx" | xargs grep -l "export.*expect\|from.*storybook/test" | head -5Repository: storybookjs/storybook
Length of output: 360
🏁 Script executed:
# Find the storybook/test export
find code -path "*storybook/test*" -type f | head -10Repository: storybookjs/storybook
Length of output: 153
🏁 Script executed:
# Search for storybook/test definition
rg "export.*expect|vitest|@vitest" code --type ts --type tsx -A 3 | head -50Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Search for storybook/test setup
rg "export.*expect" code -g "*.ts" -g "*.tsx" -A 3 | head -80Repository: storybookjs/storybook
Length of output: 691
🏁 Script executed:
# Check the test/index.ts to see what rawExpect is
cat code/core/src/test/index.tsRepository: storybookjs/storybook
Length of output: 1658
🏁 Script executed:
# Check expect.ts to see what's exported
cat code/core/src/test/expect.tsRepository: storybookjs/storybook
Length of output: 4374
🏁 Script executed:
# Verify toBeEnabled is part of `@testing-library/jest-dom` matchers
grep -r "toBeEnabled" code --include="*.ts" --include="*.tsx" -B 2 -A 2 | head -40Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check package.json for `@testing-library/jest-dom` version
grep -r "@testing-library/jest-dom" code --include="package.json"Repository: storybookjs/storybook
Length of output: 191
🌐 Web query:
@testing-library/jest-dom 6.9.1 toBeEnabled matcher
💡 Result:
In @testing-library/jest-dom v6.9.1, toBeEnabled() is available and is defined as the logical inverse of toBeDisabled() (i.e. it’s equivalent to expect(element).not.toBeDisabled() but avoids double-negation). It considers an element disabled when it’s a disable-able form control with the disabled attribute or it’s inside a disabled ancestor that can disable descendants (e.g. a <fieldset disabled>), and therefore considers it enabled when those conditions are not true. It does not take aria-disabled into account. [1]
Example:
import '@testing-library/jest-dom'
expect(screen.getByRole('button')).toBeEnabled()The 6.9.1 release itself is a small bugfix release (unrelated to toBeEnabled behavior). [2]
Sources: [1] [2]
Replace aria-disabled assertions with toBeEnabled() for native buttons.
Lines 99, 109, and 123 check not.toHaveAttribute('aria-disabled', 'true'), but for native <button> elements, the actual enabled state is determined by the native disabled attribute or disabled fieldset ancestor, not the aria-disabled accessibility attribute. Use toBeEnabled() instead to verify the button's actual enabled behavior.
Suggested patch
- await expect(button).not.toHaveAttribute('aria-disabled', 'true');
+ await expect(button).toBeEnabled();
...
- await expect(button).not.toHaveAttribute('aria-disabled', 'true');
+ await expect(button).toBeEnabled();
...
- await expect(button).not.toHaveAttribute('aria-disabled', 'true');
+ await expect(button).toBeEnabled();📝 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.
| await expect(button).not.toHaveAttribute('aria-disabled', 'true'); | |
| } | |
| await expect(button).toBeEnabled(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/component-testing/components/test-fn.stories.tsx` around lines
99 - 100, Replace the three assertions that check aria-disabled on the native
<button> element with the proper enabled-state matcher: locate the assertions
using the button variable (the lines containing await
expect(button).not.toHaveAttribute('aria-disabled', 'true')) and change them to
use await expect(button).toBeEnabled() so the test verifies the native disabled
behavior (respecting the disabled attribute and disabled fieldset ancestors)
rather than the aria-disabled attribute.
4 participants
Closes #31678
What I did
Finishing up #32325 as it came up in the SB 10.3 QA.
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
Bug Fixes
Tests
Documentation