When I add title to a radio button, this way:

$form['select_consultant']['choices'][$number] = array(
	'#id' => 'radio'.$radio_id,
	'#type' => 'radio',
	'#title' => $title,
	'#return_value' => $number,
	'#default_value' => 0,
	'#parents' => array('choices'),
	'#spawned' => TRUE,
);

the output gets rendered as 

<label class="option"><input type="radio" name="choices" value="1" class="form-radio" />something</label>

but Label functionality (when I click the label's text, the field gets selected) doesn't work this way in IE6. When I put in the for attribute, it works again perfectly in IE6:

<label class="option" for="radio1"><input type="radio" name="choices" value="1" id="radio1" class="form-radio" />something</label>

I know that according to W3C HTML 4.01 recommendation it doesn't need to have for attribute, but can we add it in? 60% of normal people still use IE6 and they would welcome the usability improvement.

W3C:
for = idref [CS]
This attribute explicitly associates the label being defined with another control. When present, the value of this attribute must be the same as the value of the id attribute of some other control in the same document. When absent, the label being defined is associated with the element's contents.

I corrected the theme_radio function in includes/form.inc to add for attribute, but I have no idea how to make a patch - if someone made it and posted it here, i'd be grateful.

function theme_radio($element) {
  _form_set_class($element, array('form-radio'));
  $output = '<input type="radio" ';
  $output .= 'name="'. $element['#name'] .'" ';
  $output .= 'value="'. $element['#return_value'] .'" ';
  if (!empty($element['#id'])) { //add this if
	$output .= 'id="'. $element['#id'] .'" ';
  }
  $output .= (check_plain($element['#value']) == $element['#return_value']) ? ' checked="checked" ' : ' ';

  $output .= drupal_attributes($element['#attributes']) .' />';
  if (!is_null($element['#title'])) { 
	if (!empty($element['#id'])) { //and this one - only add for if we know element's id
		$output = '<label class="option" for="'. $element['#id'] .'">'. $output .' '. $element['#title'] .'</label>';
	} else {
		$output = '<label class="option">'. $output .' '. $element['#title'] .'</label>';
	}
  }

  unset($element['#title']);
  return theme('form_element', $element, $output);
}

What do you think about adding this to Drupal?

CommentFileSizeAuthor
#15 issue-261284-15.patch3.28 KBlilou
#14 261284.patch3.23 KBroychri
#11 261284.patch3.38 KBroychri
#7 form.patch1.58 KBkeff
#4 radio_for.patch1.45 KBstevenpatz

Comments

damien tournoud’s picture

damien tournoud commented
Version: 6.2 » 7.x-dev

This looks reasonable. Could you provide a proper patch?

Reassigning to the development queue (7.x) because this bug is also there. Once corrected, a back-port will have to be made for Drupal 6.

keff’s picture

keff commented

The same goes for checkbox (it seems strange to me that radio buttons can be without id, but theme_checkbox uses $element[#id] without checking for emptiness - does anyone know why? Also phpdoc comment for it doesn't say that id is used...):

corrected:

/**
 * Format a checkbox.
 *
 * @param $element
 *   An associative array containing the properties of the element.
 *   Properties used:  title, value, return_value, description, required //<-see? no id here
 * @return
 *   A themed HTML string representing the checkbox.
 *
 * @ingroup themeable
 */
function theme_checkbox($element) {
  _form_set_class($element, array('form-checkbox'));
  $checkbox = '<input ';
  $checkbox .= 'type="checkbox" ';
  $checkbox .= 'name="'. $element['#name'] .'" ';
  $checkbox .= 'id="'. $element['#id'] .'" ' ;
  $checkbox .= 'value="'. $element['#return_value'] .'" ';
  $checkbox .= $element['#value'] ? ' checked="checked" ' : ' ';
  $checkbox .= drupal_attributes($element['#attributes']) .' />';

  if (!is_null($element['#title'])) {
    if (!empty($element['#id'])) {
      $checkbox = '<label class="option" for="'. $element['#id'] .'">'. $checkbox .' '. $element['#title'] .'</label>';
    } else {
      $checkbox = '<label class="option">'. $checkbox .' '. $element['#title'] .'</label>';
    }
  }

  unset($element['#title']);
  return theme('form_element', $element, $checkbox);
}
keff’s picture

keff commented

Thanks. Right now I cannot make patch - I have no idea how to do it, and during exam time, I don't have the time to install and understand all the infrastructure needed to create patch. If someone made a patch, I'd be very grafeful.

stevenpatz’s picture

stevenpatz
he/him
commented
Status: Active » Needs review
StatusFileSize
new radio_for.patch1.45 KB

here's a patch

keff’s picture

keff commented

Thanks! And, could you do another one for checkbox? Here is the code with corrected indentation:

/**
 * Format a checkbox.
 *
 * @param $element
 *   An associative array containing the properties of the element.
 *   Properties used:  title, value, return_value, description, required
 * @return
 *   A themed HTML string representing the checkbox.
 *
 * @ingroup themeable
 */
function theme_checkbox($element) {
  _form_set_class($element, array('form-checkbox'));
  $checkbox = '<input ';
  $checkbox .= 'type="checkbox" ';
  $checkbox .= 'name="'. $element['#name'] .'" ';
  $checkbox .= 'id="'. $element['#id'] .'" ' ;
  $checkbox .= 'value="'. $element['#return_value'] .'" ';
  $checkbox .= $element['#value'] ? ' checked="checked" ' : ' ';
  $checkbox .= drupal_attributes($element['#attributes']) .' />';

  if (!is_null($element['#title'])) {
    if (!empty($element['#id'])) {
       $checkbox = '<label class="option" for="'. $element['#id'] .'">'. $checkbox .' '. $element['#title'] .'</label>';
    } else {
       $checkbox = '<label class="option">'. $checkbox .' '. $element['#title'] .'</label>';
    }
  }

  unset($element['#title']);
  return theme('form_element', $element, $checkbox);
}
damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

Please do not try to diff between Drupal 6 and Drupal 7: the style convention for the concatenation operator changed. The patch needs work.

keff’s picture

keff commented
Status: Needs work » Needs review
StatusFileSize
new form.patch1.58 KB

OK, so I looked into it, made a patch against actual D7 (with D7 conventions) - drupal 'creating patches howto' was a big help - and tested the improved functionality in IE6.

damien tournoud’s picture

damien tournoud commented

The patch looks good, except for a small indentation problem.

This needs testing on several browsers to ensure that nothing breaks.

keff’s picture

keff commented

Which ones? I tried to stick to D7 style.

As for testing, it worked for me in IE6, IE7 and Opera 9, i tried IE 5.5 with IETester ( http://www.my-debugbar.com/wiki/IETester/HomePage ), but drupal's javascript was soo broken there that I had problems just navigating the admin (but that may be caused by experimental nature of IETester as well).

dries’s picture

dries commented
Status: Needs review » Needs work

There are still some coding style issues with this patch. Also, it would be good if we could add some code comments. Not everyone knows what the for-attribute is used for.

Lastly, it would be great if you could fix your own issues/questions from comment #2. :)

roychri’s picture

roychri commented
Status: Needs work » Needs review
StatusFileSize
new 261284.patch3.38 KB

phpdoc updated.
#id emptyness for checkbox fixed.
coding style fixed (checked using code-style.pl).
code comment about for-attributed added.

tested against latest version of drupal7. Performed an installation and then visited the admin/build/themes page where there's a lot of checkboxes and radio buttons.

no errors.

keff’s picture

keff commented

Thank you! I use this on one of my websites and I haven't encountered any problem with it, and since it is rather cosmetic update, I think it is ready to be put to D7 trunk.
Do you agree?

drewish’s picture

drewish commented
Status: Needs review » Needs work

doesn't apply any more.. there's also some spelling and grammar problems with the comments:

+       // Using the 'for' attribute for the label allow the user
+       // to select the radio by clicking on the label text.
+       // IE6 requires the for attributes even though the
+       // W3C HTML 4.01 specifications says it is optionnal.

I don't think we need to mention IE6 since we're following the specification. How about this instead?

The 'for' attribute associates the label with the radio button allowing the user to select the radio button by clicking on the label's text.

And for the checkbox comment you duplicated the radio button comment:

+       // Using the 'for' attribute for the label allow the user
+       // to select the radio by clicking on the label text.
+       // IE6 requires the for attributes even though the
+       // W3C HTML 4.01 specifications says it is optionnal.
The 'for' attribute associates the label with the check box allowing the user to toggle the check box by clicking on the label's text.
roychri’s picture

roychri commented
Status: Needs work » Needs review
StatusFileSize
new 261284.patch3.23 KB

I am able to apply the patch in #11 to D7 CVS and to the tarball: http://ftp.drupal.org/files/projects/drupal-7.x-dev.tar.gz
I am doing it like this:

$ wget http://ftp.drupal.org/files/projects/drupal-7.x-dev.tar.gz
--20:34:15--  http://ftp.drupal.org/files/projects/drupal-7.x-dev.tar.gz
           => `drupal-7.x-dev.tar.gz'
Resolving ftp.drupal.org... 64.50.238.52, 64.50.236.52
Connecting to ftp.drupal.org|64.50.238.52|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1,236,083 (1.2M) [application/x-gzip]

100%[=======================================>]  1,236,083    456.71K/s             

20:34:17 (455.63 KB/s) - `drupal-7.x-dev.tar.gz' saved [1236083/1236083]
$ tar xfz drupal-7.x-dev.tar.gz
$ cd drupal-7.x-dev/
$ patch -p0 < /home/roychri/drupal/patches/261284.patch
patching file includes/form.inc
$

What am I doing wrong?

I made the changes to the comments, I like what you propose for the text.
Here is the new patch.

lilou’s picture

lilou commented
StatusFileSize
new issue-261284-15.patch3.28 KB

Patch #14 doesn't applied ... reroll against CVS/HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou commented
Status: Needs work » Needs review

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

catch’s picture

catch
he/him
Primary language English
commented

Looks good, seems like we could have a test which creates the form with radios/checkboxes, then does an assertRaw for the label-for though.

Status: Needs review » Needs work

The last submitted patch failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Closed (duplicate)

Looked at the patch and seems like this is resolved, in core and backported to 6:
http://drupal.org/node/152098

The patch here was a bit more descriptive, but not sure it provides much more functionality. An empty 'for=""' is as good as a none as far as I am aware.

Wished I'd seen this before though.

Mike