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.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff-3097139-18-20.txt | 1.05 KB | pivica |
#20 | 3097139-form-text-margin-top-reset-20.patch | 816 bytes | pivica |
#11 | 3097139-form-text-margin-top-reset-11.patch | 1.08 KB | sasanikolic |
#3 | 3097139-form-text-margin-top-reset.patch | 1.01 KB | sasanikolic |
misaligned labels.png | 299.28 KB | sasanikolic |
Comments
Comment #2
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedComment #3
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedHere is the patch with the proposed solution.
Comment #4
pivica CreditAttribution: pivica as a volunteer commentedHmm, 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
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.
Comment #5
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedYeah, 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 inputs
note 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?
Comment #6
Berdir> 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.
Comment #7
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commented> 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.
Comment #8
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commentedPatch looks good, marking this for addition to next release.
Comment #9
Berdir> 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?
Comment #10
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commented> 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?
Comment #11
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedI 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?Comment #12
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedHere is an updated patch using array_search instead of the foreach.
Comment #13
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commentedCommerce web/modules/contrib/commerce/modules/price/src/Element/Number.php is also attaching form-text CSS class. Should we remove it also?
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?
Comment #14
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedNot 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.
Comment #15
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commented> 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.
Comment #16
BerdirWhy 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'])"
Comment #17
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commented> 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.
Comment #18
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedThanks for the feedback, makes sense! I did the changes in the following patch.
Comment #19
BerdirLooks fine now.
Comment #20
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commentedRemoved not used $element variable and improved a comment a bit.
Comment #22
pivica CreditAttribution: pivica as a volunteer and at MD Systems GmbH commentedCommitted.