When you create a form element of #type 'checkboxes', the final group of checkboxes does not get an id. This makes jQuery modifications much more difficult. The parent form-item/form-checkboxes should get an id (ending with -wrapper) that matches the ids of the child checkboxes, less the value part.

I can fix this temporarily with the following javascript, but this should really be fixed in the generating theme function. (I do this as a $(document).ready() instead of a behavior because I need to make sure it runs before some other code I have which moves around groups of checkboxes by id, and I don't know when the behaviors are run.)

$(document).ready(function () {
    $(".form-checkboxes").each(function () {
        var a_checkbox = $("input:checkbox", this).eq(0);
        var the_val = a_checkbox.val();
        var the_id = a_checkbox.attr("id");
        the_id = the_id.slice(0, -(1 + the_val.length)) + "-wrapper";
        if ($(this).prev("label").length == 0) {
            $(this).attr("id", the_id);
        }
        else {
            $(this).parent("div.form-item").attr("id", the_id);
        }
    });
});
CommentFileSizeAuthor
#18 checkboxes_radios_id-v2.patch2.14 KBmgifford
#12 checkboxes_radios_id.patch2.56 KBmoshe weitzman
#8 radios_checkboxes_id.patch2.04 KBDavid_Rothstein

Comments

pukku’s picture

pukku commented

It looks like there is an "unset($element['#id'])" command in theme_checkboxes() that is causing this problem. The following code gets pretty close — I think there is one extraneous div created, but I can't be sure at this hour...

function theme_checkboxes($element) {
  $class = 'form-checkboxes';
  if (isset($element['#attributes']['class'])) {
    $class .= ' '. $element['#attributes']['class'];
  }
  $id_insert = '';
  if (!$element['#title'] && !$element['#description']) {
    $id_insert = ' id="' . $element['#id'] . '-wrapper"';
  }
  $element['#children'] = '<div class="'. $class .'"' . $id_insert . '>'. (!empty($element['#children']) ? $element['#children'] : '') .'</div>';
  if ($element['#title'] || $element['#description']) {
    return theme('form_element', $element, $element['#children']);
  }
  else {
    return $element['#children'];
  }
}
nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

Can you submit this as a proper patch, please? Also, once you do, I would think this should be promoted to "critical."

pukku’s picture

pukku commented

I wish I could, but I don't have a proper patch. I haven't had a chance to figure out what removing that line does, or if there is a better way to do this.

pukku’s picture

pukku commented
Status: Active » Needs review

Hi! I still don't have a setup to create a patch (maybe I'll manage to get this done during DrupalCon), but I have tested that the following works. The only oddity is that the label created by theme_form_element() has a 'for' attribute that doesn't actually correspond to anything, but I don't think that is actually a problem?

The previous version of theme_checkboxes() was

function theme_checkboxes($element) {
  $class = 'form-checkboxes';
  if (isset($element['#attributes']['class'])) {
    $class .= ' '. $element['#attributes']['class'];
  }
  $element['#children'] = '<div class="'. $class .'">'. (!empty($element['#children']) ? $element['#children'] : '') .'</div>';
  if ($element['#title'] || $element['#description']) {
    unset($element['#id']);
    return theme('form_element', $element, $element['#children']);
  }
  else {
    return $element['#children'];
  }
}

A version that will assign an ID properly is:

function theme_checkboxes($element) {
  $class = 'form-checkboxes';
  if (isset($element['#attributes']['class'])) {
    $class .= ' '. $element['#attributes']['class'];
  }
  
  $theme_form_element = ($element['#title'] || $element['#description']);
  $id_string = '';
  if (!$theme_form_element and !empty($element['#id'])) {
    $id_string = ' id="'. $element['#id'] .'-wrapper"';
  }
  
  $element['#children'] = '<div'. $id_string .' class="'. $class .'">'. (!empty($element['#children']) ? $element['#children'] : '') .'</div>';

  if ($theme_form_element) {
    return theme('form_element', $element, $element['#children']);
  }
  else {
    return $element['#children'];
  }
}

If someone can create a patch on this, that would be very appreciated.

Also, someone suggested that this really should be applied against Drupal 7, because this behaviour shouldn't change in Drupal 6, now that it has been released. Do people agree with this? Or could it be incorporated into both Drupal 6 and Drupal 7?

Thanks,
Ricky

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

If it is a real bug, it should be applied to 6.1.

pukku’s picture

pukku commented

Well, I consider it a bug. I had to write a complicated javascript function to work around this so I could properly interact with the DOM...

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Needs review » Needs work

Then we need to get a proper patch done. I'll try to do this tomorrow.

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Needs review
StatusFileSize
new radios_checkboxes_id.patch2.04 KB

Here it is as a patch. I also applied the same exact changes to theme_radios() as theme_checkboxes() -- the two functions are essentially identical and therefore suffer from the same problem.

I'll put this as "code needs review," but it sounds from the discussion above like this still needs a little work to get it to behave properly...

David_Rothstein’s picture

David_Rothstein commented
Title: Checkboxes as a group do not get an 'id' in the final html » Checkboxes/radios as a group do not get an 'id' in the final html
pukku’s picture

