Posted by mortendk on June 18, 2009 at 5:10pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | markup |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
| Issue tags: | d4d, Favorite-of-Dries, markupmarines |
Issue Summary
This might be a small trivial thing but...
<?php
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
....
....
<?php
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";
}
?>
Comments
#1
and just to correct the code ;)
<?phpfunction 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>
#2
I am in full support of this removal. Seems like it had good intentions at the time, but has just created mess in time.
#3
Can we verify that validator.w3c.org still likes us if this change is made?
#4
its http://validator.w3.org and yes have checked it but just to be sure ill set up a huge test for it :)
#5
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 :/
#6
Look into CVS history to learn why these DIV containers are required.
#7
Aroo? Just how far back does one have to look?
#8
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).
#9
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?
#10
Tagging!
#11
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 .... ?
Guess we still have to figure out what to do with buttons that is put inside a form but a simple would do the trick inside theme_button & theme_image_button ?
#12
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.
#13
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.
#14
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...
#15
subscribe
#16
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.#17
form-wrapper?
#18
+1 for form-wrapper.
#19
#20
The last submitted patch, form-wrapper.patch, failed testing.
#21
Uh?
#22
#23
The last submitted patch, form-wrapper.patch, failed testing.
#24
Was I sleeping!?
#25
+++ 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!
#26
form-wrapper? I thought we used fieldset-wrapper for fieldsets... but you're right.
What about fieldset-wrapper, form-container, form-wrapper?
#27
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?
#28
+++ 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.
#29
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?
#30
See #667944: Javascript #states cannot hide fieldsets, radios, checkboxes
Also, it's too late to change the .form-wrapper class on #type 'container'
#31
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.