fix(Spinner): define default aria-label via destructuring#12420
fix(Spinner): define default aria-label via destructuring#12420mvanhorn wants to merge 2 commits intopatternfly:mainfrom
Conversation
The Spinner default "Contents" aria-label was emitted by an inline conditional in the JSX, so the props table for the component did not show that there was a default value at all (patternfly#11750). Move the default to the destructuring pattern so react-docgen surfaces it, and replace the three conditional spread expressions with a direct attribute plus the still-conditional aria-labelledBy spread. Existing snapshots are unchanged (`<Spinner />` still renders with aria-label="Contents"). Added unit tests cover the default value, an explicitly provided aria-label, and aria-labelledBy. Closes patternfly#11750
WalkthroughSpinner now sets ChangesSpinner ARIA Labeling Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-react-pr-12420.surge.sh A11y report: https://pf-react-pr-12420-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/components/Spinner/Spinner.tsx (1)
26-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPre-existing:
aria-labelledByis not a valid ARIA attribute name.The WAI-ARIA spec defines this attribute as
aria-labelledby(all lowercase). React passesaria-*attributes as-is to the DOM without normalization. On an SVG element (XML-based, case-sensitive),'aria-labelledBy'produces a non-standard DOM attribute that assistive technologies will not recognize.Lines 26, 36, and 46 in Spinner.tsx are pre-existing. However, the new test at lines 19–21 in Spinner.test.tsx adds coverage for this prop with the incorrect spelling, perpetuating the issue in new code. A full fix would require renaming the public prop from
'aria-labelledBy'to'aria-labelledby'(a breaking API change best addressed in a dedicated PR), but now is a good time to track this known issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/Spinner/Spinner.tsx` at line 26, The Spinner component uses the incorrect prop name 'aria-labelledBy' which produces a non-standard DOM attribute; to avoid a breaking API change, update the Spinner component (Spinner.tsx) to detect the incoming prop 'aria-labelledBy' and forward it as the correct DOM attribute name 'aria-labelledby' (ensure the prop is included in the SVG/element props passed to the DOM), update Spinner.test.tsx to assert that the rendered DOM has 'aria-labelledby' set, and add a brief TODO note in Spinner.tsx mentioning the planned public prop rename to 'aria-labelledby' in a follow-up breaking-change PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx`:
- Around line 19-22: Change the misspelled ARIA prop from "aria-labelledBy" to
the correct "aria-labelledby" across the API and tests: update the test case
(the 'renders aria-labelledBy when provided' test) to render <Spinner
aria-labelledby="external-label" /> and assert for 'aria-labelledby'; rename the
prop in SpinnerProps from 'aria-labelledBy'?: string to 'aria-labelledby'?:
string and update Spinner component's prop destructuring/spread where it uses
that key so the SVG receives aria-labelledby; if this is a breaking public API
change, mark for separate PR/deprecation notice as appropriate.
---
Outside diff comments:
In `@packages/react-core/src/components/Spinner/Spinner.tsx`:
- Line 26: The Spinner component uses the incorrect prop name 'aria-labelledBy'
which produces a non-standard DOM attribute; to avoid a breaking API change,
update the Spinner component (Spinner.tsx) to detect the incoming prop
'aria-labelledBy' and forward it as the correct DOM attribute name
'aria-labelledby' (ensure the prop is included in the SVG/element props passed
to the DOM), update Spinner.test.tsx to assert that the rendered DOM has
'aria-labelledby' set, and add a brief TODO note in Spinner.tsx mentioning the
planned public prop rename to 'aria-labelledby' in a follow-up breaking-change
PR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3812dc0-383f-44b9-a904-73a661f5b4d2
📒 Files selected for processing (2)
packages/react-core/src/components/Spinner/Spinner.tsxpackages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx
| test('renders aria-labelledBy when provided', () => { | ||
| render(<Spinner aria-labelledBy="external-label" />); | ||
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-labelledBy', 'external-label'); | ||
| }); |
There was a problem hiding this comment.
New test codifies the pre-existing aria-labelledBy (wrong casing) behavior.
The prop and assertion both use aria-labelledBy (capital B). Since React camelCases all attributes except data-* and aria-*, it passes aria-* names as-is to the DOM. On the SVG element (XML/case-sensitive), this produces a non-standard aria-labelledBy attribute that screen readers won't find — they look for aria-labelledby (all lowercase, per WAI-ARIA spec).
The test will pass today (JSDOM stores aria-labelledBy exactly as written and toHaveAttribute finds it), but it is asserting broken accessibility behavior. The correct attribute name throughout — interface, component, and test — should be aria-labelledby.
✅ Corrected test (aligned with WAI-ARIA spec)
test('renders aria-labelledBy when provided', () => {
- render(<Spinner aria-labelledBy="external-label" />);
- expect(screen.getByRole('progressbar')).toHaveAttribute('aria-labelledBy', 'external-label');
+ render(<Spinner aria-labelledby="external-label" />);
+ expect(screen.getByRole('progressbar')).toHaveAttribute('aria-labelledby', 'external-label');
});The full fix also requires renaming the prop in SpinnerProps from 'aria-labelledBy'?: string to 'aria-labelledby'?: string and updating the corresponding destructuring and spread in Spinner.tsx. Because that is a public API rename, it may warrant its own PR and a deprecation notice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx` around
lines 19 - 22, Change the misspelled ARIA prop from "aria-labelledBy" to the
correct "aria-labelledby" across the API and tests: update the test case (the
'renders aria-labelledBy when provided' test) to render <Spinner
aria-labelledby="external-label" /> and assert for 'aria-labelledby'; rename the
prop in SpinnerProps from 'aria-labelledBy'?: string to 'aria-labelledby'?:
string and update Spinner component's prop destructuring/spread where it uses
that key so the SVG receives aria-labelledby; if this is a breaking public API
change, mark for separate PR/deprecation notice as appropriate.
The dropped test rendered <Spinner aria-labelledBy='...' /> and asserted the SVG carried 'aria-labelledBy' (capital B). React passes aria-* attributes to the DOM as-is, so on the SVG element this lands as a non-standard attribute that screen readers won't find — they look for 'aria-labelledby' (all lowercase, per WAI-ARIA spec). The existing prop name in SpinnerProps still uses the camelCased form, so renaming is a public API breaking change worth its own PR with deprecation. Removing the test here keeps this PR scoped to the default aria-label change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx`:
- Around line 9-17: Add a regression test in Spinner.test.tsx that renders
<Spinner aria-labelledby="some-id" /> and asserts that
screen.getByRole('progressbar') has both aria-labelledby="some-id" and the
default aria-label="Contents"; update the test suite by adding a new test case
(similar to the existing ones) to exercise the coexistence of aria-labelledby
and the default aria-label so the Spinner component preserves aria-label when
only aria-labelledby is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 444ade7c-4f76-4a69-81e9-cc3f121b8cb2
📒 Files selected for processing (1)
packages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx
| test('uses default aria-label of "Contents" when none is provided', () => { | ||
| render(<Spinner />); | ||
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Contents'); | ||
| }); | ||
|
|
||
| test('uses a custom aria-label when one is provided', () => { | ||
| render(<Spinner aria-label="Loading users" />); | ||
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Loading users'); | ||
| }); |
There was a problem hiding this comment.
Add regression coverage for aria-labelledby + default aria-label coexistence.
These tests cover default and custom label values, but they don’t lock the third behavior this PR depends on: when only aria-labelledby is provided, aria-label="Contents" should still be present.
Suggested test addition
test('uses a custom aria-label when one is provided', () => {
render(<Spinner aria-label="Loading users" />);
expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Loading users');
});
+
+test('keeps default aria-label when aria-labelledby is provided', () => {
+ render(<Spinner aria-labelledby="external-label" />);
+ const spinner = screen.getByRole('progressbar');
+ expect(spinner).toHaveAttribute('aria-labelledby', 'external-label');
+ expect(spinner).toHaveAttribute('aria-label', 'Contents');
+});📝 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.
| test('uses default aria-label of "Contents" when none is provided', () => { | |
| render(<Spinner />); | |
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Contents'); | |
| }); | |
| test('uses a custom aria-label when one is provided', () => { | |
| render(<Spinner aria-label="Loading users" />); | |
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Loading users'); | |
| }); | |
| test('uses default aria-label of "Contents" when none is provided', () => { | |
| render(<Spinner />); | |
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Contents'); | |
| }); | |
| test('uses a custom aria-label when one is provided', () => { | |
| render(<Spinner aria-label="Loading users" />); | |
| expect(screen.getByRole('progressbar')).toHaveAttribute('aria-label', 'Loading users'); | |
| }); | |
| test('keeps default aria-label when aria-labelledby is provided', () => { | |
| render(<Spinner aria-labelledby="external-label" />); | |
| const spinner = screen.getByRole('progressbar'); | |
| expect(spinner).toHaveAttribute('aria-labelledby', 'external-label'); | |
| expect(spinner).toHaveAttribute('aria-label', 'Contents'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/components/Spinner/__tests__/Spinner.test.tsx` around
lines 9 - 17, Add a regression test in Spinner.test.tsx that renders <Spinner
aria-labelledby="some-id" /> and asserts that screen.getByRole('progressbar')
has both aria-labelledby="some-id" and the default aria-label="Contents"; update
the test suite by adding a new test case (similar to the existing ones) to
exercise the coexistence of aria-labelledby and the default aria-label so the
Spinner component preserves aria-label when only aria-labelledby is provided.
2 participants
What: Closes #11750
The Spinner component's default
aria-labelof "Contents" was emitted by an inline conditional in the JSX ({...(!ariaLabel && !ariaLabelledBy && { 'aria-label': 'Contents' })}), so the props table for the component didn't show that there was a default value at all. Per the maintainer's edit on the issue, this PR is scoped to just updating how that default is defined.The destructuring now sets the default directly:
and the three conditional spreads in the JSX collapse to a direct
aria-label={ariaLabel}plus the still-conditionalaria-labelledByspread.Behavior
<Spinner />still rendersaria-label="Contents".aria-labelcontinues to take precedence.aria-labelledBywill now also see the defaultaria-label="Contents"on the SVG. The two ARIA attributes coexist (per WAI-ARIA, an assistive technology will usearia-labelledbywhen both are present), and this matches the existing pattern for sibling components likeTabAction,DrawerCloseButton,PageToggleButton, andFileUploadFieldthat all use destructuring defaults foraria-label.Tests
Added three unit tests covering the default value, an explicitly provided value, and
aria-labelledBypropagation. Existing snapshot tests still pass unchanged.Summary by CodeRabbit
Refactor
Tests