bug: Replace disabled attr with aria-disabled in Button#32325
bug: Replace disabled attr with aria-disabled in Button#32325WioletaKolodziej wants to merge 4 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Sidnioulz
changed the title
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @WioletaKolodziej!
Let's first start with addressing the feedback from Greptile, and with fixing the PR description. Please copy the PR template that you deleted and please fill it in carefully. It helps us speed up reviews.
You'll also need to address the following:
- Keep the Button API stable, it's a public component and there's no need for a breaking change here; map the
disabledprop toaria-disabledwithout changing the public API - Update Button.stories.tsx to ensure a disabled story exists; in that story, add a play function that proves the button has the aria-disabled attribute and that clicking it does not trigger
onClick - Add a story where the button is
rendered with two sibling buttons, and add a play function showing that pressingTabafter focusing the previous button moves focus to the aria-disabled button (which is what we want to fix here) - Check if any existing test or story uses
toBeDisabledfor this button, and if so, switch totoHaveAttribute('aria-disabled', true)
PR template
Closes #
<!-- If your PR is related to an issue, provide the number(s) above; if it resolves multiple issues, be sure to break them up (e.g. "closes #1000, closes #1001"). -->
<!--
Thank you for contributing to Storybook! Please submit all PRs to the `next` branch unless they are specific to the current release. Storybook maintainers cherry-pick bug and documentation fixes into the `main` branch as part of the release process, so you shouldn't need to worry about this. For additional guidance: https://storybook.js.org/docs/contribute
-->
## What I did
<!-- Briefly describe what your PR does -->
## Checklist for Contributors
### Testing
<!-- Please check (put an "x" inside the "[ ]") the applicable items below to communicate how to test your changes -->
#### 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!_
<!-- Please include the steps to test your changes here. For example:
1. Run a sandbox for template, e.g. `yarn task --task sandbox --start-from auto --template react-vite/default-ts`
2. Open Storybook in your browser
3. Access X story
-->
### Documentation
<!-- Please check (put an "x" inside the "[ ]") the applicable items below to indicate which documentation has been updated. -->
- [ ] Add or update documentation reflecting your changes
- [ ] If you are deprecating/removing a feature, make sure to update
[MIGRATION.MD](https://github.com/storybookjs/storybook/blob/next/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:
<details>
<summary>Available labels</summary>
- `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.
</details>
### 🦋 Canary release
<!-- CANARY_RELEASE_SECTION -->
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](https://github.com/storybookjs/storybook/actions/workflows/canary-release-pr.yml) or locally with `gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>`_
<!-- CANARY_RELEASE_SECTION -->
<!-- BENCHMARK_SECTION -->
<!-- BENCHMARK_SECTION -->
Thanks!
WioletaKolodziej
force-pushed
the
fix/31678-aria-disabled-button
branch
from
b2b6297 to
9b937f8
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 2.07 MB | 2.01 MB | 🎉 -61 KB 🎉 |
| Dependency size | 10.21 MB | 10.20 MB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 11 | 11 | 0 |
| Self size | 330 KB | 374 KB | 🚨 +44 KB 🚨 |
| Dependency size | 1.30 MB | 1.30 MB | 🎉 -27 B 🎉 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 48 | 🚨 +5 🚨 |
| Self size | 30.19 MB | 30.62 MB | 🚨 +432 KB 🚨 |
| Dependency size | 17.31 MB | 17.65 MB | 🚨 +346 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 14 | 14 | 0 |
| Self size | 23 KB | 23 KB | 🎉 -2 B 🎉 |
| Dependency size | 1.67 MB | 1.71 MB | 🚨 +44 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 532 | 532 | 0 |
| Self size | 950 KB | 928 KB | 🎉 -22 KB 🎉 |
| Dependency size | 58.55 MB | 58.52 MB | 🎉 -28 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 130 | 🚨 +6 🚨 |
| Self size | 4.10 MB | 4.00 MB | 🎉 -96 KB 🎉 |
| Dependency size | 21.63 MB | 21.68 MB | 🚨 +47 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 14 | 14 | 0 |
| Self size | 14 KB | 14 KB | 🚨 +15 B 🚨 |
| Dependency size | 1.65 MB | 1.70 MB | 🚨 +44 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 157 | 163 | 🚨 +6 🚨 |
| Self size | 31 KB | 31 KB | 🎉 -14 B 🎉 |
| Dependency size | 23.02 MB | 23.07 MB | 🚨 +47 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 114 | 120 | 🚨 +6 🚨 |
| Self size | 37 KB | 36 KB | 🎉 -919 B 🎉 |
| Dependency size | 19.57 MB | 19.62 MB | 🚨 +48 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 272 | 272 | 0 |
| Self size | 25 KB | 25 KB | 🎉 -3 B 🎉 |
| Dependency size | 43.51 MB | 43.48 MB | 🎉 -28 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 19 | 19 | 0 |
| Self size | 59 KB | 58 KB | 🎉 -1 KB 🎉 |
| Dependency size | 26.79 MB | 26.83 MB | 🚨 +45 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/sveltekit
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 20 | 20 | 0 |
| Self size | 58 KB | 48 KB | 🎉 -10 KB 🎉 |
| Dependency size | 26.85 MB | 26.89 MB | 🚨 +44 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 109 | 110 | 🚨 +1 🚨 |
| Self size | 38 KB | 38 KB | 🎉 -290 B 🎉 |
| Dependency size | 43.78 MB | 43.83 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 15 | 15 | 0 |
| Self size | 20 KB | 20 KB | 🚨 +14 B 🚨 |
| Dependency size | 1.70 MB | 1.74 MB | 🚨 +44 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 220 | 🚨 +33 🚨 |
| Self size | 921 KB | 879 KB | 🎉 -43 KB 🎉 |
| Dependency size | 79.87 MB | 82.22 MB | 🚨 +2.35 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 189 | 🚨 +20 🚨 |
| Self size | 35 KB | 35 KB | 🎉 -14 B 🎉 |
| Dependency size | 76.30 MB | 77.29 MB | 🚨 +997 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 49 | 🚨 +5 🚨 |
| Self size | 1.55 MB | 1.52 MB | 🎉 -29 KB 🎉 |
| Dependency size | 47.50 MB | 48.27 MB | 🚨 +778 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/react-dom-shim
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 22 KB | 12 KB | 🎉 -10 KB 🎉 |
| Dependency size | 788 B | 785 B | 🎉 -3 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 68 | 68 | 0 |
| Self size | 36 KB | 26 KB | 🎉 -10 KB 🎉 |
| Dependency size | 5.98 MB | 5.98 MB | 🎉 -3 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 21 KB | 26 KB | 🚨 +6 KB 🚨 |
| Dependency size | 31.00 MB | 31.00 MB | 🎉 -9 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 894 KB | 872 KB | 🎉 -23 KB 🎉 |
| Dependency size | 28 KB | 18 KB | 🎉 -10 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
|
@Sidnioulz It’s important to note that even if our Button Interface handles disabled, this does not guarantee that the native HTML disabled attribute won’t end up in the DOM. Since disabled is a standard HTML attribute, React will always forward it – whether through ...props, styled-components, or simply if someone uses a plain button somewhere in the code. As a result, our Button.tsx is not resilient to such mistakes, because disabled can still leak directly into the DOM. Approach is to rely on aria-disabled, but instead of exposing disabled in the API, we should consider a custom prop name like isDisabled (or similar) that prevents the native attribute from being accidentally passed through anyway. |
That's a legitimate concern, but I don't share your analysis. The way I see it, you're arguing in favour of the Button with Button with I'm not a fan of
|
WioletaKolodziej
force-pushed
the
fix/31678-aria-disabled-button
branch
from
9b937f8 to
2824a75
Compare
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for revisiting this, @WioletaKolodziej! I've added a few comments for you. Please let me know if you need any help or clarification!
|
Hey @Sidnioulz, Can you verify whether any of these changes have already been made on the |
This is a separate issue that we haven't been addressing on the a11y branch :) It should stay open. |
Co-authored-by: Steve Dodier-Lazaro <Sidnioulz@users.noreply.github.com>
Co-authored-by: Steve Dodier-Lazaro <Sidnioulz@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughTests now assert aria-disabled instead of native disabled checks. The Button component omits native Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Button as Button (component)
participant Handler as onClick handler
User->>Button: Click
alt aria-disabled == "true"
rect #f8f3e6
Note right of Button: accessibility state set\npreventDefault + stopPropagation
Button->>Button: preventDefault & stopPropagation
Note right of Button: handler NOT invoked
end
else aria-disabled == "false"
Button->>Handler: invoke onClick(event)
Handler-->>Button: handler completes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx(1 hunks)code/core/src/components/components/Button/Button.stories.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(5 hunks)code/e2e-tests/addon-toolbars.spec.ts(1 hunks)code/e2e-tests/addon-viewport.spec.ts(2 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/e2e-tests/addon-toolbars.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/e2e-tests/addon-toolbars.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/e2e-tests/addon-viewport.spec.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/e2e-tests/addon-toolbars.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/e2e-tests/addon-toolbars.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
test-storybooks/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain test Storybook configurations under
test-storybooks/for E2E and visual testing scenarios
Files:
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
🧬 Code graph analysis (2)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx (1)
code/core/src/test/testing-library.ts (1)
userEvent(119-122)
code/core/src/components/components/Button/Button.stories.tsx (2)
code/core/src/components/components/Button/Button.tsx (2)
ButtonProps(8-17)Button(19-85)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Row(80-84)
🪛 Biome (2.1.2)
code/core/src/components/components/Button/Button.tsx
[error] 28-28: Duplicate parameter name.
The parameter overrides a preceding parameter by using the same name.
(lint/suspicious/noDuplicateParameters)
[error] 28-28: Shouldn't redeclare 'disabled'. Consider to delete it or rename it.
'disabled' is defined here:
(lint/suspicious/noRedeclare)
[error] 96-97: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '>'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (4)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx (1)
34-34: LGTM - Correctly checking for absence of aria-disabled.The assertion correctly checks that the
aria-disabledattribute is not present when the button becomes enabled. This aligns with the previous review feedback and accessibility best practices.code/e2e-tests/addon-viewport.spec.ts (1)
57-57: LGTM - Correct aria-disabled assertion.The assertion correctly checks the
aria-disabledattribute with the string value"true".code/core/src/components/components/Button/Button.stories.tsx (1)
161-169: Consider following previous review feedback on story structure.The previous review comment suggested:
- Removing the
renderfunction- Adding a
playfunction to demonstrate the button is not clickable but focusable- Creating a separate
DisabledVariantsstory with all variantsThe current implementation includes a
renderoverride and anonClickhandler. While the type constraint usingsatisfies ButtonPropsis good for type safety, consider whether the custom render is necessary or if the suggestions from the previous review would better demonstrate the disabled state behavior.Based on past review feedback, would you like to:
- Remove the render function and add a play function instead?
- Create a DisabledVariants story showing all button variants in disabled state?
code/core/src/components/components/Button/Button.tsx (1)
44-48: LGTM - Click handler correctly prevents interaction when disabled.The click handler properly guards against the disabled state by preventing default behavior and stopping propagation, which prevents the button from being clicked when disabled while still allowing it to be focusable.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx (3)
26-26: Use string value for aria-disabled attribute.The
aria-disabledattribute is a string, so you should pass'true'as a string rather than the booleantrue.Apply this diff:
- await expect(button).toHaveAttribute('aria-disabled', true); + await expect(button).toHaveAttribute('aria-disabled', 'true');
31-31: Use string value for aria-disabled attribute.The
aria-disabledattribute is a string, so you should pass'true'as a string rather than the booleantrue.Apply this diff:
- await expect(button).toHaveAttribute('aria-disabled', true); + await expect(button).toHaveAttribute('aria-disabled', 'true');
39-39: Use string value for aria-disabled attribute.The
aria-disabledattribute is a string, so you should pass'true'as a string rather than the booleantrue.Apply this diff:
- await expect(button).toHaveAttribute('aria-disabled', true); + await expect(button).toHaveAttribute('aria-disabled', 'true');code/core/src/components/components/Button/Button.tsx (1)
75-76: Remove native disabled attribute to preserve focusability.The PR objectives state that buttons should remain focusable and in the accessibility tree. However, line 76 still passes
disabled={disabled}to the underlying button element, which sets the nativedisabledattribute. This prevents keyboard navigation and contradicts the PR goals.Apply this diff:
aria-disabled={disabled} - disabled={disabled} active={active}
🧹 Nitpick comments (1)
code/e2e-tests/addon-toolbars.spec.ts (1)
35-35: Check aria-disabled attribute value for precision.The current assertion only checks for the presence of the
aria-disabledattribute. For better precision, verify that the attribute value is'true'to confirm the button is actually disabled.Apply this diff:
- await expect(button).toHaveAttribute('aria-disabled'); + await expect(button).toHaveAttribute('aria-disabled', 'true');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx(1 hunks)code/core/src/components/components/Button/Button.stories.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(5 hunks)code/e2e-tests/addon-toolbars.spec.ts(1 hunks)code/e2e-tests/addon-viewport.spec.ts(2 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.tsx
test-storybooks/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain test Storybook configurations under
test-storybooks/for E2E and visual testing scenarios
Files:
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.tsx
🧬 Code graph analysis (2)
code/core/src/components/components/Button/Button.stories.tsx (2)
code/core/src/components/components/Button/Button.tsx (2)
ButtonProps(8-17)Button(19-85)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Row(80-84)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx (1)
code/core/src/test/testing-library.ts (1)
userEvent(119-122)
🪛 Biome (2.1.2)
code/core/src/components/components/Button/Button.tsx
[error] 28-28: Duplicate parameter name.
The parameter overrides a preceding parameter by using the same name.
(lint/suspicious/noDuplicateParameters)
[error] 28-28: Shouldn't redeclare 'disabled'. Consider to delete it or rename it.
'disabled' is defined here:
(lint/suspicious/noRedeclare)
[error] 96-97: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '>'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (6)
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts (1)
217-217: Verify aria-disabled attribute value type.Playwright's
toHaveAttributeexpects a string value. Sincearia-disabledis a string attribute, you should pass'true'as a string rather than the booleantrue.Apply this diff:
- expect(watchModeButton).toHaveAttribute("aria-disabled", true), + expect(watchModeButton).toHaveAttribute("aria-disabled", "true"),code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx (1)
34-34: Verify disabled button does not have aria-disabled='true'.The current assertion checks that
aria-disableddoes not have the valuetrue. Based on the Button implementation incode/core/src/components/components/Button/Button.tsx, whendisabledisfalse, the button will havearia-disabled='false'. Consider verifying the attribute has value'false'instead of just checking it doesn't have valuetrue.Apply this diff to explicitly check for the 'false' value:
- await expect(button).not.toHaveAttribute('aria-disabled', true); + await expect(button).toHaveAttribute('aria-disabled', 'false');code/e2e-tests/addon-viewport.spec.ts (2)
33-33: LGTM: Synchronous assertions for width checks.The change from
await expectto synchronousexpectis correct for thetoBeDefined()assertions on the bounding box width properties.Also applies to: 39-39, 53-53
57-57: LGTM: Correct aria-disabled attribute check.The assertion correctly checks for the
aria-disabledattribute with string value'true'to verify the disabled state.code/core/src/components/components/Button/Button.stories.tsx (1)
163-169: Consider aligning with previous feedback on render function.A past review comment suggested either removing the
renderfunction for a single variant or creating aDisabledVariantsstory showing all variants (solid, outline, ghost) side by side. The current implementation still usesrenderfor a single variant.Consider one of these approaches:
- Remove the
renderfunction to show the disabled state with the default variant- Create a comprehensive
DisabledVariantsstory showing all three variants in their disabled stateBased on learnings
code/core/src/components/components/Button/Button.tsx (1)
44-48: LGTM: Disabled guard in click handler.The click handler correctly prevents interaction when the button is disabled by stopping propagation and preventing default behavior.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
code/core/src/components/components/Button/Button.stories.tsx (1)
157-170: Consider addressing previous reviewer's feedback on story structure.A past reviewer questioned the use of
renderhere and suggested either:
- Remove the
renderfunction and add aplayfunction to demonstrate the button is focusable but not clickable (the key accessibility improvement)- Create a
DisabledVariantsstory showing all variants and pseudo states in disabled stateThe current
renderfunction wraps a single variant without showcasing the key behavioral change (focusability while disabled) that this PR introduces.Based on past review comments.
code/core/src/components/components/Button/Button.tsx (1)
75-76: Remove the nativedisabledattribute to achieve the PR's accessibility goal.Setting both
aria-disabledand the nativedisabledattribute defeats the purpose of this PR. The nativedisabledattribute removes the button from keyboard navigation and the accessibility tree, which is precisely what the PR aims to fix by usingaria-disabledinstead.A past reviewer noted: "If you don't remove the
disabledattr, then keyboard navigation is still impossible."Apply this diff:
aria-disabled={disabled} - disabled={disabled} active={active}
🧹 Nitpick comments (2)
code/e2e-tests/addon-toolbars.spec.ts (1)
35-35: Specify the expected attribute value for more precise assertion.The assertion only checks for the presence of
aria-disabledbut doesn't validate its value. Consider checking that the value is'true'to ensure the button is actually in a disabled state.Apply this diff:
- await expect(button).toHaveAttribute('aria-disabled'); + await expect(button).toHaveAttribute('aria-disabled', 'true');code/core/src/components/components/Button/Button.tsx (1)
8-17: Clarify the intent of omitting then re-addingdisabledin ButtonProps.The type extends
Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'disabled'>to remove the nativedisabledproperty, but then immediately re-declaresdisabled?: booleanon line 14. This pattern can be confusing.If the intent is to prevent the native HTML
disabledattribute from being forwarded while still accepting adisabledprop for internal handling, this structure achieves that goal. However, consider adding a JSDoc comment explaining this design decision to help future maintainers understand whydisabledis both omitted and re-added.Example documentation:
/** * Button props. Omits the native HTML disabled attribute to prevent it from * being forwarded to the DOM (which would break keyboard navigation). * Instead, uses aria-disabled for accessibility while handling the disabled * state internally via event handlers and styling. */ export interface ButtonProps extends Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'disabled'> { // ... existing props including disabled }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx(1 hunks)code/core/src/components/components/Button/Button.stories.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(5 hunks)code/e2e-tests/addon-toolbars.spec.ts(1 hunks)code/e2e-tests/addon-viewport.spec.ts(2 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxtest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.tstest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxtest-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/e2e-tests/addon-toolbars.spec.tscode/e2e-tests/addon-viewport.spec.tscode/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsxcode/core/src/components/components/Button/Button.stories.tsxcode/core/src/components/components/Button/Button.tsx
test-storybooks/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain test Storybook configurations under
test-storybooks/for E2E and visual testing scenarios
Files:
test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
🧬 Code graph analysis (1)
code/core/src/components/components/Button/Button.stories.tsx (2)
code/core/src/components/components/Button/Button.tsx (2)
ButtonProps(8-17)Button(19-85)code/core/src/components/components/Modal/Modal.styled.tsx (1)
Row(80-84)
🪛 Biome (2.1.2)
code/core/src/components/components/Button/Button.tsx
[error] 28-28: Duplicate parameter name.
The parameter overrides a preceding parameter by using the same name.
(lint/suspicious/noDuplicateParameters)
[error] 28-28: Shouldn't redeclare 'disabled'. Consider to delete it or rename it.
'disabled' is defined here:
(lint/suspicious/noRedeclare)
[error] 96-97: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '>'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/e2e-tests/addon-viewport.spec.ts (1)
57-57: LGTM!The assertion correctly checks both the presence and value of the
aria-disabledattribute using string'true', which aligns with ARIA specifications.code/core/src/components/components/Button/Button.stories.tsx (1)
8-8: LGTM!Importing and exposing
ButtonPropsas a public type is excellent for type safety and enables consumers to properly type Button usage in their own code.
WioletaKolodziej
force-pushed
the
fix/31678-aria-disabled-button
branch
from
31368c1 to
f9f5edd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/Button/Button.tsx (1)
74-74: Consider omitting aria-disabled when not disabled.Setting
aria-disabled="false"explicitly is valid but not idiomatic. Best practice is to set the attribute to'true'when disabled or omit it entirely (usingundefined) when not disabled.Apply this diff to follow the more common pattern:
- aria-disabled={disabled ? 'true' : 'false'} + aria-disabled={disabled ? 'true' : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/components/components/Button/Button.stories.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(4 hunks)code/e2e-tests/addon-toolbars.spec.ts(1 hunks)code/e2e-tests/addon-viewport.spec.ts(2 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
- code/e2e-tests/addon-viewport.spec.ts
- code/core/src/components/components/Button/Button.stories.tsx
🧰 Additional context used
📓 Path-based instructions (6)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/e2e-tests/addon-toolbars.spec.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/e2e-tests/addon-toolbars.spec.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/e2e-tests/addon-toolbars.spec.tscode/core/src/components/components/Button/Button.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). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/components/components/Button/Button.tsx (2)
8-17: LGTM: ButtonProps correctly omits native disabled.The interface properly excludes the native HTML
disabledattribute viaOmit<ButtonHTMLAttributes<HTMLButtonElement>, 'disabled'>and defines its owndisabled?: boolean. This prevents the native attribute from being forwarded while maintaining the API surface.
43-47: LGTM: Disabled guard correctly prevents interaction.The
handleClickguard properly blocks user interaction whendisabledistrueby callingpreventDefault()andstopPropagation()before returning. This replaces the behavior of the nativedisabledattribute while keeping the button focusable.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/components/components/Button/Button.stories.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(4 hunks)code/e2e-tests/addon-toolbars.spec.ts(1 hunks)code/e2e-tests/addon-viewport.spec.ts(2 hunks)test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/components/components/Button/Button.tsx
- test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/e2e-tests/addon-viewport.spec.tscode/e2e-tests/addon-toolbars.spec.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-toolbars.spec.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/e2e-tests/addon-viewport.spec.tscode/e2e-tests/addon-toolbars.spec.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-toolbars.spec.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-toolbars.spec.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/e2e-tests/addon-viewport.spec.tscode/core/src/components/components/Button/Button.stories.tsxcode/e2e-tests/addon-toolbars.spec.ts
🧬 Code graph analysis (1)
code/core/src/components/components/Button/Button.stories.tsx (1)
code/core/src/components/components/Button/Button.tsx (1)
ButtonProps(8-17)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (4)
code/core/src/components/components/Button/Button.stories.tsx (2)
8-8: LGTM: Import aligns with type-safe story args.The import of
ButtonPropstype enables type-safe story args via thesatisfiesassertion on line 162.
161-162: Add play function to demonstrate disabled behavior.The onClick handler and type assertion are good additions, but the previous review comment requested a play function to demonstrate that the button is focusable but not clickable when disabled. Consider adding a play function that:
- Focuses the button (to show it's focusable)
- Attempts to click it (to show clicks are blocked)
- Verifies onClick was not called
Additionally, consider creating a
DisabledVariantsstory that shows all variants in the disabled state.code/e2e-tests/addon-viewport.spec.ts (1)
57-57: LGTM: Correctly verifies aria-disabled attribute.The assertion properly checks for
aria-disabled='true', which aligns with the PR's accessibility changes to replace nativedisabledwitharia-disabled.code/e2e-tests/addon-toolbars.spec.ts (1)
35-35: LGTM: Correctly verifies aria-disabled attribute value.The assertion properly checks both the presence and value of
aria-disabled='true', addressing the previous review comment and aligning with the PR's accessibility changes.
WioletaKolodziej
force-pushed
the
fix/31678-aria-disabled-button
branch
from
a20fc11 to
64fee36
Compare
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
WioletaKolodziej
force-pushed
the
fix/31678-aria-disabled-button
branch
from
64fee36 to
268d83f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/Button/Button.tsx (1)
74-74: Consider omitting aria-disabled when false.While setting
aria-disabled="false"is valid, the ARIA specification recommends omitting the attribute entirely when the element is not disabled. The absence ofaria-disabledis semantically equivalent toaria-disabled="false".Apply this diff to omit the attribute when not disabled:
- aria-disabled={disabled ? 'true' : 'false'} + aria-disabled={disabled ? 'true' : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx(1 hunks)code/core/src/components/components/Button/Button.stories.tsx(2 hunks)code/core/src/components/components/Button/Button.tsx(4 hunks)code/e2e-tests/addon-viewport.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/addons/onboarding/src/features/IntentSurvey/IntentSurvey.stories.tsx
- code/core/src/components/components/Button/Button.stories.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/components/components/Button/Button.tsxcode/e2e-tests/addon-viewport.spec.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/components/components/Button/Button.tsxcode/e2e-tests/addon-viewport.spec.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/components/components/Button/Button.tsxcode/e2e-tests/addon-viewport.spec.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/Button/Button.tsxcode/e2e-tests/addon-viewport.spec.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/e2e-tests/addon-viewport.spec.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/e2e-tests/addon-viewport.spec.ts
🧬 Code graph analysis (1)
code/e2e-tests/addon-viewport.spec.ts (1)
code/core/src/test/expect.ts (1)
expect(148-148)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (5)
code/e2e-tests/addon-viewport.spec.ts (2)
33-33: Correctly removed unnecessary await from synchronous assertions.These assertions check plain JavaScript values (width properties) from the already-resolved
boundingBox()results, not Playwright locators. The assertions are synchronous and do not requireawait. The previous review incorrectly flagged these as missing await.Also applies to: 39-39, 42-42, 53-53
57-57: LGTM! Correctly checks ARIA-disabled state.The assertion correctly verifies the
aria-disabledattribute with the expected string value'true', aligning with the PR's goal of using ARIA-based disabled state. Theawaitis appropriately retained for this Playwright locator assertion.code/core/src/components/components/Button/Button.tsx (3)
8-8: Excellent approach to prevent native disabled attribute leakage.Omitting
disabledfrom the extendedButtonHTMLAttributesprevents the native HTML disabled attribute from being accidentally forwarded while still allowing the component to accept and controldisabledas its own prop. This aligns perfectly with the PR objective to replace native disabled with aria-disabled.
30-30: Correctly prevents disabled from being forwarded to DOM.By destructuring
disabledexplicitly and usingrestPropsfor spreading, the implementation ensures the nativedisabledattribute is never forwarded to the button element. This maintains keyboard focusability as intended by the PR.Also applies to: 79-79
43-47: LGTM! Correctly prevents interaction for disabled buttons.The early return with
preventDefault()andstopPropagation()is the correct pattern for aria-disabled buttons. This prevents user interaction while keeping the button in the accessibility tree and keyboard navigation flow.
WioletaKolodziej
changed the title
|
View your CI Pipeline Execution ↗ for commit 268d83f
☁️ Nx Cloud last updated this comment at |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
@WioletaKolodziej thanks for updating the branch! As we were busy with the 10.0 release, it took me longer than it should to redo a review.
You're almost there! Just one unit test to add and one E2E test to fix and we can merge this :)
There was a problem hiding this comment.
As we are changing the behaviour of Button, we want to make sure that disabled buttons do not call their event handlers. Could you please add a play function to the disabled story, where
- you click on the button
- you focus on the button and press Enter
- you check that
onClickwas never called
|
@WioletaKolodziej I've pushed an updated version of your branch at #34166 (still crediting you for your contribution ;-)). I'll make sure it gets merged soon. Closing this older PR as it has conflicts. |
3 participants
Closes #31678
What I did
Replaced native disabled with aria-disabled for accessibility.
This keeps the button focusable and prevents conflicts with native behavior while preserving styling and click handling.
Changes made:
Replaced native disabled attribute with aria-disabled to preserve focusability and accessibility.
Updated handleClick to prevent clicks when disabled is true, instead of conditionally removing the onClick handler.
Styled button still uses disabled for cursor and opacity styling.
Reasoning:
Using native disabled removes the button from keyboard navigation and makes it invisible to screen readers.
By using aria-disabled, the button remains focusable and perceivable by assistive technologies, while still preventing interactions.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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 canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Updated On: 2025-09-10 14:51:07 UTC
This PR implements accessibility improvements by replacing the native HTML
disabledattribute witharia-disabledin the Button component. The change addresses the issue where disabled buttons are removed from the accessibility tree and cannot be navigated via keyboard, making them invisible to screen readers.The PR updates the Button component to use
aria-disabled="true"while maintaining visual styling through CSS that still references thedisabledprop. The implementation adds click prevention logic in thehandleClickmethod to prevent interactions whendisabledis true, rather than relying on the native HTML disabled behavior. This approach keeps buttons focusable and perceivable by assistive technologies while still preventing user interactions.Corresponding test files have been updated across the codebase to check for
aria-disabledattributes instead of using DOM testing library matchers liketoBeDisabled(). The changes affect E2E tests for addon-toolbars, addon-viewport, component testing, and IntentSurvey stories. The Button stories have been enhanced with proper TypeScript types and a custom render function for the disabled state demonstration.Confidence score: 1/5
disabledandaria-disabledattributes simultaneously, creating conflicting implementationsSummary by CodeRabbit
Accessibility
Bug Fixes
Tests