Fieldsets break design badly

that second issue is already outlined in #621008: "theme path" is wrong for theme process and preprocess overrides

I've been looking at the fieldset problem but the only solutions I can come up with feel really hacky and make me wanna hurl

hope u don't mind if I correct the title ("brake" = "remmen" in Dutch, "break" = "breken" or "pauze" in Dutch)

Apparently IE doesn't handle the padding-top of 30px on fieldset. We can do this.

Wow making this work for all browser is pretty hard.

This seems best solution for me: wrapping the contents of the fieldset and move the 30px padding-top from fieldset to margin-top of that wrapper.

-fieldset legend span,
-fieldset legend a {

In seven legend is always wrapped in a span so "fieldset legend a" wasn't supposed to be there.

+  display: block;

This one is necessary as it is being overridden in some cases.

Wow css/js is really messy.

I used the same classname as collapsible.js. Renamed it to seven-fieldset-wrapper. Maybe we should add that wrapper to the default theme_fieldset and alter collapsible.js (should be discussed in another issue I guess).

This patch makes it work for all browsers.

Collapsing in IE looks still bit weird, but that was already the case.

+fieldset legend > * {

This doesn't work for IE6. we should fix this in ie6.css, but that one isn't working yet.

Uh those failing tests are expecting a fieldset never contains a wrapper... Those tests really should be strengthened. But that's not related to this issue.

Huh that test isn't related (I had this before in another issue). That test also needs some love (Other issue of course).

I think a retest will pass...

I had attempted to fix this in another issue, see http://drupal.org/node/539724#comment-2222988.

I've had bad experiences in the past with wrapping fieldsets in divs. For example, the Vertical Tabs module converts fieldsets to tabs using JavaScript—it will leave behind all the wrappers, which can mess up your layout.

Edit: Looking again at the patch, I see that the div is not wrapping the fieldset, but its contents; that might be ok.

tested the patch in #10 on IE8, IE7, IE6 and FF3.5.6 on vista and all looks good xcept a few lil things

+fieldset legend > * {

that type of selector is not supported in IE6, but

+fieldset div.seven-fieldset-wrapper {
+  margin-top: 30px;
+  display: block;
 }

does work in IE6, these 2 together cause the legend to appear as it would by default (which I can personally live with), but with the margin-top on the wrapper there's this silly gap (http://gyazo.com/af849542c0415e60965e93fe9f2cf108.png)

so I see 2 solutions:
1) have IE6 fallback to the default and change fieldset div.seven-fieldset-wrapper to fieldset > div.seven-fieldset-wrapper so it will also be ignored by IE6 and the gap will disappear
2) change

fieldset legend > * {
  position: absolute;
  margin-top: 9px;
}

to

fieldset legend span,
fieldset legend a {
  position: absolute;
  margin-top: 9px;
}

fieldset legend span a,
fieldset legend a span {
  position: static;
  margin-top: 0;
}

so a <span> nested in an <a> and an <a> nested in a <span> don't get the absolute positioning and margin applied to them

personally, I'd go for the second option, but we need to make sure we're not overlooking any elements here

also, why is it called seven-fieldset-wrapper and not just fieldset-wrapper?
doing a quick

(function($) {
  console.log($('[id*=seven]'));
  console.log($('[class*=seven]'));
})(jQuery)

on a few pages didn't give me any other elements that have the themename in them, and I don't rly see any need to introduce this here

actually, to avoid confusion, can we call it fieldset-inner, as it doesn't rly wrap the fieldset but it's content?

attached patch changes it to fieldset-inner and applies solution 2 from the IE6 problem

IE6 screen as reference: http://gyazo.com/619af661649b4149fd56d1d90b5a2bc9.png

Just to say that it looks good in FF/Ubuntu too.

#19 I still would use "fieldset legend > *"

and add the following to ie6.css:

fieldset legend * {
  position: absolute;
  margin-top: 9px;
}

fieldset legend * * {
  position: static;
  margin-top: 0;
}

fieldset legend * * {
  position: static;
  margin-top: 0;
}

This could possibly override position/margins set by other stylesheets so we better have only IE6 deal with that issue.

Update:
See #8 why I used seven-fieldset-wrapper instead of fieldset-wrapper. I think we should stick to seven-fieldset-wrapper as its defined by that theme. fieldset-inner could possibly collide with css of other modules (we definitely need guidelines on CSS id/class names; other issue though).

just mentioning that similar work is going on at #563390: Seven theme lacks formatting on common html elements such as lists, paragraphs & <code> ... that issue is deferring to this one for fieldsets, but since they are close cousins I figured it was good to mention here to avoid any conflicts / cross posts

UPDATE: issue above has landed

Yike, Cross browser CSS for fieldsets is almost impossible to do cleanly

• FF has a nasty bug on the legend element; can't change position, display, width, etc.
  https://bugzilla.mozilla.org/show_bug.cgi?id=269908
• FF doesn't use left padding of fieldset on absolute "legend span". probably related to the previous issue.
• IE is adding padding-top to the fieldset above the legend, even when legend span is set to absolute. Setting top: 0, fixes this, but this isn't usable in other browsers.

• Seven theme wraps legend in a span element, but collapsible.js wraps contents in a link and adds a span.summary after the link (legend content is not wrapped in one element any more. In Seven we set children of legend to position:absolute. Link and span.summary (mostly emty, but not always) will overlap.

Solutions we could use:

1. http://www.tyssendesign.com.au/examples/firefox-legend-bug.html#ex-two
   Add a wrapper div around fieldset. Using this we could make this work pixelperfect.
   pro: Cross browser + pixelperfect + no CSS hacks + compatible with collapsible.js
   con: We need a extra div
2. Patch in #19
   Wrap content of fieldset in a div. Needs legend content to be wrapped.
   pro: Cross browser
   con: We need a extra div and legend wrapper, specific CSS for IE6, incompatible with collapsible.js
3. I found this myself. This works in all but IE6, but we have a ie6.css. Problem is that it alters margin-top of first element after legend. As all kind of elements can be added to fieldset, this could give trouble. Needs more testing. Needs legend content to be wrapped.

   fieldset legend > * {
     position: absolute;
     line-height: 30px;
   }
   fieldset legend + * {
     margin-top: 30px !important;
   }
   

   pro: Cross browser, no extra divs
   con: We need the legend wrapper span, Could break in certain situations, specific CSS for IE6, incompatible with collapsible.js

I think I like solution 1 best. I'll cost an extra div (minus span to wrap legend content), but its the most complete solution. What do you think?

I too support solution 1.

Ok patch with solution 1

- Crossbrowser + pixelperfect
- Wraps fieldset in div (used class starting with "seven-" to indicate its theme specific only; see #22)
- Wrapping legend content in span is no longer necessary (no more collision with collapsible.js)
- Had to use IE6/IE7 CSS hacks to make it pixelperfect (Used hacks are valid CSS and won't with future browsers)
- Tested in IE6/IE7/IE8, FF3.5, Opera 10, Chrome 4

Need at least some testing in FF2

Messes with vertical tabs (see screenshot)

vertical-tabs.js doesn't allow fieldsets to be wrapped...

Something like this should do; removing the fieldset wrapper from fieldsets in the vertical-tabs form element. Not really sure which hook to use to do this in a clean manner. Anyone?

@casey: much better :) however, not perfect... There is a problem with margin/padding within the vertical tabs content area (see screenshot). The content is not pushed down far enough from the top of the vtabs content area.

forgot to attach...

EDIT: note, this is with Firefox 3.5, Ubuntu

+++ themes/seven/template.php	9 Jan 2010 16:29:49 -0000
@@ -73,14 +73,14 @@
-  $output = '<fieldset' . drupal_attributes($element['#attributes']) . '>';
+  $output = '<div class="seven-fieldset-wrapper"><fieldset' . drupal_attributes($element['#attributes']) . '>';

@@ -89,11 +89,22 @@
+function seven_preprocess_vertical_tabs(&$variables) {
+  $variables['element']['#children'] = str_replace(
+    array('<div class="seven-fieldset-wrapper">', '</fieldset></div>'),
+    array('', '</fieldset>'),
+    $variables['element']['#children']
+  );
+}

What you want is to conditionally add another CSS class to the fieldset. Reverse the logic and only add the class to vertical tab fieldsets.

This str_replace() won't fly.

Powered by Dreditor.

Try something like this instead. If more than IE6 is required, then we may need to add another conditional stylesheet ie.css that applies to all IE versions.

-1 for wrapping the fieldset in a div (see #17). Any module that uses JavaScript to move fieldsets around in the DOM isn't expecting it to be wrapped, and will leave the DIV behind.

we may need to add another conditional stylesheet ie.css that applies to all IE versions.

This is what I did in http://drupal.org/node/539724#comment-2222988.

I am the subscribinator

I have the problem on Firefox as well, patch from sun in #36 does not fix it.

Attached screenshot is with FF.

This is a pretty difficult bug where you're trying to essentially relatively reposition an inline element inside of a special case inline element.

In my opinion, trying to wrap the inline elements with a block in order to subvert the User Agent positioning of the tag is more "hacky" than a clean usage of an IE conditional comment. It's exactly for situations like this that you find a real use for the conditional comment. Although, it would be better if IE stopped being different just to stand out. So the conditional comment allows you to ensure correct rendering in IE without having to do CSS contortions to get the other browsers to accept rules that will also work in IE.

However, you still want your CSS to be future proof. The legend positioning issue is particularly difficult because IE7 renders the legend 7px to the right of where IE8 does. In order to simplify the fix and not have multiple conditional comments I added a meta tag to the header to force IE7 compatibility mode in IE8 (and beyond). This reduces the additional CSS to a single file, a concession already accepted in the IE6 check, so I don't believe it can be accused of being "hacky".

I've tested this in IE7, IE8, FF3.5, FF3.6, Safari4, and Chrome 4. I've checked the regular fieldsets, collapsible fieldsets, and the tabbed fieldsets. I've validated the CSS as 2.1 at W3C.org. I think this is the best solution to the render issue.

I hope it helps.

Thanks for catching those. The first one is a nasty issue with IE.

The second looks like the meta tag for rendering in IE7 compatibility mode wasn't respected. Is it possible you had the IE dev toolbar set to IE8 mode?

And I never thought to check to see if the install screen was affected.

I'll see about modifying my patch to fix these.

I really don't find it an option to slap a IE7-mode meta tag on every page with a fieldset in it, this seems anything but future-proof

a discrepancy of 7px seems more acceptable to me

Is the reset.css in Seven the problem? I compared the Meyer Reset that is apart of the Bluetrip CSS framework and reset.css and found found only one css rule that was in Bluetrip that wasn't included.

After the huge rule resetting the line height:

html, body, ... , ul.secondary a.active, .resizable-textarea 
{
  margin:0;padding:0;border:0;outline:0;
  font-size:100%;
  vertical-align:baseline;
  background:transparent;
  line-height: inherit;
}

The Bluetrip's Meyer Reset includes this while reset.css does not:

  body{line-height:1}

Also Meyer's Reset found here: http://meyerweb.com/eric/tools/css/reset/ does not include a line-height: inherit as a property in the massive first rule. I know that line-height is one of the many areas of conflict between browser rendering engines. Is the issue I brought up relevant here?

I don't like using the meta tag either. I would certainly prefer that the solution be that IE renders things more in line with other browsers.

Also, the 7px discrepancy is the only issue that I cited. There may be other rendering differences between IE7 and IE8 that this will also correct.

I see two solutions to the problem.

1. Add another conditional comment to the template to add an IE8 specific stylesheet.

2. Add the compatibility mode meta tag.

3. Add compatibility information to the DOCTYPE declaration.

I don't think that 1 is very future proof. IE9 may have it's own inconsistencies so yet another conditional comment would be needed if we apply the same solution moving forward.

I like 2 because it is the suggested method from MS and doesn't depend on declaring a specific DOCTYPE. In this MSDN article http://msdn.microsoft.com/en-us/library/cc288325%28VS.85%29.aspx MS indicates that the meta tag is something they intend to have future versions of IE respect. When I find myself confounded by IE, I'd prefer to use MS solution, because I have some small hope that they will honor their suggested fixes in future versions.

It's a very small hope.

I think I nailed it! This time I used a div that wraps fieldsets' content.

I tested it on FF 3.6, Opera 10, Chrome 4, IE8, IE7 and IE6 on normal fieldsets, collapsible fieldsets and vertical-tabs and believe its pixel-perfect.

#10

those failing tests are expecting a fieldset never contains a wrapper... Those tests really should be strengthened. But that's not related to this issue.

Used ctrl-z one time to much.

Argh cross-browser css for fieldsets is a delicate business. Firefox didn't accept previous patch.

FF still doesn't like the patch.

I think I have a patch that solves the problem very well cross browser. I'm not sure I like how I handled the fix in maintenance-page.tpl.php but it works solidly.

It should fix all the problems from #41 including the second image which was a fault of IE not respecting the meta tag unless it is near the top of the header.

Also just tried your patch with Safari 4 and Chrome and got the attached error.

Did you refresh your drupal cache? My patch contains a new theme function for seven.

<meta http-equiv="X-UA-Compatible" content="IE=EmulateIE7"  />

Waaah...

Oww my previous patch didn't work on fieldsets containing floating elements.

#54 patch fixes the floating elements within the fieldsets, but still has the Firefox positioning error.

#55 the first issue also occurs without this patch. haven't found the sucker that is causing it yet.

#58 Argh I removed part of my patch.

Also found out that first issue in #55 is caused by slideDown() in collapse.js. No solution yet.

Attached render issues with patch in #59

FF still puts the legend in the border of the fieldset

IE7 adds an extra 7 pixels to the left of the legend and extra spacing below.

EDIT: FF screenshot incorrect

Correct screen of FF error from patch #59

FF 3.6 on OSX displayed correctly.

FF 3.5.8 on Win7 displayed incorrectly

Ok another try. Tested on winXP FF 3.6, Opera 10, Chrome 4, IE8, IE7 and IE6 on normal fieldsets, collapsible fieldsets and vertical-tabs. Also tested with javascript disabled.

@james.eliott could you retest FF 3.5.8?

- Extra spacing below legend in IE7 (and IE6) isn't part of this issue (is caused by the clearfix class on the next element)
- First issue in #55 is caused by slideDown() in collapse.js (IE7 and IE6) and isn't caused by this patch.

You should open seperate issues for those.

Downloaded a FF collection. Patch doesn't work for FF 3.5.8 and FF 2.

Safari is buggy too... IE6 seems okay (though ugly).
Screenshots attached.

Edit : it is Safari 4.0.4 btw.

@casey Tested in FF 3.5.8 and now the collapsible legends are above the fieldset border as well as the regular legends.

I made a single line addition to your patch and fixed the issue.

I've tested in FF 3.6, FF 3.5.8, Safari 4, Chrome, IE7, and IE8. Can you confirm this works?

#67: seven_fieldset.patch queued for re-testing.

@aspilicious see #10

Sorry but it seems this approach isn't going to work for FF; until FF 3.6 you can't alter 's width (https://bugzilla.mozilla.org/show_bug.cgi?id=269908).

@casey Have you tried my patch in #51? It fixes all the issues pretty well, although the fix for the IE top border might be a little "hacky"

There just landed a patch that allows using conditional comments #228818: IE: Stylesheets ignored after 31 link/style tags. I don't really like those, but maybe its the best way. I think you should use a conditional comment for IE8 as well. And hope for the best for IE9.

Telling IE8 to behave like IE7 looks like a regression to me.

So if you rewrite your patch of #51 to use a conditional comment for IE8, I am with you.

Thanks for working so hard on this!

However, most of the problems here should be resolved in a more generic manner, i.e. by adjusting fieldset markup, fieldset CSS, and collapse.js in a way to allow for custom design patterns.

We should additionally try to pull Jacine into this issue.

+++ themes/seven/style.css	25 Feb 2010 19:41:38 -0000
@@ -607,49 +607,85 @@ table tr.selected td {
+fieldset legend + .fieldset-content,
+fieldset legend + .fieldset-wrapper > .fieldset-content {
+  padding-top: 32px;
 }
...
+html.js fieldset.collapsible .fieldset-wrapper {
+  overflow: visible;
 }

See my note on .fieldset-wrapper and .fieldset-content below.

+++ themes/seven/style.css	25 Feb 2010 19:41:38 -0000
@@ -607,49 +607,85 @@ table tr.selected td {
+html.js .vertical-tabs-panes fieldset .fieldset-content {
+  margin: 0;
+  padding: 0;
+  border: 0;
 }

Ugh. That's what we get for abusing semantic markup for vertical tabs...

We may want to consider to

a) either tweak vertical-tabs.css instead (more important selectors)

b) add a CSS class for "regular" fieldsets (yuck)

Either way, since there can be regular fieldsets contained in vertical tabs, we need a more "friendly" solution here.

+++ themes/seven/style.css	25 Feb 2010 19:41:38 -0000
@@ -930,6 +962,9 @@ body.in-maintenance #content {
+*:first-child+html body.in-maintenance #content {
+  
+}

Obsolete.

+++ themes/seven/template.php	25 Feb 2010 19:41:38 -0000
@@ -80,8 +80,9 @@ function seven_fieldset($variables) {
-    $output .= '<legend><span>' . $element['#title'] . '</span></legend>';
+    $output .= '<legend>' . $element['#title'] . '</legend>';
...
+  $output .= '<div class="fieldset-content clearfix">';

@@ -89,7 +90,7 @@ function seven_fieldset($variables) {
-  $output .= "</fieldset>\n";
+  $output .= "</div></fieldset>\n";

These changes actually make the seven_fieldset() function superfluous.

Please move the wrapping .fieldset-content DIV container as static markup into theme_fieldset().

All of this requires corresponding changes to collapse.js, allowing us to greatly simplify its code.

The .fieldset-wrapper injected by collapse.js can finally vanish. To make this a bit more backwards compatible, I suggest to rename .fieldset-content to .fieldset-wrapper.

The only thing I'm not sure of is the .clearfix class you additionally applied to .fieldset-content - that should not be necessary and likely makes things more complex.

Powered by Dreditor.

Please move the wrapping .fieldset-content DIV container as static markup into theme_fieldset().

All of this requires corresponding changes to collapse.js, allowing us to greatly simplify its code.

The .fieldset-wrapper injected by collapse.js can finally vanish. To make this a bit more backwards compatible, I suggest to rename .fieldset-content to .fieldset-wrapper.

Ow wow I did consider altering theme_fieldset, but was afraid it was going to be rejected.

This indeed will make things a lot easier.

I'll keep .clearfix out of it.

This should give you a starting point.

Works in FF 3.5 for Garland and Seven.

+++ misc/collapse.js	26 Feb 2010 14:53:58 -0000
@@ -5,40 +5,37 @@
+      .find('> legend > a > span.element-invisible').html(Drupal.t('Hide'));

legend > a is wrong now.

139 critical left. Go review some!

Subscribing. Will try to give this a go tonight.

I've read through the issue, and reviewed sun's patch in #77. I agree this is a good starting point.

+++ includes/form.inc	26 Feb 2010 14:30:59 -0000
@@ -1950,8 +1950,10 @@ function theme_fieldset($variables) {
-    $output .= '<legend>' . $element['#title'] . '</legend>';
+    // Always wrap fieldset legends in a SPAN for CSS positioning.
+    $output .= '<legend><span>' . $element['#title'] . '</span></legend>';

This would be a good improvement. I know it would make morten.dk a happy camper, based on his recent post: Fieldset & Legend Behave!

+++ includes/form.inc	26 Feb 2010 14:30:59 -0000
@@ -1950,8 +1950,10 @@ function theme_fieldset($variables) {
+  $output .= '<div class="fieldset-wrapper">';

This is something I've done quite often, so I'm definitely cool with this.

I did some IE testing and some quick CSS tweaking for IE7 and IE8 in an ie.css file that targets lte IE8. Negative margins seem to do the trick, but that 7px annoyance is a problem because of course it's different in IE8, so there is a * hack in my patch for that.

I've checked in all the places I could find in the screenshots that have been posted up here, and it seems to be good. IE6 is another story and I don't have time for that right now. They look fine, just not like the design mockups (which may be good enough, not sure).

Attached patch is a combo of #77 + the CSS I did for IE.

Powered by Dreditor.

so.. lets review this

+        $('.form-actions', fieldset).show();

Is this still necessary? According to #214292: Internet Explorer: Default submit button altered by collapse.js the bug that IE alters the default submit button was fixed by excluding the button containers (div.action) from the wrapper.

I did a quick test (and removed $('.form-actions', fieldset).show(); and $('.form-actions', fieldset).hide;), and can't reproduce that bug. Maybe because we now only hide ".fieldset-wrapper" instead of "fieldset *".

@casey: The "no longer fieldset *" reasoning makes a lot of sense, thanks! So we should be able to remove yet another workaround, yay! :)

Test in IE7

I applied the patch and this might not be related, since I see this phenomena before the patch is applied too (see attachment). The table columns appear to extend their table borders, but only at the top of the overlay.

Also, the breadcrumb on #overlay=admin/people/permissions behaves strangely. It color changes from blue to white then back to blue for unobvious reasons.

@cosmicdreams: Thanks for testing! However, just disable Overlay to test this patch, please. Technically, the changes of this patch should not affect Overlay - so either the bugs you found are already known for Overlay in IE, or have to be files as separate issues. Either way, this patch won't get hold off due to Overlay issues.

+++ themes/seven/ie.css	1 Mar 2010 08:10:58 -0000
@@ -0,0 +1,10 @@
+fieldset legend {
+  *margin-left: -7px; /* IE7 */

Is that asterisk a typo or is it an IE hack that I don't know yet?

139 critical left. Go review some!

@sun It's a hack and not a good one, so I would not commit in this state. I usually use separate files for each IE version, so I'm not the queen of IE hacks :P I am surprised that IE8 is having such problems with this though.

I would like to see more work done to try and have this work without IE-specific code. Not sure if that's possible at this point, but I think it's worth putting a little more effort into it.

Also, it would help knowing what the design requirements are for IE6. Does it need to look exactly like the design (hopefully not)?

Let's make those weird tests pass first ;)

#90: drupal.fieldset.90.patch queued for re-testing.

Debugging code, bad state. After playing around with this, I realized that span.summary is still output _next_ to the fieldset legend innards, which leads to a gap in the fieldset's border + also totally breaks + invalidates the fieldset styling when being removed/adjusted. On it.

Whatever we do here, Firefox will always be broken in one version or the other, due to its lovely hard-coded styling of LEGEND: https://bugzilla.mozilla.org/show_bug.cgi?id=269908

Milestone | Rollback point

I did this, once, for a custom theme, but it required to theme fieldsets/legends entirely differently. Therefore, this patch == backup, prior nuking each + everything. ;)

So what else can I do for you? No harder challenges? ;)

Works properly in Garland + Seven theme.

Works properly in Firefox 3.5, Safari 4, Opera 9, IE6, IE7, and IE8 (beta).

Also attaching my custom.module to simplify testing those multiple fieldset variants in different browsers in a dedicated form.

Great work!

.fieldset-wrapper needs to clear floats. To fix it I added display:block to .fieldset-wrapper in seven/style.css. Note: display:inline-block doesn't work for FF2.

+fieldset .fieldset-wrapper {
+  padding: 0 13px 13px 15px;
+  display: block;

And dispay:inline-block to .fieldset-wrapper in seven/ie.css

+fieldset .fieldset-wrapper {
+  display: inline-block;
+}

Also .action-links floats in IE6 so I added (to ie6.css)

 ul.links li a,
+ul.action-links,
 #page {
   height: 1%;
 }

+++ themes/seven/ie.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,11 @@
+fieldset .fieldset-wrapper {
+  display: inline-block;
+}

Can you explain this a bit? display: block should also work in IE, so why do we need inline-block here? If it's really required, then we need an inline comment similar to the other IE tweak that exists already.

138 critical left. Go review some!

Thanks gang, sorry if my last comment was instructive, I meant to say that I had tested in IE7. I tested again today with patch from #97.

I think I might be applying the patch wrong. I used this command this morning on a fresh cvs of HEAD:

patch -p0 < drupal.fieldset.97.patch

and got this result

patching file modules/user/user.test
patching file includes/form.inc
patching file themes/seven/template.php
patching file themes/seven/style.css
patching file themes/seven/ie6.css
patching file modules/simpletest/simpletest.test
patching file themes/garland/style.css
patching file misc/collapse.js
patching file modules/system/system-behavior.css
The next patch would create the file themes/seven/ie.css,
which already exists!  Assume -R? [n] y
patching file themes/seven/ie.css
Hunk #1 FAILED at 1.
File themes/seven/ie.css is not empty after patch, as expected
1 out of 1 hunk FAILED -- saving rejects to file themes/seven/ie.css.rej

that ie.css.rej file listed this:

***************
*** 1,11 ****
- /* $Id$ */
-
- /* IE (all) renders absolute positioned fieldset legend where fieldset content starts */
- fieldset .fieldset-legend {
-   left: 0;
-   top: 0;
- }
-
- fieldset .fieldset-wrapper {
-   display: inline-block;
- }
--- 0 ----

so I manually added that code to the ie.css. I'll get back to you after my tests, but initial results shows the Chrome was rendering the modules page poorly. I'll make sure all cache is flushed before I report any more.

display:block on .fieldset-wrapper is actually necessary because of

.container-inline div, .container-inline label {
  display: inline;
}

It seems to be a selector priority problem for IE6 and IE7.

"fieldset div.fieldset-wrapper" or "display:block !important;" would do too. ie.css doesn't need an extra ruleset then. What do you prefer?

It's however weird that display:inline-block did work.

Edit
Uhh markup of admin/people is pretty weird too. It contains a div with container-inline class that wraps the "Update options" fieldset... (same in comment_admin_overview()). I think that div belongs inside the fieldset.

display:block on .fieldset-wrapper shouldn't be necessary as it is a div already... container-inline class is being mis-used.

Oh, sorry, I was being dumb there. A previous patch was still in place. I deleted the seven theme and grabbed a fresh copy. Here's what I tested:

Tested in IE 8.0.6001.18702, Chrome 5.0.307.11 beta, and Firefox 3.5 (with overlays turned off)
* /admin/content
* /node/1/edit?destination=admin/content (Edit Page)
* /
* /admin/structure
* /admin/structure/block
* /admin/structure/block/add (had to manually go to this page)
* /admin/structure/types
* /admin/structure/menu
* /admin/structure/taxonomy
* /admin/structure/taxonomy/1/edit
* /admin/structure/taxonomy/1
* /admin/structure/taxonomy/1/add
* /admin/appearance
* /admin/appearance/update (weird table bug in Chrome)
* /admin/appearance/settings
* /admin/people
* /admin/people/create
* /admin/people/permissions
* /admin/people/permissions/roles
* /admin/modules
* /admin/modules/update
* /admin/modules/uninstall
* /admin/config
* /admin/config/content/formats/1
* /admin/reports/dblog

In all my tests:
* collapsible elements were able to collapse
* Legends appeared to format properly
* No overlap issue of text over textboxes

In short, this patch worked well and looks good to me. I would like to label this as reviewed by community, but would like to test in IE 6 and firefox 3.6 first. Can other test in those browsers?

Notes that might be related to this patch
* I finally understand what's been bugging me about the /node/1/edit page. In the Comment Settings vertical menu, there is no bold-ed heading for the bullet points. This lead to the first bullet point being rendered higher than all the other starting elements in the other vertical menus of the node edit page. Compare this with /admin/structure/block/add (the Users vertical menu) and you can see how adding a label to the option list helps. I'll see if there is a bug for this.

Notes that clearly aren't related to this patch:
1. The Chrome issue I mentioned above? Forget that. What I was seeing was a change in how the padding of the word "Core" appeared on the modules page. After further review I see that it was changed to format the collapsible legends to be consistent with other browsers.

2. In Chrome, on /admin/appearance/update, The table appears to be disconnected. A border-top may need be applied.

3. In IE7, on a "Node Delete" page (ex: /node/1/delete?destination=admin/content): The Cancel link is not same horizontal lineage as the Delete button.

4. Couldn't find the Add new block link on the /admin/structure/block. I think it's not there (is this a mistake?)

5. On the home page, the lack of page title makes the add shortcut link look bad. Perhaps we can add formatting to the + icon itself so that it isn't dependent on the height of

  <h1 class="page-title">

Edit

Looks like I tested all that with IE 8 not IE 7, sorry for the miscommunication.

@casey: You might be right about the mis-use of .container-inline and unnecessary code. But to my eye, I can't tell the difference. /admin/people look to work fine for me in my newly opened IE 7 (fresh vista install). I also couldn't detect anything wrong with /admin/content/comment/approval.

Can you describe the issue you're seeing a bit more?

Ok based upon patch in #96 (not 97#). Changes since #96:

Moved padding-bottom from .fieldset-wrapper to fieldset:

 fieldset {
   border: 1px solid #ccc;
-  padding: 30px 13px 13px 14px;
+  padding: 2.5em 0 13px 0;
+  position: relative;
   margin: 0 0 10px;
 }
...
+fieldset .fieldset-wrapper {
+  padding: 0 13px 0 15px;
 }

Also .action-links floats in IE6 so I added (to ie6.css)

ul.links li a,
+ul.action-links,
#page {
   height: 1%;
}

I created an issue for the misused container-inline class: #730332: .container-inline being misused

To test properly you need to combine those two patches.

So I should test with combining the patches of drupal.fieldset.96.patch and drupal.fieldset.102.patch ?

Also, What page and what actions are you testing to reveal the error you are seeing?

drupal.fieldset.102.patch + container-inline.patch (#730332: .container-inline being misused) combined.

admin/people without container-inline.patch shows the bug.

Oww another issue: conditional stylesheets aren't added while installing; seven_preprocess_html() isn't called.

Luckily this doesn't mean that the latest patch isn't correct.

I moved padding-bottom to fieldset in #103. This padding-bottom needs to be removed when collapsed.

Ah yes: I see the issue now. I tested the following for IE 7, Firefox 3.5, and Chrome 5.0.307.11 beta (on Linux):

After applying drupal.fieldset.102.patch:

These pages render a button and select box that hug the left-most edge of its containing element.
* /admin/content
* /admin/content/comment
* /admin/content/comment/approval
* /admin/people
* /admin/config/system/actions

This is true for all browsers listed above. I didn't experience this issue when testing patch from #97.

After applying container-inline.patch:

Yes all of those pages render properly again in all the browsers I listed.

I am a bit confused why I didn't see these issues come up with the patch from #97, I know that for firefox 3.5 I was sure to disable the cache so that I knew I was getting good tests. But if you think this is a better patch, casey, then I can confirm it works for these test cases.

Hmm..... I have Opera. I test in that too. But if you don't mind I'll just test to see if those pages are working after the duel patches.

Tested in Opera 10.10 build 4742

Yes, After both patches are applied Opera does not show any related issues with these patches.

Unrelated: But... the file chooser button on a few pages (ex: /node/add/article) looks squashed. (I'll see if there's a bug for this).

IE6 with patch of #108 and #730332: .container-inline being misused

I also have Safari:

Tested in Safari 4.0.4

Looks good on all the pages I earmarked above.

@aspilicious : sorry, didn't mean to step on your status change

Removed after a comment above was fixed.

@aspilicious and @casey: Since drupal.fieldset.97.patch also seemed to solve the problem (without container-inline.patch) can either of you confirm that pages are still incorrect if you apply drupal.fieldset.97.patch on a fresh cvs install.

I'm just wondering if I missed something. If drupal.fieldset.97.patch also works I think patches drupal.fieldset.108.patch + container-inline.patch are the best candidates to be committed, since they seem to do the job with better code.

1) There was talk about two patches being required to be applied. Needs merging? Otherwise, please clarify the situation ;)

2) There's a @todo in collapse.js, I/we still need to resolve.

3) I want to add some inline docs for Seven's fieldset styling, explaining the nightmare of rendering behavior of fieldsets in different browsers, and how these styles solve the issues.

4) Final review. By me, and also Jacine.

