This might be a small trivial thing but...

function theme_form($element) {
  // Anonymous div to satisfy XHTML compliance.
  $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : '';
  return '<form ' . $action . ' accept-charset="UTF-8" method="' . $element['#method'] . '" id="' . $element['#id'] . '"' . drupal_attributes($element['#attributes']) . ">\n<div>" . $element['#children'] . "\n</div></form>\n";
}

I cant see why were having a div inside the form - both are block elements (http://www.w3.org/TR/html4/sgml/dtd.html#block))
now i know a form is only allowed to have stuff in it thats a block element or a script:
http://www.w3.org/TR/html401/interact/forms.html#h-17.3:

BUT all form elements that drupal is putting into forms surrounded by an div so that makes this div unnecessary.
removing it would mean cleaner markup:

so from

....
....
function theme_form($element) {
  // Anonymous div to satisfy XHTML compliance.
  $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : '';
  return '<form ' . $action . ' accept-charset="UTF-8" method="' . $element['#method'] . '" id="' . $element['#id'] . '"' . drupal_attributes($element['#attributes']) . ">\n" . $element['#children'] . "\n</form>\n";
}
CommentFileSizeAuthor
#27 495480.patch2.08 KBrobloach
#24 formwrapperclass.patch1.1 KBcasey
#21 form-wrapper.patch816 bytescasey
#19 form-wrapper.patch822 bytescasey

Comments

mortendk’s picture

mortendk commented

and just to correct the code ;)

function theme_form($element) {
  $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : '';
  return '<form ' . $action . ' accept-charset="UTF-8" method="' . $element['#method'] . '" id="' . $element['#id'] . '"' . drupal_attributes($element['#attributes']) . ">\n" . $element['#children'] . "\n</form>\n";
}

so from:

<form>
  <div>
   .....
  </div>
</form>

to: 

<form>
   .....
</form>
Zarabadoo’s picture

Zarabadoo commented

I am in full support of this removal. Seems like it had good intentions at the time, but has just created mess in time.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Can we verify that validator.w3c.org still likes us if this change is made?

mortendk’s picture

mortendk commented

its http://validator.w3.org and yes have checked it but just to be sure ill set up a huge test for it :)

mortendk’s picture

mortendk commented

hmm looks like everything else than buttons are packed inside divs or other block elements.
one could argue that buttons should be themed as other form elements

