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

RadioButtonGroup description #361

Closed
jonathanolson opened this issue May 14, 2018 · 40 comments
Closed

RadioButtonGroup description #361

jonathanolson opened this issue May 14, 2018 · 40 comments
Assignees

Comments

@jonathanolson
Copy link
Contributor

When working on implementing Area Model's a11y, @zepumph pointed out we may want the description for the RadioButtonGroup to go below its fieldset.

Is this desired? (Could probably easily implement using appendDescription:true)

@terracoda
Copy link
Contributor

terracoda commented May 15, 2018

A fieldset is not the only option for communicating a list of radio buttons. That aside, if descriptive help text is needed, I would think it would be better placed within the fieldset.

However, because of the way VoiceOver counts elements within certain groupings, the count will not sound accurate because VoiceOver will count each item within the group individually, putting the actual count off by one or two (3 items plus a heading plus a paragraph equals 5 things in a group by some AT).

Have you tried just using an un-ordered list? If I am not mistaken, I think that is how BASE's charge radios are implemented. For example, something like this:

<div>
<h3 id="name-for-radiogroup">Name for list of radio buttons</h3>
<p>Optional descriptive help text could go here.</p> 
  <ul role="radiogroup" aria-labelledby="name-for-radiogroup"> 
    <li><input id="radio-01" type="radio" checked><label for="radio-01">Radio 1</label></li>
    <li><input id="radio-02" type="radio" notchecked><label for="radio-02">Radio 2</label></li>
    <li><input id="radio-03"type="radio" notchecked><label for="radio-03">Radio 3</label></li>
  </ul>

<p>Alternatively, optional descriptive help text could go here!</p>
</div>

I think if you use a list structure, the AT will only count the actual list items, making for a nicer experience.

@terracoda
Copy link
Contributor

And, @jonathanolson, I would normally put the help text before the list of radio buttons.

@terracoda terracoda assigned jonathanolson and unassigned terracoda May 15, 2018
@terracoda
Copy link
Contributor

@jonathanolson, please re-assign if you have more questions.

@jonathanolson
Copy link
Contributor Author

Here's currently how the RadioButtonGroup for scene selection looks in area-model (using RadioButtonGroup, and with tabindices removed for readability):

<div id="container-7-207-549-175-184">
  <p id="description-7-207-549-175-184">Switch between a 10x10 and 12x12 board.</p>
  <fieldset id="7-207-549-175-184">
    <div id="container-7-207-549-175-184-176">
      <input id="7-207-549-175-184-176" checked="checked" name="radioButtonGroup1Member" type="radio" style="width: 1px; height: 1px;" class="">
      <label id="label-7-207-549-175-184-176" for="7-207-549-175-184-176">10 by 10 board</label>
    </div>
    <div id="container-7-207-549-175-184-180">
      <input id="7-207-549-175-184-180" name="radioButtonGroup1Member" type="radio" style="width: 1px; height: 1px;">
      <label id="label-7-207-549-175-184-180" for="7-207-549-175-184-180">12 by 12 board</label>
    </div>
  </fieldset>
</div>

It looks like the "recommended" way of handling this would be to change fieldset => ul and the divs inside => li? Can this be done for every similar structure (i.e. every time we use the RadioButtonGroup component)? (@jessegreenberg or @zepumph does that look good ok?)

@terracoda
Copy link
Contributor

@jonathanolson, no need for div's inside the li's. The list item itself wraps the radio button and its label. No need to have 2 wrappers.

@terracoda
Copy link
Contributor

@jessegreenberg, @zepumph, I will re-listen to sample radio button groups that I mocked up a while ago asap and then create an MD file that includes a couple of valid options.

I'm not convinced we need to use fieldset, but I'll look into this.

@jonathanolson
Copy link
Contributor Author

no need for div's inside the li's. The list item itself wraps the radio button and its label. No need to have 2 wrappers.

Yup understood, was just trying to indicate I meant the divs inside the fieldset would change to lis (not the div that is the parent of the fieldset).

@jessegreenberg
Copy link
Contributor

It looks like the "recommended" way of handling this would be to change fieldset => ul and the divs inside => li? Can this be done for every similar structure (i.e. every time we use the RadioButtonGroup component)?

That sounds good to me. @terracoda this change would make it look like you recommended in #361 (comment) (not using a fieldset).

Changed in the above commit, @jonathanolson can you review in aria-model-introduction?

