Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(toggle): make toggle labels accessible #2885

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented May 30, 2019

Closes #2809

This PR refactors the toggle and small toggle components to have accessible labels.

question for reviewers: Do we want to wrap the component in a div.toggle? Do we also want to support an inline toggle?

Changelog

New

  • class names for accessible toggle version (old classes and markup will be deprecated)

Changed

  • markup structure of toggle label and switch

Removed

Testing / Reviewing

Ensure the toggle and small toggle function and appear correctly

@emyarod emyarod changed the title 2809 voiceover does not announce toggle labels refactor(toggle): make toggle labels accessible May 30, 2019
@emyarod emyarod force-pushed the 2809-voiceover-does-not-announce-toggle-labels branch from ff3c634 to 72905b4 Compare May 30, 2019 17:08
@netlify
Copy link

netlify bot commented May 30, 2019

Deploy preview for the-carbon-components ready!

Built with commit 62654dc

https://deploy-preview-2885--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented May 30, 2019

Deploy preview for carbon-components-react ready!

Built with commit 62654dc

https://deploy-preview-2885--carbon-components-react.netlify.com

@asudoh
Copy link
Contributor

asudoh commented May 30, 2019

Great that you started this effort @emyarod! Any explanation on why you chose to rip/replace the markup/style?

@carbon-design-system carbon-design-system deleted a comment from netlify bot May 31, 2019
@carbon-design-system carbon-design-system deleted a comment from netlify bot May 31, 2019
@emyarod
Copy link
Member Author

emyarod commented May 31, 2019

@asudoh primarily to use proper markup for labeled toggles without breaking how the component appears. The current fieldset and legend structure is not proper usage of those elements

@emyarod emyarod force-pushed the 2809-voiceover-does-not-announce-toggle-labels branch from 72905b4 to 2ed2a44 Compare May 31, 2019 19:33
@carmacleod
Copy link
Contributor

@emyarod Nice! I tested Vanilla (https://deploy-preview-2885--the-carbon-components.netlify.com/?nav=toggle) with JAWS & NVDA in Chrome & FF, and things sound good.

I only noticed 1 problem: for the "Toggle with visible label", please delete the aria-label completely, because:

  1. It is unnecessary, because the <label> has already been properly connected to the <input> using the for attribute in the markup.
  2. It is harder to maintain 2 strings - they can easily diverge because one of them is invisible.
    (even in the example, aria-label="example toggle with visible label" differs from "Toggle with visible label").
  • Differing strings is a problem because they can be misleading to screen reader users, and also because users of speech control systems like Dragon need to be able to say "Click Toggle with visible label", and since the aria-label overrides the visible label, the toggle's name is actually "example toggle with visible label" which is not the same, so the voice command may not work.

@snidersd
Copy link

snidersd commented Jun 3, 2019

@emyarod I have tested it on macOS & VO with the same results as @carmacleod and I agree with her comments.

@emyarod emyarod force-pushed the 2809-voiceover-does-not-announce-toggle-labels branch from 2ed2a44 to c374b03 Compare June 3, 2019 19:35
@emyarod
Copy link
Member Author

emyarod commented Jun 3, 2019

got it @carmacleod @snidersd! I've made those adjustments

vanilla toggle and small toggle should be ready for code and visual review (and need design feedback on the visual changes and possible inline variant before continuing with the React version)

@netlify
Copy link

netlify bot commented Jun 3, 2019

Deploy preview for carbon-elements ready!

Built with commit 62654dc

https://deploy-preview-2885--carbon-elements.netlify.com

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need the checkmark in the small toggle, it should not be deprecated. The spec in the issue that is referenced was from Oct and is out of date. Please reference most up to date specs: https://ibm.ent.box.com/file/334120810490

Screen Shot 2019-06-13 at 11 16 21 AM

@emyarod emyarod force-pushed the 2809-voiceover-does-not-announce-toggle-labels branch from d64b1ca to 0ecd064 Compare June 26, 2019 13:31
@tw15egan tw15egan merged commit fe96203 into carbon-design-system:master Jun 26, 2019
@emyarod emyarod deleted the 2809-voiceover-does-not-announce-toggle-labels branch June 27, 2019 13:33
@jnm2377 jnm2377 mentioned this pull request Jul 15, 2019
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AVT 3 - React & Vanilla Toggle Component: VoiceOver does not announce labels
8 participants