and also I don't think anyone has tested with IE6. Does anyone even have IE6 anymore?

Edit:
Sorry didn't see that casey had in fact tested IE6.

looks to me that sun is asking this line:

+      // @todo Properly use .wrap() here
+      var $legend = $('> legend .fieldset-legend', this);
+      var text = $legend.html();
+      $legend.empty()
+        .append($('<a class="fieldset-title" href="#">' + text + '</a>')

to change to

+      var $legend = $('> legend .fieldset-legend', this).wrap('<a class="fieldset-title" href="#">');
+      var text = $legend.html();
+      $legend.empty()
+        .append($(text)

Can someone point me to the documentation about properly making patches?

Let's see if I did this right...

Deleted all my directories, updated from fresh cvs, reapplied drupal.fieldset.108.patch and container-inline.patch, changed collapse.js and created this patch.

patch 122 and 123 are my attempts to answer 1) and 2) from sun's post #118. In posts above I forgot to mention that I've tested this with IE 7 and Firefox 3.5. They both worked for me. I should have tested them more extensively before submitting so keep a look out for bugs.

These patch uploads were more of a test to see if I could create a proper patch and submit it for testing.

Unfortunately, no. If it would be that easy, I wouldn't have added that @todo ;) If someone could clarify the situation re: patch/merging/?, then I could take a stab at it, if no one beats me to it.

