Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#57 | stark_after_chrome.png | 36.95 KB | billywardrop |
#57 | stark_before_chrome.png | 37.08 KB | billywardrop |
#57 | bartik_after_chrome.png | 53.29 KB | billywardrop |
#57 | bartik_before_chrome.png | 40 KB | billywardrop |
#57 | seven_after_chrome.png | 31.88 KB | billywardrop |
Comments
Comment #0.0
xjmUpdated issue summary.
Comment #1
Devin Carlson CreditAttribution: Devin Carlson commentedAlthough #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.
Comment #2
xjmTagging.
Comment #3
rlmumfordHere's a patch, tested in IE7, IE8, Firefox 8 and Chrome 15
Comment #4
rlmumfordComment #5
xjm#3 looks perfect. Thanks!
Here's how it looks after the patch is applied:
I tested in FF3, Safari 5, and Chrome dev channel, and it's fine there as well.
Comment #5.0
xjmUpdated issue summary.
Comment #5.1
xjmUpdated issue summary.
Comment #6
xjmNote that for the D7 backport we'll need to add an IE6-specific workaround, but we can do that when we port the patch.
Comment #7
rlmumfordCollapsable fieldsets don't work at all with IE6, so I'm not sure the fix is necessary.
Comment #8
xjmHmm, 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.
Comment #9
Dries CreditAttribution: Dries commentedThis 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.
Comment #10
oriol_e9gRolled a patch for D7
Comment #11
xjmUnfortunately, before we can fix this in D7, we need to test IE6 before and after the patch and ensure that there's no regression.
Comment #12
hbergin CreditAttribution: hbergin commentedI 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.
Comment #13
xjmHmmm, #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
After
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.
Comment #14
hbergin CreditAttribution: hbergin commentedI'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.
Comment #15
xjmHm, strange. Does the collapse behavior work for you in IE6 (regardless of the patch)?
Comment #16
rlmumfordWell, It doesn't on my testing IE6 package.
Comment #17
cr0ss CreditAttribution: cr0ss commentedI'd like to present patch version which I've tested for all IE(6-9), FF, Chrome, Safari browsers.
Comment #18
xjmComment #19
xjmThere 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.
Comment #20
cr0ss CreditAttribution: cr0ss commentedSorry about new line. Fixed it and attached.
Comment #21
xjmComment #22
hbergin CreditAttribution: hbergin commentedTo 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.
Comment #23
ryan.gibson CreditAttribution: ryan.gibson commentedSo, 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
Safari
Firefox
Garland
Chrome
Safari
Firefox
Seven
Chrome
Safari
Firefox
Stark
Chrome
Safari
Firefox
After
Bartik
Chrome
Safari
Firefox
Seven
Chrome
Safari
Firefox
Stark
Chrome
Safari
Firefox
Garland
Chrome
Safari
Firefox
Comment #24
xjmExcellent testing! So we just need to test in IE 6/7/8/9 and we are good to go.
Comment #25
hrrsn CreditAttribution: hrrsn commentedComment #26
hrrsn CreditAttribution: hrrsn commentedComment #27
droplet CreditAttribution: droplet commented- 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.
Comment #28
k0teg CreditAttribution: k0teg commentedValidated the patch in IE 6-9.
Comment #29
oriol_e9g#20: non-collapsible-fieldsets-within-1361810-4.1.patch queued for re-testing.
Comment #30
oriol_e9gGo! Patch in #20
- Tested in #23 with Seven, Stark, Bartik & Garland + Firefox, Chrome & Safari
- Tested in #28 with IE6, IE7, IE8 & IE9
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the awesome testing everyone! (And sorry for the delay in reviewing this patch.)
However:
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).
Comment #31.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.
Comment #31.1
xjmRemoving myself from the author field so that I can unfollow the issue. --xjm
Comment #32
mgifford20: non-collapsible-fieldsets-within-1361810-4.1.patch queued for re-testing.
Comment #33
mgiffordThis patch still applies. It's just CSS. Would be good to get it in, even for the edge cases this fixes.
Comment #34
twofinches CreditAttribution: twofinches commentedComment #35
twofinches CreditAttribution: twofinches commentedComment #36
twofinches CreditAttribution: twofinches commented[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.
Comment #37
twofinches CreditAttribution: twofinches commentedComment #38
twofinches CreditAttribution: twofinches commentedRemoved "needs back port to D7" because it has been.
Comment #39
R13ose CreditAttribution: R13ose commentedI 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?
Comment #40
heddnIt would help to have some screenshots of #31. Also some investigation into some popular contrib themes to see how this would effect them.
Comment #41
welly CreditAttribution: welly at PreviousNext commentedI'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.
Comment #42
welly CreditAttribution: welly at PreviousNext commentedComment #43
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThanks 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.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI think at least part of the problem might be this:
Isn't "auto" not even a valid value for CSS padding?
Comment #45
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAlso adding back the backport tag, since this issue is a backport.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedTo 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.
Comment #49
stefan.r CreditAttribution: stefan.r commentedComment #50
rooby CreditAttribution: rooby commentedHere is Dries' commit ported to a D7 patch.
Comment #51
oriol_e9gsimple port, go with this.
Comment #52
rooby CreditAttribution: rooby commentedWhoops 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
Comment #53
stefan.r CreditAttribution: stefan.r commentedComment #54
Fabianx CreditAttribution: Fabianx as a volunteer commentedI 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.)
Comment #55
heddnAdding the novice tag to give some visibility to add screenshots.
Comment #56
billywardrop CreditAttribution: billywardrop as a volunteer commentedI'm making screenshots
Comment #57
billywardrop CreditAttribution: billywardrop as a volunteer commentedI 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
Comment #58
billywardrop CreditAttribution: billywardrop as a volunteer commentedComment #59
Fabianx CreditAttribution: Fabianx as a volunteer commentedThank you! Marking for commit!
Comment #63
stefan.r CreditAttribution: stefan.r commentedCommitted and pushed to 7.x, thanks!
Comment #64
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.