move collapsible fieldset logic from theme_fieldset hook to form_process_fieldset

It works. The collapsible fieldset "Advanced options" works during install with Garland and other collapsible fieldsets are also working when using Garland as the administration theme.
I re-rolled this just to have the correct line-endings (nothing else changed), I thought it would spare you the monkey job ;)

Which other "edge" cases do you think we should test ?

The patch works for me too. "Advanced options" are back during install. In addition you can now find working collapsible fieldsets here:

• Appearance /admin/appearance
• Block settings /admin/structure/block/configure/system/navigation
• Advanced search /search/node

Thanks for the fix! This looks ready for RTBC.

I had come to the same solution for this.

Patch applies cleanly and does the job.

Is there any way to add tests for this kind of stuff?
It's easy enough to change some markup, ids, classes etc, and forget that some javascript functionality is relying on it.

Oh, yay! Thanks a lot for this, it's been seriously annoying me. :)

Committed to HEAD!

Should the same drupal_add_js() call be removed from seven/template.php:

  /**
   * Override of theme_fieldset().
   *
   * Add span to legend tag, so we can style it to be inside the fieldset.
   */
  function seven_fieldset($element) {
    if (!empty($element['#collapsible'])) {
-    drupal_add_js('misc/collapse.js');

      if (!isset($element['#attributes']['class'])) {
        $element['#attributes']['class'] = array();
      }

Yes, and even more. We can remove the whole

  if (!empty($element['#collapsible'])) {

    if (!isset($element['#attributes']['class'])) {
      $element['#attributes']['class'] = array();
    }

    $element['#attributes']['class'][] = 'collapsible';
    if (!empty($element['#collapsed'])) {
      $element['#attributes']['class'][] = 'collapsed';
    }
  }
  $element['#attributes']['id'] = $element['#id'];

Because adding the javascript without the classes makes no sense.

Here's a revised patch with one important line of code retained in form_process_fieldset() that the last patch had removed:

$element['#attached_js']['misc/collapse.js'] = array();

I believe we need to keep that line of code to keep the collapsible fieldsets working. @Davy, tell me if I'm misunderstanding or if a different change is needed.

@brandonojc correct. copy past error :)

+1 for the patch in 16 from seutje. The code looks good and we reduce duplication without changing functionality. I tested and found that everything works right:

• Install.php 'Advanced options' collapsed fieldset still works
• Appearance /admin/appearance fieldset works and starts out collapsed (the class and id attributes are still correct too)
• Block settings /admin/structure/block/configure/system/navigation still works the same, with one fieldstart open by default and the others collapsed
• Advanced search /search
• Seven, Garland, and Stark themes
• And this plays nice with #541568: Link to expand / collapse fieldsets has poorly accessible link text

Please review. This is just a re-roll of the prior patch. In brief, this change reduces duplicate code without changing functionality.

Tests passed:

• Confirmed the collapsible fieldsets still work in Garland and Seven (eg /search and /admin/appearance)
• Verified in Firefox 3.5 and Safari 4 on Mac

.

Please review this one instead ;-) Sorry about that last one--dumb mistake trying to fix a blank line.

Can anyone give a quick review and test of this?

Hm. I think this makes sense. Makes the theme function a heck of a lot cleaner, anyway. I was originally concerned about that misc/collapse.js might be better as a theme-only thing, but we're already setting that and attach.js in the process function.

Committed to HEAD.

this might have been fixed for most browsers but it is still an issue for Chrome users. The Advanced Options just won't show, don't even show as a hyperlink. This was using the latest stable download for 7.2 on Chrome 12.0.742.91

Still not working on Chrome for version 7.19!
Greetings, Martijn

Still doesn't work on chrome with version 7.36, worked when I switched to firefox though.