EDIT: Seems like we have to get #730332: .container-inline being misused in first. These two patches are technically unrelated + shouldn't be merged. Sorry, if that was meant with "the other patch" earlier, then I know understand the confusion.

@sun: Where would be the best place to put the documentation you spoke of in 3) ?

Sure sun: The patch we are proposing for your review is the combined patches of:

* drupal.fieldset.108.patch AND
* container-inline.patch

I have attempted to combine them and include the requested change to collapse.js in my most recent patch: drupal.fieldset.123.patch.

But in case I've done something wrong in that patch a good starting point from you is apply the two patches you can find linked above and go from there.

Also, I see how my patch has broke the modules page (and most likely others). If you'd like I can backout my change to collapse.js and roll a patch that is simply the merging of the two patches I listed above. Would you benefit from that?

@sun: Hmm.... It appears I patched in the wrong order, I'll try the order you suggest.

@sun: I know you said that patches are unrelated and shouldn't be merged for this issue. But in our tested that we journaled above we found that without applying container-inline.patch fieldsets still appeared to be "broken" or rendered badly.

That is why we proposed to merge the two patches above.

In case you do think that the merged patch of #108 and container-inline.patch is worth reviewing, I've refreshed my cvs tree and reapplied in the patches in the order you suggested (container-inline first then 108).

