Validation messages do not display correctly with Bootstrap 4

[stevepiercy]

Bootstrap 4 made changes to the validation messages from Bootstrap 3. The migration guide notes:

• Replaced .has-error, .has-warning, and .has-success classes with HTML5 form validation via CSS’s :invalid and :valid pseudo-classes.

For Deform this means:

• Change default error_class from error to invalid-feedback.
• Add a class is-invalid to the HTML input. Note that both this and the previous must be done jointly in order for the error message to appear.
• Replace a few instances of has-error with is-invalid.
• Replace a few instances of <p class="help-block" with <div class="form-text invalid-feedback" as well as the closing tag. See https://getbootstrap.com/docs/4.5/components/forms/#help-text
• Update tests in both Deform and deformdemo to detect the proper CSS class.

Examples

deform/templates/mapping_item.pt

class="form-group ${field.error and 'is-invalid' or ''} ${field.widget.item_css_class or ''} ${field.default_item_css_class()}"
...
<div class="invalid-feedback"
...
</div>

deform/templates/textinput.pt

error_class error_class|field.widget.error_class;
...
tal:attributes="class string: form-control ${css_class or ''} ${field.error and error_class or ''};

See also https://getbootstrap.com/docs/4.5/components/forms/?#validation, and in particular https://getbootstrap.com/docs/4.5/components/forms/?#server-side

[sydoluciani]

@stevepiercy

One major change in Bootstrap 4 is moving to flexbox and dropping .form-horizontal which didn't affect Deform, since none of the vertical or horizontal alignments is used in current Deform templates.

Any of the pull requests #25, #187, #282, #216 to add horizontal support would have broken Deform after the upgrade to Bootstrap 4 and needed substantial amount of time and effort to fix them. we best remove any of alignment options from milestone we added couple of months ago, and close all open requests for horiazontal support and suggest overriding templates as instructed here.

We could set up a separate repository for additional templates that developers could share their tamplates instead of changing code to provide new alignment options.

Regarding the validation messages, it is not clear how and where error_class = "error", is used.
error_class is only set in either of widget.py, sequence_item.pt or mapping_item.pt and never actually being used.

[stevepiercy]

I agree with all that. Deform provides a basic set of templates that can be overridden. I like the idea of sharing customized Deform templates, maybe deform-templates as the repo.

Regarding error_class, I wondered exactly what you concluded. It appears that it does nothing other than insert error into the HTML class attribute for a few widgets. Additionally there is no Bootstrap style for just error, but instead has-error, so it would do nothing when inserted. There is a stray static asset deform/static/css/beautify.css that has error as a CSS class name, but that file is not used at all. See http://github.com/stevepiercy/deform/blob/master/deform/TODO#L4-L4. Thinking this through again since my initial post and with your affirmation, I think it is OK to remove all things related to it instead of replace it.

[sydoluciani]

I can confirm that beautify.css is not referenced in any of the widget requirements or in default_resources.

Let's start by removing beautify.css, and cleaning up TODO
The less dependency, the less problem in future upgrades.

Then closing the issues related to horizontal support and suggesting overriding the template instead, then cleaning TODO and adding template repository in TODO.

You are right about none existant "error" string in either of bootstrap javascript or bootstrap css files. setting error_class = "Some_Random_Words", has no effect and all tests are successfully passed, however I need to have a better understanding of Deform internals before moving forward, specifically knowing how fileld.error is being set, what would be it's value and from where it is being injected to here. these are the only places error_class value is being evaluated.

error_class seems to be an outstanding issue #168 at least since 2013, and not when it is set in Widget class but when overriden in MappingWidget or in SequenceWidget

[stevepiercy]

I removed beautify.css in 366355c.

TODOs are there for historical reference only. I wish they never existed and had been put into the issue tracker and other more appropriate places, but ¯\_(ツ)_/¯. GitHub and my IDE have useful search. I don't see any value in touching the TODOs.

Horizontal alignment issues and PRs will remain open for anyone who wants to take them on for the Deform 2.x branches. They will not be used in the 3.x branches. I changed the milestone for these issues and PRs accordingly.

I'll look into that error_class some more. It is tricky.

[sydoluciani]

help-block is dropped in Bootstrap 4 and we need to use form-text and form-text text-danger in case of error instead. this fixes the error text color.

@stevepiercy please merge deformdemo and deform latest pull requests.
Even though they are both failing, but they need to be merged at the same time to pass the tests.

[sydoluciani]

@stevepiercy with current configuration Deform latest pull request needs to be pushed to pip repository to be able to run the Deformdemo test successfully and then run the functional test on Deform.

This problem of interdependency will be solved, If we create a branch in Deform that hold latest merge before merging to master, then update the Deformdemo .travis.yml to pip install from Deform branch that hold one to the last change.

[stevepiercy]

