fix(Spinner): define default aria-label via destructuring#12420
fix(Spinner): define default aria-label via destructuring#12420mvanhorn wants to merge 5 commits into
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
WalkthroughSpinner now defaults ChangesSpinner ARIA Labeling Refactor
Estimated code review effort: 2 (Simple) | ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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'); |
There was a problem hiding this comment.
Small nit: we should use toHaveAccessibleName instead here
|
|
||
| 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'); |
|
Switched both flagged assertions to |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Just a couple more comments below, otherwise this is looking good!
| expect(screen.getByRole('progressbar')).toHaveAccessibleName('Loading users'); | ||
| }); | ||
|
|
||
| test('keeps the default aria-label when aria-labelledby is provided', () => { |
There was a problem hiding this comment.
"Renders with accessible name via aria-labelledby when passed" might be slightly more fitting test name here.
| expect(spinner).toHaveAttribute('aria-label', 'Contents'); | ||
| expect(spinner).toHaveAttribute('aria-labelledby', 'spinner-label'); |
There was a problem hiding this comment.
Can we separate the aria-label check into a separate test, "Renders with aria-label even when aria-labelledby is passed".
Then we can also remove the toHaveAttribute('aria-labelledby'... line; the check for the accessible name should suffice for ensuring aria-labelledby has the expected value.
- Use toHaveAccessibleName instead of raw attribute assertions. - Rename the aria-labelledby test to 'Renders with accessible name via aria-labelledby when passed'. - Split the default aria-label check into its own test, 'Renders with aria-label even when aria-labelledby is passed'. - Use lowercase aria-labelledby (WAI-ARIA spec) in the tests. Per @thatblindgeye's review; no Spinner.tsx / public-API changes.
|
Thanks @thatblindgeye — addressed all of these in fc9e371:
|
Use toHaveAccessibleName for the aria-label-with-aria-labelledby case, per review - the accessible-name check is sufficient to confirm the expected value.
|
Thanks @thatblindgeye! Pushed a0e3104 switching that last aria-label assertion to |
3 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
aria-labelledbywhen provided.Tests
aria-labelledbyis used.