#7348 closed enhancement (fixed)
Accessibility: Group related form elements within fieldsets in Profile > Edit screen
Reported by: | mercime | Owned by: | mercime |
---|---|---|---|
Milestone: | 2.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Extended Profile | Keywords: | has-patch early |
Cc: | rian@… |
Description
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
Attachments (9)
Change History (35)
#2
@
8 years ago
@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.
#3
@
8 years ago
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" id="see-field_67_public" 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" checked="checked"> <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 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 :-)
#5
@
8 years ago
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 label
s:
"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 :)
#7
@
8 years ago
- Keywords has-patch added
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 fieldset
s 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 thelegend
s in the newfieldset
s, then surrounded the same content in thelabel
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 respectivelabel
elements to serve as thelegend
s for the newfieldset
s and addedaria-labelledby
attribute on each of theinput
,textarea
, andselect
fields pointing to thelegend
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 id
s 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
#9
@
8 years ago
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.
#10
@
8 years ago
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)
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
8 years ago
#13
@
7 years ago
- Keywords changed from has-patch, early to has-patch early
@mercime Are any of these patches commit worthy at this stage or should we punt to 3.0?
@
7 years ago
Descriptions moved above profile visibility change set and closer to form controls described
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
7 years ago
#16
@
7 years ago
@mercime are you actually intending committing all these? Because I need to draw a line under open tickets.
#17
@
7 years ago
@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
#18
@
7 years ago
@hnla Would like to commit these but up to you whether you want it in or not for 2.9.
#23
@
7 years ago
- Owner set to mercime
- Resolution set to fixed
- Status changed from new to closed
In 11619:
#24
@
7 years ago
@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.
#25
@
7 years ago
@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 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?