_form_set_class() is currently used in 8 different theme functions, all for form elements. It's responsible for adding classes such as .form-text and .form-radio to input elements, and also adds .required and .error classes where necessary. We do not need this function.

  1. Having a type indicator class on both the wrapper and the input itself is redundant. When this code was written, theme_form_element() only provided a .form-item class. As of Drupal 7, .form-type-TYPE is also provided to the element wrapper.
  2. Since we are not supporting IE6 anymore, we can use attribute selectors such as input[type=text]. If that doesn't float your boat or you need to support IE6 or something, you can use .form-type-TYPE input. If it is determined that we need a common class for an HTML5 fallback (and I don't think we do), this function is still not needed. A common class can be added via template_preprocess_form_element().
  3. The .error and .required classes can easily be handled in theme_form_element() and having those classes at wrapper level as opposed to the element itself allow for better styling control overall, e.g. you can set a background color on the entire "row."
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jacine’s picture

Issue tags: +html5

Forgot the tag.

Jacine’s picture

Assigned: Unassigned » Jacine

Working on this now.

Jacine’s picture

Status: Active » Needs review
FileSize
29.07 KB

Ok, here's a patch for this. Here's what it does.

  1. Removes _form_set_class() function.
  2. As a result there are no more .form-text, .form-select classes on inputs (we don't need them as mentioned in the OP). One exception is .form-wrapper, which is apparently needed for collapsible fieldsets. It's added back via $element['#attributes'] in theme_fieldset().
  3. This also removes .required and .error classes at input-level and adds them back to the wrapper. I used .form-required and .form-error to be consistent with .form-disabled.
  4. Since .form-required was being used by theme_form_required_marker(), that class has been changed to .form-required-marker.
  5. Adjusts CSS and JavaScript everywhere.

It touched a LOT of code, and I tried my best not to miss anything, but it definitely needs some thorough testing and another set of eyes at least. Anyway, let's see what the testbot has to say.

Jacine’s picture

Here's another without the white space issue in theme_fieldset().

Status: Needs review » Needs work

The last submitted patch, _form_set_class.1309392.04.patch, failed testing.

Jacine’s picture

  1. Fixes the Undefined index: #attributes issue which caused the drupal_render() test to fail. $element['#attributes']['type'] = 'select'; was needed in theme_select() (the rest of the theme functions have the same code).
  2. Fixes a stupid CSS syntax error that I introduced accidentally. Oops.

Now, I need help with the test assumptions in two areas. They're currently failing because they're looking for error classes on the element, and the test needs to happen on the wrapper. I don't see any similar tests (where the wrapper is targeted), and I'm not sure how to do it.

In locale.test - testStringValidation():

      // Need help fixing this! The class is no longer on the element, it's on the wrapper.
      $form_class = $this->xpath('//form[@id="locale-translate-edit-form"]//textarea/@class');
      $this->assertNotIdentical(FALSE, strpos($form_class[0], 'error'), t('The string was rejected as unsafe.'));

In field.test testNestedFieldForm():

    // HELP! Needs to check for .form-error class on the wrapper of this element.
    $error_field = $this->xpath('//input[@id=:id and contains(@class, "error")]', array(':id' => 'edit-field-unlimited-und-1-value'));
    $this->assertTrue($error_field, t('Entity 1: the error was flagged on the correct element.'));

    // HELP! Needs to check for .form-error class on the wrapper of this element.
    $error_field = $this->xpath('//input[@id=:id and contains(@class, "error")]', array(':id' => 'edit-entity-2-field-unlimited-und-1-value'));
    $this->assertTrue($error_field, t('Entity 2: the error was flagged on the correct element.'));

This will fail again until those 3 things are fixed, so marking needs work for now.

RobLoach’s picture

Status: Needs work » Needs review

Subscribe! Errr, I mean "Follow!"..... But in all honesty, I wanted to set to needs review to see what the testing bot thought. Well done Jacine! I like this one a lot.

Jacine’s picture

Thanks Rob :D Trying to work out those xpath issues now!

Status: Needs review » Needs work

The last submitted patch, _form_set_class.1309392.06.patch, failed testing.

Jacine’s picture

Assigned: Jacine » Unassigned

Meh... I can't figure this xpath crap out. So close, yet so far... :(

Jacine’s picture

Assigned: Unassigned » Jacine
Status: Needs work » Needs review
FileSize
32.3 KB

Holy crap that was hard. Thanks to AquaPath, I think (well, I hope) I got it. Fingers crossed.

sun’s picture

Didn't review in depth, but quickly skimming over this, looks good.

However, not sure about moving the .form-error class from the input element to its wrapper. I'm not sure, but I think that, technically, multiple elements may be wrapped.

Jacine’s picture

Thanks @sun!

I actually like the .form-error class on the wrapper better because it allows for more creative styling of fields with errors. Not aware of a multiple elements issue though. That would suck. Seems like something that should have a separate implementation, such as theme_multiple_form_elements() or something. IF you can think of any instance where I could test that, please let me know. :)

sun’s picture

#type password_confirm might be a candidate.

