Fieldset CSS for node forms needs to be more specific

need some more tests, may broken other place

#1: bartik.patch queued for re-testing.

I noticed this problem, too. I applied the patch in #1. It adds vertical space where needed, but also where not needed. Instead of using the CSS selector fieldset .fieldset-wrapper, I suggest using fieldset legend + .fieldset-wrapper.

This is my first time submitting a patch. Please point me to any rules I have broken.

I have attached a patch and three screen shots. The screen shots show two blocks, one with a legend (title) and one without; unpatched, after applying the patch in #1, and after applying my patch.

@benjifisher,
how to create patch: http://drupal.org/patch/create (check out git part, more simple)

now we still supports IE6, so avoid use "+ selector".

attached a new patch for test.

@droplet,
Thanks for the advice. Maybe I will set up git tomorrow. For today, I will stick with patch and diff.

I like your idea: move the legend on top of the fieldset, then make the top margin of the fieldset big enough to hold the legend. BUT you moved the legend up too much. (At least, that is what I see: Firefox on Mac OS, looking at node/9/edit.) For legend I changed top from -30px to -2em (or -24px). (Since height is declared to be 2em, I think that makes sense.) I also removed margin-bottom and margin-top declarations from fieldset, since your margin declaration overrides them.

@benjifisher,
Thanks for the clean up. I forgot it, hehe. It's more nice now. reroll patch for drupal bots.

Tested with:
IE6 ~ IE9
FF 3.6
Chrome 10
Opera 11
** PASSED **

forgot upload it

I see why you originally used 30px for the offset, which looked bad to me. In Firefox (3.6 on Mac OS) the font-size is 12.25px; in Chrome (8.0 on Mac OS) it is 14px; and in Opera (didn't check the version, Windows 7) it is 18px. The legend has height 2em, so if you move it up or down, measure the amount in em, not px.

It might also be a good idea to use ems for the top and bottom margins.

Is there a Drupal coding standard for border radius? For fieldset, you used

  -khtml-border-radius:0 0 4px 4px;
  -moz-border-radius: 0 0 4px 4px;
  -webkit-border-radius: 0 0 4px 4px;
  border-radius: 0 0 4px 4px;

but further down, fieldset legend has

  -khtml-border-radius-topleft: 4px;
  -moz-border-radius-topleft: 4px;
  -webkit-border-top-left-radius: 4px;
  border-top-left-radius: 4px;
  -khtml-border-radius-topright: 4px;
  -moz-border-radius-topright: 4px;
  -webkit-border-top-right-radius: 4px;
  border-top-right-radius: 4px;

not sure but I think no :)

we can start a new issue to clean up it with shorthand, more clear.

I just marked #869950: Legends overlapping fieldsets as a duplicate of this issue. Comment #5 there proposes a different patch. I would test it out, but I need my beauty rest.

I am happy with the patch in #8. Am I supposed to change the status to "reviewed and tested"?

@benjifisher,
I was thought to do something like that comment #5 in that issue and I don't recommend. because sometimes, it may not following with "description".

yes. you can change to reviewed.

The patch in #8 works for me.

We usualy use the shorthand version BUT we can't use it for the webkit lines because older versions of safari don't support the short version. In a year or so those versions won't be used anymore but anyway we have to support it for a while...

@asplilicious,
can you share me which version, thanks.

and is there any chart tells which browser drupal is supported ?
here is the only chart Ive known: http://developer.yahoo.com/yui/articles/gbs/

we really need some more code standard rules for shorthand, supported broswer ..etc

Safarai <=4.
More info:
http://csswizardry.com/2010/03/a-quick-note-on-border-radius/

But when I go to the browser statistics. Safari 4 is almost dead...

AND from safari 5.0 border-radius (standard without the prefix) is supported. So actually we don't need the webkit prefix anymore...
More info:
https://developer.mozilla.org/en/css/-moz-border-radius

But for now we need to support safari 4.0 so we have to split the webkit rules.

@aspilicious,
Thanks for the correction. I made a new patch, using the long form for the webkit lines. While I was at it, I changed the top and bottom margins for fieldset from 40px and 20px to 3em and 1em. See the screen shot for Chrome 8 on Mac OS. Because of this change, I am bumping the status back to "needs review."

