As of Bootstrap 3.3.4, when using multiple collapsible fieldsets on a single page, clicking one single fieldset trigger will cause all fieldset triggers on the page to have its aria-expanded attribute set to true.

This is due to the following change, which means that Bootstrap now expects its href and data-target attibute to be set to an actual ID on the page. https://github.com/twbs/bootstrap/pull/15941/files

Since Bootstrap theme sets the trigger's data-target to a non-ID-based selector, and the actual collapsed region has no ID associated with it, the selector for this.$trigger will be inferred as [data-toggle="collapse"][href="#"],[data-toggle="collapse"][data-target="#"]. This means that every single fieldset header on the page will be treated as a trigger for the current fieldset.

A solution for this would be to assign an ID to the inner area and set the trigger to that instead. Will see if I can put together a patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geoffreyr created an issue. See original summary.

markhalliwell’s picture

Yeah, this will be interesting. Especially considering that changing anything in 7.x-3.x would technically be a BC break (at least I think so, I really don't know anyway around it). FWIW, I actually ran into this myself with the 8.x-3.x changes (which adds an ID to the body content and uses that for the target). Feel free to sift through that if it helps any.

geoffreyr’s picture

I'll take a look, thanks for the pointer.
Even if I do an ID to the inner area, I suspect I'll need to fiddle with _collapse.js as well because of the event handling manipulation performed there.

geoffreyr’s picture

Status: Active » Needs review
FileSize
5.62 KB

I've taken an initial tilt at this. It's not the nicest implementation, but I've successfully tried it against my local Bootstrap-based site. I don't expect it to be incorporated into the project or anything, but if anyone else hits the same problem, it's there if they need it.

markhalliwell’s picture

Status: Needs review » Needs work
Related issues: +#2468625: bootstrap_preprocess_bootstrap_panel() overwrites element id

This is a good initial patch. Setting to CNW though because it seems to also revert the work done in this related issue.

Also one tiny note, use --body and $body_id instead of -inner (just so it's a little in line with what I did in 8.x).

The "target" should really replace the href value too and get rid of data-target. This was why the _collapse.js file had the following lines (58-60) which would no longer be needed:

      $fieldset.find('[data-toggle=collapse]').on('click', function (e) {
        e.preventDefault();
      });
hansfn’s picture

I haven't reviewed the patch, but I can confirm that it fixes the problem on my site which uses multiple collapsible fieldsets on one page. Thx!

markhalliwell’s picture

lazzyvn’s picture

Hello,
all my fieldset does't work with collapsible. i look for bootstrap-panel.vars.php
I don't understand why

// Force grouped fieldsets to not be collapsible (for vertical tabs).
  if (!empty($element['#group'])) {
    $variables['collapsible'] = FALSE;
    $variables['collapsed'] = FALSE;
  }

i don't have any vertical tabs but i have alots of fieldset
$element['#collapsible'] is always true

i think somthing wrong this code

if (!empty($element['#group']))

it's missing condition for vertical tabs
p/s delete this code everything works right (because i don't have vertical tabs)

m.stenta’s picture

I stumbled across this issue myself when I tried adding a CSS chevron to collapsible panels to indicate whether it's open or closed (and help indicate to users that collapsed fieldsets actually open up to show more stuff). FWIW this is roughly the CSS I'm using for that: https://codepen.io/tofanelli/pen/waadRY

What I discovered upon adding that CSS, however, was that clicking ANY fieldset would cause the glyphicon to change on ALL fieldsets, regardless of whether they were collapsed or not. Further investigating brought me to this issue.

I tested the patch in #4 and it does seem to fix the problem - but I'm still seeing some quirkiness: fieldsets that are starting out as collapsed don't have the "collapsed" class, so the glyphicon starts out indicating that the fieldset is open, when it's not. I'm not sure if that's related to this, or if it's caused by something else. And, as @markcarver pointed out: it might be reverting some of the changes from #2468625: bootstrap_preprocess_bootstrap_panel() overwrites element id.

I'll try to do some more investigating and see if I can iterate on @geoffreyr's patch.

m.stenta’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
5.84 KB

Ok, here's a new patch that seems to work. Interdiff also attached for comparison. Here's what's different from the original patch:

  • Changes $inner to $body, -inner to --body, and $id_inner to $body_id (per @markcarver's comment).
  • Adds <?php print ($collapsed ? ' collapsed' : ''); ?> to the classes appended to the fieldset title link. I'm not sure if this is the correct way to fix this, but it seems that that class does not get set initially by any existing mechanism. It does get set after the page is loaded and you click on the link to open/close the fieldset.
  • Changes the body div classes from <?php print (!$collapsed ? ' in' : ''); ?><code> to <code><?php print ($collapsed ? ' collapsed' : ' in'); ?>. The original patch removes the 'collapsed' class from the wrapper div, but it doesn't add it to the body div. Again, I'm not sure if doing this in the template file is correct.
  • Lastly, this reinstates the logic from #2468625: bootstrap_preprocess_bootstrap_panel() overwrites element id that was reverted, but also changes it a bit. Instead of only creating the ID attribute if the fieldset is collapsible, now it ALWAYS creates an ID. It also always creates an ID for the fieldset body div. What do you think? Is there any harm in doing that? Doing so simplifies the logic a bit.

Let me know what you think.

m.stenta’s picture

Oh PS: I did not change the "target" like you described in #5, @markcarver. Do you think that should be fixed in #2729181: Collapsible panel titles link to nothing instead?

lazzyvn’s picture

FileSize
958 bytes

I tested carefully with many case. I think it enough to delete

// Force grouped fieldsets to not be collapsible (for vertical tabs).
  if (!empty($element['#group'])) {
    $variables['collapsible'] = FALSE;
    $variables['collapsed'] = FALSE;
  }

Here my patch

m.stenta’s picture

@lazzyvn - Please open a new issue, or search for an existing one that matches yours. This issue is specifically for fixing the symptoms described in the issue description. It is not related to vertical tabs. Here is a search of all issues in the Bootstrap theme issue queue that pertain to "vertical tabs": https://www.drupal.org/project/issues/bootstrap?text=vertical+tabs&statu...

m.stenta’s picture

@lazzyvn - Sorry I misread your first comment - you aren't using vertical tabs.

Did you test that my patch in comment #10 fixes your issue? I'm still not sure if it's the same issue as the one being discussed here.

lazzyvn’s picture

Oh i have form (also with views manage) with 10 field group fieldset it doesn't works with collapsible i found $variables['collapsible'] is always false before generate panel i debug i found $element['#group'] is always exist . vertical tabs group doesn't touch this code
in tpl bootstrap/bootstrap-panel.tpl.php there is data-target it still works fine with my version 3.3.5 (CDN Provider).
I use your patch it works fine also

m.stenta’s picture

@lazzyvn - I created a related issue for what you described above - because I ran into the same issue: #2910624: Collapsible fieldsets are disabled when group is set

markhalliwell’s picture

Status: Needs review » Fixed
FileSize
6.84 KB
7.07 KB

  • markcarver committed 941f7f3 on 7.x-3.x
    Issue #2634358 by markcarver, m.stenta, geoffreyr, lazzyvn: Multiple...
  • markcarver committed b4f4527 on 7.x-3.x
    Automated commit: grunt compile (CSS)
    
    Issue #2634358 by markcarver, m....
markhalliwell’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

hass’s picture

It is currently just a guess, but this change may cause #2948135: Fieldsets are empty in forms (7.x-3.18 or later)