Move validation errors for check and radio collections into last form-check div.

[lcreid]

Bootstrap 4 validation errors for groups of check boxes or radio buttons are supposed to be inside the <div class="form-group"> of the last check box or radio button in the group. See: https://getbootstrap.com/docs/4.0/components/forms/#supported-elements.

This PR contains some non-trivial refactoring of check_box and radio_button. Methods were extracted so that the collection... helpers could inject the error message when they knew they were working on the last control of the collection. The methods are private, but were left in the file near check_box and radio_button so it's hopefully a little easier to see the changes.

This PR also contains an instance variable that cause magic behaviour in BootstrapForm::Helpers::Bootstrap#append_and_prepend_input, so form_group doesn't output the error at the end of the form group that wraps the whole collection.

This PR was based off the code in PR #434 and depends on that code.

Fixes #418.

[bootstrap-ruby-bot]

	1 Warning
⚠️	Please update CHANGELOG.md with a description of your changes. If this PR is not a user-facing change (e.g. just refactoring), you can disregard this.

Here's an example of a CHANGELOG.md entry (place it immediately under the * Your contribution here! line):

* [#436](https://github.com/bootstrap-ruby/bootstrap_form/pull/436): Move validation errors for check and radio collections into last form-check div. - [@lcreid](https://github.com/lcreid).

Generated by 🚫 Danger

[lcreid]

There are some hard problems here. I can't see how we can output Bootstrap 4 markup without requiring some users to change their code. I hope someone can guide me to what we should do. Sorry this is so convoluted, but I can't see a way to separate one issue from another. @mattbrictson @desheikh @donv any thoughts?

Background

Bootstrap 3 (v2.7)

There are basically four use cases we're interested in:

1. Rendering any control except a bare check box or radio button outputs a validation error message and styles the control appropriately, if the object has a validation error. There are no tests in branch legacy-2.7 for the default case, but test/bootstrap_form_test.rb line 71 (and others) test default error messages and label error messages. Rendered output: https://www.bootply.com/AoHBiFn0wg
2. Rendering one or more check boxes or radio buttons inside a block given to form_group outputs a single validation error message after the check boxes or radio buttons, and styles the control appropriately, if the object has a validation error. There are no tests in branch legacy-2.7 for this. Rendered output: https://www.bootply.com/gLvYyaA6zr and https://www.bootply.com/tUxPJEklIJ
3. Rendering a bare check box or radio button not from a block given to form_group does not output an error message or style the control appropriately, if the object has a validation error. There are no tests in branch legacy-2.7 for this.
4. Rendering any other text from a block given to form_group outputs a validation error message and styles the contents of the block appropriately, if the first argument to form_group is an object with a validation error (In branch legacy-2.7, test/bootstrap_form_group_test.rb at line 171). It renders with the error message in red, but the "field" is not in red. Rendered output: https://www.bootply.com/v4HtoM93EE.

Bootstrap 4

The markup is different. The change that's causing grief is that check boxes and radio buttons are now rendered inside a div.form-check, and the validation error message for a list of check boxes or radio has to be inside the last check box's or radio button's div.form-check. That affects the above use cases in the following ways:

1. All helpers except check_box, collection_check_boxes, collection_radio_buttons, and radio_button work as of PR Move errors inside input-group and form-group. #434 (although note issue Help text rendered inside input-group instead of form-group #444). Bootstrap 4 added a div.input-group for append and prepend which required some non-trivial modifications, but the overall structure of our code is pretty similar to v2.7
2. As mentioned above, each check box and radio button now has to be wrapped in a div.form-check. Error and help messages have to be inside the last div.form-check. This means we can no longer simply add error messages after rendering one or more check boxes or radio buttons
3. Ideally, whatever solution is chosen for 4.0 does not output error messages from all instances of a bare check_box or radio_button, since that's not what happened in v2.7
4. For this use case at the time of writing, even though we output the validation error, it won't be displayed because of the way the Bootstrap 4 classes work. See: https://www.bootply.com/nv1z9gbUKv for an example of the mark-up. The Bootstrap 4 mixin for .is-invalid starts here: https://github.com/twbs/bootstrap/blob/2f4e785a8c623fbe1b011edabecafe99b9f0817b/scss/mixins/_forms.scss#L29, and the CSS that causes the display of .invalid-feedback is here: https://github.com/twbs/bootstrap/blob/2f4e785a8c623fbe1b011edabecafe99b9f0817b/scss/mixins/_forms.scss#L53-L136. This is because .invalid-feedback includes a display: none, which is overridden by display: block depending on the validation state of certain siblings. Unless the programmer ensures that the text in the block is marked up just the right way, the validation message won't be visible anyway

Possible Solutions

These are not in any particular order of preference, effort, or any other criteria.

A

• Change form_group to parse the HTML returned from its block, determine if the last direct child is a radio button or check box (div.form-check), and add the error inside that child. Otherwise, add the error at the end as always. This still doesn't make form_group-wrapped arbitrary text look exactly like legacy, because of use case 4 in the "Bootstrap 4" section above.
• Document how to mark up the text from the block so that errors will be displayed correctly (although this slightly changes the layout of the text)
• and/or
• Encourage the use of static_control if all the programmer is doing is trying to get a static or read-only control

This is the only option I can think of that avoids changing check_box, radio_button, collection_check_boxes, and collection_radio_button. Nokogiri is already loaded in Rails applications. Still, it feels like a heavy-weight solution to me.

B

• Add an error_message: true option to check_box and radio_button to cause them to output the object's validation error messages. The default is false to preserve the current behaviour that check_box and radio_button don't output error messages
• Move stuff around internally so that form_group no longer outputs error messages, but rather it's pushed to the appropriate place so that check_box and radio_button output the errors. PR Move errors inside input-group and form-group. #422 actually did this, but we decided at the time that we didn't want to break the "arbitrary text in the form_group block" case
• Document that the last check_box or radio_button in a form_group has to be called with error_message: true in order to render the validation error message
• Document that arbitrary text in a .form-group should be coded using raw Bootstrap, rather than by using the form_group helper

C

• New helper check_form_group for wrapping check boxes and radio buttons only
• Add an error_message: true option to check_box and radio_button to cause them to output the object's validation error messages
• Document that the last check_box or radio_button in a check_form_group has to be called with error_message: true in order to render the validation error message

This solution is easy to document, i.e. the programmer just has to change all form_groups wrapping check boxes or radio buttons, to check_form_group. The programmer also has to change the last helper in a group to have the error_message: true option.

Internally, it's likely that form_group and check_form_group can share almost all their code, although it might require some non-trivial refactoring to do so elegantly.

D

• Add a helper to output non-control blocks (raw_form_group?), i.e. anything not wrapping check boxes or radio buttons
• Change form_group to not output validation errors, and push that down to the relevant helpers
• Add an error_message: true option to check_box and radio_button to cause them to output the object's validation error messages
• Document that the last check_box or radio_button in a form_group has to be called with error_message: true in order to render the validation error message

This is pretty much the same as Solution C, except it requires the code changes for the "arbitrary text" use case, which I think might be the less common one compared to the "check box or radio button" use case.

[mattbrictson]

Thanks for digging into this one and documenting all the approaches.

To be honest, none of the solutions feels right to me. Solution "A" is the most desirable end-user experience, but I think your intuition is correct that this is too heavy-handed.

This might be a crazy idea, but can we solve this with just CSS? In other words, can we add our own styles that make the error messages appear even if we render the HTML the "wrong" way? Or perhaps a JS snippet that manipulates the DOM?

Also: do we know if/how simple_form has dealt with this problem? Maybe that project has some ideas we can borrow.

[lcreid]

@mattbrictson your timing is perfect. I was just sitting down to charge off with Solution B, but the CSS idea is worth pursuing. I will look into it today. I'll also look at simple_form.

Just a note that I've spent some time looking at the Bootstrap 4 source. I think it confirms that some of the struggles are because Bootstrap 4 works in a fundamentally different way, and we may want to consider adopting the "Bootstrap 4 Way", at the expense of requiring more from programmers migrating from bootstrap_form 2.7 to bootstrap_form 4.0.

[lcreid]

simple_form when using Bootstrap 4 seems to output error messages as if they were help text. Error messages don't get different colours. Also, it looks as if some of the markup is still Bootstrap 3-style.

It looks like errors from check boxes and radio buttons in simple_form don't have to deal with the same issues as we do, partly because they're not trying to do things the "Bootstrap 4" way, and partly because the easy way to generate a group of check boxes or radio buttons is with a collection, which they can then handle where to put the error message.

I should spend more time looking at simple_form, but for this issue, I don't think there's much more to learn from simple_form.

I like the fact that bootstrap_form produces output that looks just like the Bootstrap documentation says it should look.

[lcreid]

About use case 4 above, the case of a form_group with a block that returns arbitrary HTML:

One possible solution is simply to tell the programmer to add the following to their (S)CSS:

.invalid-feedback { display: block; }

This is a rather large hammer to take to the problem, but given the way bootstrap_form works (and worked in the past), I think it's relatively safe to recommend as a solution. I'm specifically suggesting that we document this in the migration guide, and leave it up to the programmer to decide whether they need to add the CSS. It should only be needed when rendering HTML without a bootstrap_form-generated input tag in the block given to form_group. Otherwise, we've already made bootstrap_form work with Bootstrap 4 errors, except for radio buttons and check boxes.

[lcreid]

Finally, for the issue of how to handle error messages when there's more than one check_box or radio_button in a block to form_group: I have an idea for how to do it. It was half-formed in my head before, but looking at the simple_form code seems to have helped crystallize it. But I need to actually try some code and see if the idea will work. I'll let you know in a day or two.

Spoiler alert: My idea will require creating some new classes, not visible to the end programmer, and likely moving some of the code, specifically the code in check_box and radio_button into those classes.

@mattbrictson if you have any thoughts about this or my previous comments today, I'm all ears. Or you can wait to see what I come up with.

[mattbrictson]

OK, I will wait to see your solution. In the meantime: I agree that it is important for bootstrap_form to output correct Bootstrap v4 markup, and if we require a little extra from the programmer in edge cases, then that is an acceptable compromise. We don't want to rely on "magic".

I'm also on board with documenting the display: block hammer for those rare (?) cases that need it.

[lcreid]

The new idea I had yesterday turned out to require more ActionView-related knowledge that I felt comfortable developing in the short term, and I could think of cases where v4.0 would still not be 100% non-breaking with v2.7. So I've basically implemented Solution B above. I want to take one more look at it tonight, and then I'll push a new PR, so we can compare and choose the best approach.

[lcreid]

This PR is no longer need since PR #448 has been merged.