@jonathanolson
Copy link
Contributor Author

Looks good to me.

Current parallel DOM for the scene selection in area-model-introduction:

<div tabindex="-1" id="container-7-586-1037-554-563">
   <p tabindex="-1" id="description-7-586-1037-554-563">Switch between a 10x10 and 12x12 board.</p>
   <ul tabindex="-1" id="7-586-1037-554-563">
      <li tabindex="-1" id="container-7-586-1037-554-563-555"><input id="7-586-1037-554-563-555" tabindex="-1" checked="checked" name="radioButtonGroup5Member" type="radio" style="width: 1px; height: 1px;" class=""><label tabindex="-1" id="label-7-586-1037-554-563-555" for="7-586-1037-554-563-555">10 by 10 board</label></li>
      <li tabindex="-1" id="container-7-586-1037-554-563-559"><input id="7-586-1037-554-563-559" tabindex="-1" name="radioButtonGroup5Member" type="radio" style="width: 1px; height: 1px;"><label tabindex="-1" id="label-7-586-1037-554-563-559" for="7-586-1037-554-563-559">12 by 12 board</label></li>
   </ul>
</div>

I think that's handled everything we wanted for this issue. @terracoda is that correct, or is there further room for improvement?

@jessegreenberg jessegreenberg removed their assignment May 17, 2018
@terracoda
Copy link
Contributor

terracoda commented May 17, 2018

@jonathanolson, the above code is valid, but the group is not identified as a radiogroup and the group does not have an explicit name (not absolutely necessary, but nice to have). Here's an example that sounds much better to me, at least in VoiceOver.

I did the following:

  • changed the description text to a heading and drastically shortened the actual content, to "Board size". Tagging @amanda-phet to ok this change.
  • added role="radiogroup" to the ul to create an explicit role, overwriting the element's implicit role
  • used the id of the h3 and aria-labelledby to associate that h3 with the radiogroup. This gives the radiogroup the name "Board size"
  • removed all the tabindex="-1" just so I could read the code better, you must leave those in as they are needed for implementation purposes.
  • added tabindex="0" to the ul, so I could place focus on it. I think you manage focus with the Javascript, so you might not need that tabindex="0" directly.
<div id="container-7-586-1037-554-563">
    <h3 id="description-7-586-1037-554-563">Board size</h3>
    <ul id="7-586-1037-554-563" role="radiogroup" aria-labelledby="description-7-586-1037-554-563" tabindex="0">
      <li id="container-7-586-1037-554-563-555">
        <input id="7-586-1037-554-563-555" type="radio" checked>
        <label id="label-7-586-1037-554-563-555" for="7-586-1037-554-563-555">10 by 10 board</label>
      </li>
      <li id="container-7-586-1037-554-563-559">
        <input id="7-586-1037-554-563-559" type="radio">
        <label id="label-7-586-1037-554-563-559" for="7-586-1037-554-563-559">12 by 12 board</label>
      </li>
    </ul>
</div>

@amanda-phet, I am tagging you as I made a content design change.

If you would like more explicit help text in addition to the content provided by the new heading with less content, I would suggest adding a paragraph after the radiogroup with content that explains without repeating the label text for the radio buttons. I suggest something like this:

<p>Explore area with different sized boards.</p>

It took me several listens to realize this, sorry I didn't think of it last week during our design sessions :-) Personally, I think you might be able to get away without the additional help text. It is up to you and users to decide :-)

Results in VoiceOver
When focus is on the radiogroup, I hear:
"Board size, radiogroup"

When focus is on the first selected radio, I hear:
"10 by 10 board, selected, radio button, Board size, radiogroup."

@terracoda
Copy link
Contributor

@jonathanolson, one other thing.

The code you pasted had checked="checked", that's xhtml code, not HTML5 code. The HTML5 state attribute is just checked, all by itself. If that is not recognized, I would say we should use the aria attribute, aria-checked="true/false".

Was checked="checked" generated from our API? If so, I should create an issue to fix that. HTML5 not strict in this sense, but I think the proper HTML5 attribute is cleaner.

@jessegreenberg jessegreenberg self-assigned this May 17, 2018
@amanda-phet
Copy link

Much of this conversation seems to be based on dated static description work. What we ended up putting in the design doc is as follows:

Radio Button Group
Accessible Name: Area Grid Size
10 by 10
12 by 12

So, the description text needs to be changed to "Area Grid Size", and the help text can be removed.

