Address additional ToggleSwitch a11y feedback

[camertron]

Closes https://github.com/github/primer/issues/2520

Changelog

New

Changed

1. aria-labelledby is now a required prop.
2. Disabled switches now use aria-disabled instead of disabled.
3. A screen reader-only "Loading" label is now displayed after a configurable time period has elapsed to let screen reader users know the toggle is still in progress.
4. Fixed toggling via the On/Off label when the switch is disabled.

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan'

A PR this summer removed the required aria-describedby attribute, and this PR adds it back in. It is possible new instances of ToggleSwitch in dotcom omitted this attribute in the interim, so care may be necessary when integrating.

EDIT: It appears there are no missing aria-labelledby attributes in dotcom code (see the integration PR). There are two failing dotcom tests that can be fixed by checking for aria-disabled instead of calling toBeDisabled() and toBeEnabled().

Testing & Reviewing

The loading-related changes are most easily testable using the "Loading with Delay" story under "Features." You can adjust the amount of toggle delay as well as the amount of delay before the "Loading" label appears and is announced by screen readers.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• Integration tests pass at github/github (Learn more about how to run integration tests)

Re: integration tests, see above.

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6cdc7b5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	105.71 KB (+0.16% 🔺)
dist/browser.umd.js	106.32 KB (+0.18% 🔺)

[joshblack]

@camertron would there be a way to have aria-labelledby be optional instead of required? I think having a prop become required would fall under our major change bucket 😞 If we could do optional (even have a warning be emitted) and then required that would be awesome

[camertron]

@joshblack unfortunately no, that prop needs to be required. I was hoping we could sneak it in, as it used to be required a few months ago and was mistakenly removed.

[TylerJDev]

I have a few comments, but overall this looks fantastic! Nice work 🎉

[broccolinisoup]

Left a couple of questions, looking forward to hearing your thoughts 🙌🏻

[TylerJDev]

@broccolinisoup Also I am not really sure if I understand this prop. How would consumers know how long they should delay the label? 🤔 I might be missing some context though, I would appreciate any insights 🙏🏻

I think as long as there is a default, this is okay. A consumer probably shouldn't change this value unless they have a good reason to. This prop is to ensure that "loading" doesn't announce if the loading state is near instantaneous, so changing it should come sparingly.

[TylerJDev]

Outside of @broccolinisoup's comments, looks great! Once those are addressed, we should be good to go! 🎉

[broccolinisoup]

Thanks for following up with the comments! I left one more comment here.

Also, regarding the fixes required at dotcom - this is still a new practise so please share any feedback you have - we are trying to include dotcom changes in the roll out plan section in the template.

EDIT: It appears there are no missing aria-labelledby attributes in dotcom code (see the https://github.com/github/github/pull/296457). There are two failing dotcom tests that can be fixed by checking for aria-disabled instead of calling toBeDisabled() and toBeEnabled().

This is great to know that there are changes required when the PR is merged into the release branch and it would be great to include a commit id to make the release coordinator's job easier when debugging the integration tests. Is that reasonable? What do you think?

[camertron]

Thanks for taking another look @broccolinisoup!

it would be great to include a commit id to make the release coordinator's job easier when debugging the integration tests. Is that reasonable? What do you think?

It sounds like a great idea to let the release coordinator know what to fix, but I'm not sure what you mean. Where should the commit ID go?

[broccolinisoup]

It sounds like a great idea to let the release coordinator know what to fix, but I'm not sure what you mean. Where should the commit ID go?

@camertron commit hash is probably a better term sorry 🙂 For example, I created this integration test PR for #3816 and because there are some changes required at dotcom when the PRC pull request is merged, I fixed those issues in the integration PR and you can see the commits here. This is still WIP but once it is ready to ship, I plan to share this commit hash (probably fixing the linting and force pushing everything into once commit) with the release coordinator, so that they can cherry-pick it on the PR that is updating the primer react version at dotcom. Is that helpful? And this is just one way really, let us know if you prefer a different way to share the changes or if you have any feedback about this "process" 🙌🏻

[camertron]

@broccolinisoup ahh ok I get it! Great idea, I can work on that 👍

[TylerJDev]

Hey @camertron 👋

I'm coming back to this to say I recently merged a PR that was in a similar state, and it initially had test failures in dotcom. I created the integration test PR, and added all of the fixes needed in that PR to make sure the tests were all green. From there, the release conductor was able to implement those changes in the release PR.

I think all we'd need to do is get the integration test PR to a point where all tests are passing, and provide that information to whomever is doing the release that week.

@broccolinisoup, can you confirm the above? I think overall this PR should be ready to go once we know those test failures can be fixed easily without friction. Also, I'm wondering if we want to formalize this process somewhere, if we haven't already? 🤔

[github-actions]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.