Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
_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.
- 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. - 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(). - The
.error
and.required
classes can easily be handled intheme_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."
Comment | File | Size | Author |
---|---|---|---|
#24 | 1309392-form-set-class-24.patch | 35.8 KB | Niklas Fiekas |
#19 | 1309392-19.patch | 32.83 KB | xjm |
#19 | hunks.txt | 11.25 KB | xjm |
#16 | 1309392-POST-APOCALYPSE.patch | 36.19 KB | xjm |
#11 | _form_set_class.1309392.11.patch | 32.3 KB | Jacine |
Comments
Comment #1
JacineForgot the tag.
Comment #2
JacineWorking on this now.
Comment #3
JacineOk, here's a patch for this. Here's what it does.
_form_set_class()
function..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']
intheme_fieldset()
..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
..form-required
was being used bytheme_form_required_marker()
, that class has been changed to.form-required-marker
.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.
Comment #4
JacineHere's another without the white space issue in theme_fieldset().
Comment #6
Jacinedrupal_render()
test to fail.$element['#attributes']['type'] = 'select';
was needed intheme_select()
(the rest of the theme functions have the same code).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():
In field.test testNestedFieldForm():
This will fail again until those 3 things are fixed, so marking needs work for now.
Comment #7
RobLoachSubscribe! 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.
Comment #8
JacineThanks Rob :D Trying to work out those xpath issues now!
Comment #10
JacineMeh... I can't figure this xpath crap out. So close, yet so far... :(
Comment #11
JacineHoly crap that was hard. Thanks to AquaPath, I think (well, I hope) I got it. Fingers crossed.
Comment #12
sunDidn'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.
Comment #13
JacineThanks @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. :)
Comment #14
sun#type password_confirm might be a candidate.
Otherwise, a structure like this should technically be possible, too:
and should theoretically lead to something like this:
Comment #15
JacineThank you! I'll do some testing with that ASAP.
Comment #16
xjmBit of a tricky reroll, but here it is.
Edit: Actually, I may have screwed it up anyway. Well, testbot will tell. :)
Comment #18
xjmWell, that only took a week.
Comment #19
xjmAlright, 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.
Comment #21
xjmTestbot hiccup.
Comment #22
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNice 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.
Comment #24
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRerolled (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?
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh ... more test failures than expected.
Comment #27
sun1) 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.
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.,
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.
Comment #28
XanoThe function seems to have been removed already.