Otherwise, a structure like this should technically be possible, too:

  $form['street'] = array(
    '#type' => 'textfield',
    '#title' => 'Street name',
  );
  $form['street']['number'] = array(
    '#type' => 'textfield',
    '#title' => 'Number',
    '#title_display' => 'attribute',
  );

and should theoretically lead to something like this:

<div class="effin-wrapper-one">
  <label>Street name</label>
  <input type="text" name="street" />
  <div class="effin-wrapper-two">
    <input type="text" name="number" title="Number" />
  </div>
</div>
Jacine’s picture

Thank you! I'll do some testing with that ASAP.

xjm’s picture

Bit of a tricky reroll, but here it is.

Edit: Actually, I may have screwed it up anyway. Well, testbot will tell. :)

Status: Needs review » Needs work

The last submitted patch, 1309392-POST-APOCALYPSE.patch, failed testing.

xjm’s picture

Well, that only took a week.

xjm’s picture

Status: Needs work » Needs review
FileSize
11.25 KB
32.83 KB

Alright, here we go.

I've also attached a manual diff of the two patches to show that only filenames, line numbers, and unpatched lines should differ.

Status: Needs review » Needs work

The last submitted patch, 1309392-19.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Testbot hiccup.

Niklas Fiekas’s picture

Status: Needs work » Needs review
Issue tags: -html5

Nice clean-up. Thank you!

It's been a while. #19: 1309392-19.patch queued for re-testing. Also more form elements have been added in the mean-time.

Maybe still applicable: [...]
(Maybe this is the right moment to sneak in toogleClass(class, switch). If not, just ignore this.) [...] (And maybe also toggle(showOrHide).)

Edit: Apperently not.

Status: Needs review » Needs work

The last submitted patch, 1309392-19.patch, failed testing.

Niklas Fiekas’s picture

Issue tags: +html5
FileSize
35.8 KB

Rerolled (also adding the new HTML5 input types). Pretty messy rebase - not sure how to produce a useful interdiff.

One problem: #1174938: Natively support the HTML5 required and aria-required FAPI properties sets the required attribute in _form_set_class() (WTF?). Caught that while running the tests locally (so expecting a few fails) - how do we do this?

Status: Needs review » Needs work

The last submitted patch, 1309392-form-set-class-24.patch, failed testing.

Niklas Fiekas’s picture

Oh ... more test failures than expected.

sun’s picture

+++ b/core/includes/form.inc
@@ -4362,6 +4353,15 @@ function theme_form_element($variables) {
+  // Add a class indicating that the form element is required.
+  if (!empty($element['#required'])) {
+    $attributes['class'][] = 'form-required';
+  }
+  // Add a class indicating that the form element contains an error.
+  if (isset($element['#parents']) && form_get_error($element)) {
+    $attributes['class'][] = 'form-error';
+  }

@@ -4479,38 +4479,6 @@ function theme_form_element_label($variables) {
-  // This function is invoked from form element theme functions, but the
-  // rendered form element may not necessarily have been processed by
-  // form_builder().
-  if (!empty($element['#required'])) {
-    $element['#attributes']['class'][] = 'required';
-    // @todo Rename the _form_set_class() function to reflect that we're setting
-    //   non-class attributes too.
-    $element['#attributes']['required'] = 'required';
-    $element['#attributes']['aria-required'] = 'true';
-  }
-  if (isset($element['#parents']) && form_get_error($element)) {
-    $element['#attributes']['class'][] = 'error';
-  }

1) Let's retain the original comment about elements potentially not having gone through form_builder(). (which apparently targets the #parents check for the error class, not the required class code, so should be moved down.)

2) The 'required' and 'aria-required' HTML attributes need to be set on the INPUT element instead of the wrapping DIV...

This gets a little bit tricky, because those two attributes should ideally be set, even if the form element is not themed/rendered via theme_form_element() (e.g., skipping the wrapper, or alternative implementation).

Thus, the two attributes need to be applied whenever a form element that takes user input is built. All of these form elements use the internal Form API property #input => TRUE, which enables the user input and #value processing. The responsible code is located in _form_builder_handle_input_element(), and as you will see, we're already handling the 'disabled' and 'readonly' attributes there. I'd suggest to put the conditional addition of the 'required' and 'aria-required' attributes right below that code.

+++ b/core/modules/system/system.theme.css
@@ -111,15 +111,16 @@ input.form-radio {
-.form-item input.error,
-.form-item textarea.error,
-.form-item select.error {
+.form-error input[type=text],
+.form-error input[type=password],
+.form-error textarea,
+.form-error select {
   border: 2px solid red;
 }

1) As mentioned in #12 and #14, form elements having an error can have child elements within them. These CSS selectors will add error borders to any possible children, too. One solution might be to only target the direct child, i.e.,

.form-error > input

However, with that, this default styling has a hard dependency on

i) the wrapping container having the .form-error class.

ii) no extra markup or wrapper between the wrapping container and the input element.

That's a lot of constraints for this expected default styling. I wonder whether it wouldn't make sense to additionally retain the .error class on the INPUT element itself, so we can avoid these limitations?

2) The previous selectors targeted any kind of INPUT elements, which automatically includes the new html5 INPUT element types. I'm not sure whether this change is intentional.

Xano’s picture

Status: Needs work » Closed (won't fix)

The function seems to have been removed already.