From 27abd7d507a8e6a878546d15b9d72dc4e955bff8 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 21 Feb 2019 11:35:50 +0000 Subject: [PATCH 1/5] Update validation order to match field order Validation is meant to run in reverse order of the fields (so that the last message emitted is for the first invalid field). --- src/components/views/auth/RegistrationForm.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index eabdcd0dd2..c4fe31f4c6 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -82,11 +82,11 @@ module.exports = React.createClass({ // is the one from the first invalid field. // It's not super ideal that this just calls // onError once for each invalid field. + this.validateField(FIELD_PHONE_NUMBER, ev.type); + this.validateField(FIELD_EMAIL, ev.type); this.validateField(FIELD_PASSWORD_CONFIRM, ev.type); this.validateField(FIELD_PASSWORD, ev.type); this.validateField(FIELD_USERNAME, ev.type); - this.validateField(FIELD_PHONE_NUMBER, ev.type); - this.validateField(FIELD_EMAIL, ev.type); const self = this; if (this.allFieldsValid()) { From acae2e9976c2d5267903cfcd353c977e4c9fa488 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 21 Feb 2019 12:36:31 +0000 Subject: [PATCH 2/5] Wait until password confirm is non-empty If password confirm is empty on blur, there's no reason to try validating it. The user may just be tabbing through fields. --- src/components/views/auth/RegistrationForm.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index c4fe31f4c6..bd597de66a 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -206,10 +206,14 @@ module.exports = React.createClass({ } break; case FIELD_PASSWORD_CONFIRM: - this.markFieldValid( - fieldID, pwd1 == pwd2, - "RegistrationForm.ERR_PASSWORD_MISMATCH", - ); + if (allowEmpty && pwd2 === "") { + this.markFieldValid(fieldID, true); + } else { + this.markFieldValid( + fieldID, pwd1 == pwd2, + "RegistrationForm.ERR_PASSWORD_MISMATCH", + ); + } break; } }, From 86a375c7dacd1d02e7a363e49e4054cb23e04627 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 21 Feb 2019 14:31:11 +0000 Subject: [PATCH 3/5] Report validity state of all registration fields on any change This passes the validity state of all fields to the consumer of `RegistrationForm` via the `onValdiationChange` callback, instead of just the most recent error. In addition, we notify the consumer for any validation change, whether success or failure. This allows old validation messages to be properly cleared. It also allows the consumer to be aware of multiple validation errors and display the next one after you have fixed the first. Fixes https://github.com/vector-im/riot-web/issues/8769 --- .../structures/auth/Registration.js | 13 ++++++-- src/components/views/auth/RegistrationForm.js | 33 ++++++++++--------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index 30b553a5b1..b00c0c193f 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -288,7 +288,16 @@ module.exports = React.createClass({ }); }, - onFormValidationFailed: function(errCode) { + onFormValidationChange: function(fieldErrors) { + // Find the first error and show that. + const errCode = Object.values(fieldErrors).find(value => value); + if (!errCode) { + this.setState({ + errorText: null, + }); + return; + } + let errMsg; switch (errCode) { case "RegistrationForm.ERR_PASSWORD_MISSING": @@ -490,7 +499,7 @@ module.exports = React.createClass({ defaultPhoneNumber={this.state.formVals.phoneNumber} defaultPassword={this.state.formVals.password} minPasswordLength={MIN_PASSWORD_LENGTH} - onError={this.onFormValidationFailed} + onValidationChange={this.onFormValidationChange} onRegisterClick={this.onFormSubmit} onEditServerDetailsClick={onEditServerDetailsClick} flows={this.state.flows} diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index bd597de66a..d031dc7bdd 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -46,7 +46,7 @@ module.exports = React.createClass({ defaultUsername: PropTypes.string, defaultPassword: PropTypes.string, minPasswordLength: PropTypes.number, - onError: PropTypes.func, + onValidationChange: PropTypes.func, onRegisterClick: PropTypes.func.isRequired, // onRegisterClick(Object) => ?Promise onEditServerDetailsClick: PropTypes.func, flows: PropTypes.arrayOf(PropTypes.object).isRequired, @@ -60,15 +60,14 @@ module.exports = React.createClass({ getDefaultProps: function() { return { minPasswordLength: 6, - onError: function(e) { - console.error(e); - }, + onValidationChange: console.error, }; }, getInitialState: function() { return { - fieldValid: {}, + // Field error codes by field ID + fieldErrors: {}, // The ISO2 country code selected in the phone number entry phoneCountry: this.props.defaultPhoneCountry, }; @@ -81,7 +80,7 @@ module.exports = React.createClass({ // the error that ends up being displayed // is the one from the first invalid field. // It's not super ideal that this just calls - // onError once for each invalid field. + // onValidationChange once for each invalid field. this.validateField(FIELD_PHONE_NUMBER, ev.type); this.validateField(FIELD_EMAIL, ev.type); this.validateField(FIELD_PASSWORD_CONFIRM, ev.type); @@ -134,9 +133,9 @@ module.exports = React.createClass({ * @returns {boolean} true if all fields were valid last time they were validated. */ allFieldsValid: function() { - const keys = Object.keys(this.state.fieldValid); + const keys = Object.keys(this.state.fieldErrors); for (let i = 0; i < keys.length; ++i) { - if (this.state.fieldValid[keys[i]] == false) { + if (this.state.fieldErrors[keys[i]]) { return false; } } @@ -218,13 +217,17 @@ module.exports = React.createClass({ } }, - markFieldValid: function(fieldID, val, errorCode) { - const fieldValid = this.state.fieldValid; - fieldValid[fieldID] = val; - this.setState({fieldValid: fieldValid}); - if (!val) { - this.props.onError(errorCode); + markFieldValid: function(fieldID, valid, errorCode) { + const { fieldErrors } = this.state; + if (valid) { + fieldErrors[fieldID] = null; + } else { + fieldErrors[fieldID] = errorCode; } + this.setState({ + fieldErrors, + }); + this.props.onValidationChange(fieldErrors); }, fieldElementById(fieldID) { @@ -244,7 +247,7 @@ module.exports = React.createClass({ _classForField: function(fieldID, ...baseClasses) { let cls = baseClasses.join(' '); - if (this.state.fieldValid[fieldID] === false) { + if (this.state.fieldErrors[fieldID]) { if (cls) cls += ' '; cls += 'error'; } From 8e32798f45ad47804b4d5a3ebe7bbc05d5a8ffd1 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 21 Feb 2019 14:41:42 +0000 Subject: [PATCH 4/5] Ensure fields with errors are clearly visible Until we have better validation, let's at least ensure fields with errors are properly marked via color. --- res/css/views/auth/_AuthBody.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/res/css/views/auth/_AuthBody.scss b/res/css/views/auth/_AuthBody.scss index 6216bdd4b8..778f5f6a4d 100644 --- a/res/css/views/auth/_AuthBody.scss +++ b/res/css/views/auth/_AuthBody.scss @@ -58,6 +58,10 @@ limitations under the License. background-color: $authpage-body-bg-color; } +.mx_AuthBody input.error { + color: $warning-color; +} + .mx_AuthBody_editServerDetails { padding-left: 1em; font-size: 12px; From 4b29d5e228f82f70344c9a26d302cdb73cc8b570 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 26 Feb 2019 16:41:17 +0000 Subject: [PATCH 5/5] Clarify finding first non-null field error --- src/components/structures/auth/Registration.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index b00c0c193f..b5707f0136 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -289,8 +289,11 @@ module.exports = React.createClass({ }, onFormValidationChange: function(fieldErrors) { - // Find the first error and show that. - const errCode = Object.values(fieldErrors).find(value => value); + // `fieldErrors` is an object mapping field IDs to error codes when there is an + // error or `null` for no error, so the values array will be something like: + // `[ null, "RegistrationForm.ERR_PASSWORD_MISSING", null]` + // Find the first non-null error code and show that. + const errCode = Object.values(fieldErrors).find(value => !!value); if (!errCode) { this.setState({ errorText: null,