The function theme_form_element shouldn't be sending blank tags if $element['#id'] isn't true.

Accessibility testing by:
http://wave.webaim.org

Identifies this as a problem.

CommentFileSizeAuthor
#16 form.inc_blank_labels_bad_v2.patch831 bytesmgifford
form.inc_blank_labels_bad.patch834 bytesmgifford

Comments

mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue tags: +Quick fix, +Accessibility
johnbarclay’s picture

johnbarclay commented

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

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Issue tags: +CSS

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:

.form-item label {
  display: block;
  font-weight: bold;
}
.form-item label.option {
  display: inline;
  font-weight: normal;
}

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?

mgifford’s picture

mgifford
he/him
Primary language English
commented

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

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

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...

cburschka’s picture

cburschka
they
commented
Status: Needs review » Needs work

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.

Everett Zufelt’s picture

Everett Zufelt commented
Title: Blank Labels shouldn't be output to the screen. » All labels should require a for attributepointing to a unique id

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?

cliff’s picture

cliff
he/him
commented

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

Everett Zufelt’s picture

Everett Zufelt commented

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.

mgifford’s picture

mgifford
he/him
Primary language English
commented

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

flickerfly’s picture

flickerfly commented

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?

Everett Zufelt’s picture

Everett Zufelt commented

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.

mgifford’s picture

mgifford
he/him
Primary language English
commented

@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

      drupal_set_message($message = 'Ack Alert No Element ID here!!', $type = 'error');

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?

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review

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":

<fieldset class="collapsible collapsed" id="edit-content-type-vis-settings"><legend><span>Content type specific visibility settings</span></legend><div class="form-item form-type-checkboxes form-item-types">
 <label for="edit-types">Show block for specific content types </label>
 <div class="form-checkboxes"><div class="form-item form-type-checkbox form-item-types-article">
 <label class="option" for="edit-types-article"><input type="checkbox" name="types[article]" id="edit-types-article" value="article"   class="form-checkbox" /> Article</label>
</div>
<div class="form-item form-type-checkbox form-item-types-page">
 <label class="option" for="edit-types-page"><input type="checkbox" name="types[page]" id="edit-types-page" value="page"   class="form-checkbox" /> Page</label>
</div>
</div>
 <div class="description">Show this block only when on a page displaying a post of the given type(s). If you select no types, there will be no type specific limitation.</div>
</div>
</fieldset>

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new form.inc_blank_labels_bad_v2.patch831 bytes

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

Everett Zufelt’s picture

Everett Zufelt commented

@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.

mgifford’s picture

mgifford
he/him
Primary language English
commented

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.

Everett Zufelt’s picture

Everett Zufelt commented
Status: Needs review » Needs work

@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.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Postponed

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

bowersox’s picture

bowersox commented

subscribing

mgifford’s picture

mgifford
he/him
Primary language English
commented

This needs to be tested to verify that it works as described in D7 before closing this issue.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Postponed » Closed (fixed)

It was fixed in the rewrite of theme_form_element(). No blank labels are being spit out now from this function.