-
Notifications
You must be signed in to change notification settings - Fork 399
fix(Toggle): hide on/off test for screen readers #1865
fix(Toggle): hide on/off test for screen readers #1865
Conversation
Deploy preview for carbon-components-react ready! Built with commit 174c972 https://deploy-preview-1865--carbon-components-react.netlify.com |
src/components/Toggle/Toggle.js
Outdated
@@ -56,9 +56,13 @@ const Toggle = ({ | |||
/> | |||
|
|||
<label className={`${prefix}--toggle__label`} htmlFor={id}> | |||
<span className={`${prefix}--toggle__text--left`}>{labelA}</span> | |||
<span className={`${prefix}--toggle__text--left`} aria-hidden="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the linked comment, do we need to be toggling which node has aria-hidden="true"
depending on state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshblack Could you elaborate what you meant here...? To better explain what this PR is all about, @carmacleod wanted to avoid our on/off labels spoken by screen reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought what she was referring to was that both pieces of text were being read, but it should only be the opposite of the currently selected value that is read. Like when toggle is off, it reads on, when the toggle is on, it reads off.
If I understand this diff correctly, it seems like this removes both on and off labels such that they can never be read by a screen reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi - I was asking for what @joshblack said, but come to think of it, removing both "Off" and "On" is better, because having "Off not checked" and "On checked" would be really confusing.
The Toggle does need a label, though, to say what it's toggling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I think I get it now 😄
The label is wrapped in the <fieldset>
which communicates the label for the control. And then, since it's a checkbox, we don't need on/off because it's communicated by the checked state for the checkbox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label is wrapped in the
<fieldset>
which communicates the label for the control.
Ah ok, I see the "Toggle w/ label" example now. :)
It's a bit odd to use fieldset, because folks would expect a group of controls, not just a label and one control. The screen reader reads as "Toggle w/ label grouping, checkbox [not] checked".
So depending on the exact text in the legend, it could be confusing, i.e. probably "Expand tabs" might be ok (people might think that was a single toggle), but "Tab expansion" would not (because it might imply multiple controls).
Consider instead having a span (or div) for the label and use aria-labelledby
to point to the span's id.
Of course, there are other ways to create a toggle or switch control without co-opting the control's label, and that way the label can be used for its proper purpose. 😀
i.e. See Example 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joshblack for clarifying and @carmacleod for the insights.
Consider instead having a span (or div) for the label and use
aria-labelledby
to point to the span's id.
Double-checking - @carmacleod Do you mean <legend className={`${prefix}--label`}>{labelText}</legend>
by "label" and <span className={`${prefix}--toggle__text--left`}>Off</span>
/<span className={`${prefix}--toggle__text--right`}>On</span>
by "span"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I mean completely get rid of the fieldset/legend, and instead have:
<div-or-span class="bx--label" id="toggle2label">Toggle w/ Label</div-or-span>
<div class="bx--form-item">
<input class="bx--toggle" id="toggle2" type="checkbox" aria-labelledby="toggle2label">
<label class="bx--toggle__label" for="toggle2">
<span class="bx--toggle__text--left" aria-hidden="true">Off</span>
<span class="bx--toggle__appearance"></span>
<span class="bx--toggle__text--right" aria-hidden="true">On</span>
</label>
</div>
where div-or-span
is a div or a span (your choice) that has an id,
and the checkbox input uses aria-labelledby
to point to that id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it - Thanks @carmacleod for clarifying! Replaced <fieldset>
/<legend>
with <div>
with aria-labelledby
reference from <input>
.
src/components/Toggle/Toggle.js
Outdated
@@ -18,7 +18,11 @@ const Toggle = ({ | |||
toggled, | |||
onChange, | |||
onToggle, | |||
id, | |||
id = (this.inputId = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this change, I think we could use: https://github.com/IBM/carbon-components-react/blob/master/src/tools/setupGetInstanceId.js for this as well if you want:
import setupGetInstanceId from '../../tools/setupGetInstanceId';
const getInstanceId = setupGetInstanceId();
const Toggle = ({
id = `__carbon-toggle__${getInstanceId()}`
}) => {
// ...
};
Is the usage of this
so that it's consistent across re-renders? Was surprised to see this
in an arrow function which is what lead me to think about other ways to write this 🤔
One note though, I believe Security folks mentioned that default params in arrow functions is something that's unsupported by Edge (or something quirky like that). Do you remember this coming up? I might have to dig if not 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshblack Oops I totally forgot <Toggle>
was stateless, have to convert to a class. Did it and made a change to use your setupGetInstanceId
utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW:
Is the usage of
this
so that it's consistent across re-renders?
Yes. In other words, to avoid redundant DOM updates upon re-rendering.
@joshblack Do you have any further comments...? Thanks! |
Merging as all comments have been addressed or answered. Anybody don't hesitate speak up if there are any concerns - Thanks! |
🎉 This PR is included in version 6.99.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Refs carbon-design-system/carbon#1794.
Changelog
New
aria-hidden
attribute for on/off test in<Toggle>
.