ok. it makes me rethink again.

1. can't only add padding to "description" because sometimes is following other elements
2. can't change top left/right border-radius to 0px, because it may have no LEGEND inside fieldset

+ top: -1.9em;
hide top left/right border-radius

+div.vertical-tabs .vertical-tabs-panes fieldset.vertical-tabs-pane .fieldset-wrapper {
+ margin-top: 0;
+}
I don't like this .class chain but lots of such pattern everywhere in Drupal, so ...... just following them.

That looks pretty good to me, but if you are setting top: -1.9em for the legend, then you should change the top margin of the fieldset to 2.9em instead of 3em:

  margin: 2.9em 0px 1em 1px; /* Leave room for the legend. */

That will make the spacing between the fieldset and the blocks above and below it more even.

I think a better solution would be to add just enough top margin (or maybe padding?) to the fieldset to contain the legend and then add 1em of top margin to the enclosing div. At least, that would make the page I am looking at (node/9/edit) more consistent. I have not thought about other pages that this CSS affects.

The patch in #18 no longer applies. It wants to replace the selector .node-form .fieldset-wrapper with .node-form .filter-wrapper .fieldset-wrapper. Unfortunately, that selector has already been replaced by .vertical-tabs .fieldset-wrapper.

I have prepared a new patch. It leaves alone the line I just mentioned, and it incorporates my suggestion from #19. Other than that, it is the same as the patch in #18.

Another problem, now that I have updated to the latest drupal 7-dev: when I add a file field to my content type, the widget no longer uses a fieldset. This is what I was using to test the patch. (It is also how I noticed the problem in the first place.) I am no longer so sure about my suggestion in #19.

#18: fieldset_6.patch queued for re-testing.

the patch still works.

change Number of values to more than 2 will uses a fieldset

@droplet,
Thanks. I started using a new test site, and I did not make all the same settings as the old one.

I think I am right about applying patches, and the system rejects the patch in #18, which I re-submitted. Did you remember to get the latest 7.x-dev before testing your patch?

The system also rejects my patch, but I think I fixed that by using git diff --no-prefix. Here we go again!

Do I have to bump the status from "needs work" to "needs review" in order to get the system to test my patch?

@benjifisher,
we make lots patch, haha. and I notice the testbots stop accepts GIT patch since few day ago.

following is also fix the "Text format" section, too much space between the editor textarea I think.

.node-form .filter-wrapper .fieldset-wrapper

@droplet,
Did you forget to attach a new patch?

Maybe it is time, as you said in #10, to finish work on this issue. If you want to make further adjustments to the spacing, that can be a new one.

Steps to reproduce bug:
1. turn off overlay and change admin theme to Bartik
2. add more help text to field and change "Number of values" to unlimited

Patch isn't only fix for create content page, every fieldsets are affect, please help test patch :)

Correct me if I am wrong: the only difference between the patches in #26 and #29 is an adjustment to the vertical spacing in the "text format" block on the node edit page.

I agree that the patch in #29 is an improvement over the patch in #26.

I think that either of the patches in #26 or #29 fixes the original problem.

I have tested in Chrome 8 and Firefox 3.6 on Mac OS.

Needs signoff from jensimmons or Jeff Burnz before RTBC.

Also, there is an odd mismatch of em's and px's throughout this patch.

+ margin: 2.9em 0px 1em 1px; /* Leave room for the legend. */
0, not 0px. And normally we leave out comments like this, especially since this would need RTL styling.
The discrepancy between 0px and 1px seems arbitrary, but if it's not, then style-rtl.css needs to have the opposite.

okay. while I reload dev. version today. seems the main error is fixed. I don't know if it still worth me spend time on it :)
but it can be an improves. I will leave here to jensimmons or Jeff Burnz.

1px is gone since X-dev. verison

@tim.plunkett,
The mix of em's and px's is not so odd. The em's all measure vertical distances (see below). I am not sure why you want to mess with the left and right margins, but the default values in my browsers seem to be 0 to 2px. There was already a comment like the one you point out; I just updated it.

I updated to the latest dev version, too (git pull) and I still see the problem. I have created a simple example at http://test.3mile.org/node/13 . Here is the HTML:

