From 37ecf2a623113bdc6dc3b95c0867fd2b125668bb Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 16 Apr 2019 15:40:04 +0100 Subject: [PATCH 01/19] Remove unused ref from Field component The `fieldInput` ref is no longer used now that we have controlled components everywhere. --- src/components/views/elements/Field.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 12e20ad789..1b9d55c221 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -74,7 +74,6 @@ export default class Field extends React.PureComponent { // Set some defaults for the element inputProps.type = inputProps.type || "text"; - inputProps.ref = "fieldInput"; inputProps.placeholder = inputProps.placeholder || inputProps.label; inputProps.onChange = this.onChange; From 338d83ab55c85ee7ba12d797c30b1d260ffed665 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 16 Apr 2019 16:52:31 +0100 Subject: [PATCH 02/19] Add validation feedback helper This adds a general validation feedback mechanism for checking input values. An initial example is wired up for the username input on registration. --- res/css/_components.scss | 1 + res/css/views/elements/_Validation.scss | 32 ++++++ src/components/views/auth/RegistrationForm.js | 56 ++++++---- src/components/views/elements/Field.js | 8 +- src/components/views/elements/Validation.js | 102 ++++++++++++++++++ src/i18n/strings/en_EN.json | 2 + 6 files changed, 176 insertions(+), 25 deletions(-) create mode 100644 res/css/views/elements/_Validation.scss create mode 100644 src/components/views/elements/Validation.js diff --git a/res/css/_components.scss b/res/css/_components.scss index 1f896d270d..8bea138acb 100644 --- a/res/css/_components.scss +++ b/res/css/_components.scss @@ -100,6 +100,7 @@ @import "./views/elements/_ToggleSwitch.scss"; @import "./views/elements/_ToolTipButton.scss"; @import "./views/elements/_Tooltip.scss"; +@import "./views/elements/_Validation.scss"; @import "./views/globals/_MatrixToolbar.scss"; @import "./views/groups/_GroupPublicityToggle.scss"; @import "./views/groups/_GroupRoomList.scss"; diff --git a/res/css/views/elements/_Validation.scss b/res/css/views/elements/_Validation.scss new file mode 100644 index 0000000000..08ae793663 --- /dev/null +++ b/res/css/views/elements/_Validation.scss @@ -0,0 +1,32 @@ +/* +Copyright 2019 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +.mx_Validation_details { + padding-left: 15px; +} + +.mx_Validation_detail { + font-weight: normal; + + // TODO: Check / cross images + &.mx_Validation_valid { + color: $input-valid-border-color; + } + + &.mx_Validation_invalid { + color: $input-invalid-border-color; + } +} diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 1784ab61c3..83ea4dfae6 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -25,6 +25,7 @@ import Modal from '../../../Modal'; import { _t } from '../../../languageHandler'; import SdkConfig from '../../../SdkConfig'; import { SAFE_LOCALPART_REGEX } from '../../../Registration'; +import withValidation from '../elements/Validation'; const FIELD_EMAIL = 'field_email'; const FIELD_PHONE_NUMBER = 'field_phone_number'; @@ -86,6 +87,7 @@ module.exports = React.createClass({ // is the one from the first invalid field. // It's not super ideal that this just calls // onValidationChange once for each invalid field. + // TODO: Change this to trigger new-style validation for an invalid fields. this.validateField(FIELD_PHONE_NUMBER, ev.type); this.validateField(FIELD_EMAIL, ev.type); this.validateField(FIELD_PASSWORD_CONFIRM, ev.type); @@ -152,6 +154,8 @@ module.exports = React.createClass({ const pwd2 = this.state.passwordConfirm.trim(); const allowEmpty = eventType === "blur"; + // TODO: Remove rules here as they are converted to new-style validation + switch (fieldID) { case FIELD_EMAIL: { const email = this.state.email; @@ -173,12 +177,6 @@ module.exports = React.createClass({ const username = this.state.username; if (allowEmpty && username === '') { this.markFieldValid(fieldID, true); - } else if (!SAFE_LOCALPART_REGEX.test(username)) { - this.markFieldValid( - fieldID, - false, - "RegistrationForm.ERR_USERNAME_INVALID", - ); } else if (username == '') { this.markFieldValid( fieldID, @@ -232,11 +230,14 @@ module.exports = React.createClass({ this.setState({ fieldErrors, }); + // TODO: Remove outer validation handling once all fields converted to new-style + // validation in the form. this.props.onValidationChange(fieldErrors); }, _classForField: function(fieldID, ...baseClasses) { let cls = baseClasses.join(' '); + // TODO: Remove this from fields as they are converted to new-style validation. if (this.state.fieldErrors[fieldID]) { if (cls) cls += ' '; cls += 'error'; @@ -291,10 +292,6 @@ module.exports = React.createClass({ }); }, - onUsernameBlur(ev) { - this.validateField(FIELD_USERNAME, ev.type); - }, - onUsernameChange(ev) { this.setState({ username: ev.target.value, @@ -325,6 +322,33 @@ module.exports = React.createClass({ }); }, + renderUsername() { + const Field = sdk.getComponent('elements.Field'); + + const onValidate = withValidation({ + description: _t("Use letters, numbers, dashes and underscores only"), + rules: [ + { + key: "safeLocalpart", + regex: SAFE_LOCALPART_REGEX, + invalid: _t("Some characters not allowed"), + }, + ], + }); + + return ; + }, + render: function() { const Field = sdk.getComponent('elements.Field'); @@ -412,17 +436,7 @@ module.exports = React.createClass({
- + {this.renderUsername()}
; @@ -108,7 +108,7 @@ export default class Field extends React.PureComponent { {prefixContainer} {fieldInput} - {feedback} + {tooltip}
; } } diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js new file mode 100644 index 0000000000..21538609c1 --- /dev/null +++ b/src/components/views/elements/Validation.js @@ -0,0 +1,102 @@ +/* +Copyright 2019 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import classNames from 'classnames'; + +/** + * Creates a validation function from a set of rules describing what to validate. + * + * @param {String} description + * Summary of the kind of value that will meet the validation rules. Shown at + * the top of the validation feedback. + * @param {Object} rules + * An array of rules describing how to check to input value. Each rule in an object + * and may have the following properties: + * - `key`: A unique ID for the rule. Required. + * - `regex`: A regex used to determine the rule's current validity. Required. + * - `valid`: Text to show when the rule is valid. Only shown if set. + * - `invalid`: Text to show when the rule is invalid. Only shown if set. + * @returns {Function} + * A validation function that takes in the current input value and returns + * the overall validity and a feedback UI that can be rendered for more detail. + */ +export default function withValidation({ description, rules }) { + return function onValidate(value) { + // TODO: Hide on blur + // TODO: Re-run only after ~200ms of inactivity + if (!value) { + return { + valid: null, + feedback: null, + }; + } + + const results = []; + let valid = true; + if (rules && rules.length) { + for (const rule of rules) { + if (!rule.key || !rule.regex) { + continue; + } + const ruleValid = rule.regex.test(value); + valid = valid && ruleValid; + if (ruleValid && rule.valid) { + // If the rule's result is valid and has text to show for + // the valid state, show it. + results.push({ + key: rule.key, + valid: true, + text: rule.valid, + }); + } else if (!ruleValid && rule.invalid) { + // If the rule's result is invalid and has text to show for + // the invalid state, show it. + results.push({ + key: rule.key, + valid: false, + text: rule.invalid, + }); + } + } + } + + let details; + if (results && results.length) { + details =
    + {results.map(result => { + const classes = classNames({ + "mx_Validation_detail": true, + "mx_Validation_valid": result.valid, + "mx_Validation_invalid": !result.valid, + }); + return
  • + {result.text} +
  • ; + })} +
; + } + + const feedback =
+
{description}
+ {details} +
; + + return { + valid, + feedback, + }; + }; +} diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index a7fae2803f..51967c39f1 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1322,6 +1322,8 @@ "Change": "Change", "Sign in with": "Sign in with", "If you don't specify an email address, you won't be able to reset your password. Are you sure?": "If you don't specify an email address, you won't be able to reset your password. Are you sure?", + "Use letters, numbers, dashes and underscores only": "Use letters, numbers, dashes and underscores only", + "Some characters not allowed": "Some characters not allowed", "Create your Matrix account": "Create your Matrix account", "Create your Matrix account on %(serverName)s": "Create your Matrix account on %(serverName)s", "Email (optional)": "Email (optional)", From 87f13cfe55b45802b45fd58879b2a4afff83d8c6 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 16 Apr 2019 18:12:13 +0100 Subject: [PATCH 03/19] Add focus handling to validation Update the Field component and validation handling to show / hide validation feedback on focus / blur events. --- src/components/views/elements/Field.js | 49 ++++++++++++++++++--- src/components/views/elements/Validation.js | 11 ++++- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 6182a80b70..6d58d29a3d 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -53,20 +53,53 @@ export default class Field extends React.PureComponent { }; } - onChange = (ev) => { - if (this.props.onValidate) { - const result = this.props.onValidate(ev.target.value); - this.setState({ - valid: result.valid, - feedback: result.feedback, - }); + onFocus = (ev) => { + this.validate({ + value: ev.target.value, + focused: true, + }); + // Parent component may have supplied its own `onFocus` as well + if (this.props.onFocus) { + this.props.onFocus(ev); } + }; + + onChange = (ev) => { + this.validate({ + value: ev.target.value, + focused: true, + }); // Parent component may have supplied its own `onChange` as well if (this.props.onChange) { this.props.onChange(ev); } }; + onBlur = (ev) => { + this.validate({ + value: ev.target.value, + focused: false, + }); + // Parent component may have supplied its own `onBlur` as well + if (this.props.onBlur) { + this.props.onBlur(ev); + } + }; + + validate({ value, focused }) { + if (!this.props.onValidate) { + return; + } + const { valid, feedback } = this.props.onValidate({ + value, + focused, + }); + this.setState({ + valid, + feedback, + }); + } + render() { const { element, prefix, onValidate, children, ...inputProps } = this.props; @@ -76,7 +109,9 @@ export default class Field extends React.PureComponent { inputProps.type = inputProps.type || "text"; inputProps.placeholder = inputProps.placeholder || inputProps.label; + inputProps.onFocus = this.onFocus; inputProps.onChange = this.onChange; + inputProps.onBlur = this.onBlur; const fieldInput = React.createElement(inputElement, inputProps, children); diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index 21538609c1..44be046a73 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -34,8 +34,7 @@ import classNames from 'classnames'; * the overall validity and a feedback UI that can be rendered for more detail. */ export default function withValidation({ description, rules }) { - return function onValidate(value) { - // TODO: Hide on blur + return function onValidate({ value, focused }) { // TODO: Re-run only after ~200ms of inactivity if (!value) { return { @@ -73,6 +72,14 @@ export default function withValidation({ description, rules }) { } } + // Hide feedback when not focused + if (!focused) { + return { + valid, + feedback: null, + }; + } + let details; if (results && results.length) { details =
    From 37e09b5569b2ce38bc41128201417d8baf2ece81 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 16 Apr 2019 18:49:03 +0100 Subject: [PATCH 04/19] Add check and x icons for validation feedback Adds icons from the Feather set with the same color as text. Tweaks validation item spacing to match the design. --- res/css/views/elements/_Validation.scss | 33 +++++++++++++++++++-- res/img/feather-customised/check.svg | 3 ++ res/img/feather-customised/x.svg | 4 +++ src/components/views/elements/Validation.js | 2 +- 4 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 res/img/feather-customised/check.svg create mode 100644 res/img/feather-customised/x.svg diff --git a/res/css/views/elements/_Validation.scss b/res/css/views/elements/_Validation.scss index 08ae793663..f101f6cd26 100644 --- a/res/css/views/elements/_Validation.scss +++ b/res/css/views/elements/_Validation.scss @@ -14,19 +14,48 @@ See the License for the specific language governing permissions and limitations under the License. */ +.mx_Validation_description { + margin-bottom: 1em; +} + .mx_Validation_details { - padding-left: 15px; + padding-left: 20px; + margin: 0; } .mx_Validation_detail { + position: relative; font-weight: normal; + list-style: none; + margin-bottom: 0.5em; + + &::before { + content: ""; + position: absolute; + width: 14px; + height: 14px; + top: 0; + left: -18px; + mask-repeat: no-repeat; + mask-position: center; + mask-size: contain; + } - // TODO: Check / cross images &.mx_Validation_valid { color: $input-valid-border-color; + + &::before { + mask-image: url('$(res)/img/feather-customised/check.svg'); + background-color: $input-valid-border-color; + } } &.mx_Validation_invalid { color: $input-invalid-border-color; + + &::before { + mask-image: url('$(res)/img/feather-customised/x.svg'); + background-color: $input-invalid-border-color; + } } } diff --git a/res/img/feather-customised/check.svg b/res/img/feather-customised/check.svg new file mode 100644 index 0000000000..5c600f8649 --- /dev/null +++ b/res/img/feather-customised/check.svg @@ -0,0 +1,3 @@ + + + diff --git a/res/img/feather-customised/x.svg b/res/img/feather-customised/x.svg new file mode 100644 index 0000000000..5468caa8aa --- /dev/null +++ b/res/img/feather-customised/x.svg @@ -0,0 +1,4 @@ + + + + diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index 44be046a73..4770c968e2 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -97,7 +97,7 @@ export default function withValidation({ description, rules }) { } const feedback =
    -
    {description}
    +
    {description}
    {details}
    ; From 62a01e7a3712e56ca8c021b813b2e372725b0889 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 17 Apr 2019 11:39:11 +0100 Subject: [PATCH 05/19] Track per-field validity with new-style validation This updates the registration form to include the new-style validation state when deciding whether the entire form is valid overall. In addition, this tweaks the validation helper to take functions instead of strings for translated text. This allows the validation helper to be create once per component instead of once every render, which improves performance. --- src/components/views/auth/RegistrationForm.js | 79 ++++++++++++------- src/components/views/elements/Validation.js | 21 +++-- 2 files changed, 64 insertions(+), 36 deletions(-) diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 83ea4dfae6..6e139d7051 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -68,7 +68,9 @@ module.exports = React.createClass({ getInitialState: function() { return { // Field error codes by field ID + // TODO: Remove `fieldErrors` once converted to new-style validation fieldErrors: {}, + fieldValid: {}, // The ISO2 country code selected in the phone number entry phoneCountry: this.props.defaultPhoneCountry, username: "", @@ -140,12 +142,19 @@ 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.fieldErrors); + // TODO: Remove `fieldErrors` here when all fields converted + let keys = Object.keys(this.state.fieldErrors); for (let i = 0; i < keys.length; ++i) { if (this.state.fieldErrors[keys[i]]) { return false; } } + keys = Object.keys(this.state.fieldValid); + for (let i = 0; i < keys.length; ++i) { + if (!this.state.fieldValid[keys[i]]) { + return false; + } + } return true; }, @@ -161,57 +170,57 @@ module.exports = React.createClass({ const email = this.state.email; const emailValid = email === '' || Email.looksValid(email); if (this._authStepIsRequired('m.login.email.identity') && (!emailValid || email === '')) { - this.markFieldValid(fieldID, false, "RegistrationForm.ERR_MISSING_EMAIL"); - } else this.markFieldValid(fieldID, emailValid, "RegistrationForm.ERR_EMAIL_INVALID"); + this.markFieldError(fieldID, false, "RegistrationForm.ERR_MISSING_EMAIL"); + } else this.markFieldError(fieldID, emailValid, "RegistrationForm.ERR_EMAIL_INVALID"); break; } case FIELD_PHONE_NUMBER: { const phoneNumber = this.state.phoneNumber; const phoneNumberValid = phoneNumber === '' || phoneNumberLooksValid(phoneNumber); if (this._authStepIsRequired('m.login.msisdn') && (!phoneNumberValid || phoneNumber === '')) { - this.markFieldValid(fieldID, false, "RegistrationForm.ERR_MISSING_PHONE_NUMBER"); - } else this.markFieldValid(fieldID, phoneNumberValid, "RegistrationForm.ERR_PHONE_NUMBER_INVALID"); + this.markFieldError(fieldID, false, "RegistrationForm.ERR_MISSING_PHONE_NUMBER"); + } else this.markFieldError(fieldID, phoneNumberValid, "RegistrationForm.ERR_PHONE_NUMBER_INVALID"); break; } case FIELD_USERNAME: { const username = this.state.username; if (allowEmpty && username === '') { - this.markFieldValid(fieldID, true); + this.markFieldError(fieldID, true); } else if (username == '') { - this.markFieldValid( + this.markFieldError( fieldID, false, "RegistrationForm.ERR_USERNAME_BLANK", ); } else { - this.markFieldValid(fieldID, true); + this.markFieldError(fieldID, true); } break; } case FIELD_PASSWORD: if (allowEmpty && pwd1 === "") { - this.markFieldValid(fieldID, true); + this.markFieldError(fieldID, true); } else if (pwd1 == '') { - this.markFieldValid( + this.markFieldError( fieldID, false, "RegistrationForm.ERR_PASSWORD_MISSING", ); } else if (pwd1.length < this.props.minPasswordLength) { - this.markFieldValid( + this.markFieldError( fieldID, false, "RegistrationForm.ERR_PASSWORD_LENGTH", ); } else { - this.markFieldValid(fieldID, true); + this.markFieldError(fieldID, true); } break; case FIELD_PASSWORD_CONFIRM: if (allowEmpty && pwd2 === "") { - this.markFieldValid(fieldID, true); + this.markFieldError(fieldID, true); } else { - this.markFieldValid( + this.markFieldError( fieldID, pwd1 == pwd2, "RegistrationForm.ERR_PASSWORD_MISMATCH", ); @@ -220,7 +229,8 @@ module.exports = React.createClass({ } }, - markFieldValid: function(fieldID, valid, errorCode) { + markFieldError: function(fieldID, valid, errorCode) { + // TODO: Remove this function once all fields converted to new-style validation. const { fieldErrors } = this.state; if (valid) { fieldErrors[fieldID] = null; @@ -235,6 +245,14 @@ module.exports = React.createClass({ this.props.onValidationChange(fieldErrors); }, + markFieldValid: function(fieldID, valid) { + const { fieldValid } = this.state; + fieldValid[fieldID] = valid; + this.setState({ + fieldValid, + }); + }, + _classForField: function(fieldID, ...baseClasses) { let cls = baseClasses.join(' '); // TODO: Remove this from fields as they are converted to new-style validation. @@ -298,6 +316,23 @@ module.exports = React.createClass({ }); }, + onUsernameValidate(fieldState) { + const result = this.validateUsernameRules(fieldState); + this.markFieldValid(FIELD_USERNAME, result.valid); + return result; + }, + + validateUsernameRules: withValidation({ + description: () => _t("Use letters, numbers, dashes and underscores only"), + rules: [ + { + key: "safeLocalpart", + regex: SAFE_LOCALPART_REGEX, + invalid: () => _t("Some characters not allowed"), + }, + ], + }), + /** * A step is required if all flows include that step. * @@ -324,18 +359,6 @@ module.exports = React.createClass({ renderUsername() { const Field = sdk.getComponent('elements.Field'); - - const onValidate = withValidation({ - description: _t("Use letters, numbers, dashes and underscores only"), - rules: [ - { - key: "safeLocalpart", - regex: SAFE_LOCALPART_REGEX, - invalid: _t("Some characters not allowed"), - }, - ], - }); - return ; }, diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index 4770c968e2..8fc584edce 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -19,16 +19,16 @@ import classNames from 'classnames'; /** * Creates a validation function from a set of rules describing what to validate. * - * @param {String} description - * Summary of the kind of value that will meet the validation rules. Shown at - * the top of the validation feedback. + * @param {Function} description + * Function that returns a string summary of the kind of value that will + * meet the validation rules. Shown at the top of the validation feedback. * @param {Object} rules * An array of rules describing how to check to input value. Each rule in an object * and may have the following properties: * - `key`: A unique ID for the rule. Required. * - `regex`: A regex used to determine the rule's current validity. Required. - * - `valid`: Text to show when the rule is valid. Only shown if set. - * - `invalid`: Text to show when the rule is invalid. Only shown if set. + * - `valid`: Function returning text to show when the rule is valid. Only shown if set. + * - `invalid`: Function returning text to show when the rule is invalid. Only shown if set. * @returns {Function} * A validation function that takes in the current input value and returns * the overall validity and a feedback UI that can be rendered for more detail. @@ -58,7 +58,7 @@ export default function withValidation({ description, rules }) { results.push({ key: rule.key, valid: true, - text: rule.valid, + text: rule.valid(), }); } else if (!ruleValid && rule.invalid) { // If the rule's result is invalid and has text to show for @@ -66,7 +66,7 @@ export default function withValidation({ description, rules }) { results.push({ key: rule.key, valid: false, - text: rule.invalid, + text: rule.invalid(), }); } } @@ -96,8 +96,13 @@ export default function withValidation({ description, rules }) {
; } + let summary; + if (description) { + summary =
{description()}
; + } + const feedback =
-
{description}
+ {summary} {details}
; From 5d95c318756d3cf2e72ca7748f48c828eede57f2 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 17 Apr 2019 14:23:59 +0100 Subject: [PATCH 06/19] Focus the first invalid field This adjusts the submission step to focus the first invalid field and redisplay validation. This also rearranges the older style field error handling on registration which is slated for removal once we convert all fields to the new style. --- src/components/views/auth/RegistrationForm.js | 76 ++++++++++++++----- src/components/views/elements/Field.js | 5 ++ 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 6e139d7051..c680d058c5 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -89,34 +89,37 @@ module.exports = React.createClass({ // is the one from the first invalid field. // It's not super ideal that this just calls // onValidationChange once for each invalid field. - // TODO: Change this to trigger new-style validation for an invalid fields. + // TODO: Remove these calls once converted to new-style validation. 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); + const allFieldsValid = this.verifyFieldsBeforeSubmit(); + if (!allFieldsValid) { + return; + } + const self = this; - if (this.allFieldsValid()) { - if (this.state.email == '') { - const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog"); - Modal.createTrackedDialog('If you don\'t specify an email address...', '', QuestionDialog, { - title: _t("Warning!"), - description: -
- { _t("If you don't specify an email address, you won't be able to reset your password. " + - "Are you sure?") } -
, - button: _t("Continue"), - onFinished: function(confirmed) { - if (confirmed) { - self._doSubmit(ev); - } - }, - }); - } else { - self._doSubmit(ev); - } + if (this.state.email == '') { + const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog"); + Modal.createTrackedDialog('If you don\'t specify an email address...', '', QuestionDialog, { + title: _t("Warning!"), + description: +
+ { _t("If you don't specify an email address, you won't be able to reset your password. " + + "Are you sure?") } +
, + button: _t("Continue"), + onFinished: function(confirmed) { + if (confirmed) { + self._doSubmit(ev); + } + }, + }); + } else { + self._doSubmit(ev); } }, @@ -138,6 +141,27 @@ module.exports = React.createClass({ } }, + verifyFieldsBeforeSubmit() { + if (this.allFieldsValid()) { + return true; + } + + const invalidField = this.findFirstInvalidField([ + FIELD_USERNAME, + FIELD_PASSWORD, + FIELD_PASSWORD_CONFIRM, + FIELD_EMAIL, + FIELD_PHONE_NUMBER, + ]); + + if (!invalidField) { + return true; + } + + invalidField.focus(); + return false; + }, + /** * @returns {boolean} true if all fields were valid last time they were validated. */ @@ -158,6 +182,15 @@ module.exports = React.createClass({ return true; }, + findFirstInvalidField(fieldIDs) { + for (const fieldID of fieldIDs) { + if (!this.state.fieldValid[fieldID] && this[fieldID]) { + return this[fieldID]; + } + } + return null; + }, + validateField: function(fieldID, eventType) { const pwd1 = this.state.password.trim(); const pwd2 = this.state.passwordConfirm.trim(); @@ -362,6 +395,7 @@ module.exports = React.createClass({ return this[FIELD_USERNAME] = field} type="text" autoFocus={true} label={_t("Username")} diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 6d58d29a3d..93d1f1e71d 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -86,6 +86,10 @@ export default class Field extends React.PureComponent { } }; + focus() { + this.input.focus(); + } + validate({ value, focused }) { if (!this.props.onValidate) { return; @@ -107,6 +111,7 @@ export default class Field extends React.PureComponent { // Set some defaults for the element inputProps.type = inputProps.type || "text"; + inputProps.ref = input => this.input = input; inputProps.placeholder = inputProps.placeholder || inputProps.label; inputProps.onFocus = this.onFocus; From 778697abf13e6d1aa548bfabaa7b5498de16ca4d Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 18 Apr 2019 17:37:07 +0100 Subject: [PATCH 07/19] Use input element's value directly Since we're keeping the input as a ref anyway, let's use that rather than requiring the value to be passed to `validate`. This allows others to call `validate` as well. --- src/components/views/elements/Field.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 93d1f1e71d..dfe3a51697 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -55,7 +55,6 @@ export default class Field extends React.PureComponent { onFocus = (ev) => { this.validate({ - value: ev.target.value, focused: true, }); // Parent component may have supplied its own `onFocus` as well @@ -66,7 +65,6 @@ export default class Field extends React.PureComponent { onChange = (ev) => { this.validate({ - value: ev.target.value, focused: true, }); // Parent component may have supplied its own `onChange` as well @@ -77,7 +75,6 @@ export default class Field extends React.PureComponent { onBlur = (ev) => { this.validate({ - value: ev.target.value, focused: false, }); // Parent component may have supplied its own `onBlur` as well @@ -90,10 +87,11 @@ export default class Field extends React.PureComponent { this.input.focus(); } - validate({ value, focused }) { + validate({ focused }) { if (!this.props.onValidate) { return; } + const { value } = this.input; const { valid, feedback } = this.props.onValidate({ value, focused, From a7c37733b8f741f220a32384732f2e30348f8d29 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 18 Apr 2019 21:22:37 +0100 Subject: [PATCH 08/19] Rebalance margins in validation tooltip --- res/css/views/elements/_Validation.scss | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/res/css/views/elements/_Validation.scss b/res/css/views/elements/_Validation.scss index f101f6cd26..4c059f9747 100644 --- a/res/css/views/elements/_Validation.scss +++ b/res/css/views/elements/_Validation.scss @@ -14,21 +14,25 @@ See the License for the specific language governing permissions and limitations under the License. */ -.mx_Validation_description { - margin-bottom: 1em; -} - .mx_Validation_details { padding-left: 20px; margin: 0; } +.mx_Validation_description + .mx_Validation_details { + margin: 1em 0 0; +} + .mx_Validation_detail { position: relative; font-weight: normal; list-style: none; margin-bottom: 0.5em; + &:last-child { + margin-bottom: 0; + } + &::before { content: ""; position: absolute; From 1cbb4be6f7480690b57c2196edd3713584f12225 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 18 Apr 2019 21:33:37 +0100 Subject: [PATCH 09/19] Add support for validating more strictly at submit time When submitting a form, we want to validate more strictly to check for empty values that might be required. A separate mode is used since we want to ignore this issue when visiting a field one by one to enter data. As an example, we convert the pre-existing logic for the username requirement using this new support. --- .../structures/auth/Registration.js | 6 --- src/components/views/auth/RegistrationForm.js | 51 ++++++++++--------- src/components/views/elements/Field.js | 5 +- src/components/views/elements/Validation.js | 10 ++-- src/i18n/strings/en_EN.json | 2 +- 5 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index 0d36e592f8..b19be60fbe 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -344,12 +344,6 @@ module.exports = React.createClass({ case "RegistrationForm.ERR_MISSING_PHONE_NUMBER": errMsg = _t('A phone number is required to register on this homeserver.'); break; - case "RegistrationForm.ERR_USERNAME_INVALID": - errMsg = _t("A username can only contain lower case letters, numbers and '=_-./'"); - break; - case "RegistrationForm.ERR_USERNAME_BLANK": - errMsg = _t('You need to enter a username.'); - break; default: console.error("Unknown error code: %s", errCode); errMsg = _t('An unknown error occurred.'); diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index c680d058c5..1f30af3710 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -94,7 +94,6 @@ module.exports = React.createClass({ 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); const allFieldsValid = this.verifyFieldsBeforeSubmit(); if (!allFieldsValid) { @@ -142,23 +141,38 @@ module.exports = React.createClass({ }, verifyFieldsBeforeSubmit() { - if (this.allFieldsValid()) { - return true; - } - - const invalidField = this.findFirstInvalidField([ + const fieldIDsInDisplayOrder = [ FIELD_USERNAME, FIELD_PASSWORD, FIELD_PASSWORD_CONFIRM, FIELD_EMAIL, FIELD_PHONE_NUMBER, - ]); + ]; + + // Run all fields with stricter validation that no longer allows empty + // values for required fields. + for (const fieldID of fieldIDsInDisplayOrder) { + const field = this[fieldID]; + if (!field) { + continue; + } + field.validate({ allowEmpty: false }); + } + + if (this.allFieldsValid()) { + return true; + } + + const invalidField = this.findFirstInvalidField(fieldIDsInDisplayOrder); if (!invalidField) { return true; } + // Focus the first invalid field and show feedback in the stricter mode + // that no longer allows empty values for required fields. invalidField.focus(); + invalidField.validate({ allowEmpty: false, focused: true }); return false; }, @@ -215,21 +229,6 @@ module.exports = React.createClass({ } else this.markFieldError(fieldID, phoneNumberValid, "RegistrationForm.ERR_PHONE_NUMBER_INVALID"); break; } - case FIELD_USERNAME: { - const username = this.state.username; - if (allowEmpty && username === '') { - this.markFieldError(fieldID, true); - } else if (username == '') { - this.markFieldError( - fieldID, - false, - "RegistrationForm.ERR_USERNAME_BLANK", - ); - } else { - this.markFieldError(fieldID, true); - } - break; - } case FIELD_PASSWORD: if (allowEmpty && pwd1 === "") { this.markFieldError(fieldID, true); @@ -358,9 +357,14 @@ module.exports = React.createClass({ validateUsernameRules: withValidation({ description: () => _t("Use letters, numbers, dashes and underscores only"), rules: [ + { + key: "required", + test: ({ value, allowEmpty }) => allowEmpty || !!value, + invalid: () => _t("Enter username"), + }, { key: "safeLocalpart", - regex: SAFE_LOCALPART_REGEX, + test: ({ value }) => !value || SAFE_LOCALPART_REGEX.test(value), invalid: () => _t("Some characters not allowed"), }, ], @@ -393,7 +397,6 @@ module.exports = React.createClass({ renderUsername() { const Field = sdk.getComponent('elements.Field'); return this[FIELD_USERNAME] = field} type="text" diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index dfe3a51697..6eba832523 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -87,14 +87,15 @@ export default class Field extends React.PureComponent { this.input.focus(); } - validate({ focused }) { + validate({ focused, allowEmpty = true }) { if (!this.props.onValidate) { return; } - const { value } = this.input; + const value = this.input ? this.input.value : null; const { valid, feedback } = this.props.onValidate({ value, focused, + allowEmpty, }); this.setState({ valid, diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index 8fc584edce..8142dfcd65 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -26,7 +26,7 @@ import classNames from 'classnames'; * An array of rules describing how to check to input value. Each rule in an object * and may have the following properties: * - `key`: A unique ID for the rule. Required. - * - `regex`: A regex used to determine the rule's current validity. Required. + * - `test`: A function used to determine the rule's current validity. Required. * - `valid`: Function returning text to show when the rule is valid. Only shown if set. * - `invalid`: Function returning text to show when the rule is invalid. Only shown if set. * @returns {Function} @@ -34,9 +34,9 @@ import classNames from 'classnames'; * the overall validity and a feedback UI that can be rendered for more detail. */ export default function withValidation({ description, rules }) { - return function onValidate({ value, focused }) { + return function onValidate({ value, focused, allowEmpty = true }) { // TODO: Re-run only after ~200ms of inactivity - if (!value) { + if (!value && allowEmpty) { return { valid: null, feedback: null, @@ -47,10 +47,10 @@ export default function withValidation({ description, rules }) { let valid = true; if (rules && rules.length) { for (const rule of rules) { - if (!rule.key || !rule.regex) { + if (!rule.key || !rule.test) { continue; } - const ruleValid = rule.regex.test(value); + const ruleValid = rule.test({ value, allowEmpty }); valid = valid && ruleValid; if (ruleValid && rule.valid) { // If the rule's result is valid and has text to show for diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 51967c39f1..bafb516af7 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1324,6 +1324,7 @@ "If you don't specify an email address, you won't be able to reset your password. Are you sure?": "If you don't specify an email address, you won't be able to reset your password. Are you sure?", "Use letters, numbers, dashes and underscores only": "Use letters, numbers, dashes and underscores only", "Some characters not allowed": "Some characters not allowed", + "Enter username": "Enter username", "Create your Matrix account": "Create your Matrix account", "Create your Matrix account on %(serverName)s": "Create your Matrix account on %(serverName)s", "Email (optional)": "Email (optional)", @@ -1524,7 +1525,6 @@ "This doesn't look like a valid phone number.": "This doesn't look like a valid phone number.", "An email address is required to register on this homeserver.": "An email address is required to register on this homeserver.", "A phone number is required to register on this homeserver.": "A phone number is required to register on this homeserver.", - "You need to enter a username.": "You need to enter a username.", "An unknown error occurred.": "An unknown error occurred.", "Create your account": "Create your account", "Commands": "Commands", From 9064875312cfea5b23073946e89cbdf7e30e07fd Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 18 Apr 2019 22:53:46 +0100 Subject: [PATCH 10/19] Migrate email on registration to new validation --- .../structures/auth/Registration.js | 6 -- src/components/views/auth/RegistrationForm.js | 85 ++++++++++--------- src/components/views/elements/Validation.js | 16 ++-- src/i18n/strings/en_EN.json | 12 +-- 4 files changed, 65 insertions(+), 54 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index b19be60fbe..22dd8c0ea9 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -332,15 +332,9 @@ module.exports = React.createClass({ case "RegistrationForm.ERR_PASSWORD_LENGTH": errMsg = _t('Password too short (min %(MIN_PASSWORD_LENGTH)s).', {MIN_PASSWORD_LENGTH}); break; - case "RegistrationForm.ERR_EMAIL_INVALID": - errMsg = _t('This doesn\'t look like a valid email address.'); - break; case "RegistrationForm.ERR_PHONE_NUMBER_INVALID": errMsg = _t('This doesn\'t look like a valid phone number.'); break; - case "RegistrationForm.ERR_MISSING_EMAIL": - errMsg = _t('An email address is required to register on this homeserver.'); - break; case "RegistrationForm.ERR_MISSING_PHONE_NUMBER": errMsg = _t('A phone number is required to register on this homeserver.'); break; diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 1f30af3710..5f2a9a8dcc 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -91,7 +91,6 @@ module.exports = React.createClass({ // onValidationChange once for each invalid field. // TODO: Remove these calls once converted to new-style validation. 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); @@ -213,14 +212,6 @@ module.exports = React.createClass({ // TODO: Remove rules here as they are converted to new-style validation switch (fieldID) { - case FIELD_EMAIL: { - const email = this.state.email; - const emailValid = email === '' || Email.looksValid(email); - if (this._authStepIsRequired('m.login.email.identity') && (!emailValid || email === '')) { - this.markFieldError(fieldID, false, "RegistrationForm.ERR_MISSING_EMAIL"); - } else this.markFieldError(fieldID, emailValid, "RegistrationForm.ERR_EMAIL_INVALID"); - break; - } case FIELD_PHONE_NUMBER: { const phoneNumber = this.state.phoneNumber; const phoneNumberValid = phoneNumber === '' || phoneNumberLooksValid(phoneNumber); @@ -295,16 +286,36 @@ module.exports = React.createClass({ return cls; }, - onEmailBlur(ev) { - this.validateField(FIELD_EMAIL, ev.type); - }, - onEmailChange(ev) { this.setState({ email: ev.target.value, }); }, + onEmailValidate(fieldState) { + const result = this.validateEmailRules(fieldState); + this.markFieldValid(FIELD_EMAIL, result.valid); + return result; + }, + + validateEmailRules: withValidation({ + description: () => _t("Use an email address to recover your account"), + rules: [ + { + key: "required", + test: function({ value, allowEmpty }) { + return allowEmpty || !this._authStepIsRequired('m.login.email.identity') || !!value; + }, + invalid: () => _t("Enter email address (required on this homeserver)"), + }, + { + key: "email", + test: ({ value }) => !value || Email.looksValid(value), + invalid: () => _t("Doesn't look like a valid email address"), + }, + ], + }), + onPasswordBlur(ev) { this.validateField(FIELD_PASSWORD, ev.type); }, @@ -394,6 +405,26 @@ module.exports = React.createClass({ }); }, + renderEmail() { + if (!this._authStepIsUsed('m.login.email.identity')) { + return null; + } + const Field = sdk.getComponent('elements.Field'); + const emailPlaceholder = this._authStepIsRequired('m.login.email.identity') ? + _t("Email") : + _t("Email (optional)"); + return this[FIELD_EMAIL] = field} + type="text" + label={emailPlaceholder} + defaultValue={this.props.defaultEmail} + value={this.state.email} + onChange={this.onEmailChange} + onValidate={this.onEmailValidate} + />; + }, + renderUsername() { const Field = sdk.getComponent('elements.Field'); return ; } - let emailSection; - if (this._authStepIsUsed('m.login.email.identity')) { - const emailPlaceholder = this._authStepIsRequired('m.login.email.identity') ? - _t("Email") : - _t("Email (optional)"); - - emailSection = ( - - ); - } - const threePidLogin = !SdkConfig.get().disable_3pid_login; const CountryDropdown = sdk.getComponent('views.auth.CountryDropdown'); let phoneSection; @@ -521,13 +532,11 @@ module.exports = React.createClass({ />
- { emailSection } + {this.renderEmail()} { phoneSection }
- {_t( - "Use an email address to recover your account. Other users " + - "can invite you to rooms using your contact details.", - )} + {_t("Use an email address to recover your account.") + " "} + {_t("Other users can invite you to rooms using your contact details.")} { registerButton } diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index 8142dfcd65..458d000e0e 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -50,7 +50,10 @@ export default function withValidation({ description, rules }) { if (!rule.key || !rule.test) { continue; } - const ruleValid = rule.test({ value, allowEmpty }); + // We're setting `this` to whichever component hold the validation + // function. That allows rules to access the state of the component. + // eslint-disable-next-line babel/no-invalid-this + const ruleValid = rule.test.call(this, { value, allowEmpty }); valid = valid && ruleValid; if (ruleValid && rule.valid) { // If the rule's result is valid and has text to show for @@ -101,10 +104,13 @@ export default function withValidation({ description, rules }) { summary =
{description()}
; } - const feedback =
- {summary} - {details} -
; + let feedback; + if (summary || details) { + feedback =
+ {summary} + {details} +
; + } return { valid, diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index bafb516af7..d219b3a19a 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1322,15 +1322,19 @@ "Change": "Change", "Sign in with": "Sign in with", "If you don't specify an email address, you won't be able to reset your password. Are you sure?": "If you don't specify an email address, you won't be able to reset your password. Are you sure?", + "Use an email address to recover your account": "Use an email address to recover your account", + "Enter email address (required on this homeserver)": "Enter email address (required on this homeserver)", + "Doesn't look like a valid email address": "Doesn't look like a valid email address", "Use letters, numbers, dashes and underscores only": "Use letters, numbers, dashes and underscores only", - "Some characters not allowed": "Some characters not allowed", "Enter username": "Enter username", + "Some characters not allowed": "Some characters not allowed", + "Email (optional)": "Email (optional)", "Create your Matrix account": "Create your Matrix account", "Create your Matrix account on %(serverName)s": "Create your Matrix account on %(serverName)s", - "Email (optional)": "Email (optional)", "Phone (optional)": "Phone (optional)", "Confirm": "Confirm", - "Use an email address to recover your account. Other users can invite you to rooms using your contact details.": "Use an email address to recover your account. Other users can invite you to rooms using your contact details.", + "Use an email address to recover your account.": "Use an email address to recover your account.", + "Other users can invite you to rooms using your contact details.": "Other users can invite you to rooms using your contact details.", "Other servers": "Other servers", "Enter custom server URLs What does this mean?": "Enter custom server URLs What does this mean?", "Homeserver URL": "Homeserver URL", @@ -1521,9 +1525,7 @@ "Missing password.": "Missing password.", "Passwords don't match.": "Passwords don't match.", "Password too short (min %(MIN_PASSWORD_LENGTH)s).": "Password too short (min %(MIN_PASSWORD_LENGTH)s).", - "This doesn't look like a valid email address.": "This doesn't look like a valid email address.", "This doesn't look like a valid phone number.": "This doesn't look like a valid phone number.", - "An email address is required to register on this homeserver.": "An email address is required to register on this homeserver.", "A phone number is required to register on this homeserver.": "A phone number is required to register on this homeserver.", "An unknown error occurred.": "An unknown error occurred.", "Create your account": "Create your account", From aaf745ae2a6578d9ca21327dc65925527b4bacf7 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 18 Apr 2019 23:05:55 +0100 Subject: [PATCH 11/19] Migrate phone number on registration to new validation --- .../structures/auth/Registration.js | 6 -- src/components/views/auth/RegistrationForm.js | 95 +++++++++++-------- src/i18n/strings/en_EN.json | 7 +- 3 files changed, 58 insertions(+), 50 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index 22dd8c0ea9..90e7d99bfe 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -332,12 +332,6 @@ module.exports = React.createClass({ case "RegistrationForm.ERR_PASSWORD_LENGTH": errMsg = _t('Password too short (min %(MIN_PASSWORD_LENGTH)s).', {MIN_PASSWORD_LENGTH}); break; - case "RegistrationForm.ERR_PHONE_NUMBER_INVALID": - errMsg = _t('This doesn\'t look like a valid phone number.'); - break; - case "RegistrationForm.ERR_MISSING_PHONE_NUMBER": - errMsg = _t('A phone number is required to register on this homeserver.'); - break; default: console.error("Unknown error code: %s", errCode); errMsg = _t('An unknown error occurred.'); diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 5f2a9a8dcc..a3816829e7 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -90,7 +90,6 @@ module.exports = React.createClass({ // It's not super ideal that this just calls // onValidationChange once for each invalid field. // TODO: Remove these calls once converted to new-style validation. - this.validateField(FIELD_PHONE_NUMBER, ev.type); this.validateField(FIELD_PASSWORD_CONFIRM, ev.type); this.validateField(FIELD_PASSWORD, ev.type); @@ -212,14 +211,6 @@ module.exports = React.createClass({ // TODO: Remove rules here as they are converted to new-style validation switch (fieldID) { - case FIELD_PHONE_NUMBER: { - const phoneNumber = this.state.phoneNumber; - const phoneNumberValid = phoneNumber === '' || phoneNumberLooksValid(phoneNumber); - if (this._authStepIsRequired('m.login.msisdn') && (!phoneNumberValid || phoneNumber === '')) { - this.markFieldError(fieldID, false, "RegistrationForm.ERR_MISSING_PHONE_NUMBER"); - } else this.markFieldError(fieldID, phoneNumberValid, "RegistrationForm.ERR_PHONE_NUMBER_INVALID"); - break; - } case FIELD_PASSWORD: if (allowEmpty && pwd1 === "") { this.markFieldError(fieldID, true); @@ -343,16 +334,36 @@ module.exports = React.createClass({ }); }, - onPhoneNumberBlur(ev) { - this.validateField(FIELD_PHONE_NUMBER, ev.type); - }, - onPhoneNumberChange(ev) { this.setState({ phoneNumber: ev.target.value, }); }, + onPhoneNumberValidate(fieldState) { + const result = this.validatePhoneNumberRules(fieldState); + this.markFieldValid(FIELD_PHONE_NUMBER, result.valid); + return result; + }, + + validatePhoneNumberRules: withValidation({ + description: () => _t("Other users can invite you to rooms using your contact details"), + rules: [ + { + key: "required", + test: function({ value, allowEmpty }) { + return allowEmpty || !this._authStepIsRequired('m.login.msisdn') || !!value; + }, + invalid: () => _t("Enter phone number (required on this homeserver)"), + }, + { + key: "email", + test: ({ value }) => !value || phoneNumberLooksValid(value), + invalid: () => _t("Doesn't look like a valid phone number"), + }, + ], + }), + onUsernameChange(ev) { this.setState({ username: ev.target.value, @@ -425,6 +436,35 @@ module.exports = React.createClass({ />; }, + renderPhoneNumber() { + const threePidLogin = !SdkConfig.get().disable_3pid_login; + if (!threePidLogin || !this._authStepIsUsed('m.login.msisdn')) { + return null; + } + const CountryDropdown = sdk.getComponent('views.auth.CountryDropdown'); + const Field = sdk.getComponent('elements.Field'); + const phoneLabel = this._authStepIsRequired('m.login.msisdn') ? + _t("Phone") : + _t("Phone (optional)"); + const phoneCountry = ; + return this[FIELD_PHONE_NUMBER] = field} + type="text" + label={phoneLabel} + defaultValue={this.props.defaultPhoneNumber} + value={this.state.phoneNumber} + prefix={phoneCountry} + onChange={this.onPhoneNumberChange} + onValidate={this.onPhoneNumberValidate} + />; + }, + renderUsername() { const Field = sdk.getComponent('elements.Field'); return ; } - const threePidLogin = !SdkConfig.get().disable_3pid_login; - const CountryDropdown = sdk.getComponent('views.auth.CountryDropdown'); - let phoneSection; - if (threePidLogin && this._authStepIsUsed('m.login.msisdn')) { - const phoneLabel = this._authStepIsRequired('m.login.msisdn') ? - _t("Phone") : - _t("Phone (optional)"); - const phoneCountry = ; - - phoneSection = ; - } - const registerButton = ( ); @@ -533,7 +546,7 @@ module.exports = React.createClass({
{this.renderEmail()} - { phoneSection } + {this.renderPhoneNumber()}
{_t("Use an email address to recover your account.") + " "} {_t("Other users can invite you to rooms using your contact details.")} diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index d219b3a19a..910fc8f74f 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1325,13 +1325,16 @@ "Use an email address to recover your account": "Use an email address to recover your account", "Enter email address (required on this homeserver)": "Enter email address (required on this homeserver)", "Doesn't look like a valid email address": "Doesn't look like a valid email address", + "Other users can invite you to rooms using your contact details": "Other users can invite you to rooms using your contact details", + "Enter phone number (required on this homeserver)": "Enter phone number (required on this homeserver)", + "Doesn't look like a valid phone number": "Doesn't look like a valid phone number", "Use letters, numbers, dashes and underscores only": "Use letters, numbers, dashes and underscores only", "Enter username": "Enter username", "Some characters not allowed": "Some characters not allowed", "Email (optional)": "Email (optional)", + "Phone (optional)": "Phone (optional)", "Create your Matrix account": "Create your Matrix account", "Create your Matrix account on %(serverName)s": "Create your Matrix account on %(serverName)s", - "Phone (optional)": "Phone (optional)", "Confirm": "Confirm", "Use an email address to recover your account.": "Use an email address to recover your account.", "Other users can invite you to rooms using your contact details.": "Other users can invite you to rooms using your contact details.", @@ -1525,8 +1528,6 @@ "Missing password.": "Missing password.", "Passwords don't match.": "Passwords don't match.", "Password too short (min %(MIN_PASSWORD_LENGTH)s).": "Password too short (min %(MIN_PASSWORD_LENGTH)s).", - "This doesn't look like a valid phone number.": "This doesn't look like a valid phone number.", - "A phone number is required to register on this homeserver.": "A phone number is required to register on this homeserver.", "An unknown error occurred.": "An unknown error occurred.", "Create your account": "Create your account", "Commands": "Commands", From 008ca3543bdfe6afefd4d37331c1b193718ce5ab Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 18 Apr 2019 23:29:05 +0100 Subject: [PATCH 12/19] Migrate passwords on registration to new validation In addition to migrating password fields, this also removes the remaining support for old-style validation in registration now that all checks have been converted. --- .../structures/auth/Registration.js | 35 ---- src/components/views/auth/RegistrationForm.js | 191 +++++++----------- src/components/views/elements/Validation.js | 7 +- src/i18n/strings/en_EN.json | 8 +- 4 files changed, 85 insertions(+), 156 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index 90e7d99bfe..87fea3ec4b 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -308,40 +308,6 @@ module.exports = React.createClass({ }); }, - onFormValidationChange: function(fieldErrors) { - // `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, - }); - return; - } - - let errMsg; - switch (errCode) { - case "RegistrationForm.ERR_PASSWORD_MISSING": - errMsg = _t('Missing password.'); - break; - case "RegistrationForm.ERR_PASSWORD_MISMATCH": - errMsg = _t('Passwords don\'t match.'); - break; - case "RegistrationForm.ERR_PASSWORD_LENGTH": - errMsg = _t('Password too short (min %(MIN_PASSWORD_LENGTH)s).', {MIN_PASSWORD_LENGTH}); - break; - default: - console.error("Unknown error code: %s", errCode); - errMsg = _t('An unknown error occurred.'); - break; - } - this.setState({ - errorText: errMsg, - }); - }, - onLoginClick: function(ev) { ev.preventDefault(); ev.stopPropagation(); @@ -517,7 +483,6 @@ module.exports = React.createClass({ defaultPhoneNumber={this.state.formVals.phoneNumber} defaultPassword={this.state.formVals.password} minPasswordLength={MIN_PASSWORD_LENGTH} - 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 a3816829e7..a7d7efee0c 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -47,7 +47,6 @@ module.exports = React.createClass({ defaultUsername: PropTypes.string, defaultPassword: PropTypes.string, minPasswordLength: PropTypes.number, - onValidationChange: PropTypes.func, onRegisterClick: PropTypes.func.isRequired, // onRegisterClick(Object) => ?Promise onEditServerDetailsClick: PropTypes.func, flows: PropTypes.arrayOf(PropTypes.object).isRequired, @@ -68,8 +67,6 @@ module.exports = React.createClass({ getInitialState: function() { return { // Field error codes by field ID - // TODO: Remove `fieldErrors` once converted to new-style validation - fieldErrors: {}, fieldValid: {}, // The ISO2 country code selected in the phone number entry phoneCountry: this.props.defaultPhoneCountry, @@ -84,15 +81,6 @@ module.exports = React.createClass({ onSubmit: function(ev) { ev.preventDefault(); - // validate everything, in reverse order so - // the error that ends up being displayed - // is the one from the first invalid field. - // It's not super ideal that this just calls - // onValidationChange once for each invalid field. - // TODO: Remove these calls once converted to new-style validation. - this.validateField(FIELD_PASSWORD_CONFIRM, ev.type); - this.validateField(FIELD_PASSWORD, ev.type); - const allFieldsValid = this.verifyFieldsBeforeSubmit(); if (!allFieldsValid) { return; @@ -178,14 +166,7 @@ module.exports = React.createClass({ * @returns {boolean} true if all fields were valid last time they were validated. */ allFieldsValid: function() { - // TODO: Remove `fieldErrors` here when all fields converted - let keys = Object.keys(this.state.fieldErrors); - for (let i = 0; i < keys.length; ++i) { - if (this.state.fieldErrors[keys[i]]) { - return false; - } - } - keys = Object.keys(this.state.fieldValid); + const keys = Object.keys(this.state.fieldValid); for (let i = 0; i < keys.length; ++i) { if (!this.state.fieldValid[keys[i]]) { return false; @@ -203,62 +184,6 @@ module.exports = React.createClass({ return null; }, - validateField: function(fieldID, eventType) { - const pwd1 = this.state.password.trim(); - const pwd2 = this.state.passwordConfirm.trim(); - const allowEmpty = eventType === "blur"; - - // TODO: Remove rules here as they are converted to new-style validation - - switch (fieldID) { - case FIELD_PASSWORD: - if (allowEmpty && pwd1 === "") { - this.markFieldError(fieldID, true); - } else if (pwd1 == '') { - this.markFieldError( - fieldID, - false, - "RegistrationForm.ERR_PASSWORD_MISSING", - ); - } else if (pwd1.length < this.props.minPasswordLength) { - this.markFieldError( - fieldID, - false, - "RegistrationForm.ERR_PASSWORD_LENGTH", - ); - } else { - this.markFieldError(fieldID, true); - } - break; - case FIELD_PASSWORD_CONFIRM: - if (allowEmpty && pwd2 === "") { - this.markFieldError(fieldID, true); - } else { - this.markFieldError( - fieldID, pwd1 == pwd2, - "RegistrationForm.ERR_PASSWORD_MISMATCH", - ); - } - break; - } - }, - - markFieldError: function(fieldID, valid, errorCode) { - // TODO: Remove this function once all fields converted to new-style validation. - const { fieldErrors } = this.state; - if (valid) { - fieldErrors[fieldID] = null; - } else { - fieldErrors[fieldID] = errorCode; - } - this.setState({ - fieldErrors, - }); - // TODO: Remove outer validation handling once all fields converted to new-style - // validation in the form. - this.props.onValidationChange(fieldErrors); - }, - markFieldValid: function(fieldID, valid) { const { fieldValid } = this.state; fieldValid[fieldID] = valid; @@ -267,16 +192,6 @@ module.exports = React.createClass({ }); }, - _classForField: function(fieldID, ...baseClasses) { - let cls = baseClasses.join(' '); - // TODO: Remove this from fields as they are converted to new-style validation. - if (this.state.fieldErrors[fieldID]) { - if (cls) cls += ' '; - cls += 'error'; - } - return cls; - }, - onEmailChange(ev) { this.setState({ email: ev.target.value, @@ -307,26 +222,66 @@ module.exports = React.createClass({ ], }), - onPasswordBlur(ev) { - this.validateField(FIELD_PASSWORD, ev.type); - }, - onPasswordChange(ev) { this.setState({ password: ev.target.value, }); }, - onPasswordConfirmBlur(ev) { - this.validateField(FIELD_PASSWORD_CONFIRM, ev.type); + onPasswordValidate(fieldState) { + const result = this.validatePasswordRules(fieldState); + this.markFieldValid(FIELD_PASSWORD, result.valid); + return result; }, + validatePasswordRules: withValidation({ + rules: [ + { + key: "required", + test: ({ value, allowEmpty }) => allowEmpty || !!value, + invalid: () => _t("Enter password"), + }, + { + key: "minLength", + test: function({ value }) { + return !value || value.length >= this.props.minPasswordLength; + }, + invalid: function() { + return _t("Too short (min %(length)s)", { length: this.props.minPasswordLength }); + }, + }, + ], + }), + onPasswordConfirmChange(ev) { this.setState({ passwordConfirm: ev.target.value, }); }, + onPasswordConfirmValidate(fieldState) { + const result = this.validatePasswordConfirmRules(fieldState); + this.markFieldValid(FIELD_PASSWORD_CONFIRM, result.valid); + return result; + }, + + validatePasswordConfirmRules: withValidation({ + rules: [ + { + key: "required", + test: ({ value, allowEmpty }) => allowEmpty || !!value, + invalid: () => _t("Confirm password"), + }, + { + key: "match", + test: function({ value }) { + return !value || value === this.state.password; + }, + invalid: () => _t("Passwords don't match"), + }, + ], + }), + onPhoneCountryChange(newVal) { this.setState({ phoneCountry: newVal.iso2, @@ -436,6 +391,34 @@ module.exports = React.createClass({ />; }, + renderPassword() { + const Field = sdk.getComponent('elements.Field'); + return this[FIELD_PASSWORD] = field} + type="password" + label={_t("Password")} + defaultValue={this.props.defaultPassword} + value={this.state.password} + onChange={this.onPasswordChange} + onValidate={this.onPasswordValidate} + />; + }, + + renderPasswordConfirm() { + const Field = sdk.getComponent('elements.Field'); + return this[FIELD_PASSWORD_CONFIRM] = field} + type="password" + label={_t("Confirm")} + defaultValue={this.props.defaultPassword} + value={this.state.passwordConfirm} + onChange={this.onPasswordConfirmChange} + onValidate={this.onPasswordConfirmValidate} + />; + }, + renderPhoneNumber() { const threePidLogin = !SdkConfig.get().disable_3pid_login; if (!threePidLogin || !this._authStepIsUsed('m.login.msisdn')) { @@ -481,8 +464,6 @@ module.exports = React.createClass({ }, render: function() { - const Field = sdk.getComponent('elements.Field'); - let yourMatrixAccountText = _t('Create your Matrix account'); if (this.props.hsName) { yourMatrixAccountText = _t('Create your Matrix account on %(serverName)s', { @@ -523,26 +504,8 @@ module.exports = React.createClass({ {this.renderUsername()}
- - + {this.renderPassword()} + {this.renderPasswordConfirm()}
{this.renderEmail()} diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index 458d000e0e..b567eb4058 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -52,7 +52,7 @@ export default function withValidation({ description, rules }) { } // We're setting `this` to whichever component hold the validation // function. That allows rules to access the state of the component. - // eslint-disable-next-line babel/no-invalid-this + /* eslint-disable babel/no-invalid-this */ const ruleValid = rule.test.call(this, { value, allowEmpty }); valid = valid && ruleValid; if (ruleValid && rule.valid) { @@ -61,7 +61,7 @@ export default function withValidation({ description, rules }) { results.push({ key: rule.key, valid: true, - text: rule.valid(), + text: rule.valid.call(this), }); } else if (!ruleValid && rule.invalid) { // If the rule's result is invalid and has text to show for @@ -69,9 +69,10 @@ export default function withValidation({ description, rules }) { results.push({ key: rule.key, valid: false, - text: rule.invalid(), + text: rule.invalid.call(this), }); } + /* eslint-enable babel/no-invalid-this */ } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 910fc8f74f..5fd97a4305 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1325,6 +1325,9 @@ "Use an email address to recover your account": "Use an email address to recover your account", "Enter email address (required on this homeserver)": "Enter email address (required on this homeserver)", "Doesn't look like a valid email address": "Doesn't look like a valid email address", + "Enter password": "Enter password", + "Too short (min %(length)s)": "Too short (min %(length)s)", + "Passwords don't match": "Passwords don't match", "Other users can invite you to rooms using your contact details": "Other users can invite you to rooms using your contact details", "Enter phone number (required on this homeserver)": "Enter phone number (required on this homeserver)", "Doesn't look like a valid phone number": "Doesn't look like a valid phone number", @@ -1332,10 +1335,10 @@ "Enter username": "Enter username", "Some characters not allowed": "Some characters not allowed", "Email (optional)": "Email (optional)", + "Confirm": "Confirm", "Phone (optional)": "Phone (optional)", "Create your Matrix account": "Create your Matrix account", "Create your Matrix account on %(serverName)s": "Create your Matrix account on %(serverName)s", - "Confirm": "Confirm", "Use an email address to recover your account.": "Use an email address to recover your account.", "Other users can invite you to rooms using your contact details.": "Other users can invite you to rooms using your contact details.", "Other servers": "Other servers", @@ -1525,9 +1528,6 @@ "Registration has been disabled on this homeserver.": "Registration has been disabled on this homeserver.", "Unable to query for supported registration methods.": "Unable to query for supported registration methods.", "This server does not support authentication with a phone number.": "This server does not support authentication with a phone number.", - "Missing password.": "Missing password.", - "Passwords don't match.": "Passwords don't match.", - "Password too short (min %(MIN_PASSWORD_LENGTH)s).": "Password too short (min %(MIN_PASSWORD_LENGTH)s).", "An unknown error occurred.": "An unknown error occurred.", "Create your account": "Create your account", "Commands": "Commands", From 4f41161a474f35f244e27af24f16314608683035 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 23 Apr 2019 14:55:57 +0100 Subject: [PATCH 13/19] Check password complexity during registration This adds a password complexity rule during registration to require strong passwords. This is based on the `zxcvbn` module that we already use for key backup passphrases. In addition, this also tweaks validation more generally to allow rules to be async functions. --- src/components/views/auth/RegistrationForm.js | 64 +++++++++++++++---- src/components/views/elements/Field.js | 4 +- src/components/views/elements/Validation.js | 27 +++++--- src/i18n/strings/en_EN.json | 4 +- src/utils/PasswordScorer.js | 4 +- 5 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index a7d7efee0c..2ba01dc0c6 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -33,6 +33,8 @@ const FIELD_USERNAME = 'field_username'; const FIELD_PASSWORD = 'field_password'; const FIELD_PASSWORD_CONFIRM = 'field_password_confirm'; +const PASSWORD_MIN_SCORE = 4; // So secure, many characters, much complex, wow, etc, etc. + /** * A pure UI component which displays a registration form. */ @@ -75,13 +77,14 @@ module.exports = React.createClass({ phoneNumber: "", password: "", passwordConfirm: "", + passwordComplexity: null, }; }, - onSubmit: function(ev) { + onSubmit: async function(ev) { ev.preventDefault(); - const allFieldsValid = this.verifyFieldsBeforeSubmit(); + const allFieldsValid = await this.verifyFieldsBeforeSubmit(); if (!allFieldsValid) { return; } @@ -126,7 +129,7 @@ module.exports = React.createClass({ } }, - verifyFieldsBeforeSubmit() { + async verifyFieldsBeforeSubmit() { const fieldIDsInDisplayOrder = [ FIELD_USERNAME, FIELD_PASSWORD, @@ -145,6 +148,10 @@ module.exports = React.createClass({ field.validate({ allowEmpty: false }); } + // Validation and state updates are async, so we need to wait for them to complete + // first. Queue a `setState` callback and wait for it to resolve. + await new Promise(resolve => this.setState({}, resolve)); + if (this.allFieldsValid()) { return true; } @@ -198,8 +205,8 @@ module.exports = React.createClass({ }); }, - onEmailValidate(fieldState) { - const result = this.validateEmailRules(fieldState); + async onEmailValidate(fieldState) { + const result = await this.validateEmailRules(fieldState); this.markFieldValid(FIELD_EMAIL, result.valid); return result; }, @@ -228,13 +235,21 @@ module.exports = React.createClass({ }); }, - onPasswordValidate(fieldState) { - const result = this.validatePasswordRules(fieldState); + async onPasswordValidate(fieldState) { + const result = await this.validatePasswordRules(fieldState); this.markFieldValid(FIELD_PASSWORD, result.valid); return result; }, validatePasswordRules: withValidation({ + description: function() { + const complexity = this.state.passwordComplexity; + const score = complexity ? complexity.score : 0; + return ; + }, rules: [ { key: "required", @@ -250,6 +265,29 @@ module.exports = React.createClass({ return _t("Too short (min %(length)s)", { length: this.props.minPasswordLength }); }, }, + { + key: "complexity", + test: async function({ value }) { + if (!value) { + return false; + } + const { scorePassword } = await import('../../../utils/PasswordScorer'); + const complexity = scorePassword(value); + this.setState({ + passwordComplexity: complexity, + }); + return complexity.score >= PASSWORD_MIN_SCORE; + }, + valid: () => _t("Nice, strong password!"), + invalid: function() { + const complexity = this.state.passwordComplexity; + if (!complexity) { + return null; + } + const { feedback } = complexity; + return feedback.warning || feedback.suggestions[0] || _t("Keep going..."); + }, + }, ], }), @@ -259,8 +297,8 @@ module.exports = React.createClass({ }); }, - onPasswordConfirmValidate(fieldState) { - const result = this.validatePasswordConfirmRules(fieldState); + async onPasswordConfirmValidate(fieldState) { + const result = await this.validatePasswordConfirmRules(fieldState); this.markFieldValid(FIELD_PASSWORD_CONFIRM, result.valid); return result; }, @@ -295,8 +333,8 @@ module.exports = React.createClass({ }); }, - onPhoneNumberValidate(fieldState) { - const result = this.validatePhoneNumberRules(fieldState); + async onPhoneNumberValidate(fieldState) { + const result = await this.validatePhoneNumberRules(fieldState); this.markFieldValid(FIELD_PHONE_NUMBER, result.valid); return result; }, @@ -325,8 +363,8 @@ module.exports = React.createClass({ }); }, - onUsernameValidate(fieldState) { - const result = this.validateUsernameRules(fieldState); + async onUsernameValidate(fieldState) { + const result = await this.validateUsernameRules(fieldState); this.markFieldValid(FIELD_USERNAME, result.valid); return result; }, diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 6eba832523..8f0746dff2 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -87,12 +87,12 @@ export default class Field extends React.PureComponent { this.input.focus(); } - validate({ focused, allowEmpty = true }) { + async validate({ focused, allowEmpty = true }) { if (!this.props.onValidate) { return; } const value = this.input ? this.input.value : null; - const { valid, feedback } = this.props.onValidate({ + const { valid, feedback } = await this.props.onValidate({ value, focused, allowEmpty, diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index b567eb4058..c37970e534 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and limitations under the License. */ +/* eslint-disable babel/no-invalid-this */ + import classNames from 'classnames'; /** @@ -34,7 +36,7 @@ import classNames from 'classnames'; * the overall validity and a feedback UI that can be rendered for more detail. */ export default function withValidation({ description, rules }) { - return function onValidate({ value, focused, allowEmpty = true }) { + return async function onValidate({ value, focused, allowEmpty = true }) { // TODO: Re-run only after ~200ms of inactivity if (!value && allowEmpty) { return { @@ -50,29 +52,35 @@ export default function withValidation({ description, rules }) { if (!rule.key || !rule.test) { continue; } - // We're setting `this` to whichever component hold the validation + // We're setting `this` to whichever component holds the validation // function. That allows rules to access the state of the component. - /* eslint-disable babel/no-invalid-this */ - const ruleValid = rule.test.call(this, { value, allowEmpty }); + const ruleValid = await rule.test.call(this, { value, allowEmpty }); valid = valid && ruleValid; if (ruleValid && rule.valid) { // If the rule's result is valid and has text to show for // the valid state, show it. + const text = rule.valid.call(this); + if (!text) { + continue; + } results.push({ key: rule.key, valid: true, - text: rule.valid.call(this), + text, }); } else if (!ruleValid && rule.invalid) { // If the rule's result is invalid and has text to show for // the invalid state, show it. + const text = rule.invalid.call(this); + if (!text) { + continue; + } results.push({ key: rule.key, valid: false, - text: rule.invalid.call(this), + text, }); } - /* eslint-enable babel/no-invalid-this */ } } @@ -102,7 +110,10 @@ export default function withValidation({ description, rules }) { let summary; if (description) { - summary =
{description()}
; + // We're setting `this` to whichever component holds the validation + // function. That allows rules to access the state of the component. + const content = description.call(this); + summary =
{content}
; } let feedback; diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 5fd97a4305..262e143ae6 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1327,6 +1327,8 @@ "Doesn't look like a valid email address": "Doesn't look like a valid email address", "Enter password": "Enter password", "Too short (min %(length)s)": "Too short (min %(length)s)", + "Nice, strong password!": "Nice, strong password!", + "Keep going...": "Keep going...", "Passwords don't match": "Passwords don't match", "Other users can invite you to rooms using your contact details": "Other users can invite you to rooms using your contact details", "Enter phone number (required on this homeserver)": "Enter phone number (required on this homeserver)", @@ -1528,7 +1530,6 @@ "Registration has been disabled on this homeserver.": "Registration has been disabled on this homeserver.", "Unable to query for supported registration methods.": "Unable to query for supported registration methods.", "This server does not support authentication with a phone number.": "This server does not support authentication with a phone number.", - "An unknown error occurred.": "An unknown error occurred.", "Create your account": "Create your account", "Commands": "Commands", "Results from DuckDuckGo": "Results from DuckDuckGo", @@ -1567,7 +1568,6 @@ "File to import": "File to import", "Import": "Import", "Great! This passphrase looks strong enough.": "Great! This passphrase looks strong enough.", - "Keep going...": "Keep going...", "We'll store an encrypted copy of your keys on our server. Protect your backup with a passphrase to keep it secure.": "We'll store an encrypted copy of your keys on our server. Protect your backup with a passphrase to keep it secure.", "For maximum security, this should be different from your account password.": "For maximum security, this should be different from your account password.", "Enter a passphrase...": "Enter a passphrase...", diff --git a/src/utils/PasswordScorer.js b/src/utils/PasswordScorer.js index 647436c131..3c366a73f8 100644 --- a/src/utils/PasswordScorer.js +++ b/src/utils/PasswordScorer.js @@ -67,7 +67,9 @@ export function scorePassword(password) { if (password.length === 0) return null; const userInputs = ZXCVBN_USER_INPUTS.slice(); - userInputs.push(MatrixClientPeg.get().getUserIdLocalpart()); + if (MatrixClientPeg.get()) { + userInputs.push(MatrixClientPeg.get().getUserIdLocalpart()); + } let zxcvbnResult = zxcvbn(password, userInputs); // Work around https://github.com/dropbox/zxcvbn/issues/216 From a20d23daf317786728e71793411abdd8ce649e8a Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 23 Apr 2019 15:05:03 +0100 Subject: [PATCH 14/19] Remove older password length check Now that we have a fancier password complexity check, remove the older minimum length to avoid the feeling of two password style guides fighting each other. --- src/components/structures/auth/Registration.js | 3 --- src/components/views/auth/RegistrationForm.js | 11 ----------- src/i18n/strings/en_EN.json | 1 - 3 files changed, 15 deletions(-) diff --git a/src/components/structures/auth/Registration.js b/src/components/structures/auth/Registration.js index 87fea3ec4b..df87c1b9ca 100644 --- a/src/components/structures/auth/Registration.js +++ b/src/components/structures/auth/Registration.js @@ -28,8 +28,6 @@ import SdkConfig from '../../../SdkConfig'; import { messageForResourceLimitError } from '../../../utils/ErrorUtils'; import * as ServerType from '../../views/auth/ServerTypeSelector'; -const MIN_PASSWORD_LENGTH = 6; - // Phases // Show controls to configure server details const PHASE_SERVER_DETAILS = 0; @@ -482,7 +480,6 @@ module.exports = React.createClass({ defaultPhoneCountry={this.state.formVals.phoneCountry} defaultPhoneNumber={this.state.formVals.phoneNumber} defaultPassword={this.state.formVals.password} - minPasswordLength={MIN_PASSWORD_LENGTH} 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 2ba01dc0c6..21c2743301 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -48,7 +48,6 @@ module.exports = React.createClass({ defaultPhoneNumber: PropTypes.string, defaultUsername: PropTypes.string, defaultPassword: PropTypes.string, - minPasswordLength: PropTypes.number, onRegisterClick: PropTypes.func.isRequired, // onRegisterClick(Object) => ?Promise onEditServerDetailsClick: PropTypes.func, flows: PropTypes.arrayOf(PropTypes.object).isRequired, @@ -61,7 +60,6 @@ module.exports = React.createClass({ getDefaultProps: function() { return { - minPasswordLength: 6, onValidationChange: console.error, }; }, @@ -256,15 +254,6 @@ module.exports = React.createClass({ test: ({ value, allowEmpty }) => allowEmpty || !!value, invalid: () => _t("Enter password"), }, - { - key: "minLength", - test: function({ value }) { - return !value || value.length >= this.props.minPasswordLength; - }, - invalid: function() { - return _t("Too short (min %(length)s)", { length: this.props.minPasswordLength }); - }, - }, { key: "complexity", test: async function({ value }) { diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 262e143ae6..d1ff8b2695 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1326,7 +1326,6 @@ "Enter email address (required on this homeserver)": "Enter email address (required on this homeserver)", "Doesn't look like a valid email address": "Doesn't look like a valid email address", "Enter password": "Enter password", - "Too short (min %(length)s)": "Too short (min %(length)s)", "Nice, strong password!": "Nice, strong password!", "Keep going...": "Keep going...", "Passwords don't match": "Passwords don't match", From 67d7091dcdbdb51457aebaaca6b3f9b909f6c54b Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 23 Apr 2019 16:35:11 +0100 Subject: [PATCH 15/19] Password score progress should be full width in tooltip --- res/css/views/auth/_AuthBody.scss | 4 ++++ res/css/views/elements/_Field.scss | 1 + src/components/views/auth/RegistrationForm.js | 1 + 3 files changed, 6 insertions(+) diff --git a/res/css/views/auth/_AuthBody.scss b/res/css/views/auth/_AuthBody.scss index fa034095b6..ce597f1416 100644 --- a/res/css/views/auth/_AuthBody.scss +++ b/res/css/views/auth/_AuthBody.scss @@ -130,3 +130,7 @@ limitations under the License. .mx_AuthBody_spinner { margin: 1em 0; } + +.mx_AuthBody_passwordScore { + width: 100%; +} diff --git a/res/css/views/elements/_Field.scss b/res/css/views/elements/_Field.scss index 20b1efd28b..147bb3b471 100644 --- a/res/css/views/elements/_Field.scss +++ b/res/css/views/elements/_Field.scss @@ -168,6 +168,7 @@ limitations under the License. .mx_Field_tooltip { margin-top: -12px; margin-left: 4px; + width: 200px; } .mx_Field_tooltip.mx_Field_valid { diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 21c2743301..0045b4dd59 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -244,6 +244,7 @@ module.exports = React.createClass({ const complexity = this.state.passwordComplexity; const score = complexity ? complexity.score : 0; return ; From aec14e64facc3e172e60af1cba18b075bf52a6af Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 23 Apr 2019 17:59:19 +0100 Subject: [PATCH 16/19] Throttle validation in response to user input This avoids the case of the password complexity progress jumping wildly for every character you type. --- src/components/views/elements/Field.js | 14 +++++++++++--- src/components/views/elements/Validation.js | 1 - 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 8f0746dff2..91447a8846 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -18,6 +18,10 @@ import React from 'react'; import PropTypes from 'prop-types'; import classNames from 'classnames'; import sdk from '../../../index'; +import { throttle } from 'lodash'; + +// Invoke validation from user input (when typing, etc.) at most once every N ms. +const VALIDATION_THROTTLE_MS = 200; export default class Field extends React.PureComponent { static propTypes = { @@ -64,9 +68,7 @@ export default class Field extends React.PureComponent { }; onChange = (ev) => { - this.validate({ - focused: true, - }); + this.validateOnChange(); // Parent component may have supplied its own `onChange` as well if (this.props.onChange) { this.props.onChange(ev); @@ -103,6 +105,12 @@ export default class Field extends React.PureComponent { }); } + validateOnChange = throttle(() => { + this.validate({ + focused: true, + }); + }, VALIDATION_THROTTLE_MS); + render() { const { element, prefix, onValidate, children, ...inputProps } = this.props; diff --git a/src/components/views/elements/Validation.js b/src/components/views/elements/Validation.js index c37970e534..31363b87c8 100644 --- a/src/components/views/elements/Validation.js +++ b/src/components/views/elements/Validation.js @@ -37,7 +37,6 @@ import classNames from 'classnames'; */ export default function withValidation({ description, rules }) { return async function onValidate({ value, focused, allowEmpty = true }) { - // TODO: Re-run only after ~200ms of inactivity if (!value && allowEmpty) { return { valid: null, From 0b42ded007691f5b15dcb40c9644d5f3cdacd487 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 24 Apr 2019 09:42:52 +0100 Subject: [PATCH 17/19] Style complexity progress bars more heavily This disables the native progress appearance and uses the green color from our themes. --- res/css/views/auth/_AuthBody.scss | 20 ++++++++++++++++++++ res/css/views/elements/_Validation.scss | 4 ++++ 2 files changed, 24 insertions(+) diff --git a/res/css/views/auth/_AuthBody.scss b/res/css/views/auth/_AuthBody.scss index ce597f1416..16ac876869 100644 --- a/res/css/views/auth/_AuthBody.scss +++ b/res/css/views/auth/_AuthBody.scss @@ -133,4 +133,24 @@ limitations under the License. .mx_AuthBody_passwordScore { width: 100%; + appearance: none; + height: 4px; + border: 0; + border-radius: 2px; + position: absolute; + top: -12px; + + &::-moz-progress-bar { + border-radius: 2px; + background-color: $accent-color; + } + + &::-webkit-progress-bar, + &::-webkit-progress-value { + border-radius: 2px; + } + + &::-webkit-progress-value { + background-color: $accent-color; + } } diff --git a/res/css/views/elements/_Validation.scss b/res/css/views/elements/_Validation.scss index 4c059f9747..1f9bd880e6 100644 --- a/res/css/views/elements/_Validation.scss +++ b/res/css/views/elements/_Validation.scss @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ +.mx_Validation { + position: relative; +} + .mx_Validation_details { padding-left: 20px; margin: 0; From 26f732723ebbfc749dc0cc08ecde243a730df58e Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 25 Apr 2019 11:10:35 +0100 Subject: [PATCH 18/19] Animate tooltips when hiding as well as showing This uses the same animation style as on show, but twice as fast. --- res/css/views/elements/_Tooltip.scss | 9 ++++++++- src/components/views/elements/Field.js | 22 ++++++++++++++++++---- src/components/views/elements/Tooltip.js | 15 ++++++++++++++- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/res/css/views/elements/_Tooltip.scss b/res/css/views/elements/_Tooltip.scss index 2f35bd338e..43ddf6dde5 100644 --- a/res/css/views/elements/_Tooltip.scss +++ b/res/css/views/elements/_Tooltip.scss @@ -50,7 +50,6 @@ limitations under the License. .mx_Tooltip { display: none; - animation: mx_fadein 0.2s; position: fixed; border: 1px solid $menu-border-color; border-radius: 4px; @@ -66,4 +65,12 @@ limitations under the License. max-width: 200px; word-break: break-word; margin-right: 50px; + + &.mx_Tooltip_visible { + animation: mx_fadein 0.2s forwards; + } + + &.mx_Tooltip_invisible { + animation: mx_fadeout 0.1s forwards; + } } diff --git a/src/components/views/elements/Field.js b/src/components/views/elements/Field.js index 91447a8846..93bea70fc8 100644 --- a/src/components/views/elements/Field.js +++ b/src/components/views/elements/Field.js @@ -99,10 +99,23 @@ export default class Field extends React.PureComponent { focused, allowEmpty, }); - this.setState({ - valid, - feedback, - }); + + if (feedback) { + this.setState({ + valid, + feedback, + feedbackVisible: true, + }); + } else { + // When we receive null `feedback`, we want to hide the tooltip. + // We leave the previous `feedback` content in state without updating it, + // so that we can hide the tooltip containing the most recent feedback + // via CSS animation. + this.setState({ + valid, + feedbackVisible: false, + }); + } } validateOnChange = throttle(() => { @@ -147,6 +160,7 @@ export default class Field extends React.PureComponent { if (this.state.feedback) { tooltip = ; } diff --git a/src/components/views/elements/Tooltip.js b/src/components/views/elements/Tooltip.js index 473aeb3bdc..1cc82978ed 100644 --- a/src/components/views/elements/Tooltip.js +++ b/src/components/views/elements/Tooltip.js @@ -31,10 +31,20 @@ module.exports = React.createClass({ className: React.PropTypes.string, // Class applied to the tooltip itself tooltipClassName: React.PropTypes.string, + // Whether the tooltip is visible or hidden. + // The hidden state allows animating the tooltip away via CSS. + // Defaults to visible if unset. + visible: React.PropTypes.bool, // the react element to put into the tooltip label: React.PropTypes.node, }, + getDefaultProps() { + return { + visible: true, + }; + }, + // Create a wrapper for the tooltip outside the parent and attach it to the body element componentDidMount: function() { this.tooltipContainer = document.createElement("div"); @@ -85,7 +95,10 @@ module.exports = React.createClass({ style = this._updatePosition(style); style.display = "block"; - const tooltipClasses = classNames("mx_Tooltip", this.props.tooltipClassName); + const tooltipClasses = classNames("mx_Tooltip", this.props.tooltipClassName, { + "mx_Tooltip_visible": this.props.visible, + "mx_Tooltip_invisible": !this.props.visible, + }); const tooltip = (
From af178292290337c8b7cdedac3a24d02b1670a646 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 25 Apr 2019 11:27:03 +0100 Subject: [PATCH 19/19] Blur active field before submit validation --- src/components/views/auth/RegistrationForm.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/components/views/auth/RegistrationForm.js b/src/components/views/auth/RegistrationForm.js index 0045b4dd59..6e55581af0 100644 --- a/src/components/views/auth/RegistrationForm.js +++ b/src/components/views/auth/RegistrationForm.js @@ -128,6 +128,13 @@ module.exports = React.createClass({ }, async verifyFieldsBeforeSubmit() { + // Blur the active element if any, so we first run its blur validation, + // which is less strict than the pass we're about to do below for all fields. + const activeElement = document.activeElement; + if (activeElement) { + activeElement.blur(); + } + const fieldIDsInDisplayOrder = [ FIELD_USERNAME, FIELD_PASSWORD,