Let's get the other patch in first: #730332: .container-inline being misused (separately, not in this issue)

Dang. Just read the OP in #730332: .container-inline being misused once again and it states that the patch depends on this patch due to the .fieldset-wrapper DIV container. This means we have to merge them. Sorry. Marked the other issue as duplicate.

@sun: if we have to merge them, how does drupal.fieldset.129.patch look to you. That was my attempt to merge the two patches.

You guys are great :D

@cosmicdreams, took a quick look at #129 and noticed it's missing the seven/ie.css file.

Will fully test later on.

@Jacine: your right, don't know why it wasn't included. I was sure to start from scratch. Perhaps someone else can compile the patch and show me what I'm doing wrong.

@sun: some test code I'm working on is showing that .wrapInner() has promise. I'll try to include a better patch when I see it working properly.

@cosmicdreams - Ah, I think you just need -N in your patch command, i.e. cvs diff -u -p -N > foo.patch

Still on it. Here's my new, advanced testing module, which should cover almost all use-cases we found throughout core.

Here we go.

- Added docs.

- Resolved the @todo in collapse.js.

- Reverted padding-bottom change by casey from #103, since it broke styling in IE, and there was no explanation for this change.

- Had to fix Seven styles for .form-item, since the .container-inline behavior wasn't applied at all.

