making the gender radio buttons more accessible #1964

alicewriteswrongs · 2016-12-05T15:38:46Z

What are the relevant tickets?

What's this PR do?

Hopefully this makes the gender radio buttons more accessible, by giving them proper labels and so on.

@singingwolfboy I would love to get your feedback on this, since I'm not sure this is the proper way to address this. MaterialUI is a bit frustrating to deal with here.

codecov-io · 2016-12-05T16:44:01Z

Current coverage is 91.49% (diff: 100%)

Merging #1964 into master will decrease coverage by <.01%

@@             master      #1964   diff @@
==========================================
  Files           290        290          
  Lines         20631      20635     +4   
  Methods        2508       2510     +2   
  Messages          0          0          
  Branches       1826       1825     -1   
==========================================
+ Hits          18877      18879     +2   
+ Misses         1726       1724     -2   
- Partials         28         32     +4

Powered by Codecov. Last update d62fd53...5351b2e

rhysyngsun · 2016-12-06T14:40:59Z

static/js/util/profile_edit.js

      />
    );
  });

  const value = String(_.get(profile, keySet));
  return (
-    <div className={validationErrorSelector(errors, keySet)}>
+    <div
+      role="group"


I think radiogroup would be better here. Could you also add a aria-labelledby attribute here that points to the profile-radio-group-label just below? That will make screen readers will annoucne this section as "Gender Radio Button Group".

singingwolfboy · 2016-12-06T14:55:30Z

In order for this to be properly accessible, these three <input> elements need to be wrapped inside of a <fieldset> element, with a <legend> element that describes the choice that the user is making. As an example, notice how the day/month/year fields are wrapped in a <fieldset> with a <legend> element, to inform screen readers that these three fields are all closely related to each other.

singingwolfboy · 2016-12-06T14:49:05Z

static/js/util/profile_edit.js

@@ -48,8 +48,9 @@ export function boundRadioGroupField(keySet: string[], label: string, options: O
    if (obj.helper) {
      helper = `${obj.helper}`;
    }
+    let labelId = `gender-option-${obj.label}`;


This doesn't work properly if obj.label has spaces in it, such as "Other/Prefer not to say".

singingwolfboy · 2016-12-06T14:53:19Z

static/js/util/profile_edit.js

@@ -62,13 +63,17 @@ export function boundRadioGroupField(keySet: string[], label: string, options: O
        labelStyle={styles.labelStyle}
        value={obj.value}
        label={label}
+        aria-labelledby={labelId}


It's better to use a <label> element rather than aria-labelledby if possible. You can nest the <input> element inside of the <label> element, or use the for attribute on the label to point to the element ID that is being labelled. Using <label> also makes the browser automatically understand that when the user clicks on the label text, the <input> field should be selected -- although it looks like Material UI is manually recreating that behavior using Javascript.

material-ui is a whole ball of wax here...I'll wrap the RadioButton in a label tag, since when you pass the label prop in to RadioButton it doesn't seem to get properly connected to the input.

Unfortunately I don't think we can do something like:

<label> A label <RadioButton prop={val} /> </label>

because we render those RadioButtons as the children of a RadioButtonGroup. RadioButtonGroup assumes that all of it's children are RadioButton elements at the top level...annoying.

Not sure what to do here. When I set the label attribute on RadioButton it doesn't render the input inside of the label element OR use aria-labelledby to point the screenreader to the appropriate label, at least as far as I can tell. Anyway, tryna figure out what to do here.

What I ended up doing is passing a React element to the label prop on RadioButton, which is then rendered sort of next to it. I can then set aria-labelledby on the actual input in order to point to that label.

rhysyngsun · 2016-12-06T15:20:23Z

@singingwolfboy if you'd like to take over review on this feel free to reassign

alicewriteswrongs · 2016-12-12T16:42:01Z

@singingwolfboy I updated to use <fieldset> and <legend> elements, let me know what you think!

singingwolfboy

This works great from an accessibility point of view: a screen reader reads out the radio buttons perfectly. However, the labels are bold now -- is that intentional? Is that something that might be resolved just by rebasing onto master?

alicewriteswrongs · 2016-12-12T18:43:52Z

oops, I think I accidentally forgot to add the right class back onto the label element.

alicewriteswrongs · 2016-12-12T18:46:04Z

just added it back real quick

singingwolfboy · 2016-12-12T18:48:28Z

Now the labels for the radio buttons are no longer bold, but all the dropdown menus still are bold.

alicewriteswrongs · 2016-12-12T18:49:34Z

oh that was a silly CSS error. two seconds.

singingwolfboy · 2016-12-12T18:56:32Z

All better!

This wraps the whole affair with a `<fieldset>` tag, adds a legend, and ensures that each option has a label that is associated with it. closes #1849 pr #1964

odlbot temporarily deployed to micromasters-ci-pr-1964 December 5, 2016 15:38 Inactive

alicewriteswrongs force-pushed the ap/accessible-gender branch from ed4b79f to 8958485 Compare December 5, 2016 15:42

alicewriteswrongs deployed to micromasters-ci-pr-1964 December 5, 2016 15:42 View deployment

alicewriteswrongs added the Needs Review label Dec 5, 2016

rhysyngsun self-assigned this Dec 6, 2016

rhysyngsun requested changes Dec 6, 2016

View reviewed changes

rhysyngsun added Waiting on Author and removed Needs Review labels Dec 6, 2016

singingwolfboy suggested changes Dec 6, 2016

View reviewed changes

singingwolfboy assigned singingwolfboy and unassigned rhysyngsun Dec 6, 2016

alicewriteswrongs force-pushed the ap/accessible-gender branch from 8958485 to 145a638 Compare December 12, 2016 16:22

alicewriteswrongs added Needs Review and removed Waiting on Author labels Dec 12, 2016

singingwolfboy approved these changes Dec 12, 2016

View reviewed changes

alicewriteswrongs force-pushed the ap/accessible-gender branch from 145a638 to 5351b2e Compare December 12, 2016 18:46

Made the profile gender radio buttons more accessible

0be9d6c

This wraps the whole affair with a `<fieldset>` tag, adds a legend, and ensures that each option has a label that is associated with it. closes #1849 pr #1964

alicewriteswrongs force-pushed the ap/accessible-gender branch from 4c244e6 to 0be9d6c Compare December 12, 2016 19:03

alicewriteswrongs merged commit 0be9d6c into master Dec 12, 2016

alicewriteswrongs deleted the ap/accessible-gender branch December 12, 2016 19:04

alicewriteswrongs temporarily deployed to micromasters-ci December 12, 2016 19:15 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

making the gender radio buttons more accessible #1964

making the gender radio buttons more accessible #1964

alicewriteswrongs commented Dec 5, 2016

codecov-io commented Dec 5, 2016 •

edited

rhysyngsun Dec 6, 2016

singingwolfboy commented Dec 6, 2016

singingwolfboy Dec 6, 2016

singingwolfboy Dec 6, 2016

alicewriteswrongs Dec 6, 2016

alicewriteswrongs Dec 6, 2016

alicewriteswrongs Dec 12, 2016 •

edited

rhysyngsun commented Dec 6, 2016

alicewriteswrongs commented Dec 12, 2016

singingwolfboy left a comment

alicewriteswrongs commented Dec 12, 2016

alicewriteswrongs commented Dec 12, 2016

singingwolfboy commented Dec 12, 2016

alicewriteswrongs commented Dec 12, 2016

singingwolfboy commented Dec 12, 2016

making the gender radio buttons more accessible #1964

making the gender radio buttons more accessible #1964

Conversation

alicewriteswrongs commented Dec 5, 2016

What are the relevant tickets?

What's this PR do?

codecov-io commented Dec 5, 2016 • edited

Current coverage is 91.49% (diff: 100%)

rhysyngsun Dec 6, 2016

Choose a reason for hiding this comment

singingwolfboy commented Dec 6, 2016

singingwolfboy Dec 6, 2016

Choose a reason for hiding this comment

singingwolfboy Dec 6, 2016

Choose a reason for hiding this comment

alicewriteswrongs Dec 6, 2016

Choose a reason for hiding this comment

alicewriteswrongs Dec 6, 2016

Choose a reason for hiding this comment

alicewriteswrongs Dec 12, 2016 • edited

Choose a reason for hiding this comment

rhysyngsun commented Dec 6, 2016

alicewriteswrongs commented Dec 12, 2016

singingwolfboy left a comment

Choose a reason for hiding this comment

alicewriteswrongs commented Dec 12, 2016

alicewriteswrongs commented Dec 12, 2016

singingwolfboy commented Dec 12, 2016

alicewriteswrongs commented Dec 12, 2016

singingwolfboy commented Dec 12, 2016

codecov-io commented Dec 5, 2016 •

edited

alicewriteswrongs Dec 12, 2016 •

edited