@jessegreenberg
Copy link
Contributor

@terracoda I am all for the recommended changes in #361 (comment), except for making the ul focusable. That seems very atypical, what is the reason to add that?

The code you pasted had checked="checked", that's xhtml code, not HTML5 code. Was checked="checked" generated from our API?

We are using the typical JavaScript setter for checked, something like
someHTMLElement.checked = true

the browser is adding checked="checked" for us.

@terracoda
Copy link
Contributor

terracoda commented May 17, 2018

@jessegreenberg, didn't mean to suggest making the ul focusable. I need to do to that in my simple mock-ups, so I can Tab onto groups to see what it sounds like. I am not able to focus on groups otherwise.

Regarding the checked syntax, I guess it's fine if (finishing my post..) that is what is produced by the browser. It doesn't throw any HTML errors ;-)

@jessegreenberg
Copy link
Contributor

I understand now, thanks @terracoda. In the above commit, the aria-labelledby attribute was added, pointing to a default h3 tag. The role=radiogroup was also added.

@jonathanolson back to you, if you added a labelTagName of p in aria-model-introduction, that probably needs to be removed now.

@terracoda
Copy link
Contributor

@jonathanolson, the h3does not replace the radiogroup, it names the radiogroup. In our experience thus far, the Play Area is an h2, so generally objects and groups directly under the Play Area would be named with an h3 if an name is needed.

Is that more clear?

@terracoda
Copy link
Contributor

And for more clarification, headings usually go with other elements containing content. Each heading starts a new section or subsection. Often in our case they are used to name a group of things or an object made up of multiple elements. So a group of radio buttons is usually named by a h3 heading. A custom object like the balloon in Balloons and Static Electricity is a group of elements, so the group too, is named by an h3 heading. In the PDOM these groups and objects are like subsections of content within the Play Area section. The heading is a way to give the object or group a name. The hierarchical heading structure is very useful for scaffolding non-visual users.

@terracoda
Copy link
Contributor

terracoda commented May 25, 2018

@jessegreenberg, @jonathanolson, @mbarlow12, @zepumph the following HTML provides the best described experience (in my opinion in VoiceOver). I moved the radiogroup role to the parent wrapping element.

I'll use this as our design pattern, and then we can test with NVDA and JAWS.

<div aria-labelledby="rg11-heading" role="radiogroup">
   <h3 id="rg11-heading">Charge Settings</h3>
   <ul class="rgroup" aria-labelledby="rg11-heading">
     <li><input id="r11-1" type="radio" checked tabindex="0">
            <label for="r11-1">Show all charges</label>
      </li>
      <li><input id="r11-2" type="radio">
            <label for="r11-2">Show no charges</label>
      </li>
      <li><input id="r11-3" type="radio">
             <label for="r11-3">Show charge differences</label>
      </li>
    </ul>
</div>

Upon focus of the selected radiobutton, I hear the follow:
"Show all charges, selected radio button list Charge Settings 3 items."

We do not hear "radiogroup", but I think "radio button list" is equivalent. We do not get any name repetition and we get the count of the items which I think users find very helpful.

I would guess that in other screen readers we might hear "radiogroup" instead of "radio button list" and still get item count.

@terracoda
Copy link
Contributor

Actually, aria-labelledby is on both the parent wrapper div and the ul. This sounds best in VoiceOver in Safari and Chrome and ChromVox. I can only test on those 2 systems.

It would be nice to verify if there is repetition of the group name in either NVDA and JAWS.

@jessegreenberg
Copy link
Contributor

Discussed during 5/25/18 meeting.

@terracoda is going to keep testing. We were curious about whether aria-labelledby is required on both the parent element and the ul. @terracoda will test it out on VO and then paste some examples here to be tested by someone with access to NVDA and JAWS.

@jessegreenberg jessegreenberg removed their assignment May 25, 2018
@terracoda
Copy link
Contributor

@jessegreenberg, I committed 3 more examples to the radiogroup-examples.html file in a11y-research. There are some differences which I will have to summarize in a post later tonight, sorry for the delay.

@terracoda
Copy link
Contributor

@jessegreenberg, using aria-controls on the parent div also ensures the group's name is included. I am going to run through again using aria-controls to see the differences.

@terracoda
Copy link
Contributor

@jessegreenberg, I take #361 (comment) back. It does not seem to be consistent.

@terracoda
Copy link
Contributor

