silverstripe / silverstripe-admin

Silverstripe Admin Component
BSD 3-Clause "New" or "Revised" License
25 stars 91 forks source link

Client-side validation in React forms #45

Closed chillu closed 6 years ago

chillu commented 7 years ago

Entwine/PJAX forms have been traditionally server-side validated only. React forms are currently working the same way, but could be much more responsive to invalid user input.

Acceptance Criteria

Out of scope

Notes

Resources

Pull requests

phalkunz commented 6 years ago

@chillu @flamerohr Some fields use browser built-in validation (e.g. email field) and validation in Validator.js at the same time. How should this work? Is this expected? Or only one of them should be used at one time? If so, which one has higher priority?

chillu commented 6 years ago

@phalkunz HTML5 fields do more than just affect validation - on mobile, they define what the keyboard layout or input element you get. So ideally we'd keep those type declarations, and overwrite just the validation with Validator.js - which should take precedence over built-in browser validation. In some cases that might not be possible, e.g. type=date shows a date picker and then stores the input value in ISO - so no point running Validator.js over it.

unclecheese commented 6 years ago

This is an old card. Trying to sort though what's changed since it was written.

There are three layers of validation here that are all competing:

As far as I can tell, the last two could be merged. The execution of Validator.js could just as easily be another middleware function, applied by default.

Clarification on the AC's:

Validator.js: Any field type that can be validated with HTML5 should not be in Validator.js. We should take inventory of what Validator.js is actually doing that HTML5 cannot and consider using the JSON schema rules to apply HTML5 attributes to the form fields that replicate what Validator.js is doing. Surely Validator.js is filling in some gaps, but right now it appears to be doing way too much if we want HTML5 to be the default. Either that, or we turn off HTML5 validation and use Validator.js as the sole solution, but that would mean a lot of reinventing of the wheel.

Showing feedback: redux-forms is configured to show immediate feedback once a field has been touched. I think we should leave it that way. Instant feedback isn't always the best approach.

Server error styling: This seems very hard to replicate, since validation rules come down through the schema. It's hard to see a scenario where server-side validation is not mirrored by client-side as well. We were able to brute-force it in by disabling validation on the component to see what happens, and the result is an error message that is not consistent. This needs to go through the same validation API as FormBuilder.validateForm (i.e. through the redux store) to keep these consistent and to support the removal/replacement of errors described in the last two ACs.

flamerohr commented 6 years ago

Sorry, I was meant to clean this up in the morning (and had travis issues distracting me)

I'm more convinced with disabling html5 validation, it's good to see if "overall" the form can be submitted. But where html5 validation falls short, it's also not easy to link together with the Javascript to pick it back up.

FormDOM.checkValidity(); only returns true or false for if all fields are valid or not. Field error messages are locked for your browser to decide (or ignored if not supported), it also ignores what your user profile or page locale is - so we'll lose consistency in our interface there.

If we want the customisations which we had been designing for before stable release (validation messages) we'd almost need to rely on at least ValidationMiddleware to catch those and report back to redux-form. I guess we could have a middleware which utilise the html5 validation, if desired... I'm not too sure how far this could reach - documentation has been scarce :(

Validator.js could be converted and plugged in as a middleware I think as part of this story.

chillu commented 6 years ago

redux-forms is configured to show immediate feedback once a field has been touched. I think we should leave it that way. Instant feedback isn't always the best approach.

Agreed.

Server error styling: This seems very hard to replicate

Here's an example: A tagfield disallows use of certain tag combinations, which is checked on the server due to implementation complexity (e.g. "dogs" and "cats" can be used separately, but not in combination). Tagfield also has a client-side rule that requires at least one tag. Would a server-generated error about a wrong tag combo get cleared and replaced by "at least one tag required" when the user empties out the field? Maybe that example is too contrived, and we should move server-driven validation into an API, but for now a server-driven response would be the default way to implement this scenario.

If its too hard to combine HTML5 with Validator.js rules, I'm leaning towards just using Validator.js, assuming that those "wheels" have already been invented. This would allow more consistency on execution paths, e.g. if a numeric field fails on number format (HTML5), but also on a custom rule like "requires rounded cents" (a Validator.js callback). Assuming we need the latter, what's the benefit of using HTML5? I see HTML5 inputs as a great way to assist user input (type=date), but in terms of validation it might be a bit too limiting?

robbieaverill commented 6 years ago

Resolved via https://github.com/silverstripe/silverstripe-framework/pull/7697

flamerohr commented 6 years ago

https://github.com/silverstripe/silverstripe-admin/pull/380 has not merged yet