but unless buttons are packed inside a block element it woudnt be a good idea to remove the div :/

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Closed (won't fix)

Look into CVS history to learn why these DIV containers are required.

senpai’s picture

senpai commented

Aroo? Just how far back does one have to look?

damien tournoud’s picture

damien tournoud commented
Status: Closed (won't fix) » Active

It has been like that since the first checkout of form.inc in Drupal CVS. As far as I understand, this is not necessary (anymore).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

For putting buttons in a block element, there's #482816: Make a consistent wrapper around submit buttons which I think will do what you mean?

dries’s picture

dries commented
Issue tags: +Favorite-of-Dries

Tagging!

mortendk’s picture

mortendk commented

it needs 2 things:

A wrapper around the buttons (submit, button, cancel, image ) + a wrapper around hidden fields.

it would be an huuuge improvement if we could collect the submit, delete etc buttons in a wrapper - cause aligning all the buttons to the right today is a pain in the neck, and involes all kinds of nasty float:right trix

The hidden fields are afaik always added at the bottom at the form they could get collected in their own little

adrinux’s picture

adrinux commented

I have a gut feeling that div is there for a reason, probably to deal with various IE layout bugs, for instance if you wanted margin/padding/borders on the form and to center it...I think there's also an IE6 bug where any styling applied to the form tag results in white space being added to the bottom of the form?
But, IE6 is sliding slowly into irrelevance and similar affects can probably be achieved with wrappers on elements.
So:
+1 to a wrapper around the buttons

But this change will need some serious testing in IE, methinks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I never had any issues to apply different, advanced form layouts to Drupal's output. And I did a lot in the past. But most often sticking to a two-column form layout (where label appears left and the field right next to it).

I fear that removing the wrapper-DIV adds a lot of unnecessary burden to the rendering of form elements - while making Drupal's output potentially XHTML-invalid or deferring penalty to themers that need to support IE properly.

Won't fix for me.

robloach’s picture

robloach
he/him
commented

That empty div has driven me crazy for years! We should at the very least give it a class so that we have some context to it...

seutje’s picture

seutje commented

subscribe

jacine’s picture

jacine
she/her
Location New York
commented

The hidden fields are afaik always added at the bottom at the form they could get collected in their own little .... ?

Even with #482816: Make a consistent wrapper around submit buttons, it seems that this will still be a problem... No?

I do think the if the <div> stays, it desperately needs a class.

casey’s picture

casey commented

form-wrapper?

cosmicdreams’s picture

cosmicdreams commented

+1 for form-wrapper.

casey’s picture

casey commented
Title: unnecessary divs inside the form theme_form() » Add class to wrapper div of form elements theme_form()
Status: Active » Needs review
StatusFileSize
new form-wrapper.patch822 bytes

Status: Needs review » Needs work

The last submitted patch, form-wrapper.patch, failed testing.

casey’s picture

casey commented
StatusFileSize
new form-wrapper.patch816 bytes

Uh?

casey’s picture

casey commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, form-wrapper.patch, failed testing.

casey’s picture

casey commented
Status: Needs work » Needs review
StatusFileSize
new formwrapperclass.patch1.1 KB

Was I sleeping!?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ includes/form.inc	28 Mar 2010 22:04:42 -0000
@@ -2855,9 +2855,12 @@
+    'class' => array('form-wrapper'),

Hrm. We now apply this very class to #type 'container', fieldsets, and with this patch, also to the ultimate parent DIV in a form. Effectively lost its meaning? (if there was one) At least unusable for JS/CSS. Not good.

133 critical left. Go review some!

casey’s picture

casey commented

form-wrapper? I thought we used fieldset-wrapper for fieldsets... but you're right.

What about fieldset-wrapper, form-container, form-wrapper?

robloach’s picture

robloach
he/him
commented
StatusFileSize
new 495480.patch2.08 KB

This uses theme('container') for the wrapper div, and theme('html_tag') for the form element. Yay for writing no manual HTML!... theme('container') gives us form-wrapper. Will we want to inject another one in there to be safe?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ includes/form.inc	28 Mar 2010 23:31:32 -0000
@@ -2855,9 +2855,36 @@ function theme_textfield($variables) {
+    $element['#attributes']['action'] = check_url($element['#action']);

Bad show-stopper for this: #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain()

Actually, I think I buried this change in at least 2 other patches in the queue already, but all of them are hold off by aforementioned issue.

+++ includes/form.inc	28 Mar 2010 23:31:32 -0000
@@ -2855,9 +2855,36 @@ function theme_textfield($variables) {
+  $children = theme('container', array(
+    'element' => array(
+      '#children' => $element['#children'],
+    ),
+  ));
+
+  // Construct the entire form tag.
+  return theme('html_tag', array(
+    'element' => array(
+      '#tag' => 'form',
+      '#attributes' => $element['#attributes'],
+      '#value' => $children,
+    ),
+  ));

Um. Yeah. :P

How much more abstraction do we need to write a simple string?

This is not alterable in any way (we immediately return a string), and just using drupal_attributes() would have the identical effect.

Count me -1 for this change, sorry.

Powered by Dreditor.

jacine’s picture

jacine
she/her
Location New York
commented

Why are we applying a "form-wrapper" class to fieldsets and containers? Sigh.

Maybe this should use .container instead of .form-wrapper: http://api.drupal.org/api/function/theme_container/7?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

See #667944: Javascript #states cannot hide fieldsets, radios, checkboxes

Also, it's too late to change the .form-wrapper class on #type 'container'

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs work » Closed (won't fix)

I really think this deserves a .form-wrapper over theme_container, which should clearly be .container or .form-container. I mean, really... It's called theme_container(), it's doc says "Returns HTML for a container for grouped form items" but then it's class is .form-wrapper? WTF.

But, if it's too late for that, anything we do here is going to be less than ideal, so there is no point in doing anything here IMO.

mortendk’s picture

mortendk commented

yeah this div is now finally removed : #2250381: Remove the inner div wrapper from forms