feat(toggle): improve accessibility #IX-3949#2405
Conversation
|
Summary of ChangesHello @alexkaduk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accessibility of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the accessibility of the ix-toggle component by making it keyboard-focusable, adding keyboard activation, improving screen reader announcements, and fixing the focus ring. The changes follow modern accessibility best practices, such as using a visually-hidden input and making the host element the focusable switch. The addition of Storybook stories for accessibility is also a great enhancement. I've added a couple of suggestions to further improve keyboard interaction and internationalization. Overall, this is a high-quality contribution.
| }} | ||
| /> | ||
| {!this.hideText && ( | ||
| <ix-typography class="label">{toggleText}</ix-typography> |
There was a problem hiding this comment.
This introduces redundant text to the a11y tree, making screenreader read "On switch on". We should hide it using aria-hidden.
see: https://www.w3.org/WAI/ARIA/apg/patterns/switch/examples/switch/
There was a problem hiding this comment.
thanks for highlighting. here https://www.w3.org/WAI/ARIA/apg/patterns/switch/examples/switch-checkbox/ it states
There was a problem hiding this comment.
at the same time it is required to have aria-label. so did it like this aria-hidden={ariaLabelAttr ? 'true' : undefined}
There was a problem hiding this comment.
@alexkaduk Will discuss this with a11y team and then we can make a decision
| role="switch" | ||
| tabindex={this.disabled ? -1 : 0} | ||
| aria-label={ariaLabel} | ||
| aria-checked={a11yBoolean(this.checked)} |
There was a problem hiding this comment.
For indeterminate state the screenreader currently reads "off". aria-checked also supports the value "mixed" for this case.
There was a problem hiding this comment.
@lzeiml based on this https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/switch_role#description
the switch role does NOT support "mixed" value
it can be done like
`const ariaChecked = this.indeterminate
? 'mixed'
: a11yBoolean(this.checked);
return (
<Host
role={this.indeterminate ? 'checkbox' : 'switch'}
aria-checked={ariaChecked}
`
might it make sense? or no updates might be a way to go?
There was a problem hiding this comment.
@alexkaduk I think the role should be constant. Considering indeterminate state is possible, checkbox role would be warranted here as a general role.
|
2 participants
Toggle Component Accessibility & Keyboard Navigation Fix
💡 What is the current behavior?
display: none, removing it from the tab order. Tab and Space keys had no effect on the toggle.aria-labeland visible text being read.aria-allowed-roleviolation when usingrole="presentation"on<label>element, and didn't account for the intentional nested-interactive pattern.GitHub Issue Number: #3949
🆕 What is the new behavior?
Keyboard Focus & Navigation
position: absolute+clip(notdisplay: none) so assistive technologies can still access it.tabindex={0}when enabled,tabindex={-1}when disabled. The input hastabindex={-1}and forwards focus to the host viaonFocus={() => this.hostElement.focus()}.onKeyDownhandles both Space and Enter keys withpreventDefault()to toggle state without scrolling the page.Focus Ring
:host(:focus), :host(:focus-visible) { outline: none })..switch > .slider) only when the host has:focus-visible(:host(:not(.disabled):focus-visible) .switch > .slider), matching Figma design specs.Screen Reader Accessibility
role="switch",aria-checked, andaria-disabledare set on the Host element.hideText={false}): Noaria-labelis set (value isundefined). The visible text (ix-typography) provides the accessible name naturally.hideText={true}):aria-labeluses the current state text (textOn/textOff/textIndeterminate).aria-labelattribute takes precedence.aria-hidden="true"so screen readers only announce from the Host.Click Handling
<div class="wrapper">has anonClickhandler that callsonCheckedChange()to expand the clickable area for mouse users.onClickwithe.preventDefault()ande.stopPropagation()to:jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions) is suppressed with a comment explaining that keyboard interaction is properly handled by the Host element.Testing
<label role="presentation">(axe violation) to<div class="wrapper">(no semantic role needed).nested-interactivefor toggle preview test IDs viaTOGGLE_AXE_TEST_IDS(now aSetfor O(1) lookup) intesting/framework-tests/src/main.ts:toggletoggle-checkedtoggle-custom-labeltoggle-disabledtoggle-indeterminaterole="switch") with an internal checkbox (aria-hiddenandtabindex={-1}). This is the correct accessible pattern for a custom switch component.📋 Technical Details & Reasoning
Why Host is Focusable (Not the Input)
Screen readers need to announce the switch role and state (e.g., VoiceOver: "Off, switch") instead of the inner checkbox ("unchecked, checkbox"). The input is visually hidden (
position: absolute+clipin SCSS) and hastabindex={-1}; itsonFocusforwards tothis.hostElement.focus()so only the Host receives focus.Why onKeyDown is on the Host
The Host is the focusable element, so Space and Enter must be handled there. The handler:
this.onCheckedChange(!this.checked)to togglee.preventDefault()so the browser doesn't scroll on SpaceWhy Wrapper Div Has onClick
To expand the clickable area for mouse users. The wrapper div is not keyboard-focusable (no tabindex). All keyboard interaction goes through the Host element. This avoids:
Why Input Has preventDefault + stopPropagation
preventDefault(): Prevents the browser from auto-toggling the checkbox (we control the state)stopPropagation(): Stops the click from bubbling up to prevent double-togglingArchitecture
🏁 Checklist
role="switch", proper announcements)textOn,textOff,textIndeterminate) are props that can be customizedpnpm test)pnpm test.ct)preventDefaulttest passespnpm lint)onClickon wrapper div are intentionally suppressed with explanatory commentpnpm build)👨💻 Help & Support
N/A - Ready for review