- Removed some more (older) CSS hacks for IE for fieldsets.

- Removed an obsolete tweak for Firefox for fieldsets in Garland.

- Fastened CSS selectors for fieldset legends by removing "legend" where .fieldset-legend is selected.

- Fixed stale styles for fieldset descriptions, which used a class of .description previously, but are using .fieldset-description for quite some time already.

Tested in FF 3.5, IE6, IE7, IE8 (beta).

So:

1) Final cross-browser tests required.

2) Final code review required.

bump

Interesting fix and interesting note about .wrapInner(). I'll test with the browsers I have.

Sun, you must have pretty good working knowledge of the theming layer in order to make that many changes with this patch. I'm looking forward to running it through it's paces.

Tested with Chrome 5.0.307.11 beta, Chrome 4.0.249.89, Firefox 3.5, Firefox 3.0.18, Opera 10.10, IE 7, IE8, and Safari 4.0.4

Everything looks good to me.

I think we've tested in all the relevant browsers except for Firefox 3.6. I'll change the status to reviewed and tested by the community to raise the awareness of this patch.

This was no easy task, but the result is perfection. I checked in IE6, IE7, IE8, Safari 4, Chrome 5 (dev), Opera 10, Firefox 2, Firefox 3.6, and really it looks great!

In the CSS:

- Love how we are being less specific here (removing the div from div.form-item)
- Love all the docs.

Awesome work all around. This is a really great improvement for themers!

141 critical left. Go review some!

Should we test with other themes as extensively as we have with Seven? This issue is no longer tied to Seven specifically.

I also tested Garland, and if I'm not mistaken, then Jacine even tested Stark.

Yes, I tested Seven, Garland and Stark. All good!

Amazing how many browsers some people have installed. I guess it means we have more themers nowadays -- a good thing. ;-)

I committed the patch to CVS HEAD. Thanks all.

yay! We missed the added file themes/seven/ie.css though :)

Good point Sun, the ie.css is apart of the http://drupal.org/files/issues/drupal.fieldset.139.patch but wasn't in the CVS commit: http://drupal.org/cvs?commit=336688

oh snap, this suddenly went fast. many thanks guys (even if this broke 4 of my other patches :P), it was a tough nut, but looks like you cracked it anyway

I see the padding-bottom is moved back to .fieldset-wrapper. Sorry for not adding any comment.

Moving the padding-bottom to fieldset is necessary as it isn't visible when a fieldset only contains floating content.

@casey, you know I had seen that too, but didn't think it was related. Looks like we have room to fix our two outstanding issues:

1. The missing ie.css AND
2. the missing padding-bottom to fieldsets.

