CSS classes broken on all form elements

is this problem specific to select elements

I suspect we need a better way to set classes in the first place, rather than adding them to '#attributes'; _form_get_class() should return all classes. Should we introduce a '#class' selector for form elements, which would be an array? This would then be passed to and used by _form_get_class().

I like this latest attempt. Looking at form.inc (currently) it seems each different theme_element is doing it its own unique way!
Adding this form_get_class() function should help consistancy.

I too was getting both blank class="" and then class="mine" inserted in my select, I was trying to set it using the old #attributes['class']='mine' way.
I'd like to see a #class tag, but don't mind it staying in the #attributes array.

The patch seems to work for what I wanted, but because I didn't like seeing that space in class=" myclass" I rewrote it slightly into

function _form_get_attributes($element, $class = '') {
  $classes = array();
  if ($class)                  { $classes[] = $class; }
  if ($element['#required'])   { $classes[] = 'required'; }
  if (form_get_error($element)){ $classes[] = 'error'; }
  if ($element['#class'])      { $classes[] = $element['#class']; }
  if (isset($element['#attributes']['class'])) {
    $classes[] = $element['#attributes']['class'];
  }

  if ($classes){
    $element['#attributes']['class'] = implode(' ',$classes);
  }
  return drupal_attributes($element['#attributes']);
}  

... just an idea. Either way I think this issue is reasonably important. Classes need to be encouraged.

Nice work Henrique.

Your patch addresses the basic issue that's causing duplication--that we shouldn't be mixing two different ways of handling form class attributes--(a) manually coded and (b) as part of drupal_attributes(). Your approach--remove the manual coding, leave the classes to drupal_attributes - makes good sense.

Because the special handling is limited to classes, it might be better to have a class-specific function, rather than a special handling of all attributes. So I've attached a version with some small changes.

Instead of eliminating _form_get_class(), we change it to a setter, _form_set_class(), that's called before we generate the HTML form element. This way we add the needed class names to any existing ones in $element['#attributes']['class'].

The patch:
* passes the $element array by reference to _form_set_class(), so that $element['#attributes']['class'] can be updated.
* uses an array as per dman's suggestion to avoid unneeded spaces.

I've also added documentation of _form_set_class().

In sum:

We currently have an error of duplicate (invalid) class attributes being added to form elements. This patch solves the issue by removing the manual generation of class attributes from the form element theme functions and instead adding needed class names to the $element['#attributes']['class'] value.

This is a priority issue because, in addition to CSS display concerns, javascript behaviours depend on the ability to add classes to elements.

Wrong patch, that was my previous take ;)

Here's the right one.

Changing the title to make it clearer that this is a major problem. We need to fix this before a full release.

marking critical to not forget it.

When we set the class for a select element using '#attributes' the tag is rendered with two class attributes, becoming this attribute unusable for javascript.

The underlined statement is not completely true. The standards allow to use more than one class for any given element. It is the javascript snippet reponsability to know how to deal with more than one class.

An example:
http://www.alistapart.com/comments/scripttriggers?page=13#122

sorry, I didn't check the issue further. lol

maybe you were meaning something like class="foo" class="bar". If so, oops.

However, this is interesting. Does that happen with all browsers? Did the (X)HTML validator return anything? ...just curious.

lol ...to answer my own question, did a test:

<div id="test" class="foo" class="bar">
This is an element with 2 class attributes. Oops!
</div>

Only the first class attribute is detected. ie. the definitions assigned to the second class are not processed. class="bar" is ignored by the browser parser (tested with FF 1.5 and MSIE 6). alert(document.getElementById('test').className) showed 'foo'.

Oddly enough, the validator extension for FF reported:
Warning: <div> dropping value "foo" for repeated attribute "class"

applied.

what would happen if people forget to call the _form_get_class function.

wouldn't it make more sense to call it once, in form_builder or something instead of for every element ?

scratch that.

this is something that can't be done for all elements.

One more question on this: when I try to call this function from form_alter I get an error in form_get_error becuase the form hasn't been built yet and has no #parents. How is it possible to add class attributes to a form in hook_form_alter?

Robert, form_set_error is called in validate and form_alter predates that. So... what?

so how do I set a class on a form in form_alter?

So if I understand correctly, the way to add classes to a form through hook_form_alter is like this:

$element['#attributes']['class'] .= ' '. $classname;

Is this the best way?

@robertDouglas: yes.

Attached is maybe a slightly improved and cleaner approach to my previous (applied) patch.

Refinements:
* set classes on all elements before theming them, as per Adrian's suggestion.
* set the initial class names in the _elements hook, rather than waiting and adding them in the theme.

I still wonder if it wouldn't be better to introduce a '#class' = array() selector, rather than doing this as part of '#attributes'. The class names would be more cleanly handled as an array.

Anyway, downgrading this as the original issue is solved. We can safely wait until after 4.7 for any further changes.

Nope, bad approach in that patch. We can't set the '#attributes' array in the _elements hook because values there will (I assume) be overwritten when we create element instances, e.g.,

$form['title'] = array(
  ...
  '#attributes' => array('class' => 'whatever'),
  ...
);

what is the status here? Can we mark it fixed?

Yes, the original issue is fixed. I think the solution could use some refining, but we can open a new issue.