After repeat testing in VoiceOver & Safari and Chrome & ChromeVox, the best option is actually, html sketch Example 4 that I made using a fieldset, a legend and an ul containing the radio buttons.

For the record, this structure sounds the same as Example 11 see #361 (comment) with the 2 instances of aria-labelledby (one on the list and one on the parent group container).

The reason is that when not using a fieldset as the group's parent, (i.e., a div as the parent wrapper) the differences that were very confusing in earlier testing was that VoiceOver likes aria-labelledby on the ul and ChromeVox likes aria-labelledby on the parent.

And for these two screen readers, the same goes for aria-label. If one does not use a heading as the name for the group and uses aria-label instead, VoiceOver likes aria-label on the ul containing the radio buttons, and ChromeVox likes aria-label on the parent div with the role="radiogroup".

New Recommendations for testing in NVDA and JAWS

<fieldset role="radiogroup">
  <legend>Charge Settings</legend>
  <p>Choose how you see or hear charge information.</p>
  <ul>
    <li><input id="r4-1" type="radio" checked tabindex="0">
           <label for="r4-1">Show all charges</label></li>
    <li><input id="r4-2" type="radio">
          <label for="r4-2">Show no charges</label></li>
     <li><input id="r4-3" type="radio">
   	    <label for="r4-3">Show charge differences</label></li>
   </ul>
</fieldset>

If one replaces the legend with a heading for design reasons, aria-labelledby must be used to create the same experience in VoiceOver and ChromeVox:

<fieldset role="radiogroup" aria-labelledby="rg16-heading">
      <h3 id="rg16-heading">Charge Settings</h3>
      <p>Choose how you see or hear charge information.</p>
      <ul>
		<li><input id="r16-1" type="radio" tabindex="0" checked>
			<label for="r16-1">Show all charges</label>
		</li>
		<li><input id="r16-2" type="radio">
			<label for="r16-2">Show no charges</label>
		</li>
		<li><input id="r16-3" type="radio">
			<label for="r16-3">Show charge differences</label>
		</li>
	</ul>
   </fieldset>

To quote @zepumph, this one was a honker!

@terracoda
Copy link
Contributor

The coded examples for the ARIA Practices 1.1 sound similar, but still different than the raw html, I've posted above.

@jessegreenberg, @zepumph and @mbarlow12 could one of explain to me how they are getting the count semantics even when they do not use a list structure.

Example 2 and example 1, seems to sound exactly the same, but only example 2 uses a list structure.

@terracoda
Copy link
Contributor

@jonathanolson, @jessegreenberg, @zepumph, @mbarlow12 I have updated (and committed) the RadioGroup.md design based on the testing that I did.

I'd like to discuss the further with @jessegreenberg, @zepumph or @mbarlow12.

@terracoda
Copy link
Contributor

The html code in the renamed sun/doc/RadioButtonGroup.md was discussed at the A11y Dev meeting, and it is fine to implement it now. There are two examples in the mark down file.

Please note that I am not sure which method is currently being used to select a radio button, ARIA or HTML. The sample code uses html5's attribute checked. It is well supported, but if we have any issues with the HTML5 checked attribute we should switch to the ARIA equivalent, aria-checked="true/false".

I am un-assigning myself, please re-assign if there are any questions. I will start a new issue for other parts of the mark down file.

@terracoda
Copy link
Contributor

@jessegreenberg could you assign to the appropriate person if it is not you :-)

@jessegreenberg
Copy link
Contributor

Ill work on this, removing the meeting label.

@jessegreenberg
Copy link
Contributor

We should proceed by implementing the HTML in https://github.com/phetsims/sun/blob/master/doc/RadioButtonGroup.md, with the legend with aria-labelledby structure as the default. A heading can be added if necessary, but is not the default so that the heading structure is not impacted by the addition of a RadioButtonGroup by default.

@terracoda
Copy link
Contributor

@jessegreenberg, if you do not want to use a heading with aria-labelledby as the default, I would recommend using the legend as the default label for the fieldset. With a legend element, aria-labelledby is not required. The legend implicitly labels by the fieldset automatically.

Does that make sense?
Then if you want to use anything other than a legend to label, i.e., a heading, you will need to add aria-labelledby if you want it to actually be connected to the fieldset in the accessibility tree.

@marlitas
Copy link
Contributor

marlitas commented Nov 5, 2024

Meeting 11/5/24

  • This has been resolved in another issue. Closing as stale!

@marlitas marlitas closed this as completed Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants