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.

CommentFileSizeAuthor
#102 before_applying_patch.png11.6 KBaLearner
#102 before_applying_patch.png8.41 KBaLearner
#102 after_applying_patch_bartik.png9.12 KBaLearner
#102 after_applying_patch_garland.png8.3 KBaLearner
#102 after_applying_patch_seven.png9.04 KBaLearner
#102 after_applying_patch_stark.png11.81 KBaLearner
#99 ie6_seven.png9.27 KBxjm
#95 fieldset_test.tar_.gz583 bytesxjm
#92 1015798-90-vertical-tab-legends.patch1.1 KBdimitriseng
#90 1015798-90-vertical-tab-legends.patch1.1 KBdimitriseng
#86 1015798-vertical-tab-legends.patch448 bytesmstrelan
#68 vertical-tab-legends-1015798-68.patch3.15 KBoriol_e9g
#66 vertical-tab-legends-1015798-66.patch3.17 KBoriol_e9g
#65 vertical-tab-legends-1015798-65.patch2.69 KBoriol_e9g
#62 vertical-tab-legends-1015798-62.patch3.18 KBoriol_e9g
#57 with-contents.png3.36 KBxjm
#56 ie7.png1.3 KBxjm
#56 ie8.png1.28 KBxjm
#56 ie9.png1.97 KBxjm
#52 ie6-after.png751 bytesxjm
#51 vtabs-nesting.png41.7 KByoroy
#49 stark-before.png2.52 KBxjm
#49 stark-after.png2.72 KBxjm
#49 garland-before.png2.55 KBxjm
#49 garland-after.png3.07 KBxjm
#49 bartik-before.png2.87 KBxjm
#49 bartik-after.png3.63 KBxjm
#46 vertical-tab-legends.patch1022 bytesxjm
#46 before.png2.06 KBxjm
#46 after.png2.97 KBxjm
#29 garland d8 FF3.6 before.jpg106.17 KBjonathan1055
#29 garland d8 FF3.6 after.jpg231.64 KBjonathan1055
#29 garland d7 FF3.6 after.jpg225.02 KBjonathan1055
#29 seven d8 FF3.6 before.jpg84.31 KBjonathan1055
#29 seven d8 FF3.6 after.jpg231.44 KBjonathan1055
#29 seven d7 FF3.6 after.jpg221.25 KBjonathan1055
#29 stark d8 FF3.6 before.jpg125.94 KBjonathan1055
#29 stark d8 FF3.6 after.jpg283.27 KBjonathan1055
#29 stark d7 FF3.6 after.jpg269.28 KBjonathan1055
#29 bartik d8 FF3.6 before.jpg120.49 KBjonathan1055
#29 bartik d8 FF3.6 after.jpg242.08 KBjonathan1055
#29 bartik d7 FF3.6 after.jpg252.66 KBjonathan1055
#9 node-type vertical tabs FF3.6 bartik.jpg237.51 KBjonathan1055
#9 node-type vertical tabs FF3.6 seven.jpg198.36 KBjonathan1055
#9 node-type vertical tabs FF3.6 garland.jpg221.03 KBjonathan1055
#9 node-type vertical tabs FF3.6 stark.jpg248.02 KBjonathan1055
#9 node-type vertical tabs Opera11 bartik.jpg220.72 KBjonathan1055
#9 node-type vertical tabs Opera11 seven.jpg205.33 KBjonathan1055
#9 node-type vertical tabs Opera11 garland.jpg191.45 KBjonathan1055
#9 node-type vertical tabs Opera11 stark.jpg258.88 KBjonathan1055
#7 vertical-tabs_collapsed-fieldsets.patch1.31 KBMohammed J. Razem
#7 vertical_tabs_ie7.png106.63 KBMohammed J. Razem
#7 vertical_tabs_ie8.png109.17 KBMohammed J. Razem
#3 Screenshot.png13.33 KBdrunken monkey
#2 vertical-tabs_collapsed-fieldsets.patch1.3 KBMohammed J. Razem
vertical-tabs_collapsed-fieldsets.patch1.19 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

Status: Active » Needs review
Mohammed J. Razem’s picture

Priority: Normal » Major
FileSize
1.3 KB

I 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.

drunken monkey’s picture

Status: Needs review » Needs work
FileSize
13.33 KB

Doesn't really work for me in Seven.

But thanks for the additions!

Mohammed J. Razem’s picture

Status: Needs work » Needs review

Make sure you patch it against a clean Drupal 7 core, as this looks to be caused from margin-bottom: 2em; in the rule div.vertical-tabs .vertical-tabs-panes fieldset fieldset legend from the previous patch.

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

I 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!

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review

This needs to be confirmed in IE, we need at least IE7 and IE8 screenshots for this.
Fieldset have proven to be hard in those.

Mohammed J. Razem’s picture

display: inline-block; instead of display: block; was needed to work properly with IE7.

Attached are screenshots for IE7 and IE8 as well as the modified patch.

aspilicious’s picture

1) 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?

jonathan1055’s picture

I 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

longwave’s picture

This 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?

yoroy’s picture

Issue tags: +Usability, +ui-pattern

In 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.

longwave’s picture

This affects all fieldsets with legends inside vertical tabs, which can still be useful for grouping related fields whether they are collapsible or not.

drunken monkey’s picture

In 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.

Also, 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.

bfroehle’s picture

I 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).

mrfelton’s picture

Patch 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.

Bojhan’s picture

Patch 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.

jonathan1055’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

Bojhan’s picture

@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.

bfroehle’s picture

Sigh, 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

yoroy’s picture

Note that we never argued that the bug should not be fixed.

gordon’s picture

subscribe

Shadlington’s picture

Just chipping in to say the patch worked for me too

Jacine’s picture

oy, subscribe.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

D8 first. Let's get these fixed on D8 and into D7.

ckng’s picture

The patch works for me.
Encounter this problem while adding collapsible fieldset to display token tree in Submitted By.

jonathan1055’s picture

To 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.

jonathan1055’s picture

drunken monkey’s picture

@ 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.

jonathan1055’s picture

Thanks, 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

rfay’s picture

Issue tags: +Needs backport to D7

Yeah, 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!

sun’s picture

keithm’s picture

subscribe

sun’s picture

Status: Needs review » Needs work
+++ themes/seven/vertical-tabs.css	5 Jan 2011 14:44:13 -0000
@@ -73,6 +73,14 @@ div.vertical-tabs .vertical-tabs-panes {
+div.vertical-tabs .vertical-tabs-panes fieldset fieldset {
+  border: 1px solid #ccc;
+  margin: 1em 0;
+  padding: 2.5em 0 0;
+}

This 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.

Bojhan’s picture

Priority: Major » Normal

This is a bug, but definitely not major - because it is not an advised practice anyway.

abbasmousavi’s picture

subscribe.

I use this feature in DruTeX configuration page
see #1207286: The fieldsets in DruTeX configuration do not display correctly

jonathan1055’s picture

Hi 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

dtarc’s picture

subscribing

tamasd’s picture

subscribing (I am looking forward to have this bug fixed in D7 core).

jessebeach’s picture

Adding 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

Gábor Hojtsy’s picture

We 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:

<div class="vertical-tabs clearfix">
  <ul class="vertical-tabs-list">....</ul>
  <div class="vertical-tabs-panes">
    <fieldset class="form-wrapper vertical-tabs-pane">
        <div class="fieldset-wrapper">
          ..... if there is a fieldset in here, it would have the form-wrapper class but not the vertical-tabs-pane class .....
        </div>
    </fieldset>
  </div>
</div>

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:

.vertical-tabs legend {
  display: none;
}

Should be:

fieldset.vertical-tabs-pane > legend {
  display: none;
}

In seven's override:

div.vertical-tabs fieldset {
  border: 0;
  padding: 0;
  margin: 0;
}
...
div.vertical-tabs .vertical-tabs-panes legend {
  display: none;
}

Should be:

fieldset.vertical-tabs-pane > fieldset {
  border: 0;
  padding: 0;
  margin: 0;
}
...
fieldset.vertical-tabs-pane > legend {
  display: none;
}

This was just some research, please help me verify, discuss.

Dave Reid’s picture

Subscribing - this is affecting several of my contrib modules.

jstoller’s picture

Can 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.

jonathan1055’s picture

In 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

mstrelan’s picture

+1 for jstoller's comment in #42.

xjm’s picture

@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:

  1. Collapsible fieldsets within vertical tabs give the user too many disparate things to click on, and too much clicking to get to content.
  2. Since vertical tabs are intended for small subsets of information, having fieldsets within them might indicate that there's too much information in the vertical tab.

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.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
2.06 KB
1022 bytes

Attached is a slight modification of Gabor's suggested changes above.

Before

before.png

After

after.png

Jacine’s picture

Issue tags: +Needs manual testing

This 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.

xjm’s picture

Yeah, for the D7 backport, we need to do something like:

fieldset.vertical-tabs-pane legend {
  display: none;
}
fieldset.vertical-tabs-pane fieldset legend {
  display: block;
}

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?

xjm’s picture

Stark before and after

Edit: Missing side border in the second is just bad cropping, sorry.

stark-before.png

stark-after.png

Garland before and after

garland-before.png

garland-after.png

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.
bartik-before.png

bartik-after.png

xjm’s picture

Oh, I forgot to mention, but I also tested that clicking on different tabs works in each above theme with the patch.

yoroy’s picture

FileSize
41.7 KB

By all means fix the bug, we established that in #20 already.

Maybe this helps explain, xjm gets it right in #45:

wireframe showing how nesting and grouping too much elements into a single vertical tabs risks overloading it

xjm’s picture

FileSize
751 bytes

This is why we'll need to add special IE6 CSS in D7. With the patch applied in IE6:
ie6-after.png

Patch in #46 still needs review for D8, though. Screenshots of the change in all core themes are in #49.

anavarre’s picture

This affects the Metatags module for D7.

JStanton’s picture

Status: Needs review » Reviewed & tested by the community

Applies cleanly to 8.x and works in Chrome, FF & Safari. (Sorry, on OSX so can't test IE).

webchick’s picture

Status: Reviewed & tested by the community » Needs review

We need IE testing before proceeding.

xjm’s picture

FileSize
1.97 KB
1.28 KB
1.3 KB

So 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

ie7.png

IE 8

ie8.png

IE 9

ie9.png

IE6 is in #52. The current patch is for D8 only for that reason.

My patch, so I can't RTBC. :)

xjm’s picture

FileSize
3.36 KB

Occurs 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.
with-contents.png

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok looks good

Everett Zufelt’s picture

I haven't tested, but looking at the patch in #46 I don't see anything that would appear to be problematic.

webchick’s picture

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

Awesome, 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.

oriol_e9g’s picture

Which 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:

fieldset.vertical-tabs-pane legend {
  display: none;
}
fieldset.vertical-tabs-pane fieldset legend {
  display: block;
}

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.

oriol_e9g’s picture

oriol_e9g’s picture

Status: Patch (to be ported) » Needs review

Patch with option #2. If you prefer another approach I can roll a new patch.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/themes/seven/ie6.cssundefined
@@ -15,3 +15,13 @@ ul.links li a,
+* html .vertical-tabs .form-type-textfield,
+* html .vertical-tabs .form-textarea-wrapper {
+  width: 95%;

we 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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
oriol_e9g’s picture

Missing file, sorry for the noise.

xjm’s picture

Status: Needs review » Needs work

Looks 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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
3.15 KB

Status: Needs review » Needs work
Issue tags: -Usability, -Metatags, -ui-pattern, -Needs manual testing, -Needs backport to D7, -Contributed project blocker

The last submitted patch, vertical-tab-legends-1015798-68.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Metatags, +ui-pattern, +Needs manual testing, +Needs backport to D7, +Contributed project blocker
dimitriseng’s picture

I 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.

oriol_e9g’s picture

We need a review (like you :D) and a testing with IE, then we can change the state to "Reviewed & tested by the community" :D

xjm’s picture

Yep, like it says in #67:

After that, let's get some additional manual testing in IE6, both for the vertical tabs and for the other moved rules.

klonos’s picture

...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 ;)

oriol_e9g’s picture

D7 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

klonos’s picture

Yep, 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).

klonos’s picture

...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.

tsi’s picture

IE6 ? even microsoft don't want it anymore, only developers use it for this kind of nonsense :)

Jacine’s picture

Status: Needs review » Needs work

Please, 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.

Jacine’s picture

Also, 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

xjm’s picture

Status: Needs work » Needs review

Er... 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?

Jacine’s picture

Er... the issue was already well-tested for IE7 before it was committed to D8. See #56 - #57.

I realize that, but Drupal 7 has different requirements than Drupal 8.

Also, a separate IE6 stylesheet is the preferred way to add IE6-only styling, unless I'm mistaken?

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.

xjm’s picture

Ah! 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:

fieldset legend {
  stuff;
}
fieldset fieldset legend {
  stuff;
}
Jacine’s picture

Status: Needs review » Needs work

Yep, exactly. Just a straight bug fix. :)