Here's a patch to fix the padding issue the casey found. In my tests of the fieldset padding-bottom I found that only one fieldset was adversely effected by patch #139,

<fieldset id="edit-filters">

That is why I added a line to the style.css to add padding-bottom to only that id. For consistency's sake I made the padding-bottom 13px, even though 15px looked better to me.

I've also included the ie.css as a text file in hopes that someone with write permissions on the cvs repository will add it as a css file to to the seven theme folder.

Please commit that forgotten ie.css first.

Sorry but patch from #155 is just too specific (and incorrect, now only that fieldset has a padding-bottom).

Marking ie.css from #55 RTBC, although it's probably still in the filesystem in webchick or Dries' checkout sitting there all lonely wondering why it wasn't committed with the rest of the patch ready to go.

+++ themes/seven/style.css	4 Mar 2010 06:09:19 -0000
@@ -628,6 +628,8 @@ fieldset {
+fieldset#edit-filters { padding-bottom: 13px; }

@@ -636,7 +638,7 @@ fieldset .fieldset-legend {
-  padding: 0 13px 13px 15px;
+  padding: 0 13px 0px 15px;

All other fieldsets need the padding-bottom, so we cannot remove the it from the "defaults".

I guess this applies to the user and node filter form?

Options:

1) Leave the default padding in place; just add additional padding for this fieldset. (ugh)

2) Play around with .clearfix.

3) Add an inner container for those filter forms and apply .clearfix there.

143 critical left. Go review some!

Why not what I did before? Add padding-bottom to fieldset and remove from .fieldset-wrapper?

@casey: Because that leads to a too large padding in the .collapsed state. You don't have to try to change this, because I can already tell you that splitting the large padding-top into padding-top and padding-bottom will make it work in .collapsed, but then utterly fail in .collapsible ;)

I think the most sane we can do is 3) -- If a module outputs floating content, then it's the responsibility of that module to output those contents in a container with proper clearing.

@casey: I tested the idea of applying the padding-bottom to the fieldset and removing the padding-bottom from the .fieldset-wrapper last night. What I found was that the padding at the bottom of fieldset was not consistent and just looked wrong to me.

What dictates the bottom margin of feildsets that aren't #edit-filters is a margin that is applied to the bottom of some of the elements those fieldsets contain. Sorry if that's a bit unclear but I'm depending on my memory of what I tried last night.

I'm curious to see the result of 3). If I have time today (I probably won't) I'll give that a try myself.

Committed ie.css from #155. Back to... needs review?

Actually I think "needs work" is most appropriate since the latest patch to fix the last remaining issue (drupal.fieldset.155-4.patch) is not generally accepted in our discussion.

I agree with @sun in #162.

I think those awkward dl.multiselect filter forms are very problematic and I would like to see their markup changed, as any site that styles definition lists is damned to fix the nonsense going on with them. I don't think we should fix this here, so created a new issue for this: #732914: Improve the markup/CSS for content and user filter forms. Whether or not it's too late to fix the markup problem remains to be seen, but it can certainly be fixed/improved there.

This small patch is my take on what sun is talking about in http://drupal.org/node/676800#comment-2674954

I looked through the comments above to see if I'm trying something that has been rejected as a bad idea before, but could find anything. The patch fixed the padding-bottom issue for me in FF 3.5 and I'm in the process of testing it with other browsers.

It looks like all browsers are showing the same issue:

The update button for the "Update Options" moves to the right of the select box in its fieldset. For the Firefox family of browsers this isn't a huge issue. But for Opera and IE 7 it looks broken.

So making this simple change does more harm than good. This result leads me to think that applying a specific alteration to #edit-filters may be necessary. Although, I'll continue to pursue a solution that is in the spirit of 3) in http://drupal.org/node/676800#comment-2674954

Implements 3)

Tested with Chrome 5.0.307.11 beta, Chrome 4.0.249.89, Firefox 3.5, Firefox 3.0.18, Opera 10.10, IE 7 (IE8 in compatibility view), IE8, and Safari 4.0.4

Everything looked fine except that in Safari the "bottom-padding" appeared to be smaller than in the other browsers. Let me see if I can upload an example of what i'm seeing.

* IE 8 is on the left, Safari 4 is on the right.
* Looks like the issue is the size of the select boxes in Safari.

I don't this issue is related to your patch and shouldn't hold this back.

This is an epic issue with some awesome collaboration. It's great to see so much care and energy put into front-end issues.

Agree with #170 that the little difference in how Safari renders this is negligable here.

Committed to HEAD. Thanks!

Heya themers!

I'd like to invite everyone from this issue to continue (technically the same) changes for resizable textareas in #735628: Resizable textarea behavior leads to unpredictable results :)

There's a working and really really beautiful patch over there already, so your reviews and help would be immediately needed!

btw: in general, it would be plain AWESOME, if all of us would have a closer look at the issues in the "markup" component queue:

http://drupal.org/project/issues/drupal?status=Open&component=markup

By doing so, we are able to ensure that D7 (and beyond) will output and use solid + sane markup and totally rock for themers. This issue really is the proof that we can.

Thanks for shedding a light on that issue sun (Oh man, I need less coffee). Anyway, I'll stop over there and see if I can help out.

Not sure if this should have been a new issue, but I think it is related. Let me know if you want me to re-post.

The committed code appears to fix the problems within the overlay, but is still broken during installation on ie7. The attached 2 screenshots show the problem.

Please also leave a link to the new issue here, thanks!

The follow up issue to fix the installation problem is #740136: css doesn't get included during install.

In rtl layout the legend aligned left and move out from the fieldset.
those lines needed to be add in style-rtl.css

fieldset .fieldset-legend {
        right:0;
}

@avinoam: Please create a new issue. This one was closed a long time ago.