-
-
Notifications
You must be signed in to change notification settings - Fork 585
XWIKI-20630: Improve usability of the "Change Password" page #4188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
* Updated the error message so that it's easier to understand that it's asynchronous (contrarily to live validation which happens as the user fills the form up) * Updated the error message creation to use a standard velocimacro. * Removed an unecessary line of information. This info is now always shown to users and handled by the live validation script.
@@ -677,7 +677,7 @@ platform.core.profile.passwd.reenterPassword=Reenter password | |||
platform.core.profile.passwd.submit=Save | |||
platform.core.profile.passwd.cancel=Cancel and return to profile | |||
platform.core.profile.passwd.passwordMissmatch=The two passwords do not match. | |||
platform.core.profile.passwd.invalidOriginalPassword=Current password is invalid. | |||
platform.core.profile.passwd.invalidOriginalPassword=Current password was invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure about this change, AFAICS we never use past tense for validating stuff. Check above "the two passwords do not match" etc. Feels like it would be more consistent to keep present tense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other validations are done live (or at least now there's livevalidation blocking the submission of the form until all rules are respected, so we shouldn't get any other error unless the user bypasses livevalidation somehow :) ).
This validation happens when the user submits the form, and the password field gets emptied out. At least to me, it was weird to have the same tense to talk about two different things:
- Livevalidation uses the present tense to say
Your new password must have at least 6 characters
--> the "new password" is the one currently filled up by the user. - The warning uses the present tense to say
Your old password is invalid.
--> the "old password" is the one that was filled up before the last form submission
Here, I submit the password change request for the first time with an invalid password:
Here, I filled up the whole form a second time afterwards.
This warning box is the only part of the UI that doesn't adapt when I fill the form back up and even for me knowing how this all works, it was a bit confusing, so I figured out a non technical user experience would probably be quite bad. Using the past tense is a way to clearly tell the user that we're not talking about what they currently have in the field. That's the minimal effort solution, a better solution would probably be to hide this message once the user starts filling the password field again. AFAIK, we don't want livevalidation on existing passwords, for security reasons :)
What solution do you think we should go for? This is not the main problem highlighted in the ticket description, but I figured out we might as well improve things here in this PR. If we want to discuss it further, I think we should move this change to another ticket :)
PS: The wording "Current password" is also a bit confusing. After the first form submission, "Current password" can be two things:
- The old password if the changepassword request has failed
- The new password if the changepassword request has succeeded
This might also be part of why I was a bit confused at first with this warning message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying, the UX of this form is a bit terrible...
So yeah I understand why you want to have the past tense: users might read the message only "too late" after filling again the form basically. So ok if you feel it's less confusing, let's keep your change.
PS: The wording "Current password" is also a bit confusing.
Well a simple improvment we could do is to change it for Current password field was invalid.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah I understand why you want to have the past tense: users might read the message only "too late" after filling again the form basically. So ok if you feel it's less confusing, let's keep your change.
When coming back to this with a fresh mind and reading your message, I figured out that moving the error message to the start of the form could also help. AFAIK, we usually put non-live validation errors before the form fields.
Let me know if you want me to move it around before a merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, we usually put non-live validation errors before the form fields.
Let me know if you want me to move it around before a merge :)
Yes it would be better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what it looks like now:
Thanks for your feedback :)
I implemented what we discussed in 57078c0
Note that I also decided to change the style to use the standalone error box instead of the inline one. The inline error looked weird on the top of the page.
...-core/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/passwd.vm
Show resolved
Hide resolved
* Moved the async validation error before the form. * Reworded the error message to make it a bit clearer * Changed the async validation error from inline to standalone. Not discussed but looks better in this new context IMO.
@@ -102,12 +101,13 @@ | |||
## This key fits better. | |||
#set ($fields[$index].label = $services.localization.render('xe.admin.passwordReset.step2.newPasswordVerification.label')) | |||
<form class="xform third" action="" method="post" autocomplete="off"> | |||
## Async validation feedback | |||
#if ($request.xwikipassword && !$isValidPassword) | |||
#error($stringtool.join($errorMessage, "<br/>")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#error($stringtool.join($errorMessage, "<br/>")) | |
#error($stringtool.join($errorMessage, '<br/>')) |
It would avoid an interpretation that is not needed.
Jira URL
Changes
Description
Clarifications
Screenshots & Video
Not much changed visually, those are screenshots of an instance with the changes from the PR applied:


Executed Tests
Manual tests only.
Expected merging strategy