Accessibility: Group related form elements within fieldsets in Profile > Edit screen

[mercime]

Semantic grouping for related form elements is easier for screen readers to use especially where there are a number of form controls to deal with like in the Profile > Edit screen. The Fieldset and Legend provides a logical organization of the different but related form elements we have for each field.

For the checkbox, radio button, and datebox fields:
@dcavins has already worked on surrounding these with fieldsets in #6678. However, we also need to add in the profile visibility settings as well as description in respective fieldsets. We will also bind the aforementioned to the respective form controls.

For the select box, multi select box, text area, number, text box, and URL fields:
we will be surrounding all the relevant form controls including the description and profile visibility settings within fieldsets and bind the latter to the respective form controls.

For this ticket, ​WCAG 2.0 Success Criteria 1.3.1 contains the following Techniques to address the issue:
​H71: Providing a description for groups of form controls using fieldset and legend elements
​H82: Grouping form controls with FIELDSET and LEGEND

[dcavins]

@mercime What is preferred way to handle related fieldsets, like a set of radio buttons and the related visibility settings, which is also a set of radio buttons?

[mercime]

@dcavins :) Thanks for your question and sorry for the delay. I thought it better to show you what I meant with the images below.

We will have a profile visibility fieldset within each existing and new fieldset. It would have been ideal if we didn't have nested fieldsets because of the extra noise in some assistive technologies (AT). However, the impact here for some AT's at most is that the legend of the main fieldset will be repeated along with the legend of the profile visibility for the radio buttons. In other ATs, the legend of the main fieldset is not repeated at all.

As you might have noted in the image above, the field's description has been moved above the profile visibility section. This is because the description, if any, will be bound to the input field by adding an id attribute and value to the <p> description so that the new aria-describedby attribute of the text input field with value pointing to the new id of the <p> description and ATs will hear/learn how that field is described.

Then, since the label of that text input field (all fields except checkboxes, radio buttons, and date fields) is now the legend for the fieldset, we could add an id attribute to the legend then add aria-labelledby attribute to the text input field which points to the id of the legend. Note that I have only checked this with one screen reader and passed as I was making this. Need to double-check how this works with other ATs as like the browsers, they have different behaviors for ARIA attributes and nested fieldsets. I'm open to other suggestions though.

e.g.

<div class="editfield field_67 field_text-field optional-field visibility-public field_type_textbox">
	<fieldset>
		<legend id="legend-67">Text Field</legend> <!-- Add ID to legend -->
		<input id="field-67" name="field-67" type="text" aria-labelledby="legend-67" aria-describedby="description-67"> <!-- Add aria-labelledby and aria-describedby attributes -->
		<p id="description-67" class="description">Description of this text field.</p>

		<p class="field-visibility-settings-toggle" id="field-visibility-settings-toggle-67" tabindex="0"> <!-- add tabindex so that this section is included in tab order and read whether they can change visibility of the field or not when in "forms" mode -->
			This field can be seen by: <span class="current-visibility-level">Everyone</span>
			<button type="button" class="visibility-toggle-link">Change</a> <!-- Change empty link <a href="#">...</a> to <button> in another ticket -->
		</p>

		<div class="field-visibility-settings" id="field-visibility-settings-67">
			<fieldset>
				<legend>Who can see this field?</legend>
				<ul class="radio"> <!-- Checking: do we really need the UL and LIs here? -->
					<li class="public">
						<label for="see-field_67_public">
							<input type="radio" id="see-field_67_public" name="field_67_visibility" value="public" checked="checked">
							<span class="field-visibility-text">Everyone</span>
						</label>
					</li>
					<li class="adminsonly">
						<label for="see-field_67_adminsonly">
							<input type="radio" id="see-field_67_adminsonly" name="field_67_visibility" value="adminsonly">
							<span class="field-visibility-text">Only Me</span>
						</label>
					</li>
					<li class="loggedin">
						<label for="see-field_67_loggedin">
							<input type="radio" id="see-field_67_loggedin" name="field_67_visibility" value="loggedin">
							<span class="field-visibility-text">All Members</span>
						</label>
					</li>
					<li class="friends">
						<label for="see-field_67_friends">
							<input type="radio" id="see-field_67_friends" name="field_67_visibility" value="friends">
							<span class="field-visibility-text">My Friends</span>
						</label>
					</li>
				</ul>
			</fieldset>
			<button type="button" class="field-visibility-settings-close" aria-label="Close to save selection">Close</button> <!-- add aria-label to explain what closing the button will do ---- Change empty link <a href="#">...</a> to <button> in another ticket -->
		</div>
  </fieldset>
