The problem is: All <legend>
elements inside of vertical tabs are always hidden, therefore fieldset titles disappear, without which collapsible fieldsets can't be collapsed (or — worse — uncollapsed). The attached patch would fix this, as far as I can tell.
Comment | File | Size | Author |
---|---|---|---|
#102 | before_applying_patch.png | 8.41 KB | aLearner |
#102 | after_applying_patch_bartik.png | 9.12 KB | aLearner |
#102 | after_applying_patch_garland.png | 8.3 KB | aLearner |
#102 | after_applying_patch_seven.png | 9.04 KB | aLearner |
#102 | after_applying_patch_stark.png | 11.81 KB | aLearner |
Comments
Comment #1
drunken monkeyComment #2
Mohammed J. RazemI noticed this bug while porting Better Messages to Drupal 7.
I confirm the patch does fix the problem. But the fieldsets inside vertical tabs does not look formatted. So attach a modified patch to add borders and some padding.
Comment #3
drunken monkeyDoesn't really work for me in Seven.
But thanks for the additions!
Comment #4
Mohammed J. RazemMake sure you patch it against a clean Drupal 7 core, as this looks to be caused from
margin-bottom: 2em;
in the rulediv.vertical-tabs .vertical-tabs-panes fieldset fieldset legend
from the previous patch.Comment #5
drunken monkeyI always make sure — but it seems there was a problem with the browser cache or something.
Looked at it again and now it looks perfect in all core themes.
Thanks again!
Comment #6
aspilicious CreditAttribution: aspilicious commentedThis needs to be confirmed in IE, we need at least IE7 and IE8 screenshots for this.
Fieldset have proven to be hard in those.
Comment #7
Mohammed J. Razemdisplay: inline-block;
instead ofdisplay: block;
was needed to work properly with IE7.Attached are screenshots for IE7 and IE8 as well as the modified patch.
Comment #8
aspilicious CreditAttribution: aspilicious commented1) do we need to add "display: inline block" in 2 places?
2) in ie8 there is more whitespace in the box, can we fix that?
3) could you add some chrome and firefox screenshots to?
Comment #9
jonathan1055 CreditAttribution: jonathan1055 commentedI don't have Fulltext and cannot try better_messages because there is no public D7 version yet ;-) but here are screen shots from Scheduler http://drupal.org/project/scheduler in Firefox 3.6 and Opera11 (both on mac)
All the supplied D7 themes show correctly in FireFox. There is a bit of extra whitespace in Opera, which is fine in Bartik and Seven, very close in Stark, but in Garland the fieldset border cuts through the legend text when collapsed. But that is minor, compared to not having the fieldsets openable!
We discovered this problem and solved it in #1035398: Vertical tabs in node type form are wrong by creating our own scheduler.css file to hold the additions. This was better than re-engineering our fildsets then having to revert them once this css gets into core. I mention it here so that other module conversions to D7 can use the same temporary fix we did, so as not so slow up the conversion.
I'd be happy for RTBC, but I know there are other OS and browsers which need checking.
Jonathan
Comment #10
longwaveThis is fairly major for any module that has complicated configuration forms (in my case, Ubercart). Patch also works for me, what other browsers need testing?
Comment #11
yoroy CreditAttribution: yoroy commentedIn general it seems like a bad idea to have collapsible fieldsets within vertical tabs. It overloads the vt pattern and is a sign you should rethink your user interface.
Comment #12
longwaveThis affects all fieldsets with legends inside vertical tabs, which can still be useful for grouping related fields whether they are collapsible or not.
Comment #13
drunken monkeyAlso, even if this is true in general (it might), you can't say this for all use cases. (In mine, this is even dictated by another component.) Thus, generally forbidding this with CSS that makes them unusable, and has no further benefit (apart from three less lines in the CSS file), can hardly be a good idea.
Additionally, you won't keep developers from using this pattern, if it seems appropriate to them, with buggy CSS. They'll just work around it with their own CSS file, adding useless clutter and probably not coming to a solution that works as well for all browsers as the one in this issue.
Comment #14
bfroehle CreditAttribution: bfroehle commentedI agree with Drunken Monkey. Also, fieldsets in tabs may actually be a good idea in some cases... say, for example a tab needs you to configure some token substitution string, an provides a collapsible fieldset showing all possible tokens.
This issue also effects the backport of Seven to D6. The issue there (#1034518: Location + Vertical Tabs UI collapsable fields error on node edit content type page.) is awaiting resolution here. For what it's worth, the patch in #7 worked fine in Chrome 8 (Mac).
Comment #15
mrfelton CreditAttribution: mrfelton commentedPatch works for me. I think this is much needed. Complex configurations within vertical tabs become difficult to read/use without the ability to properly group things into fieldsets with titles.
Comment #16
Bojhan CreditAttribution: Bojhan commentedPatch seems to work here too, I do agree with yoroy its likely you need to rethink your interaction pattern. I am not convinced the few edge cases proposed here, are most benefited by a (collapsed) field set.
Comment #17
jonathan1055 CreditAttribution: jonathan1055 commentedI don't think it is a case of us discussing whether collapsed fieldsets are a good idea inside vertical tabs. They already exist in many implementations, and this patch is only making the D6 functionality useable in D7.
With many people saying it is tested and OK, I'm setting it RTBC.
Comment #18
Bojhan CreditAttribution: Bojhan commented@jonathan1055 Doesn't matter, we need to make sure who ever is searching on how to do this - also is exposed to the idea that he/she should pursue a better interaction.
Comment #19
bfroehle CreditAttribution: bfroehle commentedSigh, whatever. I agree that collapsible fieldsets don't necessarily make a lot of sense within vertical tabs, but that is no excuse to leave them as broken as they currently are
Comment #20
yoroy CreditAttribution: yoroy commentedNote that we never argued that the bug should not be fixed.
Comment #21
gordon CreditAttribution: gordon commentedsubscribe
Comment #22
Shadlington CreditAttribution: Shadlington commentedJust chipping in to say the patch worked for me too
Comment #23
Jacineoy, subscribe.
Comment #24
rfayD8 first. Let's get these fixed on D8 and into D7.
Comment #25
ckngThe patch works for me.
Encounter this problem while adding collapsible fieldset to display token tree in Submitted By.
Comment #26
jonathan1055 CreditAttribution: jonathan1055 commentedTo rfay in #24, if you change the version to D8 how on earth are we going to get it included in D7 in the near future? Lots of modules break without it, or have to include a duplicate of the necessary css. Do we need to find someone who will commit this in D8? If we don't then the issue will just stagnate.
Comment #27
jonathan1055 CreditAttribution: jonathan1055 commented#7: vertical-tabs_collapsed-fieldsets.patch queued for re-testing.
Comment #28
drunken monkey@ jonathan1055: Bug fixes always have to go into HEAD first and can then be backported. While this can of course delay the process a bit, this is a very sensible approach regarding managing the project. Otherwise, regressions would practically be programmed.
The difficulty of finding a core committer to look at an issue exists in both branches, nothing special about D8 there. If this is tested for both branches, maybe Dries will even commit it to both at once when he gets to this issue.
Comment #29
jonathan1055 CreditAttribution: jonathan1055 commentedThanks, yes you are right, I knew really that it was the proper thing to get it fixed in D8 first ;-)
So, I've just created my first D8 site and tried the patch, which applied ok to both files, just with a one line offset. Here are screen shots of the four supplied themes, before and after the patch, on Firefox3.6 and the same after the D7 fix just for reference.
Garland, Seven and Stark all look exactly like the D7 fixed version. The only change is in Bartik, where the content has slid upwards and is obscured behind the fieldset legend.
Would anyone else like to compare the Bartik css from D7 to D8 and find out why/how this changed and how it should be fixed?
Jonathan
Comment #30
rfayYeah, sorry. It's not my policy. But the core committers are completely convinced it's the way to do it.
Adding the tag to make sure it gets remembered!
Comment #31
sunRelated: #1168246: Freedom For Fieldsets! Long Live The DETAILS.
Comment #32
keithm CreditAttribution: keithm commentedsubscribe
Comment #33
sunThis doesn't look right to me? Don't we assign a special class to "pane" fieldsets? If we do, then we need to change vertical-tabs.css to only override fieldsets having that class. If we don't, then we need to change form.inc to apply a special class.
Powered by Dreditor.
Comment #34
Bojhan CreditAttribution: Bojhan commentedThis is a bug, but definitely not major - because it is not an advised practice anyway.
Comment #35
abbasmousavi CreditAttribution: abbasmousavi commentedsubscribe.
I use this feature in DruTeX configuration page
see #1207286: The fieldsets in DruTeX configuration do not display correctly
Comment #36
jonathan1055 CreditAttribution: jonathan1055 commentedHi abbasmousavi,
I have developed an improved fix for this problem. See issue #1172040: Contrib solution for non-collapsible fieldsets and missing titles patch in #14. You are welcome to use this code in your module, if you cannot wait for the core patch to be implemented.
Jonathan
Comment #37
dtarc CreditAttribution: dtarc commentedsubscribing
Comment #38
tamasd CreditAttribution: tamasd commentedsubscribing (I am looking forward to have this bug fixed in D7 core).
Comment #39
jessebeach CreditAttribution: jessebeach commentedAdding a related issue. It's closed as (Works as designs) but I would argue it's not designed well.
#1127988: Collapsible fieldsets inside vertical tab pane are invisible
Comment #40
Gábor HojtsyWe bumped into this problem as well in modules in Drupal Gardens. We have similar CSS to fix the problem, except that we chose to style all fieldsets proper under vertical tabs and only skip the styling for the first one inside a pane. The suggested patch uses a selector to ensure two levels of fieldset nesting to achieve the same effect.
@sun: the tag structure used for vertical tabs is as follows:
So indeed it looks like we could direct the override on the right fieldsets. The problem is to some degree in misc/vertical-tabs.css and then in seven's vertical-tabs.css override. Too broad selectors in misc/vertical-tabs.ccss:
Should be:
In seven's override:
Should be:
This was just some research, please help me verify, discuss.
Comment #41
Dave ReidSubscribing - this is affecting several of my contrib modules.
Comment #42
jstollerCan someone please explain to me why I wouldn't want to nest fieldsets inside a vertical tab? I've now seen several references saying it is "not an advised practice" with no explanation. I can think of numerous situations where nested fieldsets make perfect sense from a usability standpoint and these are not edge cases.
I for one would like to see this fixed in D7 as soon as possible.
Comment #43
jonathan1055 CreditAttribution: jonathan1055 commentedIn the meantime, if anyone wants to fix their own module, I have added the css and jquery files to #1172040: Contrib solution for non-collapsible fieldsets and missing titles comment #19 so that it is easy for you to see how I solved it and take and use the files.
Jonathan
Comment #44
mstrelan CreditAttribution: mstrelan commented+1 for jstoller's comment in #42.
Comment #45
xjm@Bojhan -- The issue applies to all fieldsets, not just collapsible ones.
For others: Here are the documents with the UI guidelines for fieldsets and vertical tabs:
http://drupal.org/node/1087106
http://drupal.org/node/1087100
See also: #1168246: Freedom For Fieldsets! Long Live The DETAILS.
So I see two potential criticisms:
However, it is a bug that it is currently impossible for a fieldset inside a vertical tab to display a legend without overriding core CSS. There are plenty of legitimate reasons to use a fieldset to group form elements within a vertical tab. Semantically, and for accessibility reasons (see #1168246-19: Freedom For Fieldsets! Long Live The DETAILS.), we should use fieldsets to group related elements. They can be inline, they can have no borders or padding--but it should be possible to create one without its legend disappearing.
Comment #46
xjmAttached is a slight modification of Gabor's suggested changes above.
Before
After
Comment #47
JacineThis is an acceptable fix for Drupal 8, but not Drupal 7, because it does not work in IE6 (support for the child selector ">" starts in IE7).
Also, the Bartik and Garland themes need to be checked out to ensure these changes don't introduce any issues.
Comment #48
xjmYeah, for the D7 backport, we need to do something like:
Edit: Do that in the IE6 stylesheet, that is, and use the orignal CSS for decent browsers. Edit 2: So basically, adding
vertical-tabs.ie6.css
.Edit 2: Uh. Do vertical tabs even work in IE6?
Comment #49
xjmStark before and after
Edit: Missing side border in the second is just bad cropping, sorry.
Garland before and after
Bartik before and after
Note: The missing bottom border occurs both with and without the patch; that seems to be a separate bug in Bartik. Also, the right border isn't different; I just cropped a little sloppily.
Comment #50
xjmOh, I forgot to mention, but I also tested that clicking on different tabs works in each above theme with the patch.
Comment #51
yoroy CreditAttribution: yoroy commentedBy all means fix the bug, we established that in #20 already.
Maybe this helps explain, xjm gets it right in #45:
Comment #52
xjmThis is why we'll need to add special IE6 CSS in D7. With the patch applied in IE6:
Patch in #46 still needs review for D8, though. Screenshots of the change in all core themes are in #49.
Comment #53
anavarreThis affects the Metatags module for D7.
Comment #54
JStanton CreditAttribution: JStanton commentedApplies cleanly to 8.x and works in Chrome, FF & Safari. (Sorry, on OSX so can't test IE).
Comment #55
webchickWe need IE testing before proceeding.
Comment #56
xjmSo I started to write out instructions for how to test this but got lazy and just did it myself using http://netrenderer.com. :)
IE 7
IE 8
IE 9
IE6 is in #52. The current patch is for D8 only for that reason.
My patch, so I can't RTBC. :)
Comment #57
xjmOccurs to me it would be useful to show actual contents of a form rather than just empty fieldsets. ;) Attached is a form with some descriptions and child elements in IE7. 8 and 9 are the same.
Comment #58
aspilicious CreditAttribution: aspilicious commentedOk looks good
Comment #59
Everett Zufelt CreditAttribution: Everett Zufelt commentedI haven't tested, but looking at the patch in #46 I don't see anything that would appear to be problematic.
Comment #60
webchickAwesome, thanks! That netrenderer.com thing is a neat trick! :D
Committed and pushed to 8.x. This will need porting to 7.x, for IE6 compatibility. :\
Also moving to the correct component.
Comment #61
oriol_e9gWhich is the best way to solve this problem in D7?
Option 1) We have a lot of IE6 stuff in misc/vertical-tabs.css, so simply add here:
This seems that works reasonably in IE6,7,8 +plus:FF,Chrome,...
Option 2) Create a vertical-tabs.ie6.css in misc/. In this case, we need to move all the vertical-tab IE6 stuff in this new file.
Option 3) Use the CCS fix with ">" in misc/ and include the IE6 fix in all core themes.
Comment #62
oriol_e9gComment #63
oriol_e9gPatch with option #2. If you prefer another approach I can roll a new patch.
Comment #64
aspilicious CreditAttribution: aspilicious commentedwe don't need the '*' if it's an ie6 only css file.
And we use a dash before ie6, in stead of a dot, in core if we use ie6 css files iirc.
-17 days to next Drupal core point release.
Comment #65
oriol_e9gComment #66
oriol_e9gMissing file, sorry for the noise.
Comment #67
xjmLooks good! One more thing I think we can change is to remove the HTML as well as the * (they are part of the same IE hack).
After that, let's get some additional manual testing in IE6, both for the vertical tabs and for the other moved rules.
Comment #68
oriol_e9gComment #70
aspilicious CreditAttribution: aspilicious commented#68: vertical-tab-legends-1015798-68.patch queued for re-testing.
Comment #71
dimitriseng CreditAttribution: dimitriseng commentedI was looking at this functionality for an issue with the metatag module. Using Drupal 7.12. I can confirm that patch at #68 is resolving this issue as far as I can see. I have tested with latest versions of Firefox and Chrome, but cannot test with IE.
Is there anything missing from getting this commited? Thank you for the good work.
Comment #72
oriol_e9gWe need a review (like you :D) and a testing with IE, then we can change the state to "Reviewed & tested by the community" :D
Comment #73
xjmYep, like it says in #67:
Comment #74
klonos...blocking this by waiting on a manual test from a IE6 user is kinda stupid now that IE6 has only 1% of the market share. How about we commit this and let this 1% of internet users report a bug if they so happen to face this issue. I believe that the odds are really slim that this small amount of users will happen to come across a site built on Drupal that will happen to use vertical tabs that happen to include fieldset in them and also be critical for the fieldset to be collapsed. This is too far-fetched IMO.
I personally don't care if 10 out of every 1000 of my site visitors use IE6 and the site breaks for them. If my site breaks, then so does the most of the rest of the internet in some way. They should upgrade to a newer version of IE or get a real browser.-
PS: ...since some might say that my comment if biased, I'm refraining from marking it RTBC myself ;)
Comment #75
oriol_e9gD7 has support for IE6 and in this issue we are creating an especific file for IE6 (misc/vertical-tabs-ie6.css), so, why not test it?
I'm sure someone will make a test with IE6 :D
Comment #76
klonosYep, someone will possibly test IE6 ...eventually ...some day, but that is no reason to hold up fixes for other, more popular versions of IE and other browsers. We should commit this and open a followup for IE6 or simply assume there are no issues and let people open a followup IE6-specific issue (if any problems arise that is).
Comment #77
klonos...oh, I get the joke now. You suggest that if I want this in so bad I should setup a WinXP-IE6 environment to test this + provide screenshots. Fuuuunny... no, I don't have time for this.
Comment #78
tsi CreditAttribution: tsi commentedIE6 ? even microsoft don't want it anymore, only developers use it for this kind of nonsense :)
Comment #79
JacinePlease, let's stop complaining about the fact that Drupal 7 supports IE6 and 7. It's just a fact of life we have to deal with and we can do whatever refactors we want for Drupal 8.
Why are we adding a separate IE stylesheet??? We don't do that anywhere else in core and should not start now.
Also, we did a crapload of work and thorough testing when the fact that vertical tabs didn't work in IE6 AND IE7 was a critical D7 bug (see #925350: Vertical tabs broken). And there are many sites out there in D7 that are using that code. It absolutely needs to be tested in both IE6 and IE7.
Comment #80
JacineAlso, it's really NOT hard to test in IE anymore... Come on.
It will take you all of 5-10 minutes with a free BrowserStack account. Click the "free plan" link here: http://www.browserstack.com/pricing
Comment #81
xjmEr... the issue was already well-tested for IE7 before it was committed to D8. See #56 - #57.
Also, a separate IE6 stylesheet is the preferred way to add IE6-only styling, unless I'm mistaken?
Comment #82
JacineI realize that, but Drupal 7 has different requirements than Drupal 8.
There are zero IE specific stylesheets in core, except in themes. Adding one here would be a really bad call IMO. We can't forget there are are sites out there that still support IE6/7, and themes out there that have overridden this file. If this goes through, they'll suddenly have another file that will not be overridden and could royally screw with things.
Comment #83
xjmAh! That is a good point (re: overriding the stylesheet). So, for the backport, is it preferable to just use the redundant rule for all browsers? Like:
Comment #84
JacineYep, exactly. Just a straight bug fix. :)
Marking needs work for that.
Comment #85
xjmGood novice issue, I think. The task is to go back to the patch from #46 and, instead of making an IE6-only stylesheet (which won't work at this point in the release cycle), replace the child selectors in the main stylesheet with:
Comment #86
mstrelan CreditAttribution: mstrelan commentedYou mean like this? Does anything need to change in the Seven theme?
Comment #87
xjmYep, similar changes should be added to Seven as well. See: http://drupal.org/files/vertical-tab-legends.patch Thanks @mstrelan!
Comment #88
dimitriseng CreditAttribution: dimitriseng commented@ mstrelan and xjm, thanks for the good work on this. Can you update #86 with the remaining changes required as per #87? Once this is available I can restest this with Firefox and Chrome, but unfortunately I don't have IE6 installed anywhere, if anybody else has it would be good to give that also a quick test to make sure that this also working so that we can get this commited.
Comment #89
xjm@dimitriseng, the issue is marked needs work, for the changes to be made to the patch. It's also tagged as a novice issue, so I am certainly not going to swipe that opportunity. ;) You could even roll the patch yourself! :) http://drupal.org/patch/create
Comment #90
dimitriseng CreditAttribution: dimitriseng commented@xjm, I gave it a try and the attached patch applies ok to my D7.12 installation and it resolves the issue for me as far as I can see, I hope that it does not break anything else...
Can you please review? Unfortunately I do not have IE6 so hopefully somebody else could test this... I have not used '>' as I believe that this does not work with IE6 as per comments in previous posts. Thank you.
Comment #92
dimitriseng CreditAttribution: dimitriseng commentedOne more try...
Comment #93
dimitriseng CreditAttribution: dimitriseng commentedI have managed to find an IE6 (and IE8) and tested this also with that and this also seems to be working ok, I had already tested with Firefox and Chrome. So if you can please review #92 and correct any errors it would be great, also if somebody also wants to test with IE6 to confirm, thanks.
Comment #94
xjmThe CSS in #92 looks correct. Thanks @dimitriseng!
Comment #95
xjmHere's the little module I made that creates the form in my screenshots above, for anyone to test with. Just go to
http://example.com/fieldset_test
.Comment #96
xjmaLearner tested the patch in all four themes in Firefox and confirmed the same results as in #49.
Comment #97
ryan.gibson CreditAttribution: ryan.gibson commentedOkay, I've tested the patch in all four themes in both Chrome and Safari - the results in #49 stand firm there as well.
Comment #98
xjmThanks guys! So we just need testing for IE 6-9.
Comment #99
xjmAlright, I just used http://www.browserstack.com/ as Jacine suggested to test in all four themes in IE6 and IE7. Everything was fine, except that I found the following minor issue in both browsers in Seven (the theme). When the patch is applied, the child fieldset is missing a top border and/or has a really wide top margin for the legend:
Comment #100
xjmAlright, I just screwed with the CSS for an hour and couldn't find any simple solution that makes that stupid top border appear (above the stupid legend and not below it) in IE6/7, without adding mountains of CSS that could cause headaches for existing themes. So I'd like an opinion on whether this can go in as-is for IE6/7 in D7, since it does result in a significant improvement already (that is, the legend is actually displayed)... and if anyone really cares we could open a followup for that silly top border. Like I mentioned, the patch allows the legend to be displayed properly in all four themes in IE6+.
Comment #101
xjmSo I pinged webchick about this, and she's okay with opening a followup to fix the seven border thing if anyone cares enough. Phew!
Comment #102
aLearner CreditAttribution: aLearner commentedJust posting my results as a follow up to comment # 96.
I tested the module + patch in Firefox 9.0.1 (Mozilla Firefox for Ubuntu - canonical 1.0).
Please find enclosed the screenshots taken before the patch was applied - and then after it was applied - for the following themes: Bartik, Garland, Seven and Stark.
P.S: Oops! Looks like I uploaded 'before_applying_patch.png' two times. Sorry!
Comment #103
xjmExcellent, thanks @aLearner. All those screenshots look correct as well, so we are RTBC.
Comment #104
aLearner CreditAttribution: aLearner commentedxjm,
Cool beans. Thank you.
Comment #105
webchickGreat; thanks for all the work on this folks!
Committed and pushed to 7.x. Thanks!
Comment #106
yoroy CreditAttribution: yoroy commentedGood job seeing this through folks.
Comment #107
klonosWoohoo! yet one less patch to "babysit". Thanx Angie ;)
Comment #108
pbfleetwood CreditAttribution: pbfleetwood commentedRe. #105:
Will the fix be committed in v7.13? I hope, I hope, I hope.
Comment #109
Dave ReidAwesome job everyone!
Comment #110
klonosAFAIK each next stable release is created from the current dev. So, as long as this change remains in the current dev (not rolled back for some reason), it will of course be included in 7.13.
Comment #111
mstrelan CreditAttribution: mstrelan commentedUnless of course 7.13 is a security-only release, in which case it should be released in 7.14, which should come out at the same time.
Comment #113
realityloopThis still doesn't appear to be committed to 7.15..?
Comment #114
realityloopnevermind