[Spinner] - hardcoded aria-label attribute

[dgdavid]

Describe the problem

Edit 4/11/2025:

This issue should be scoped to just updating how we're defining the default value of the aria-label on Spinner. Ideally we want the default value to be rendered in the props table, rather than rendering as though there isn't any default value currently.

Original comment:

The Spinner component hardcodes aria-label attribute when neither aria-label nor aria-labelledby is provided.

https://github.com/patternfly/patternfly-react/blob/3ba79b0c6c6e750996f6134251dc6c6bd5eeb28a/packages/react-core/src/components/Spinner/Spinner.tsx#L47

How do you reproduce the problem?

Mount a Spinner without providing an aria-label or aria-labelledby.

Expected behavior

A spinner without a hardcoded English value. The component should allow consumers to use it without aria-label or aria-labelledby. It might be just a complementary visual hint hidden from the a11y tree using aria-hidden when rendered alongside relevant text.

Is this issue blocking you?

Somehow. A workaround exists by setting aria-labelledby, but it would be preferable if, as said above, the spinner could be used as a visual hint for sighted users while being excluded from the a11y tree.

Screenshots

N/A

What is your environment?

N/A

What is your product and what release date are you targeting?

N/A

Any other information?

• The Spinner props documentation does not mention a default value for aria-label.

• This behavior was introduced in commit 09733cb4f0fc5cd67f5df284 as part of https://github.com/patternfly/patternfly-react/pull/6730, but I was not able to find the reasoning there.

• aria-valuetext also has a hardcoded default value, even when it may not be applicable.

  aria-valuetext is only needed when the numeric value of aria-valuenow is not meaningful.

  MDN Documentation

[thatblindgeye]

I'd probably reconsider whether it should be hidden from the accessibility tree, even if there's other text close to the Spinner that conveys some sort of loading state. Main reason being is that the spinner will convey that the loading progress is "indeterminate", which surrounding text may or may not do. Semantically having the Spinner exposed may make more sense as well than just plain text.

At least according to MDN, either aria-label or aria-labelledby is also required on the progressbar role. That I think makes sense since just the aria-valuenow content (be it "Loading" or something similar) doesn't provide great context (what exactly is loading?).

Though I might strongly recommend against it, if aria-hidden is being passed to a Spinner, that should somewhat resolve the issue of having a default aria-label applied. While the aria-label will still be there, the Spinner won't be in the a11y tree.

I do think we can tweak the Spinner code so that the default value of aria-label is rendered in the props table, though.

[dgdavid]

First of all, thanks a lot for your detailed and insightful answer.

I'd probably reconsider whether it should be hidden from the accessibility tree,

Sure, after reading your explanations I will not hide it.

even if there's other text close to the Spinner that conveys some sort of loading state. Main reason being is that the spinner will convey that the loading progress is "indeterminate", which surrounding text may or may not do. Semantically having the Spinner exposed may make more sense as well than just plain text.

At least according to MDN, either aria-label or aria-labelledby is also required on the progressbar role.

Arg! My fault! I completely overlooked the progressbar role that the Spinner SVG is using! That’s why I wasn’t fully understanding the decision of going for these default values.

Regarding having both the aria-label and the static text, I thought it might be redundant and possibly annoying for screen reader users, especially if both contain the same message. In any case, now that I have a better understanding, I’ll rework my use of the Spinner to include only one descriptive text. Either by using aria-labelledby or by adding aria-hidden to the static text if it doesn’t provide any additional value.

That I think makes sense since just the aria-valuenow content (be it "Loading" or something similar) doesn't provide great context (what exactly is loading?).

Fully agree. "Loading" alone lacks the necessary context.

Though I might strongly recommend against it, if aria-hidden is being passed to a Spinner, that should somewhat resolve the issue of having a default aria-label applied. While the aria-label will still be there, the Spinner won't be in the a11y tree.

No doubts, I’ve got a long road ahead on my accessibility learning journey, as this issue has proven. That said, in my tests, when aria-hidden is applied, the entire Spinner is removed from the accessibility tree, including its aria-label (checked with Chrome and Firefox developer tools) .

I do think we can tweak the Spinner code so that the default value of aria-label is rendered in the props table, though.

Again, thank you for your enlightening explanation and please excuse me for reporting this as an issue when it clearly wasn’t one 🙏

[thatblindgeye]

It's better to raise an issue that may turn out to be intended behavior than have something sneak by us, so absolutely no worries opening this! We don't know what we don't know and all, and whether it leads to us fixing/adding something to improve the user experience or just a good discussion, it's worth it.

I'll edit your original comment just to note what this issue should go towards resolving (updating how we're defining the default value). If you'd be interested in working on it just let me know and I can assign you (no pressure to, though), otherwise I can put a "good first issue" label for others to pick it up.

[dgdavid]

If you'd be interested in working on it just let me know and I can assign you (no pressure to, though), otherwise I can put a "good first issue" label for others to pick it up.

I fear I'm a bit busy these days 😢 for investing the time it needs to be not only changed but also properly tested. So, I'd label it as good first issue and give others a chance to work on it.

That said, just in case could be useful for others, as far as I could check, https://github.com/patternfly/patternfly-org is relying on https://github.com/reactjs/react-docgen for generating the PropsTable. Thus, I guess it could be as simple as moving the default value to the props initialization as shown in below diff

diff --git a/packages/react-core/src/components/Spinner/Spinner.tsx b/packages/react-core/src/components/Spinner/Spinner.tsx
index 77e599773..4099da094 100644
--- a/packages/react-core/src/components/Spinner/Spinner.tsx
+++ b/packages/react-core/src/components/Spinner/Spinner.tsx
@@ -32,7 +32,7 @@ export const Spinner: React.FunctionComponent<SpinnerProps> = ({
   'aria-valuetext': ariaValueText = 'Loading...',
   diameter,
   isInline = false,
-  'aria-label': ariaLabel,
+  'aria-label': ariaLabel = 'Contents',
   'aria-labelledBy': ariaLabelledBy,
   ...props
 }: SpinnerProps) => (
@@ -42,9 +42,7 @@ export const Spinner: React.FunctionComponent<SpinnerProps> = ({
     aria-valuetext={ariaValueText}
     viewBox="0 0 100 100"
     {...(diameter && { style: { [cssDiameter.name]: diameter } as React.CSSProperties })}
-    {...(ariaLabel && { 'aria-label': ariaLabel })}
-    {...(ariaLabelledBy && { 'aria-labelledBy': ariaLabelledBy })}
-    {...(!ariaLabel && !ariaLabelledBy && { 'aria-label': 'Contents' })}
+    {...(ariaLabelledBy ? { 'aria-labelledBy': ariaLabelledBy } : { 'aria-label': ariaLabel })}
     {...props}
   >
     <circle className={styles.spinnerPath} cx="50" cy="50" r="45" fill="none" />

However, this approach puts the component at risk of having neither aria-label nor aria-labelledby if the consumer passes an empty string for the former and omits the latter. While this is technically a misuse, I'm not sure if it's something you want to guard against or leave up to the consumer.

[thatblindgeye]

Yep exactly what I was thinking! And we can add a console warning if both aria-label and aria-labelledby are falsey, we do that in some other components and could make sense here as well.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.