</div>

Image below shows the prototype I designed so that you can see the fieldsets around each profile field.

[rianrietveld]

Hi,
Paul Gibbs asked me to look at the proposed structure above and to give my opinion. Thanks @mercime for working on accessibility! I totally agree with using fieldsets and legends. This makes a form so much more understandable for users of assistive technology.

But also: I think code should be semantic by itself and the use of aria to fix non semantic constructions needs to be kept to a minimum. They are not working for all assistive technology (AT) the same.

I'm not familiar with BuddyPress and also don't know how much this code is fixed or legacy and if the screen-reader-text class is present in all the themes. So, if I'm making totally undoable suggestions, please forgive me and please correct me.

Giving the p.field-visibility-settings-toggle a tab-index=0 maybe not necessary: better make the p standalone (inline?) and link it with aria-describedby the button. And an aria-label in the button would also be nice.
Screen-reader users can call a list of buttons and then it's nice if the button describes the action.

If there is a button that toggles a hide/visible element, it's nice to tell a screen reader what the element is that is toggled and if it's expanded or not.
To make this work the aria-expanded="false" should set to true when the div.field-visibility-settings is visible.

The ul/li construction can go, it just adds more clutter to the code.

If you have an implicitly label construction the for/id's are not necessary:

<label>
  <input type="radio" name="field_67_visibility" value="public">
  <span class="field-visibility-text">Everyone</span>
</label>

But better: use an explicit label.

<input type="radio" id="see-field_67_public" name="field_67_visibility" value="public">
<label for="see-field_67_public"><span class="field-visibility-text">Only Me</span></label>

Then you are certain it works on all devices. See ​WordPress code standards.

Here a HTML code example as I think it should be (again, not sure how much of this is legacy code).

<div class="editfield field_67 field_text-field optional-field visibility-public field_type_textbox">
	<fieldset>
		<legend>Text Field options</legend>

		<label for="field-67">Text Field</label>
		<input id="field-67" name="field-67" type="text" aria-describedby="description-67"> 
		<p id="description-67" class="description">Description of this text field.</p>

		<p class="field-visibility-settings-toggle" id="field-visibility-settings-toggle-67" id="toggle-text-67"> 
			This field can be seen by: <span class="current-visibility-level">Everyone</span>
		</p>
		<button type="button" class="visibility-toggle-link" aria-describedby="toggle-text-67" aria-expanded="false" aria-controls="collapsible-67" aria-label="Change visibility of the text label">Change</button>

		<div class="field-visibility-settings" id="field-visibility-settings-67" id="collapsible-67">
			<fieldset>
				<legend>Who can see this field?</legend>

				<input type="radio" id="see-field_67_public" name="field_67_visibility" value="public" checked="checked">
				<label for="see-field_67_public"><span class="field-visibility-text">Only Me</span></label>

				<input type="radio" id="see-field_67_adminsonly" name="field_67_visibility" value="adminsonly">
				<label for="see-field_67_adminsonly"><span class="field-visibility-text">Only Me</span></label>
				
				<input type="radio" id="see-field_67_loggedin" name="field_67_visibility" value="loggedin">
				<label for="see-field_67_loggedin">	<span class="field-visibility-text">All Members</span></label>
				
				<input type="radio" id="see-field_67_friends" name="field_67_visibility" value="friends">
				<label for="see-field_67_friends">	<span class="field-visibility-text">My Friends</span></label>
			</fieldset>

			<button type="button" class="field-visibility-settings-close" aria-label="Save the visibility of the text label">Close</button> 
		</div>
  </fieldset>