Looking over this again, and I realized a few things. My opening comment has too many mistakes and overlooked a lot of stuff. This comment supersedes it.

We need to adapt to the server-side rendering advice of forms for both help text and validation, specifically server-side validation classes in Bootstrap 4.

In Deform 2.x:

• We wrapped a field with a <div> and applied the class has-error to it when it was not valid.
• We used control-label and required classes in labels.
• We used form-control class for controls.
• We used one <p class="help-block"> for the description and a second one for validation error messages.

<div
    class="form-group has-error item-text"
    title="Enter some text"
    id="item-deformField1">
    <label
        for="deformField1"
        class="control-label required"
        id="req-deformField1">Text</label>
    <input
        type="text"
        name="text"
        value=""
        id="deformField1"
        class=" form-control "/>
    <p class="help-block" id="error-deformField1">Required</p>
    <p class="help-block" >Enter some text</p>
</div>

In Bootstrap 4:

• The wrapping <div> is gone.
• Classes on labels and inputs are gone.
• Help text/description is now in <small class="form-text text-muted">.
• Validation error messages are now in <div class="invalid-feedback">.

This makes more work to update all the templates and functional tests, but I think it is better to comply with Bootstrap 4 because:

• Makes it easier to customize going forward.
• Makes it easier in functional tests to grab the error message when there is a validation error.
• It improves accessibility with aria-describedby on the invalid field.

Example of help text/description only

<label for="inputPassword">Password</label>
<input type="password" id="inputPassword" class="form-control" aria-describedby="passwordHelpBlock">
<small id="passwordHelpBlock" class="form-text text-muted">
  Your password must be 8-20 characters long, contain letters and numbers, and must not contain spaces, special characters, or emoji.
</small>

Example of server-side validation error only

<label for="inputPassword">Password</label>
<input type="text" class="form-control is-invalid" id="inputPassword" aria-describedby="inputPasswordFeedback" required>
<div id="inputPasswordFeedback" class="invalid-feedback">
You entered only 7 characters, but at least 8 are required.
</div>

Example of server-side validation error style with description

<label for="inputPassword">Password</label>
<input type="text" class="form-control is-invalid" id="inputPassword" aria-describedby="inputPasswordFeedback" required>
<div id="inputPasswordFeedback" class="invalid-feedback">
You entered only 7 characters, but at least 8 are required.
</div>
<small id="passwordHelpBlock" class="form-text text-danger">
  Your password must be 8-20 characters long, contain letters and numbers, and must not contain spaces, special characters, or emoji.
</small>

@sydoluciani I'd like your thoughts on this before we do more work.

[sydoluciani]

@stevepiercy I think we best continue fixing issues gradually and with minimal changes to adapt to Bootstrap 4 and closing outstanding issues or pull requests. there are some structural changes needs to be done and we should consider current bugs and issues before changing the structure of the templates, this way we can discuss and then change the logic as well as the structure as we go.

For example, my latest pull request, fixes only error messages that was using help-block, and after merge we can close pull request 354

On next pull request we should be able to close #168

Then we upgrade the bootstrap to version 4.5 and closing #432

Then we upgrade select2 , closing issue #433

On every pull request, as we change Deform code, we need to change the functional tests to pass the test and gradually replace all those findid functions with ActionChains if we get error.

[sydoluciani]

@stevepiercy

I was not able to point Deformdemo's Deform installation in setup.py to different branch in github, but we can create a deform_3_dev branch and merge new pull requests to deform_3_dev branch and finally merge the deform_3_dev branch to master branch when happy with overal changes, this way we can try pointing Deformdemo to deform_3_dev branch to break the interdependency loop and make the process easier, instead of pushing every pull request to pip repository.

[sydoluciani]

@stevepiercy

We can create deformdemo_3_dev branch, that is pointing to deform_3_dev instead of using pip repository, and merge these to master branch when happy with overal pull request changes merged to dev branch.

It can't be one big change, and definitely there will be issues or bugs that is going to deviate any plan to change template structure, so it is best to make small changes and go forward.

[stevepiercy]

master in both repos should serve as development. This may be contrary to the philosophy of "master should always be deployable", but that is OK. See how we handle this situation with Pyramid and pyramid-cookiecutter-starter repos: https://github.com/Pylons/pyramid-cookiecutter-starter/#versions

We have 2.0-branch and master branches in Deform. How about I create new branches 2.0-branch and latest in deformdemo, with latest as default branch, and tweak configuration in files as needed? Thus master on both repos is development. I think that will work, but the proof is in the doing of it.

[stevepiercy]

Also I agree that small incremental changes are better than one huge change. It helps to have a big picture of the huge change, broken down into the small changes. We have a 3.0 milestone that can serve as the big picture, and each issue can be a small change for what we have discussed so far. That will also help focus discussion. I'll create smaller issues out of what we have discussed here.