<p>Here is some text in a paragraph above the fieldset.</p>
<fieldset>
<legend>This is the legend.</legend>
<p>This is a paragraph inside the fieldset.</p>
</fieldset>

<fieldset>
<p>This is a paragraph inside the fieldset.  This fieldset has no legend.</p>
</fieldset>

(The text format is set to "Full HTML.")

I propose a simpler patch than the ones droplet suggested. Here is the original CSS (showing just the values that affect my patch)

fieldset {
  margin-top: 10px;
  margin-bottom: 32px;
  position: relative;
  top: 12px; /* Offsets the negative margin of legends */
}
fieldset legend {
  height: 2em;
  position: absolute;
  top: -12px;
}

My patch simply changes the top values to 2em and -2em, to match the height of the fieldset; I think this is the only reliable way to do it. Then, I change the top and bottom margins of the fieldset to 1em and 3em. This may be more white space than we really want, but you need at least 2em at the bottom since we shifted the fieldset down by that much. The font height is different in different browsers, so I think all of these measurements should be in em's.

I have attached screen shots (before and after, Firefox and Chrome, all done on a Mac) and a patch.

The only really ugly part is all the extra space when you have a fieldset without a legend. I cannot think of a way to avoid this.

subscribing

Can some please state exactly what the issue is, there is discussion on how to fix the problem but no description of the bug.

For example what form elements are we talking about here - I see reference in the OP to "help text" which I assume is #type => 'markup', placed above something - does it have any wrapping markup such as paragraph tags? This is important - please see the docs on FAPI.

If there is anywhere in core that shows this (even a contrib module please), so I can look at this. I have seen it but I can't recall where, I need to know precisely what is causing the issue before we start looking at solutions.

@Jeff,
Probably issue do not exists, it's pretty old topic (2month) and some change already make into Dev version. anyway, I hope you can review our patch and see if it has something better than exist codes. (you can think it is a css cleanup & improve more than a bug now.)

Other than showing where this might occur (core or contrib) can someone give me some tips on how to structure form elements within a fieldset to consistently reproduce the problem. Right now I can't comment about the fixes because I don't even know that the problem is to start with - in my mind this is actually a form markup issue (FAPI) but I want to see the problem first hand to confirm it.

I had a crack at trying to reproduce this myself, so this is what I found:

1. The OP screeshot shows a filefield with help text inside a fieldset - however core does not do this normally, please see bartik-filefield.png - you can see not only is this not in a fieldset but the help text is actually below the file field.

2. I made some custom fieldsets using FAPI, see the second screenshot, in the first fieldset I have some text using #markup and a fieldset description (which has a div wrapper by default) and in the second fieldset its the same markup text but no fieldset description, as you can see there is no issue, this was taken in FF4 however this looks basically identical in FF3 and Chrome 10.

I don't know how old Chrome 8 is but normally we only support the latest version of browsers that do not have a substantial market share, I don't have Chrome 8 so I can't test with it, in any case this is either 1) a real edge case or 2) I'm missing something fundamental here.

Currently this is marked as postponed, I'm gonna leave it as so for now.

Bartik Filefield.

Custom Fieldsets

I ran into this issue with the Field Group module, specifically in this issue: #1040494: Fieldset overlaps field titles

So its not limited to file fields.
When I'm editing a content type that I have added a field group to (with the 'fieldset' format selected), the title of the first field in the group is overlapped by the title of the field set. Increasing the padding-top of the class 'fieldset-wrapper' pushes the field title down to make it visible (not suggesting that that is a solution).
I am using Bartik on my content edit pages and I have overlay disabled, if that's relevant at all.
I am using the latest chrome. Also tested with FF3. Identical in both.

Attached is a screenshot.

Change....

style.css  (line 1139)
.node-form .fieldset-wrapper {
    margin-top: 0;
}

to...

.node-form .vertical-tabs .fieldset-wrapper {
  margin-top: 0;
}

Problem solved, can someone please roll a patch, thank-you Shadlington, you made me realize to look at the node form instead of just the front end.

Like this?

#41, yes just like that.

Just like that, without Windows line endings :)

Tested the patch, works like a charm :)

I am confused. Patches #29, #33, #44 get shorter every time. Is that OK? Or should I apply all three. Thanks.

