`Spinner` is inaccessible by default

[iansan5653]

The Spinner component is, at first glance, a convenient way to render a loading state:

<Box>
  {isLoading ? <Spinner /> : content}
</Box>

This appears straightforward, but in reality this is a bad idea because it's completely inaccessible. Spinner has no label by default, so the box will simply appear empty to screen readers.

The US Government CMS design system renders Spinner as a status with hidden "Loading" text. I think we probably should do the same by default, rendering it like so (hiding the SVG to avoid duplication):

<Box role="status">
  <VisuallyHidden>Loading</VisuallyHidden>
  <SpinnerSvg aria-hidden />
</Box>

This would make the accessible path much easier to follow.

[lesliecdubs]

Thanks for the suggestion, this does seem like a nice improvement to me as the current Spinner doesn't appear to have any label at all!

@TylerJDev do you concur?

[iansan5653]

One alternative for labelling might be to label the SVG properly, however I'm not sure that's the best approach in this case because the spinner being an image is not useful context. image labelled "Loading" makes less sense than just "Loading".

---

One concern I do have with this status approach is that while it announces a loading state, it doesn't announce loading completion. Unmounting the Spinner removes the status from the UI, which results in no announcement. I'm not sure if this is a big deal -- do we need to announce loading completion? One approach could be to do useEffect(() => () => announce("Loading completed"), []) where announce causes an aria-live announcement.

[TylerJDev]

Definitely agree. The <Spinner> component as it is now could be improved for accessibility.

The US Government CMS design system renders Spinner as a status with hidden "Loading" text.

I like this pattern! If we did utilize a live region, using plaintext is probably the way to go. Any <Spinner> usage that exists now that uses some sort of text alternative (e.g. aria-label) for the component would need to be adjusted so that it uses the new pattern instead. If the current <Spinner> is hidden (i.e. aria-hidden), then we might want to respect that with this pattern, as this might suggest that there's an alternative text-based loading indication to supplement the visual.

The only thing I'm slightly wary about is providing the live region by default. Sometimes loading indicators aren't needed if they are quick (e.g. < 2 seconds). Although, this is something we could probably handle internally within the component. We may also want to make sure that the role can be adjusted.

The other concern is that we might need to check any existing usage where there might be multiple <Spinner> components on a page at the same time and if this is a common occurrence, but only if we did go the route of providing a live region within the component by default.

One concern I do have with this status approach is that while it announces a loading state, it doesn't announce loading completion.

This is a great point. I'm wondering if this should be the responsibility of this component though? We definitely could add something here, but I think this may be mainly on the developer, or a different type of component entirely.

[lesliecdubs]

@iansan5653 as an Accessibility Champion, do you have any interest in contributing this improvement?

If not, Primer will move this in the backlog and aim to prioritize it when we pick up Spinner for accessibility remediation. For now, we're focused first on components that have the most usage within strategic services as per https://github.com/github/primer/issues/1973.

[iansan5653]

@lesliecdubs I might be able to in a few weeks. I don't want to commit to that but if I can, I'll pick it up.

[lesliecdubs]

Cross-linking this duplicate issue (now closed) https://github.com/github/primer/issues/2203 for context, which also includes recommended implementation guidance from our accessibility vendor TetraLogical. They advise adding a role="img" to the <svg> with an accessible name, such as aria-label="Loading".

@mperrotti feel free to bring this to Accessibility Engineering Office Hours if the differing guidance between these issues creates a lack of clarity on the best path forward.

[mperrotti]

Thanks for the feedback @TylerJDev - sorry I missed it. I just scheduled an a11y office hours session to discuss further: https://github.com/github/accessibility/issues/5454

[mperrotti]

@TylerJDev - during office hours I was told we're working on resolving the issues with aria live regions: https://github.com/github/accessibility/issues/4887

[patrickhlauke]

I mentioned this in a thread in Slack that pointed towards this issue ... it may have already been discussed/noted, but just just in case: simply adding a status role may not work consistently though - as with all live regions, in general (barring a few idiosyncracies/heuristics in certain browser/screen reader combinations), live regions need to first be primed (present in the markup), and only as a second step can be populated with content that will then be "seen" by screen readers as a change and announced. Generating a fully-populated live region container does not consistently result in its content being announced.