Problem/Motivation

  • Non-collapsible fieldsets that are the children of collapsible fieldsets get the little triangle icon that indicates a fieldset is "expanded," despite that they cannot be expanded or collapsed. See screenshot:
  • The issue occurs in Seven, Stark, and Bartik, so it's probably a part of system.module CSS or something.
  • This is probably caused by a CSS rule that is too greedy, probably something like:
    fieldset.collapsible legend { /* ADD A TRIANGLE! */ }

Proposed resolution

  • Only render the icon and related styling on legends that are the direct children of the collapsible fieldset. Probably just a matter of using the > selector.
  • Patch in #20 implements this solution.
  • The solution leaves a (depending on the theme) noticeable, perhaps undesirable gap in the place of the little triangle.
  • Patch in #3 has been committed to 8.x by Dries (#9)

Remaining tasks

  • Find a contrib theme in Drupal 7 that is heavily stylized to better test this patch (as per David_Rothstein's request on #31).
  • See what the community thinks: Is the gap introduced by this patch worse than the unnecessary little triangle icon it eliminates?

User interface changes

  • Icon on non-collapsible child fieldsets go away. (See screenshot above.)

API changes

  • None.
CommentFileSizeAuthor
#57 stark_after_chrome.png36.95 KBbillywardrop
#57 stark_before_chrome.png37.08 KBbillywardrop
#57 bartik_after_chrome.png53.29 KBbillywardrop
#57 bartik_before_chrome.png40 KBbillywardrop
#57 seven_after_chrome.png31.88 KBbillywardrop
#57 seven_before_chrome.png47.53 KBbillywardrop
#50 drupal-collapsible_fieldset_nesting-1361810-55.patch1.37 KBrooby
#41 chrome-shiny-after.png15.43 KBwelly
#41 chrome-ember-after.png15.55 KBwelly
#41 chrome-adminimal-after.png15.49 KBwelly
#41 chrome-mayo-after.png14.93 KBwelly
#41 chrome-ohm-after.png16.81 KBwelly
#41 chrome-bootstrap-after.png16.11 KBwelly
#31 drupal-1361810-helper-patch-do-not-test.patch1.08 KBDavid_Rothstein
#28 ie6-before.png4.44 KBk0teg
#28 ie6-after.png4.25 KBk0teg
#28 ie7-before.png4.52 KBk0teg
#28 ie7-after.png4.53 KBk0teg
#28 ie8-before.png4.57 KBk0teg
#28 ie8-after.png4.41 KBk0teg
#28 ie9-before.png5.61 KBk0teg
#28 ie9-after.png4.82 KBk0teg
#23 seven-firefox-before.png17.23 KBryan.gibson
#23 stark-chrome-before.png14.81 KBryan.gibson
#23 stark-safari-before.png14.52 KBryan.gibson
#23 stark-firefox-before.png15.85 KBryan.gibson
#23 bartik-firefox-before.png18.54 KBryan.gibson
#23 bartik-safari-before.png17.21 KBryan.gibson
#23 bartik-chrome-before.png16.99 KBryan.gibson
#23 seven-chrome-before.png14.77 KBryan.gibson
#23 seven-safari-before.png12.24 KBryan.gibson
#23 garland-chrome-before.png14.28 KBryan.gibson
#23 garland-firefox-before.png15.84 KBryan.gibson
#23 garland-safari-before.png14.06 KBryan.gibson
#23 bartik-chrome-after.png16.99 KBryan.gibson
#23 bartik-safari-after.png16.67 KBryan.gibson
#23 bartik-firefox-after.png17.99 KBryan.gibson
#23 seven-chrome-after.png12.35 KBryan.gibson
#23 seven-safari-after.png12.48 KBryan.gibson
#23 seven-firefox-after.png12.76 KBryan.gibson
#23 stark-chrome-after.png14.72 KBryan.gibson
#23 stark-safari-after.png14.79 KBryan.gibson
#23 stark-firefox-after.png16.03 KBryan.gibson
#23 garland-chrome-after.png11.54 KBryan.gibson
#23 garland-firefox-after.png15.82 KBryan.gibson
#23 garland-safari-after.png14.27 KBryan.gibson
#20 non-collapsible-fieldsets-within-1361810-4.1.patch1.95 KBcr0ss
#17 non-collapsible-fieldsets-within-1361810-4.patch1.97 KBcr0ss
#17 fieldsets.jpg11.77 KBcr0ss
#14 IE6_TEST_VERSION.png12.59 KBhbergin
#14 IE7_VERSION.png29.08 KBhbergin
#13 before.png572 bytesxjm
#13 after.png495 bytesxjm
#12 IE6_TEST_BEFORE.png2.55 KBhbergin
#12 IE6_TEST_AFTER.png2.5 KBhbergin
#10 non-collapsible-fields-1361810-10.patch1.31 KBoriol_e9g
#5 after-patch.png5.3 KBxjm
#3 non-collapsible-fieldsets-within-1361810-3.patch1.39 KBrlmumford
fieldsets.png5.32 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes

Updated issue summary.

Devin Carlson’s picture

Although #1286786: Fieldset within a collapsible fieldset inherits inappropriate styles was posted a little while ago, this issue has a better summary so I'm marking the other one as a duplicate.

xjm’s picture

Issue tags: +Needs backport to D7

Tagging.

rlmumford’s picture

Here's a patch, tested in IE7, IE8, Firefox 8 and Chrome 15

rlmumford’s picture

Status: Active » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.3 KB

#3 looks perfect. Thanks!

Here's how it looks after the patch is applied:

after-patch.png

I tested in FF3, Safari 5, and Chrome dev channel, and it's fine there as well.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Note that for the D7 backport we'll need to add an IE6-specific workaround, but we can do that when we port the patch.

rlmumford’s picture

Collapsable fieldsets don't work at all with IE6, so I'm not sure the fix is necessary.

xjm’s picture

Hmm, I forgot about that. We should probably test in IE6 in either case to ensure the child selector doesn't break the fieldsets. But we don't need to worry about it for D8.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This patch looks good and I've committed it to 8.x. Moving it to 7.x.

In general, I'm not a fan of nesting fieldsets. In my mind, it usually (but not always) suggests poor information design. Of course, that is beyond the goal of this patch. It is still good to fix this in cases nested fieldsets are proper.

oriol_e9g’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.31 KB

Rolled a patch for D7

xjm’s picture

Issue tags: +Needs manual testing

Unfortunately, before we can fix this in D7, we need to test IE6 before and after the patch and ensure that there's no regression.

hbergin’s picture

FileSize
2.5 KB
2.55 KB

I had a Win XP instance running IE6 with SP3 so was able to test the d7 patch. I captured an image of the nested fieldset before and after applying the patch. The outer fieldset was collapsible, the inner one was not. Hope this was what you needed to move the d7 patch forward.

xjm’s picture

FileSize
495 bytes
572 bytes

Hmmm, #12 does not show what I see when I test IE6 using http://netrenderer.com/index.php. Are you sure the machine in question isn't using IE7?

Before

before.png

After

after.png

So the parent fieldset loses its icon, as expected because IE6 cannot interpret the selector. If the collapse behavior doesn't work at all I guess maybe we don't care? But I don't have an actual machine where I can test IE6 atm, so I can't confirm.

hbergin’s picture

FileSize
29.08 KB
12.59 KB

I'm not sure why we are seeing different results. I've attached what is shown when I do Help\About Internet Explorer on the IE6 browser menu where I did the test.

I do have an IE7 installation on another Win XP instance, and I've attached what the Help\About Internet Explorer shows me there also, so that you can see the difference. But the IE7 installation is not where I ran the test.

xjm’s picture

Hm, strange. Does the collapse behavior work for you in IE6 (regardless of the patch)?

rlmumford’s picture

Well, It doesn't on my testing IE6 package.

cr0ss’s picture

Category: bug » feature
Status: Needs review » Patch (to be ported)
FileSize
11.77 KB
1.97 KB

I'd like to present patch version which I've tested for all IE(6-9), FF, Chrome, Safari browsers.

xjm’s picture

Category: feature » bug
Status: Patch (to be ported) » Needs review
xjm’s picture

Status: Needs review » Needs work
+++ b/modules/system/system.theme-rtl.cssundefined
@@ -41,13 +41,19 @@ th {
\ No newline at end of file

There needs to be a newline at the end of the file here.

Other than that, the patch looks correct to me at first glance. Let's have a couple people confirming the testing in cross-browser in all core themes.

cr0ss’s picture

Sorry about new line. Fixed it and attached.

xjm’s picture

Status: Needs work » Needs review
hbergin’s picture

To answer #15, yes, the collapse behavior worked regardless of the patch, so probably best to exclude my IE6 results, as my system does not seem to have triggered the behavior you are trying to address.

ryan.gibson’s picture

So, I wasn't able to test out the IE browsers this time, but did get around to Chrome, Safari, and FF. Worked great!

Before


Bartik

Chrome

bartik-chrome-berfore

Safari

Only local images are allowed.

Firefox

Only local images are allowed.


Garland

Chrome

garland-chrome-before

Safari

garland-safari-before

Firefox

garland-firefox-before

Seven

Chrome

Only local images are allowed.

Safari

Only local images are allowed.

Firefox

seven-firefox-before

Stark

Chrome

stark-chrome-before

Safari

stark-safari-before

Firefox

stark-firefox-before


After


Bartik

Chrome

bartik-chrome-after

Safari

bartik-safari-after

Firefox

bartik-firefox-after


Seven

Chrome

seven-chrome-after

Safari

seven-safari-after

Firefox

seven-firefox-after


Stark

Chrome

stark-chrome-after

Safari

stark-safari-after

Firefox

stark-firefox-after


Garland

Chrome

garland-chrome-after

Safari

garland-safari-after

Firefox

garland-firefox-after

xjm’s picture

Excellent testing! So we just need to test in IE 6/7/8/9 and we are good to go.

hrrsn’s picture

Assigned: Unassigned » hrrsn
Issue tags: +tcdrupal2012
hrrsn’s picture

Assigned: hrrsn » Unassigned
droplet’s picture

- Only icon go away ?? It leaves a lot of space before the fieldset title.
- can it have 2+ fieldsets inside a fieldset in "DRUPAL" ? how does it looks like after patch?

@ryanissamson,
can you share your testing code, thanks.

k0teg’s picture

FileSize
4.82 KB
5.61 KB
4.41 KB
4.57 KB
4.53 KB
4.52 KB
4.25 KB
4.44 KB

Validated the patch in IE 6-9.

oriol_e9g’s picture

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Go! Patch in #20

- Tested in #23 with Seven, Stark, Bartik & Garland + Firefox, Chrome & Safari
- Tested in #28 with IE6, IE7, IE8 & IE9

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.08 KB

Thanks for the awesome testing everyone! (And sorry for the delay in reviewing this patch.)

However:

- Only icon go away ?? It leaves a lot of space before the fieldset title.
- can it have 2+ fieldsets inside a fieldset in "DRUPAL" ? how does it looks like after patch?

These questions by @droplet look like good ones (especially the first question). You can actually see the problem in the screenshots in #23. Seven doesn't seem to be affected, but all the other core themes are (it's especially noticeable for Garland and Stark)... I'm attaching a quick patch I used to test this; with the patch applied you can go to admin/config/people/accounts and you'll see a nice mix of different kinds of fieldsets. The discrepancy @droplet pointed out is pretty noticeable there.

Is the gap introduced by this patch worse than the bug it's fixing? Hard to say, but I think for some of the themes the answer is yes; the gap makes things look pretty awkward and lopsided, whereas the arrow at least doesn't.

So, I think we have to bump this back to "needs review". (I'm also not sure if the same issues exist in Drupal 8 or not.)

In addition, collapsible fieldsets are something that people tend to display on public-facing pages of websites and consequently they are often heavily styled. Before committing this to Drupal 7, it might be useful to discuss what potential this has to break any themes that are heavily styling their fieldsets. Does anyone know of any contrib themes in Drupal 7 that do this, that this patch could be tested against? I'm hoping it's OK (since the patch does fix a bug, and for the most part it seems to be adding new, more specific CSS rules rather than modifying existing ones) but it would be good to get other people's thoughts.

Finally, just a heads up that this patch conflicts with #948516: Usability bug: down arrow of fieldsets in D7 overlays should be part of the link... (though I just rolled that back from Drupal 7, so at the moment at least this one still applies).

David_Rothstein’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Removing myself from the author field so that I can unfollow the issue. --xjm

mgifford’s picture

mgifford’s picture

Issue summary: View changes

This patch still applies. It's just CSS. Would be good to get it in, even for the edge cases this fixes.

twofinches’s picture

Issue summary: View changes
twofinches’s picture

Issue summary: View changes
twofinches’s picture

Issue summary: View changes

[EDIT] I replaced the screenshots with screenshots from Drupal 7 and added an after screenshot. I updated the remaining tasks to reflect what David Rothstien requested.

twofinches’s picture

Issue summary: View changes
twofinches’s picture

Issue summary: View changes
Issue tags: -Needs backport to D7

Removed "needs back port to D7" because it has been.

R13ose’s picture

I applied the patch in #31 and then applied the patch in #20. From testing this in Chrome and Firefox, this works for Drupal 7.42-dev.

The only question left is if the patch in #20 still conflicts with the issue listed in #31?

heddn’s picture

It would help to have some screenshots of #31. Also some investigation into some popular contrib themes to see how this would effect them.

welly’s picture

I've run some quick tests after applying the patch on a few contrib themes, as can be seen below:

Chrome - Mayo

Chrome - Ohm (Omega based demo theme)

Chrome - Bootstrap

Chrome - Adminimal

Chrome - Ember

Chrome - Shiny

It varies in appearance from theme to theme but I would suggest that as long as it works correctly in core themes and doesn't specifically break contrib themes, it is really up to contrib themes to tidy up the loose ends. I would say that while the gap introduced (why can't we lose this gap?) isn't exactly pretty, from a UX angle it's certainly an improvement on having the triangle there.

welly’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the screenshots! However as I pointed out in #31 (and as is visible in #23) it actually does break core themes.

And among the contrib themes in #41, it looks pretty bad to me in Mayo and Adminimal too.

I don't think I agree this is an improvement as is currently stands. At least the downward-facing arrow takes up the allotted space, and for most themes it doesn't seem like its presence there will confuse anyone.

I think we need to deal with the gap if we're going to change this.

David_Rothstein’s picture

I think at least part of the problem might be this:

+html.js fieldset.collapsible fieldset .fieldset-legend {
+  background: none; /* LTR */
+  padding-left: auto; /* LTR */
+}

Isn't "auto" not even a valid value for CSS padding?

David_Rothstein’s picture

Issue tags: +Needs backport to D7

Also adding back the backport tag, since this issue is a backport.

David_Rothstein’s picture

To the extent we don't care about IE6 support anymore (? - I certainly don't), we could consider just going back to something that has more in common with the patch that was committed to Drupal 8.

I just marked #2741305: Nested fieldset legends show collapsible open icon regardless of collapsible setting as duplicate, which contains a patch that is a start at doing that, although it would need some work.

  • Dries committed 1de37b3 on 8.3.x
    - Patch #1361810 by rlmumford: non-collapsible fieldsets within...

  • Dries committed 1de37b3 on 8.3.x
    - Patch #1361810 by rlmumford: non-collapsible fieldsets within...
stefan.r’s picture

Issue tags: -Novice
rooby’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

Here is Dries' commit ported to a D7 patch.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7

simple port, go with this.

rooby’s picture

Whoops I named the patch wrong. For some reason the comment number when you view your comment preview is wrong.

[EDIT] I opened an issue about that: #2813107: Comment cid is sometime incorrect on comment preview

stefan.r’s picture

Fabianx’s picture

I am +1 to fix this, but we need before and after screenshots, please! (Leaving as RTBC, but with a screenshot this is quicker to commit.)

heddn’s picture

Issue tags: +Novice

Adding the novice tag to give some visibility to add screenshots.

billywardrop’s picture

I'm making screenshots

billywardrop’s picture

I created a test content type with a fieldset and sub fieldset. I took screenshots before and after applying the patch. I tested on Seven, Bartik and Stark.

SEVEN BEFORE

SEVEN AFTER

BARTIK BEFORE

BARTIK AFTER

STARK BEFORE

STARK AFTER

This is my first screenshot contribution, If they need re-uploaded let me know

billywardrop’s picture

Issue tags: -Needs screenshots
Fabianx’s picture

Issue tags: -Needs manual testing, -Novice +Pending Drupal 7 commit

Thank you! Marking for commit!

  • Dries committed 1de37b3 on 8.4.x
    - Patch #1361810 by rlmumford: non-collapsible fieldsets within...

  • Dries committed 1de37b3 on 8.4.x
    - Patch #1361810 by rlmumford: non-collapsible fieldsets within...

  • 6fa7f7a committed on 7.x
    Issue #1361810 by cr0ss, David_Rothstein, rlmumford, oriol_e9g, rooby,...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x, thanks!

David_Rothstein’s picture

Status: Fixed » Needs work

I had to revert this unfortunately since it broke collapsible fieldset styling in Garland. (The patch did not change the CSS rules for Garland itself, so because of the CSS specificity change in the default core styling, that took over for Garland too.)

This also gives me pause about the patch overall, since other non-core themes could have the same problem. I guess we need to think about this more.

  • David_Rothstein committed fcf5b45 on 7.x
    Revert "Issue #1361810 by cr0ss, David_Rothstein, rlmumford, oriol_e9g,...

  • Dries committed 1de37b3 on 9.1.x
    - Patch #1361810 by rlmumford: non-collapsible fieldsets within...