There is currently a TODO item listed in theme_form_element() in theme.inc:

   else {
    if ($id) {
      $title = "<label for=\"$id\" />";
      // TODO: does this make sense to do?
    }
  }

This does not make sense, and actually results in invalid HTML. According to the HTML spec (http://www.w3.org/TR/html401/interact/forms.html#edef-LABEL), LABEL requires opening and closing tags.

If no title is included for the element, there should be no label. The attached patch simply deletes these lines of code. This pertains to 4.4 and CVS (and previous?). This is marked as critical due to the fact that the output is invalid HTML and some browsers choke (or at least incorrectly handle) items with no closing label tag.

CommentFileSizeAuthor
#4 theme_form_element2.patch625 bytesjhriggs
theme_form_element.patch582 bytesjhriggs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Please check CVS, before stating that such a bug exists in the CVS version :) It was fixed yesterday in CVS and 4.4

jhriggs’s picture

Priority: Critical » Normal

I was working in CVS, but had done an update prior to this commit, I guess.

Regardless, I think that code should be removed rather than just "corrected" by adding the closing tag. It makes no sense to have an empty label, does it? If no title is provided for a theme element, it should not be a labeled. Perhaps the title/label for that item is being positioned "manually" by the theme elsewhere with a slightly different layout than the other form elements.

Reopening.

Gábor Hojtsy’s picture

Since the code changed, your patch will not apply, so there is no real point in setting this bug to the patch status. Also if some theme would like to put the labels elsewhere, the developer is free to define a theme function for the form elements.

jhriggs’s picture

FileSize
625 bytes

OK. A new patch is attached.

I understand that the theme can redefine form-related functions if desired. My point/question is this: does it make any sense to have an empty label? What benefit is that providing to any user agent? This adds nothing with regards to usability or accessibility. It basically results in some bloated code, no? We have enough of that. ;-)

jhriggs’s picture

Additionally, it appears the patch was only applied to 4.4.

Dries’s picture

Committed to HEAD.

Anonymous’s picture