Problem/Motivation

The issue is that Drupal core in combination with Bootstrap. Drupal is adding form-* classes to input fields directly.
In this case this has a side-effect. In Textfield::preRenderTextfield(), the class form-text is added to the input field, while bootstrap expects that class to be used on help text for the input field: https://getbootstrap.com/docs/4.3/components/forms/#help-text

So we have this: <input xyz class="form-text form-control">
Instead of:

<input xyz class="form-control">
<small xyz class="form-text text-muted">
 Help text
</small>

The issue is that bootstrap itself adds a small margin-top, which makes the labels in the form misaligned. This is mostly visible in the case we have a select and an input form next to each other.

.form-text {
  display: block;
  margin-top: $form-text-margin-top;
}

See the screenshot attached.

Proposed resolution

Since we can't just remove that class from Drupal core, I suggest we reset the input.form-text explicitly in bs_bootstrap forms.scss.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sasanikolic created an issue. See original summary.

sasanikolic’s picture

Issue summary: View changes
sasanikolic’s picture

Status: Active » Needs review
FileSize
1.01 KB

Here is the patch with the proposed solution.

pivica’s picture

Status: Needs review » Needs work

Hmm, this is quite unfortunate, that we have this collision with the core :(

I would suggest we first check should we remove .form-text from the input field, because it place is in the description and it is already there.

I did a quick search in core and the only place where form-text CSS class is used in code is in web/core/modules/color/color.js. @sasanikolic what do you think we actually remove this class from input field?

Also found this in node.css

/* @todo File an issue to add a standard class to all text-like inputs */
  .layout-region-node-secondary .form-autocomplete,
  .layout-region-node-secondary .form-text,
  .layout-region-node-secondary .form-tel,
  .layout-region-node-secondary .form-email,
  .layout-region-node-secondary .form-url,
  .layout-region-node-secondary .form-search,
  .layout-region-node-secondary .form-number,
  .layout-region-node-secondary .form-color,
  .layout-region-node-secondary textarea {
    box-sizing: border-box;
    width: 100%;
    max-width: 100%;
  }

So the core is recognizing that form-*** is a bad practice already.

There is also form-color here so i am not sure what color.js is doing with that form-text, maybe that input has input.form-color.form-text, if yes that JS code should use input.form-color for it jquery selector.

sasanikolic’s picture

Yeah, I noticed it was used in color core module, but there are many more occurances in the core folder when I search for it, such as node.module.css, bartik/form.css, seven/print.css, stable/off-canvas.form.css.

However, there is a @todo File an issue to add a standard class to all text-like inputsnote in node.module.css which might be related (code that you noted above).

We can try opening a drupal core issue with that comment as a reference, but I'm afraid it's a relatively big change to do this in core and all submodules for such a small issue and bootstrap use-case?

Berdir’s picture

> We can try opening a drupal core issue with that comment as a reference, but I'm afraid it's a relatively big change to do this in core and all submodules for such a small issue and bootstrap use-case?

I assume by remove, Ivica meant here in a template/preprocess. Sure, we can't do that in core. Core is struggling with the tiniest markup changes due to BC.

pivica’s picture

> but there are many more occurances in the core folder when I search for it, such as node.module.css, bartik/form.css, seven/print.css, stable/off-canvas.form.css.

All that 'many other occurrences' are mostly in themes and we do not care about that CSS rules because we are not loading them anyway. node.css rules that are using form-text class are probably not important to us.

Quick search in contrib modules reveals webforms is using this class for tests, paragraphs in tests also, webforms in a couple of places in CSS and some other minor usages. Probably nothing important.

In our custom themes, we are using form-text in zero places.

> I assume by remove, Ivica meant here in a template/preprocess.

Yep, i meant that.

The only question is color module JS code, it will break if we remove this class, but maybe that can be fixed with a patch if that jquery selector can be changed?

It's ugly to leave this CSS class and reset margin because it is colliding with Bootstrap form-text but it is probably much faster and safer to do, soo yeah lets test this approach than first.

pivica’s picture

Status: Needs work » Needs review
Issue tags: +next-release

Patch looks good, marking this for addition to next release.

Berdir’s picture

> The only question is color module JS code, it will break if we remove this class, but maybe that can be fixed with a patch if that jquery selector can be changed?

I don't think we care because we have no plans to support color.module?

pivica’s picture

> I don't think we care because we have no plans to support color.module?

No plans for that at all.

@Berdir you recommend then trying to remove form-text CSS class then in preprocessor?

sasanikolic’s picture

I removed the css change and added the preprocess that unsets the form-text class.

Note that I could have just written unset($variables['attributes']['class'][0]); because that class is always set in the first place in the class array, but with a foreach check I think it's more future-proof in case that changes at some point, although the unset with [0] is faster. Is that okey?

sasanikolic’s picture

Here is an updated patch using array_search instead of the foreach.

pivica’s picture

Status: Needs review » Needs work
  1. +++ b/themes/bs_bootstrap/bs_bootstrap.theme
    @@ -421,11 +421,22 @@ function bs_bootstrap_preprocess_form_element(&$variables) {
    +  $unset_types = ['textfield', 'password', 'webform_email_multiple'];
    +  if (isset($element['#type']) && in_array($element['#type'], $unset_types)) {
    

    Commerce web/modules/contrib/commerce/modules/price/src/Element/Number.php is also attaching form-text CSS class. Should we remove it also?

  2. +++ b/themes/bs_bootstrap/bs_bootstrap.theme
    @@ -421,11 +421,22 @@ function bs_bootstrap_preprocess_form_element(&$variables) {
    +  if (isset($element['#type']) && in_array($element['#type'], $unset_types)) {
    

    We are using $unset_types array just on one place, should we write this array definition inline then in in_array and save one line?

    Alternativly put static $unset_types.

    Not sure what is better and offers more chance for a compiler to optimize this?

sasanikolic’s picture

Not sure this theme should support custom modules by default. Although commerce is quite widely used, adding custom modules fixes in a base theme sounds like an overkill to me.

However, here is an improved patch.

pivica’s picture

> Not sure this theme should support custom modules by default.

You already supported webform_email_multiple element (contrib/custom module0 in the previous patch so i guess it makes sense to add support for commerce also. Also, it makes sense that the theme takes care of this because contrib module can not know which CSS framework theme is using.

Patch looks good now, @Berdir can take one more look if he has time.

Berdir’s picture

Status: Needs review » Needs work
+++ b/themes/bs_bootstrap/bs_bootstrap.theme
@@ -421,11 +421,22 @@ function bs_bootstrap_preprocess_form_element(&$variables) {
+
+  // Remove 'form-text' class from textfield, password, webform multiple email
+  // and number (commerce) inputs, because bootstrap expects it on the help text
+  // element.
+  if (isset($element['#type']) && in_array($element['#type'], ['textfield', 'password', 'webform_email_multiple', 'number'])) {
+    $key = array_search('form-text', $variables['attributes']['class']);
+    if ($key !== FALSE) {

Why do we even bother to check the type, why not just remove *all* form-text classes on input elements?

I can guarantee you that the performance overhead of doing a few more array_search() check tiny and not measurable compared with all the other things that are going on when rendering a form.

So instead of checking the type, just check "!empty($variables['attributes']['class'])" and maybe to be extra careful, add a "is_array($variables['attributes']['class'])"

pivica’s picture

> Why do we even bother to check the type, why not just remove *all* form-text classes on input elements?

Makes total sense. We do not need form-text on any input field.

sasanikolic’s picture

Status: Needs work » Needs review
FileSize
996 bytes
1008 bytes

Thanks for the feedback, makes sense! I did the changes in the following patch.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine now.

pivica’s picture

Removed not used $element variable and improved a comment a bit.

  • pivica committed 0e455b3 on 8.x-1.x
    Issue #3097139 by sasanikolic, pivica, Berdir: form-text class on input...
pivica’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.