label tag has a forced break

You can change that through CSS:
br { display: none};

I don't know that that is the best solution. Shouldn't the label and the value each be in a div or will that somehow break the label? Then the theme can have more control over each of the items and display inline if desired or block if desired. We try to use as little hard-coded layout html in core as possible. I too have noticed these breaks and think that they should go.

My CSS was of course only the most basic version. It would make all br tags non displayable. So you should prefix it with the right CSS class or ID.

I think my fix is a valid one. If we'd remove the brs we could not add them through CSS again.

If we removed this break and wrapped the items in divs, the appearance would be the same unless the theme decided to make the divs display inline...

It is very bad behaviour to fix your HTML with CSS. CSS should be used *on top* of HTML, not to fix HTML.

If this label cannot be themed we have a bug, since *all* outputted HTML should have passed trough a theme function. All? Yes all! (IMHO ;) )

At the very least these elements should be themeable as Bèr suggests. In my opinion it would be easier to style forms if the BRs were removed. Applying "label {display:block;}" to the theme stylesheet will serve the same purpose as the BR elements. I think that trying to remove BR with CSS is hackish and would prove to be problematic as the html structure can change quite a bit in different modules.

I had an issue with so I wrote a theme specific version of the function like (wnpj is the name of the theme)

function wnpj_form_element($title, $value, $description = NULL, $id = NULL, $required = FALSE, $error = FALSE) {

  $output  = "<div class=\"form-item\">\n";
  $required = $required ? theme('mark') : '';

  if ($title) {
    if ($id) {
      $output .= " <label for=\"$id\">$title:</label>$required\n";
    }
    else {
      $output .= " <label>$title:</label>$required\n";
    }
  }

  $output .= "<div class='content'>$value</div>\n";

  if ($description) {
    $output .= " <div class=\"description\">$description</div>\n";
  }

  $output .= "</div>\n";

  return $output;
}

This works fine for node output but not so well for form input. So with css I can force a break for forms while having no break for node ouput. It would be nice if the theme function was passed the form element type so this could be included as part of the class information. This way text fields could be single line, while elements that are naturally multiline (a list of options) could use a line break (it looks odd other wise)

You can even use this with xtemplates with a small trick. Make a module (I used wnpj_theme.module) with the above code in it then enable the module under the administation -> modules menu. Make sure you change wnpj to the name of the theme you are using.

tangent's February 16 summarizes the situation well, I think. Using CSS to fix an HTML problem is backwards, and it is possible to cleanly theme for inline or block labels (which is what the br element produces) with maintstream CSS.

I propose to fix this by removing the br elements and adding a rule in drupal.css to make label elements display: block, and add a comment for theme developers which says "to display labels inline, change to display: inline" or something to that effect.

Or it could be documented in the handbook under theme development someplace.

Hi,

I've been hitting a wall with this issue as well. Besides the <br /> tag, I'm also very unhappy over the hard-coding of <div class="form-item"> (and similar DIVs). There really is no way to customise this besides overriding the theme function [theme_form_element]. IMHO this should atleast be overridable by using #prefix/#suffix in the Forms API.

Overriding via the theme is not efficient esp. if you have many themes to deal with. I'm unaware of any method by which this can be overridden in the module (for that module alone). If there are any alternatives, please let me know :)

I'm reclassifying this as a bug and updating this to 4.7.0-beta2.

Thanks!
-K

As far as I know, modifying the html output is correctly done by modifying the theme function. If you need to support more than one theme then perhaps using CSS in drupal.css is the easier solution?

I think some commenters here are trying too hard to customize their themes in php when a tiny bit of CSS would probably solve their problem. Feel free to contact me offline if you need advice with that as this bug isn't the place for that.

This isn't really a question of whether I can fix the issue or not with a workaround:

a) I don't want to override the theme function as I'll have to do this for every theme every time every where.
b) I don't want to muck around with drupal.css for the very same reason.
c) I would prefer my module(s) to be be self-contained. Else they won't really be "modules". It shouldn't be "install module" and then do this with this core file and then do that with all your themes, and let's hope that this doesn't break your entire site.. And vice versa during an uninstall..
d) The Form API + Theme system shouldn't compel me to stick everything in a block element and then resort to floats. This essentially leads to a bunch of useless tags and needless complication of basic mark-up.
e) IMHO, there shouldn't be a <br /> tag anywhere in Drupal except in content. It just ain't right.

Everybody can work around this, but imo it will be better to just do this right :)

Thanks for listening :)
-K

I agree 100%. You should *not* use <br /> if you don't need to, very few cases you need to. This patch fixes this problem and implements the *correct* way to achieve the same results without this markup hack.

This patch is small. It makes drupal render better HTML. It makes drupal a bit better themeable. It fixes a small "bug" of hardcoded HTML. So all in all 3x +1.

The more I think about it, the more I love this patch. Incredibly simple, yet so powerful to themers who want labels on the same line as texfields etc, easy to override in CSS: label {display:inline} the way it *should* be done. Mmm.

At stake here is not just what the default style should be but also how the page should look in the absence of style. With the BR it seems that the intention was to make labels display as block level elements in the absence of style. If we wish to retain this layout then DIV elements should be wrapped around either the label or the form field.

Otherwise, this is exactly how I would have handled it. I have no preference what the default appearance should be because the theme will override it if necessary. +1 from me.

@Zen:
You made no previous mention of having a problem with a module regarding HTML markup. If you have a module related problem (and not a theme one) I suggest you give more info about the problem you're having. Modifying your theme(s) to make your site look the way you want is not a workaround. Drupal cannot serve everyone's needs by default.

I have myself also problems for tailoring labels and form-items and I would like also a fix for that problem.
I don't understand why there are <br /> and DIV (rather than SPAN).
Style should be left to CSS.
I like Zen's idea when he says:

IMHO this should at least be overridable by using #prefix/#suffix in the Forms API.

This could help for example to suprress the colon after the label when needed.
It would be good also to add a second class giving the type of form-item beside the class="form-item" , for example: class="textarea" or class="radio" or class="select".
giving something like: <SPAN class="form-item textarea">.

This will allows to style differently the different form-item when needed.

@Montuelle create another issue, what you want to do is outside the scope of this patch. This patch and original issue was to merely take out the extraneous
with labels and the above patch fixes that.

The issue you bring up is more involved and more widespread around core (not just with forms) and we need a better and more comprehensive solution for that. So start a new thread and we'll work on a more comprehensive plan & patch to do that (talk to berkes, he is a big advocate of this and has worked on it much more than I have).

Setting this back to commit.

Nice patch. Feel free to clean up more HTML or to improve the themability of Drupal. It can still make its way to Drupal 4.7. Committed to HEAD.