pukku commented

Thanks for the patch! I had forgotten about radio buttons, so thank you for remembering. I'm not sure what still needs work -- the only oddity I can see is an errant 'for' attribute; if this is really a huge problem, instead of return theme('form_element'...), the result could be received into a string and post-processed before being returned...

moshe weitzman’s picture

moshe weitzman commented

I also need this for some basic jqeury enable/disable stuff. It works as advertised. Once a D7 version of this patch appears here, I will RTBC it for that version. Then we will discuss backport.

moshe weitzman’s picture

moshe weitzman commented
StatusFileSize
new checkboxes_radios_id.patch2.56 KB

OK, I moved the patch from #231256: checkboxes and radio button wrapper divs do not get an id. This one is for D7

AltaVida’s picture

AltaVida commented

Is there anything I can do to help get this patch backported to D6 ?

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Issue tags: +Needs backport to D6

Adding backport tag

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Needs review » Needs work

Does not apply. It looks like this problem was supposed to have been fixed, yet #453400: "-wrapper" HTML ID not output for #type checkboxes/radios is still an issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Checkboxes/radios as a group do not get an 'id' in the final html » Checkboxes/radios wrapper container does not get an 'id' attribute
Version: 6.x-dev » 7.x-dev

It doesn't seem like anything has been committed to D7 yet.

We probably rather want to add a common #pre_render to change the wrapping element into #type 'container' (http://api.drupal.org/api/function/theme_container/7) for both http://api.drupal.org/api/function/theme_radios/7 and http://api.drupal.org/api/function/theme_checkboxes/7

Skeleton code to base further patches on:

function form_pre_render_container($element) {
  // Apply CSS classes based on the original element's #type.
  if (isset($element['#type']) {
    $element['#attributes']['class'][] = drupal_html_class('form-type-' . $element['#type']);
  }
  if (isset($element['#name']) {
    $element['#attributes']['class'][] = drupal_html_class('form-type-' . $element['#name']);
  }
  // Convert this element into a wrapping container.
  $element['#type'] = 'container';

  return $element;
}

Not sure whether those further CSS classes don't even exist on the element already. In addition to this snippet, this function needs to be appended as #pre_render for #type 'radios' and 'checkboxes' in system_element_info().

mgifford’s picture

mgifford
he/him
Primary language English
commented

Definitely would like to see this get into D7. I can test some code on this issue, but not sure if I'll have time to roll the patch just yet.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new checkboxes_radios_id-v2.patch2.14 KB

Ok, I've rolled up @sun's approach into a #D7 patch. Mostly to keep this moving ahead.

I don't see how this adds id's to the radios & checkboxes elements. I can see that it would add a class, but that's not what they were looking for.

Or maybe Sun was discussing having this patch applied as well as Moshe's changes to theme_checkboxes & theme_radios..

I think I'm just not familiar enough with theme_container which does add in the missing id's..

Anyways, hopefully this helps.

vatavale’s picture

vatavale commented

(subscribe) How about backport to D6?

mgifford’s picture

mgifford
he/him
Primary language English
commented

If we can get it into D7 then we could see about mobilizing to bring it into D6. However, right now we need more reviews of this so that we can bring it into the core of D7.

Please review this patch!

Alex Andrascu’s picture

Alex Andrascu commented

I made a hack for Drupal 6 here #453400: "-wrapper" HTML ID not output for #type checkboxes/radios. Not sure how polished or unpolished is but it got my job done :)

lotyrin’s picture

lotyrin commented

Subscribe. Also interested in this for D6.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: Unassigned » sun
mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue tags: -Needs backport to D6

#18: checkboxes_radios_id-v2.patch queued for re-testing.

lotyrin’s picture

lotyrin commented
Issue tags: +Needs backport to D6

#18: checkboxes_radios_id-v2.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Postponed (maintainer needs more info)

As far as D7 is concerned, we no longer output any "-wrapper" IDs, because in general, HTML IDs are not suitable for a highly modular system like Drupal, and in particular, not compatible with AJAX operations. Instead, D7 implements much better CSS classes, which allow targeting via jQuery.

In other words: All element types do not get an 'id' attribute on the wrapping container in D7. Effectively making this issue won't fix, as aforementioned issue focuses on D6 already.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Just want to put in a reference to this related issue #504962: Provide a compound form element with accessible labels

If there was a fieldset around radios & checkboxes then I think this issue would be resolved.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)
Issue tags: -Needs backport to D6

I guess this bug report is bogus.

There are two containers involved in theming checkboxes/radios: An outer .form-item container, which received a "*-wrapper" HTML ID previously in D6. This ID attribute no longer exists, because we removed all auto-generated *-wrapper IDs everywhere. The other, inner container surrounds the actual radio/checkbox input elements, and, this container gets an especially crafted ID attribute. If I'm not mistaken, then this "inner" container did not exist in D6. But as that container gets an ID, and is the only wrapping container that could/should get one, the topic of this issue is effectively resolved.

#453400: "-wrapper" HTML ID not output for #type checkboxes/radios remains to be discussed for D6.