I only applied #44

Unless otherwise specified you normally only test the latest patch.

This one looks good - I haven't checked if we have any other special casing for fieldset wrapper, if someone can check that and we don't I'd say this is good to go.

So, each time you do more with less code?

In this particular instance it seems so, yes.
I think the various patches were trying to fix this in different ways.
The final method was actually a very small code change.

Go ahead, apply the patch. It'll work.

OK, I'll be bold :-)
1 more question:
the patch says

-.vertical-tabs .fieldset-wrapper {
+.node-form .vertical-tabs .fieldset-wrapper {

But my code looks like:

-.node-form .fieldset-wrapper {
+.node-form .vertical-tabs .fieldset-wrapper {

This shouldn't be a problem, I suppose.

Ops good spotting johnv!

Needs to be like #40, so needs work.

I don't like what I see..
I was used to the Fieldset legend as in #4 and #38. Now, the title of the first field in the fieldset is shown, but the grey area has disappeared, leaving a pale impression on the user.

Huh. I didn't see that change at all.

Also, I didn't notice the patch problem as I performed it manually due to its small size, my bad :(

Sorry, just have some time to look back on this issue today. I'm the very first person in this thread but getting lost now.

seems comments after #37 is going to redo my & benjifisher works. if you read again #27 & #33, that already give the solution you are taking after #37. (AND THIS IS NOT EXIST IN -DEV VERSION)

NOW..
what I see is too much SPACE between node form & text format filter.

@Jeff Burnz,
are you the original author of this theme ??
Is it anywhere have some docs about this theme stardard.. see attach(space_problem.jpg). it has space issue

Thanks ALL

correction on my findings #53: The patch looks good. I did not remove the old line, but created a comment with '//'. Apparently this is not good css-style :-( .

Please find a patch attached as in #40, since #41/#44 does not coincide with my code, or with #40.

@droplet, #55: I am not sure what you mean: does latest patch not repair your problem, and #1 does?

I am bumping the status so that some of these patches will get tested.

Last I looked, Bartik is the default theme for Drupal 7. As such, it should be able to handle simple HTML, such as the example I gave in #33 above, along with before and after screenshots and a link to a live example. Or do we assume that no one will ever type into the body of a node with input format "full HTML"?

As far as I can tell, bartik/css/style.css has not changed since I wrote comment #33. The problem I described is still there, and my patch is still an improvement. The patch in #44 does not seem to make any difference.

#33: fieldset_10.patch queued for re-testing.

Moving to D8, the patch in 44 is the right fix, so adding it here again and bumping to D8.

#57 the issue you raised in #33 is not the issue, its a separate issue and frankly I think a bit of an edge case - if anyone is adding fieldset markup directly to a node then its rather trivial to add a fieldset wrapper with the right class also, otherwise we're gonna be jumping through CSS hoops here to support two entirely different markup structures which I think is really a waste of time considering almost no one will do this.

Looking good.

Just changing the title actually, so its more accurate for Dries to know what this about etc.

I don't really get why #33 is not the same as this issue, so I'll just stick to patch #44/#60.

Jeff, the correct patch should be #40 as discussed in #51/#52. patch #60 is again as in #44.
(my file id is: $Id: style.css,v 1.53 2011/01/04 06:23:29 dries Exp $)
The patch corrects the situation in a normal field set: the legend is not covering the first line of content anymore.
However, the legend still covers the 'show row weights' line in a filefield upload widget (see attachment).
If different occurrences of this symptom belong to different issues, the issue titel should be updated accordingly.
I know nothing of css, so i can only test&compare.

Correcting the issue stuff, this is the code from #40, which was formalized into a proper patch in #44.

If it doesn't work in the situation you describe then it needs work. At the very least please explain the steps to reproduce this issue - I dont even know how you are getting your file field into the vertical tab like that, so please explain how to reproduce the issue - include the browser version also.

and re-tag...

The wrong fieldset from #63 is created via a simple multivalue field of type Image, shown in Chrome. I used http://drupal.org/project/field_group to put the field in a vertical tab (since this seems to be the only working elements-like-module ), but this is not necessary to recreate the case.

Committed to 7.x and 8.x. Thanks.