Add form name class to forms for styling purposes

sub.

We discussed this several times before and now it's time for doing it. I'm willing to TEST this when there is a patch ready :).

Not sure what this has to do with html5 nor an actual bug. :/

@davereid this is needed for the html5 css cleanup. So it's related to the html5 initiative.

Well there wasn't any explanation at all how it relates to HTML5...

Possibly / somewhat related #1244338: Add generic #class property for form elements

I think we can do this in form_builder(), but there might be somewhere better, I'm not sure if form_preprocess_form() will work, it doesn't currently exist.

In form_builder we can add:

if (isset($element['#type']) && $element['#type'] == 'form) {
  $element['#attributes']['#classes'][] = $form_id;
}

I haven't tested this, but I believe it will omit numbering in the class name if the same form is rendered twice on a page.

This is incorrect *i think* because forms with numbered id's: #form-2 will not get the .form class.

@aspilicious

I think the unique (multiple forms) id is stored in $element['#id'] even when #type = form. Leaving $form_id untouched.

In drupal_prepare_form():

  if (!isset($form['#id'])) {
    $form['#id'] = 
drupal_html_id
($form_id);
  }

Removed code mentioned in #12 , which causes duplicate classes on two forms. I have no idea what is going on with search-block-form (it is garbage), so may try again later if noone has fixed.

this possible brakes some themes if they used the added classes.
We should check for those.

In search.module"

1. the search block content is from drupal_get_form('search_block_form');
2. This is preprocessed in template_preprocess_search_block_form() where all form children are rendered into $variables['search_form']
3. in search-block-form-tpl.php $search_form is printed.

Nowhere is the $form parent rendered / printed, yet in the generated markup <form ...> ... </form> is wrapped around the markup generated from search-block-form.tpl.php.

So, where is <form ...> being rendered and printed?
Edit:

