update ToggleSwitch to support autofocus #3319
Conversation
🦋 Changeset detectedLatest commit: 154e1ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This reverts commit bb0c6cd.
ktravers
changed the title
| @@ -62,6 +62,10 @@ def with_bad_csrf_token | |||
| def with_turbo | |||
| render(Primer::Alpha::ToggleSwitch.new(src: UrlHelpers.toggle_switch_index_path, turbo: true)) | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
Should this new preview be snapshotted? 🤔
There was a problem hiding this comment.
I would say yes, but only if it's visually different from other previews.
There was a problem hiding this comment.
The only visual difference would be whatever focus styles are in place. I've got aggressive settings on my local machine when it comes to focus, so it's always visually distinct, so I'm not sure if that's actually true for this component as well or just me 😅 Lemme take a quick look at the CSS and/or disable my focus stuff to confirm.
There was a problem hiding this comment.
Ok, AFAICT ToggleSwitch has focus styling, but it's only visible when :focus-visible is applied via the browser. So I don't think we need a separate preview 👍
ktravers
added
the
a11y eng
There was a problem hiding this comment.
Nice, thanks! I actually think we should implement both of the approaches you outlined 😄 IMO, autofocus: should be an argument to ToggleSwitch.new, but so should button_arguments:. We actually do this in a number of places when we want to accept arguments for child elements.
Inside the constructor, we can do something like:
@button_arguments = button_arguments
@button_arguments[:autofocus] = true if autofocus
@button_arguments[:aria] = merge_aria(
@button_arguments,
{ aria: { pressed: on? } }
)| @@ -62,6 +62,10 @@ def with_bad_csrf_token | |||
| def with_turbo | |||
| render(Primer::Alpha::ToggleSwitch.new(src: UrlHelpers.toggle_switch_index_path, turbo: true)) | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
I would say yes, but only if it's visually different from other previews.
@camertron Cool, that sounds good to me! Would it be ok to ship this PR as is to unblock https://github.com/github/github/pull/361376, then implement |
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans. Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
Related to https://github.com/github/accessibility-audits/issues/10006
Required by https://github.com/github/github/pull/361376
What are you trying to accomplish?
This PR updates the ToggleSwitch component to allow the
autofocusproperty to be applied to its button element. Doing so will allow this component to be fully accessible when used in conjunction with the Rails turbo framework. See https://github.com/github/accessibility-audits/issues/10006 for more context.Screenshots
toggle-switch.mov
Integration
No prod changes required.
List the issues that this change affects.
Risk Assessment
What approach did you choose and why?
I explored two approaches for allowing users to optionally apply the
autofocusattribute to the ToggleSwitch button element:button_argumentsargument, allowing users to pass any supported attribute to the buttonautofocusargument, so users can apply this single attribute directlyI opted to use the second approach, as it is the minimum viable change required to make this component accessible when used with turbo-rails. Using a more surgical, targeted approach seemed safer and easier to maintain/test than the more permissive, open-ended first approach. This also seemed more in line with Primer view component design principles.
Anything you want to highlight for special attention from reviewers?
button_argumentsapproach? 🤔 I didn't see this kind of argument in many existing components; however, it does resemble generic system arguments, which are allowed for all components (if not the child elements they render, necessarily).autofocuson a toggle switch button? It is an allowed attribute for buttons in general, but I wasn't sure if there was some reason we didn't already support it here.Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.