All labels should require a for attributepointing to a unique id

Thanks. This patch works for me. Having accessible out of the box Drupal will get me a long way.

I think you are right when you say empty label tags shouldn't be outputted (they should either have to for attribute or enclose a form element).

My concern is that by removing the label tag, themers will lose a hook to style these elements, resulting in inconsistent styling on forms. System.css itself has some style rules for labels:

And undoubtedly a lot of themes target label. If this gets commited, I think we need to have an alternative way to style these elements... Are there any pages in Drupal core where labels are not associated with a form element?

What about if we wrapped it all in a div tag or had an alternate div tag with would that do it?

There aren't many instances where the label is blank, but there are enough that it is a bit of a concern for screen readers.

There really shouldn't be any instances where labels aren't associated with forms, however I haven't done a survey. Suppose I could grep and double check, but not sure if that would get it all either.

Mike

What about if we wrapped it all in a div tag or had an alternate div tag with would that do it?

div-tags would do the trick from a themers perspective. Unfortunately, Drupal already suffers from divitis and classitis as it is, so I'd hate to see us put in more of those.

There really shouldn't be any instances where labels aren't associated with forms

I have the same feeling. I'm wondering if allowing for this edge case doesn't constitute babysitting broken code. I'd love to get some examples of where this situation occurs in core.

We should consider removing the else-branch altogether (so outputting neither the <label>-tag nor the title text) unless we can come up with a scenario in which having a #title but no #id is required...

Hm, yeah. Are there form elements for which FAPI does not generate an ID attribute? If this is a problem, I'd rather approach it from the end of adding ID attributes to all elements, rather than removing labels from the other ones.

Extra IDs don't hurt anything as long as they are sure to be unique.

Changed name to reflect comment 6 with which I agree.

fapi should never render a label element without a for attribute pointing to the unique id of a field. Semantically, labels label fields and should be programmatically associated.

Can we have an example of where a label without a for-id is being generated?

+1 that this is an issue and therefore an important patch

As I understand it now, the issue is that some theme functions are capable of outputting either a label without the for attribute set, or a label containing no text (or input element with the title attribute set).

If a label is output then:
1. It must have its for attribute set to the unique id of a single form element
2. It must contain text.

#501444: Administer > Modules pages make improper use of form labels attempts to fix this for theme_radio and theme_checkbox by making sure that $element['#title'] is not empty before outputting a label.

This is the same type of issue I was trying to address with the theme_form_element() function. I don't think there should be any instance where a blank label ie. is sent to the browser.

Can anyone think of any instance where a blank label should be produced?

Mike

For clarity, is it a problem for screen readers because it

A) it has a label tag without content
B) the label tag isn't useful

If A, then a check for a blank label and removing the tag seems like the obvious answer. If B, then some further logic needs to be built to figure out what the label should be.

As I understand what I read, it is A which seems like a pretty simple fix that could mimic #501444: Administer > Modules pages make improper use of form labels. This then steps into the question, are the two bugs the same problem in different places? 2 birds, 1 stone?

It sounds like what we need here is some help from someone familiar with FAPI to understand if there is a use case for fapi outputting a form element without a #id. If it is actually the case that fapi does output some form elements without a #id then we can evaluate whether fapi should be modified, and whether we should get rid of the else branch that tests for no #id and outputs a label without a for attribute.

@flickerfly

It is a matter of A. No form should be presented to the page without an attribute ID and text equivalent. Both WCAG 1 & 2 are pretty clear on this.

http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H44.html

Forms aren't going to be accessible if they don't have either labels or proper title attributes:

http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H65.html

Some more reading material:
http://www.jimthatcher.com/webcourse8.htm
http://www.usability.com.au/resources/wcag2/

@Everett

Can you think of a programmatic way to determine what forms don't have element ID's at the moment? Suppose we could just insert into function theme_form_element() something like

Maybe that error message should come up instead of the form?? Where should that error occur if they somehow don't have a form ID?

Ok, so it seems that the problem is the new form_pre_render_conditional_form_element() function which in all of the cases I could find (all checkboxes & radios) performs unset($element['#id']), thus clearing all of the form ID information:
http://api.drupal.org/api/function/form_pre_render_conditional_form_elem...

Now this is good as that label doesn't apply to anything. There is no form id "edit-types":

The current structure does give labeled values for the checkboxes & radios so it should be accessible.

So I think we're back to the original patch I proposed with this issue will fix it correctly. We want the text, but not the label.

Here are the places I found the blank labels:

/admin/structure/types
modules/node/content_types.inc: '#title' => t('Preview before submitting'),
modules/node/content_types.inc: '#title' => t('Default options'),

/admin/structure/block/configure/user/login
modules/block/block.admin.inc: '#title' => t('Show block on specific pages'),
modules/block/block.admin.inc: '#title' => t('Show block for specific roles'),
modules/block/block.admin.inc: '#title' => t('Show block for specific content types'),
modules/block/block.admin.inc: '#title' => t('Custom visibility settings'),

/admin/people/create
modules/user/user.admin.inc: $form['checkboxes'][$rid] = array('#type' => 'checkboxes', '#options' => $options, '#default_value' => isset($status[$rid]) ? $status[$rid] :
modules/user/user.admin.inc: $row[] = array('data' => drupal_render($form['checkboxes'][$rid][$key]), 'class' => array('checkbox'), 'title' => $roles[$rid] . ' : ' . t($key));

/admin/config/media/file-system
modules/system/system.admin.inc: '#title' => t('Default download method'),

/admin/config/development/performance
modules/system/system.admin.inc: '#title' => t('Page cache for anonymous users'),

/admin/config/search/settings/reindex
modules/search/search.admin.inc: '#title' => t('Active search modules'),

There may be other places in core where this might show up, but I did a pretty decent search.

The last submitted patch failed testing.

ok. that was a patch from Feb. Trying against today's CVS.

@mgifford

I see that in the patch you are modifying theme_form_element to not output a label if there is no id for the form element. I don't see how this improves accessibility. Ideally we should be looking to ensure that fapi never creates a form element without an id, so that the second branch of the if statement is never called (and the if statement is therefore unnecessary).

If there is some reason that fapi must create some form elements without an id, then having a label with no for attribute will not hurt accessibility anymore than having no label at all. But, having the label will be useful for theming, even if there is no for attribute.

The content that is being wrapped in a will never have a form ID associated with it. They all come from descriptive text of checkboxes & radios, all of which have labels properly wrapping the elements.

Blank are always bad for accessibility (from all I have been able to tell). This removes this problem, but there are still labels for input forms.

@mgifford

If the checkboxes, radios and date element types are the only types that generate a label with no for attribute, then we need to correct the problem for these three types and get ride of the if condition altogether.

My recommendation would b to work on the patch at #558928: Form element labeling is inconsistent, inflexible and bad for accessibility comment #63, which switches the theme wrapper function for the checkboxes, date and radios elements to theme_fieldset instead of theme_form_element.

Marking as postponed to see what happens with #558928: Form element labeling is inconsistent, inflexible and bad for accessibility

subscribing