</div>

If you want to hide the <label for="field-67">Text Field</label> you can do that with the screen-reader-class.
Since ( I think) WP 4.2 this is a ​WordPress Generated Class
so

<label for="field-67" class="screen-reader-text">Text Field</label>

will hide text from screen but not from a screen reader.

I hope this helps :-)

[mercime]

Replying to rianrietveld:

Hi,
Paul Gibbs asked me to look at the proposed structure above and to give my opinion. Thanks @mercime for working on accessibility! I totally agree with using fieldsets and legends. This makes a form so much more understandable for users of assistive technology.

But also: I think code should be semantic by itself and the use of aria to fix non semantic constructions needs to be kept to a minimum. They are not working for all assistive technology (AT) the same.

@rianrietveld Wow, thank you so much for taking the time from your busy schedule to review and comment on the proposed restructure! Thanks to @DJPaul as well for faciliating this review :)

I'm not familiar with BuddyPress and also don't know how much this code is fixed or legacy and if the screen-reader-text class is present in all the themes. So, if I'm making totally undoable suggestions, please forgive me and please correct me.

The proposed structure is a medley of old and new form elements. Aside from the new fieldset, the button element just committed (#7358) and aria-* are new. Please know that your suggestions are most welcome and we are grateful for your contributions.

Giving the p.field-visibility-settings-toggle a tab-index=0 maybe not necessary: better make the p standalone (inline?) and link it with aria-describedby the button. And an aria-label in the button would also be nice.
Screen-reader users can call a list of buttons and then it's nice if the button describes the action.

If there is a button that toggles a hide/visible element, it's nice to tell a screen reader what the element is that is toggled and if it's expanded or not.
To make this work the aria-expanded="false" should set to true when the div.field-visibility-settings is visible.

Sounds great. Let's see what we can do here. When the change button is pressed, the whole <p class="field-visibility-settings-toggle"... along with the button within disappears.

The ul/li construction can go, it just adds more clutter to the code.

Thank you for answering my question there. I totally agree.

If you have an implicitly label construction the for/id's are not necessary:

<label>
  <input type="radio" name="field_67_visibility" value="public">
  <span class="field-visibility-text">Everyone</span>
</label>

But better: use an explicit label.

<input type="radio" id="see-field_67_public" name="field_67_visibility" value="public">
<label for="see-field_67_public"><span class="field-visibility-text">Only Me</span></label>

Then you are certain it works on all devices. See ​WordPress code standards.

You'll find that all implicitly labelled fields in the BuddyPress codebase do have the for/id to make the explicit association because when I went through all of the forms during the 3rd-4th quarter of 2015, I followed the ​Mozilla Developer Network guideline when there were implicit labels:
"A <label> element is bound to its widget with the for attribute. The for attribute references the id attribute of the corresponding widget. A widget can be nested inside its <label> element but even in that case, it is considered best practice to set the for attribute because some assistive technologies do not understand implicit relationships between labels and widgets."

Having said that, I concur that it's better to have explicit labelling without nesting. Therefore, moving forward and starting with this ticket, we'll proceed with such structure and fix others along the way.

Here a HTML code example as I think it should be (again, not sure how much of this is legacy code).

 <div class="editfield field_67 field_text-field optional-field visibility-public field_type_textbox">
 	<fieldset>
 		<legend>Text Field options</legend>
 
 		<label for="field-67">Text Field</label>
 		<input id="field-67" name="field-67" type="text" aria-describedby="description-67"> 
 		<p id="description-67" class="description">Description of this text field.</p>
		...
		...
   </fieldset>
 </div>

Thank you. It must have surprised you to see the aria-labelledby to the legend in the proposed structure :) I used the label generated as the legend for a quick fix. It was more of finding a backward compatible solution to legacy code for 6 out of the 9 different types of profile fields that BP has for the member profile screens. Following is the original rendered source code:

<div class="editfield field_67 field_text-field optional-field visibility-public field_type_textbox">

	<label for="field_67">Text Field</label>
	<input  id="field_67" name="field_67" type="text" value="">

	<p class="field-visibility-settings-toggle" id="field-visibility-settings-toggle-67">
		This field can be seen by: <span class="current-visibility-level">Everyone</span>
		<a href="#" class="visibility-toggle-link">Change</a>
	</p>

	<div class="field-visibility-settings" id="field-visibility-settings-67">
		<fieldset>
			<legend>Who can see this field?</legend>
			<ul class="radio">
				<li class="public">
					<label for="see-field_67_public">
						<input type="radio" id="see-field_67_public" name="field_67_visibility" value="public"  checked='checked' />
						<span class="field-visibility-text">Everyone</span>
					</label>
				</li>
				<li class="adminsonly">
					<label for="see-field_67_adminsonly">
						<input type="radio" id="see-field_67_adminsonly" name="field_67_visibility" value="adminsonly"  />
						<span class="field-visibility-text">Only Me</span>
					</label>
				</li>
				<li class="loggedin">
					<label for="see-field_67_loggedin">
						<input type="radio" id="see-field_67_loggedin" name="field_67_visibility" value="loggedin"  />
						<span class="field-visibility-text">All Members</span>
					</label>
				</li>
				<li class="friends">
					<label for="see-field_67_friends">
						<input type="radio" id="see-field_67_friends" name="field_67_visibility" value="friends"  />
						<span class="field-visibility-text">My Friends</span>
					</label>
				</li>
			</ul>
		</fieldset>
		<a class="field-visibility-settings-close" href="#">Close</a>
	</div>

	<p class="description">Description of this text box.</p>
</div>

I hope this helps :-)

Your astute comments definitely helped. Thanks once again @rianrietveld!

I'll prepare the first pass of the patches for the 9 extended profile fields over the next few days :)

[mercime]

Attached patches 7348.patch and 7348-alt.patch have the same changes except for #2 below with explanations for the differences.

1/ Extended the fieldset already wrapped around the radio, checkbox, and datebox fields to include the other form elements related to the aforementioned fields.

2/ Created new fieldsets to surround the select box, multi select box, text area, number, text box, and URL fields.

• in 7348.patch I re-used the content of label elements as the content of the legends in the new fieldsets, then surrounded the same content in the label element with <span class="bp-screen-reader-text"> to hide the same info from sighted users.
• in 7348-alt.patch I removed the label element of each and appropriated the content of the said respective label elements to serve as the legends for the new fieldsets and added aria-labelledby attribute on each of the input, textarea, and select fields pointing to the legend element.

3/ Removed the UL and LI surrounding the radio controls of the profile field visibility settings. Attached screenshot shows the before and after. ​https://cldup.com/ST3qQNREro.jpg

4/ Kept the Change button within p.field-visibility-settings-toggle and added <span id="<?php bp_the_profile_field_input_name(); ?>-2"> to surround the text within the paragraph which the Change button will point to in its aria-describedby value.

5/ Moved up the field description to right under the field control and above the profile field visibility settings fieldset.

6/ Connected the field controls with respective aria-describedby dynamic values pointing to the new dynamic ids of the field descriptions.

7/ Updated the localization of the Change button on the Profile > Edit template for parity with the same element on the Register template.

8/ Added role="button" to the Clear links

[dcavins]

Hi @mercime-

Thanks for working on this. It'll be great to take a uniform approach to all of these inputs.