Marking needs work for that.

xjm’s picture

Issue tags: +Novice

Good 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:

fieldset.vertical-tabs-pane legend {
  display: none;
}
fieldset.vertical-tabs-pane fieldset legend {
  display: block;
}
mstrelan’s picture

Status: Needs work » Needs review
FileSize
448 bytes

You mean like this? Does anything need to change in the Seven theme?

xjm’s picture

Status: Needs review » Needs work

Yep, similar changes should be added to Seven as well. See: http://drupal.org/files/vertical-tab-legends.patch Thanks @mstrelan!

dimitriseng’s picture

@ 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.

xjm’s picture

@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

dimitriseng’s picture

Status: Needs work » Needs review
FileSize
1.1 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 1015798-90-vertical-tab-legends.patch, failed testing.

dimitriseng’s picture

Status: Needs work » Needs review
FileSize
1.1 KB

One more try...

dimitriseng’s picture

I 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.

xjm’s picture

Issue tags: -Novice

The CSS in #92 looks correct. Thanks @dimitriseng!

xjm’s picture

FileSize
583 bytes

Here'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.

xjm’s picture

aLearner tested the patch in all four themes in Firefox and confirmed the same results as in #49.

ryan.gibson’s picture

Okay, I've tested the patch in all four themes in both Chrome and Safari - the results in #49 stand firm there as well.

xjm’s picture

Thanks guys! So we just need testing for IE 6-9.

xjm’s picture

Status: Needs review » Needs work
FileSize
9.27 KB

Alright, 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:

ie6_seven.png

xjm’s picture

Status: Needs work » Needs review

Alright, 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+.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

So I pinged webchick about this, and she's okay with opening a followup to fix the seven border thing if anyone cares enough. Phew!

aLearner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.81 KB
9.04 KB
8.3 KB
9.12 KB
8.41 KB
11.6 KB

Just 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!

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, thanks @aLearner. All those screenshots look correct as well, so we are RTBC.

aLearner’s picture

xjm,

Cool beans. Thank you.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great; thanks for all the work on this folks!

Committed and pushed to 7.x. Thanks!

yoroy’s picture

Good job seeing this through folks.

klonos’s picture

Woohoo! yet one less patch to "babysit". Thanx Angie ;)

pbfleetwood’s picture

Re. #105:

Committed and pushed to 7.x. Thanks!

Will the fix be committed in v7.13? I hope, I hope, I hope.

Dave Reid’s picture

Awesome job everyone!

klonos’s picture

Will the fix be committed in v7.13?

AFAIK 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.

mstrelan’s picture

AFAIK 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.

Unless 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.

Status: Fixed » Closed (fixed)

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

realityloop’s picture

Status: Closed (fixed) » Active

This still doesn't appear to be committed to 7.15..?

realityloop’s picture

Status: Active » Closed (fixed)

nevermind