I believe that <form ...> is being added because the search-block-form render element is 'form'.

  return array(
    'search_block_form' => array(
      'render element' => 'form',
      'template' => 'search-block-form',
    ),

@aspilicious

The only classes removed were two that were manually addd to node form and search form, but that will now be added automatically by form_builder(), so no change to markup.

See #1285364: Remove search-block-form template for form rendering consistency

Updating failing test in #13. search_block_form will not get a class until #1285364: Remove search-block-form template for form rendering consistency is fixed.

Re: changing class names, it is true that the classes on node/add/x will change from 'node-[machine-name]-form' to '[machine-name]-node-form'. E.g. from 'node-page-form' to 'page-node-form'. I searched and nothing in Core is referring to the old class names.

Tagging for the current sprint

Same as #19, but properly using drupal_html_class() to generate class name.

Now that #1285364: Remove search-block-form template for form rendering consistency is committed the patch in #21 will add the form name as the class on every form in Core.

The $form_id of node forms is set in node_forms(). It is set as $type . 'node_form'.

We could:

1. Change the pattern for node form ids, and refactor the code that touches them.

2. Special case the node forms in form_builder() where the patch adds the class.

If we do 2 then can someone tell me what the most reliable way is to determin if an element passed into form_builder() is a node form?

Regarding #23, what about simply adding a 'form-' prefix?

(Note that not all form constructor function names end in _form; that's a "best practice", but not enforced anywhere; it's perfectly valid to have form constructor function name of foo_bar(), which would turn into .foo-bar with the current patch, and .form-foo-bar with the prefix proposal.)

That said: We are, inevitably, going to run into class name clashes, no matter how hard we try.

Even the "form-" prefix is used by various other form-related things throughout core. Due to that, the only safe thing to do when actually using these classes in CSS is form.foo-bar, regardless of "form-" class prefix or not.

And to directly account for the already expected CSS selector specificity shitstorm: No, there are commonly only a few forms on a page, so the performance difference between form.foo-bar and .foo-bar is negligible.

+++ b/includes/form.inc
@@ -1711,6 +1711,8 @@ function form_builder($form_id, &$element, &$form_state) {
+  $element['#attributes']['class'][] = drupal_html_class($form_id);

This should go into drupal_prepare_form() or drupal_retrieve_form(), so it can be changed by hook_form_alter() implementations.

Re-rolled patch in #21, incorporating input from #25, moving the addition of the class from form_builder() to drupal_prepare_form().

+++ b/includes/form.inc
@@ -944,6 +944,7 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
+  $form['#attributes']['class'][] = drupal_html_class($form_id);

+++ b/modules/node/node.pages.inc
@@ -111,9 +111,6 @@ function node_form($form, &$form_state, $node) {
-  if (!empty($node->type)) {
-    $form['#attributes']['class'][] = 'node-' . $node->type . '-form';
-  }

Alright, thanks.

We need to discuss the expected and behavior a bit more.

With the class being added in drupal_prepare_form(), the form constructor is not able to "prevent" it, because drupal_prepare_form() is invoked afterwards.

If we want form constructors to override the default suggestion - for which node_form() could be an example (but not necessarily) - then we'd have to pre-set the class in drupal_retrieve_form(), right before the initial $form array is created and passed to the constructor. That way, the constructor (and only the constructor) is able to completely replace $form['#attributes']['class'] with a new/custom definition, throwing away the default class name.

I think that would be the desired behavior/workflow.

If we are going to do it this way, then the line adding the class in drupal_retrieve_form() will need an inline comment explaining why it is done there and not in drupal_prepare_form() (i.e., basically what I outlined above, only shorter ;))

1 days to next Drupal core point release.

So, the question is if we want to supplement the class(es) provided by the form constructor with drupal_html_class($form_id), or if we want to have drupal_html_class($form_id) be the default, which can be completely overridden by the form constructor.

Someone else is going to need to roll against drupal_retrieve_form. I took a look and I don't see where to add the class. It appears like an array merge is required (which makes me wonder why we don't just do that in drupal_prepare_form(). It also seems like adding a class is more of a 'prepare' behavior than a 'retrieve' behavior.

Attached patch implements the default CSS class name in drupal_retrieve_form(), and additionally adds the base form ID as class name, too.

I like.

Is there a reason why we aren't using drupal_html_id() here? I'm guessing there is, but I'm curious.

As the name implies, drupal_html_class() is for CSS classes, drupal_html_id() is for HTML IDs. The former ensures minimal requirements for the resulting string only, the latter combines the HTML and CSS specs to ensure a universally valid and unique HTML identifier.

I think this class is useful and should be kept.

The base form ID is "node_form", and this patch automatically takes the base form ID of all forms into account (if any) and adds them as second default CSS class.

I.e., all node forms still have the 'node-form' class. And not only node forms, but also comment forms and other $form_ids that are re-using the same base form constructor automatically get the respective class name of the base form ID. (This is actually the point where this patch really starts to make sense ;))

See http://api.drupal.org/api/search/8/forms for implementations of hook_forms() -- namely, comment_forms(), node_forms(), and search_forms().

+++ b/modules/node/node.pages.inc
@@ -110,10 +110,9 @@ function node_form($form, &$form_state, $node) {
+  // Override the default CSS class name, since the user-defined node type name
+  // in 'TYPE-node-form' potentially clashes with third-party class names.
+  $form['#attributes']['class'][0] = drupal_html_class('node-' . $node->type . '-form');

Note that this special case is subject to change for Drupal 8 already:

#798062: Rename TYPE_node_form to node_form_TYPE

The current code is a namespace violation on its own. To be discussed and changed over there.

Shouldn't hold up this patch.

Is testing all that is left with this patch?

"search_box" is completely outdated and old code either way. Dates back to earlier core versions, in which themes contained a $search_box variable, which in turn wasn't a block, but a box.tpl.php (also gone), etc.

Totally whack form structure: http://api.drupal.org/api/drupal/modules--search--search.module/function...

Today, we only have the form in the block (search_block_form) and on the page (search_form).

Due to search_forms(), the form ID of the block is search_block_form, but it calls into search_box instead of search_form. Hence, the form in the block gets the .search-block-form and .search-box classes.

Actually, I think that figuring out appropriate classes for theming and rewriting those inane search forms is a separate issue. This patch does not change the CSS classes being output on search forms, aside from automatically adding the form IDs as classes (which applies to all forms, not limited to search forms). If we want to review and adjust the automatically added classes for all forms throughout core, then we will still review this patch in a year from now.

I reviewed this and it's fine but i would love to see a followup issue opened to change foo_node_form to node_form_foo finally. It's totally against the rules.

@chx: already mentioned 6 comments before ;) #798062: Rename TYPE_node_form to node_form_TYPE

Looks good to me, committed to 8.x.