theme_fieldset() does not handle '#collapsible', '#collapsed' arguments as claimed

Good work, thanks!

Let's try this...

It works for me too. But why is such an important patch not yet committed?

All of this work already takes place in form_process_fieldset().

Are you directly calling theme('fieldset', ...) ? I think you'd be better off using drupal_render($element).

Okay, on second thought we should really fix up the docs on theme_fieldset() to say that it doesn't use #collapsed and #collapsible, but instead those should already be set as '#attributes' => array('class' => array('collapsed', 'collapsible')) and that one should generally create a fieldset using drupal_render().

And let's get the tags right while we are at it...

still in the docs ... can't edit ...

Subscribe. I was simply trying to use drupal_render() to render a full page, and I created a few fieldsets in the page array, expecting #collapsible and #collapsed to have some effect (they didn't). My original page building code:

  $page['members'] = array(
    '#type' => 'fieldset',
    '#title' => t('Members'),
    '#collapsible' => TRUE, // Doesn't work for regular page arrays.
  );
...
  drupal_render($page);

I had to modify the code to add the library to the page manually, and then set classes on the fieldsets rather than use the fieldsets' actual properties for collapsible/collapsed... surely there's a better way?

  // Allow collapsible form elements.
  drupal_add_library('system', 'drupal.collapse');

  $page['members'] = array(
    '#type' => 'fieldset',
    '#title' => t('Members'),
    '#attributes' => array('class' => array('collapsible')),
  );
...
  drupal_render($page);

I think we need make theme('fieldset', ...) work properly as the documentation states.

Let me see whether I can get JohnAlbin in here to decide -- do you like collapsible/collapsed processed into classes before theming or should we ask anyone overriding to handle those attributes themselves whether they want classes or not etc?

Sorry it took so long to get back to this. I kept trying to find the relevant history for this during random free time. Which I just found. (Thanks for passing this one by me, chx! I think it will save the people affected by this some grief.)

#859602: #cache does not record the #attached declared in child elements shows that you can't use drupal_add_css() and drupal_add_js() inside a theme function because it won't cache properly in the render API cache. The fix in that issue, rightly, did not break an API in D7. Which means the proper fix for this issue in D7 is to correct the documentation for theme_fieldset().

For D8, I would like to see a follow-up issue that deals with the underlying awkwardness. Maybe hook_theme() should register attached js and css?

Also, when is it safe to call drupal_add_css() and drupal_add_js()? How does a themer using a preprocess/process/theme function know if its content will become part of a larger Render API cache? ugh.

This is the patch that fixes the documentation of the function.

This is the patch for using the #collapsible, and the #collapsed attributes in the code, making the code match the documentation.

I am more in favor of the other patch, as this patch just allows the code to use '#collapsible' => TRUE, instead of '#attributes' => array('class' => array('collapsible')). It would make sense if the CSS classes necessary to make a fieldset collapsible would be changed in the future, but I am not really sure there is the necessity of such code.

but I am not really sure there is the necessity of such code

The fieldset element has the ability to be rendered as collapsible. It's not a behavior that is linked to forms, so why only attach the functionality if the fieldset is within a form?
Also, if you choose, at a later date, to change the method that creates collapsibility, it should be handled by the theming function.

And last but not least: does this patch have a chance of making it into core? I know more changes are needed (the process_fieldset function should no longer deal with collapsibility), but it's a change that does make sense to me.

It's not a behavior that is linked to forms, so why only attach the functionality if the fieldset is within a form?

Any form field is usually associated with a form, but there isn't anything that says if that is the case, or not.

What I meant is that '#collapsible' => TRUE makes sense when the classes to use to make a fieldset collapsible can change in future. As the code that handles '#collapsible' is not able to add the necessary JavaScript to the page being served, it makes few sense, to use such property.

Attached is trivial backport to Drupal 7 that works for me.

Well... My naïve Drupal 7 backport i #19 was a bit too naïve...

Although it made fieldsets collapsible it also made fieldsets in vertical_tabs collapsible with the result that collapsed fieldsets in vertical_tabs would be hidden.

The new patch handles that problem by removing #collapsible and #collapsed from elements which are children of a vertical_tabs. That seems kind of messy and is probably not the correct way to handle it.

The D8 patches are obsolete due to #1168246: Freedom For Fieldsets! Long Live The DETAILS.

#4: collapsible-fieldset.1099132.4.patch queued for re-testing.

#4 still works, but JohnAlbin writes in #13:

the proper fix for this issue in D7 is to correct the documentation for theme_fieldset()

So this is working as designed and the work-around is given in the summary.

It is also worth noting in the doco that another work around is to pass in the collapsible classes like:

$element['#attributes']['class'][] = 'collapsible';
$element['#attributes']['class'][] = 'collapsed';

Add issue summary

So in some cases it needs both '#collapsible' => TRUE and

$element['#attributes']['class'][] = 'collapsible';
$element['#attributes']['class'][] = 'collapsed';