Label for radio button doesn't have 'for' attribute, thus label functionality doesn't work in IE6
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs review) |
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?
#1
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.
#2
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);
}
#3
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.
#4
here's a patch
#5
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);
}
#6
Please do not try to diff between Drupal 6 and Drupal 7: the style convention for the concatenation operator changed. The patch needs work.
#7
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.
#8
The patch looks good, except for a small indentation problem.
This needs testing on several browsers to ensure that nothing breaks.
#9
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).
#10
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. :)
#11
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.
#12
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?
#13
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?
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.
#14
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.