In the patch, there are some changes that I can't figure out, like removing the closing </fieldset> tags in src/bp-xprofile/classes/class-bp-xprofile-field-type-checkbox.php, src/bp-xprofile/classes/class-bp-xprofile-field-type-datebox.php and src/bp-xprofile/classes/class-bp-xprofile-field-type-radiobutton.php, but the opening <fieldset> tags are still in place. In other files, new <fieldset> tags are opened but never closed--is there some universal closer being used? (I'm guessing I don't understand how the outputs are completely built.)

Thanks again for taking this on.

[mercime]

Thank you @dcavins :) In the new patch attached, I did not add indentations for new legends et. al. so the revisions will be clearer (can add them later). Patch includes some improvements from the first patch and the following:

1/ Added closing </fieldset> tags in the Register and Profile > Edit files. Thinking whether we should move the opening <fieldset> tags there as well.

2/ Added aria-expanded="false" to <button> element for the default closed div.field-visibility-settings and when the panel is closed. The attribute value is changed to aria-expanded="true" when div.field-visibility-settings is toggled open. Added aria-label attributes and values to the buttons as well.

3/ When available, the description is placed before a group of form controls
(checkboxes, radio buttons, dateboxes, and text area/wp_editor)

4/ When available, the description is placed after a form control
(select box, multi select box, text box, number and URL fields)

[hnla]

@mercime Are any of these patches commit worthy at this stage or should we punt to 3.0?

[mercime]

Descriptions moved above profile visibility change set and closer to form controls described

[hnla]

Is this a commit then? or would you rather punt to 3.0?

[hnla]

@mercime are you actually intending committing all these? Because I need to draw a line under open tickets.

[mercime]

@hnla Instead of one big patch, I've separated the changes needed into separate stand-alone patches:

7348-remove-ul-wrapper.patch​ - removes the UL wrapper for the change profile visibility radio buttons

7348-move-desc-close-to-form-controls.patch​ - moves the descriptions from under the change profile visibility section to either above or below the profile form controls. descriptions-moved-above-ch.png​ shows:

• Descriptions are moved right above multi-control fields such as the radio, checkbox, and datebox fields with tabindex="0" so that it will be read when screen readers tab through the form when in form mode.
• Descriptions are moved right below single field controls such as the multiselect, number, selectbox, textarea, textbox, and url fields. All descriptions are associated with respective form controls with the aria-describedby attribute.

7348-outer-fieldset-structure.patch​ - shows the new structure for the outer fieldset surrounding the form control/s with respective change profile visilibity fieldset.

7348-visibility-toggle-button.patch​ - deals with the button's changing values for the aria-expanded attr

[mercime]

@hnla Would like to commit these but up to you whether you want it in or not for 2.9.

[hnla]

Not up to me I've held things open so if ready to and happy to, commit away.

[mercime]

In 11616:

xProfile: Remove UL wrapper surrounding change profile visibility radio buttons.

See #7348.

[mercime]

In 11617:

xProfile: Move descriptions to right beside respective form controls.

See #7348.

[mercime]

In 11618:

xProfile: Add fieldset to group related form controls.

See #7348.

[mercime]

In 11619:

xProfile: Add and toggle aria-expanded attr for change-profile-visibility button.

Fixes #7348.

[Offereins]

@mercime The recent changes in r11617 and r11618 can really mess up styling and custom template overrides for the XProfile component, as well as for custom field types (since the description is now suddenly in the field type, not in the template)!

That will certainly need some highlighting in the beta/release communications.

[mercime]

@Offereins Noted. The accessibility updates have been mentioned by @hnla in his announcements:
​https://buddypress.org/2017/06/buddypress-2-9-0-beta-2/
​https://bpdevel.wordpress.com/2017/06/24/buddypress-2-9-beta-2/

[mercime]

In 11645:

Add ARIA attr to associate single field with description.

Fixes #7348.