Updated: Comment #122
See details about the Usability Study at http://groups.drupal.org/node/163894

This issue contains suggestions and attempts to modify the display of these to fields to make it clear to the user how to 'update' the account password.

Problem/Motivation

During a usability test, users logged into Drupal could not determine how to update their account password. Users did not understand that the existing 'password' and 'confirm password' fields on the user/edit page served the purpose of a password update.

screenshot of existing, non-dynamic user edit form. 3 password fields are visible at all times.

Proposed resolution

Proposed resolution from #59:
1. Change new password label from "Password" to "New password".
2. Move "Current password" to below "New password" and use the #states system to show it only when "Email address" or "New password" fields change.
3. Only show "Confirm new password" field if "New password" field has been populated. Makes sense to use the #states system for this. Additionally it helps save vertical distance to avoid missing the shown "Current password" field when editing username.

Screenshots to explain a proposed resolution

1. Upon load, only one password field visible:
Proposed changes, upon load. Only one password field is visible
2. Upon changing email address, "current password" field becomes visible:
Upon changing email address, 'current password' field becomes visible
3. Upon entering text in "New Password" field, "Current password" (and "Confirm password") fields become visible:
Upon entering new password, all 3 password fields are visible

Criticisms of proposed solution

1. @webchick is concerned about the visual distance between the Email and Confirm password fields. See #65 and #86. She also suggested a review by someone not involved in the patch, this is what @victorkane did in #89 which verified that the proposed solution is already in use on other sites and that the proposed solution provides the proper usability.

Previous/alternative proposal summary

1. Change the name of the field to "New Password"
Suggested in #11
2. Move password fields to the end of the form
3. (rejected) Move password fields into a fieldset
See #19. Concern is that users do not put in the old password when changing a password, so they have to fill out the form again.
4. (rejected) Move password fields into a collapsed fieldset
Same concerns as for fieldset.
5. Move everything but password, email, and username fields to their own tab
See #9
6. Modal: On submit, if either the email or password field has changed, a modal appears prompting the user to fill in the previous password field.
See #70

Remaining tasks

Review patch in #122, decide if this solution works. If not asses other solutions, e.g. #70.

CommentFileSizeAuthor
#147 drupal-improve_user_password_edit-1259892-147.patch15.38 KBaaronbauman
#146 admin___sfd8.png129.33 KBaaronbauman
#146 admin___sfd8.png100.31 KBaaronbauman
#146 admin___sfd8.png80.47 KBaaronbauman
#146 superadmin___sfd8.png94.12 KBaaronbauman
#136 interdiff.txt525 bytesManjit.Singh
#135 users_could_not_find-1259892-135.patch15.37 KBManjit.Singh
#133 interdiff-1259892-127-133.txt3.02 KBpixelmord
#133 users_could_not_find-1259892-133.patch16.19 KBpixelmord
#127 interdiff-1259892-125-127.txt6.71 KBpixelmord
#127 improve_user_password_edit-1259892-127.patch15.31 KBpixelmord
#125 drupal8_patch_password_change.png198.49 KBpixelmord
#125 drupal8_patch_email_change.png215.13 KBpixelmord
#125 interdiff-1259892-122-125.txt5.73 KBpixelmord
#125 improve_user_password_edit-1259892-125.patch9.83 KBpixelmord
#122 improve_user_password_edit-1259892-122.patch6.44 KBfenstrat
#121 users_could_not_find-1259892-121.patch7.51 KBmpdonadio
#117 improve_user_password_edit-1259892-117.patch7.29 KBdruprad
#116 improve_user_password_edit-1259892-116.patch7.3 KBdruprad
#113 patch_failed.JPG245.18 KBTruptti
#46 1259892-46-improve_user_password_edit.patch5.64 KBfenstrat
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,961 pass(es), 6 fail(s), and 3,022 exception(s). View
#46 1259892-conditionally-show-current-password.png46.24 KBfenstrat
#46 1259892-change-password-labels.png46.51 KBfenstrat
#44 interdiff-1259892-42-44.txt1022 bytesamitgoyal
#44 user_improved-1259892-44.patch2.02 KBamitgoyal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,571 pass(es), 5 fail(s), and 1 exception(s). View
#42 user_improved-1259892-42.patch910 bytesjoshi.rohit100
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,494 pass(es), 3 fail(s), and 1 exception(s). View
#41 Screen Shot 2014-02-28 at 14.16.18.jpg140.09 KBLewisNyman
#37 user_improved.patch998 bytesneetu morwani
PASSED: [[SimpleTest]]: [MySQL] 64,226 pass(es). View
#28 changepasswordbefore.png48.17 KBandymartha
#28 changepasswordafter.png63.04 KBandymartha
#27 1259892-d7-27.patch989 bytesKevin Morse
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1259892-d7-27.patch. Unable to apply patch. See the log in the details link for more information. View
#19 1259892.019.patch820 byteskarschsp
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1259892.019.patch. Unable to apply patch. See the log in the details link for more information. View
#19 change-password-collapsed.png43.61 KBkarschsp
#19 change-password-expanded.png36.13 KBkarschsp
#16 leave_email_and_password_fields_alone_1259892.patch25.21 KBjuampynr
PASSED: [[SimpleTest]]: [MySQL] 32,904 pass(es). View
#14 leave_email_and_password_fields_alone_1259892.patch26.48 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 32,885 pass(es), 15 fail(s), and 0 exception(es). View
#12 leave_email_and_password_fields_alone_1259892.patch24.15 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 32,910 pass(es), 7 fail(s), and 0 exception(es). View
#9 leave_email_and_password_fields_alone_1259892.patch24.15 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 32,892 pass(es), 7 fail(s), and 0 exception(es). View
#9 user_account_form.png60.61 KBjuampynr
#9 user_edit_current.png172.84 KBjuampynr
#9 user_personal_form.png144.57 KBjuampynr
#4 change_password_in_separate_tab_1259892_3.patch5.97 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 35,941 pass(es), 22 fail(s), and 23 exception(es). View
#2 UA2_change_password_problems_1259892.patch500 bytesjuampynr
PASSED: [[SimpleTest]]: [MySQL] 35,827 pass(es). View
#49 1259892-conditionally-show-current-password-2.png44.91 KBfenstrat
#49 1259892-49-improve_user_password_edit.patch3.62 KBfenstrat
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,799 pass(es), 2 fail(s), and 0 exception(s). View
#49 interdiff.txt4.67 KBfenstrat
#52 1259892-52-improve_user_password_edit.patch5.3 KBfenstrat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,997 pass(es). View
#52 interdiff.txt1.68 KBfenstrat
#57 1259892-57-improve_user_password_edit.patch5.54 KBfenstrat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,188 pass(es). View
#57 interdiff.txt656 bytesfenstrat
#57 1259892-conditionally-show-current-password-3.png55.94 KBfenstrat
#59 1259892-59-improve_user_password_edit.patch5.73 KBfenstrat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,746 pass(es). View
#59 interdiff.txt1.79 KBfenstrat
#59 1259892-conditionally-show-current-password-4.png.png46.11 KBfenstrat
#60 1259892-conditionally-show-current-password-4.png46.11 KBfenstrat
#66 interdiff.1259892.59.66.txt3.33 KBrealityloop
#66 improve_user_password_edit-1259892-66.patch7.37 KBrealityloop
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,968 pass(es). View
#67 interdiff.1259892.66.67.txt1.49 KBrealityloop
#67 improve_user_password_edit-1259892-67.patch13.29 KBrealityloop
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,000 pass(es). View
#72 improve_user_password_edit-1259892-72.patch13.21 KBAkshayKalose
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,139 pass(es), 354 fail(s), and 131 exception(s). View
#74 improve_user_password_edit-1259892-74.patch13.22 KBAkshayKalose
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch improve_user_password_edit-1259892-74.patch. Unable to apply patch. See the log in the details link for more information. View
#78 interdiff-1259892-72-74.txt1.09 KBadci_contributor
#78 improve_user_password_edit-1259892-78.patch13.09 KBadci_contributor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,917 pass(es). View
#84 improve_user_password_edit-1259892-84.patch13.09 KBibonelli
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,701 pass(es). View
#84 interdiff-1259892-78-84.txt1.59 KBibonelli
#85 1-original-version.jpg69.75 KBmgfong
#85 2-proposed-changes.jpg95.34 KBmgfong
#85 3-applied-changes.jpg86.25 KBmgfong
#89 Account Settings.png30.5 KBvictorkane
#92 1259892.92.testsonly.patch2.34 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,814 pass(es), 3 fail(s), and 0 exception(s). View
#92 improve_user_password_edit-1259892-92.patch8.58 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,759 pass(es). View
#92 interdiff.1259892.84.92.txt4.51 KBYesCT
#93 improve_user_password_edit-1259892-93.patch8.58 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,816 pass(es). View
#93 interdiff.1259892.92.93.txt741 bytesYesCT
#96 improve_user_password_edit-1259892-95.patch7.55 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,821 pass(es). View
#96 interdiff.93.95.txt1.21 KBYesCT
#98 improve_user_password_edit-1259892-98.patch7.55 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch improve_user_password_edit-1259892-98.patch. Unable to apply patch. See the log in the details link for more information. View
#98 interdiff.1259892.95.98.txt3.39 KBYesCT
#106 improve_user_password_edit-1259892-106.patch6.53 KBepari.siva
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,240 pass(es), 3 fail(s), and 0 exception(s). View
#108 interdiff.txt2.34 KBfenstrat
#108 improve_user_password_edit-1259892-108.patch6.52 KBfenstrat
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,691 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

lnunesbr’s picture

Actually in D6, this information is under Account Information fieldset, and in D7, there is no wrapper for this kind of information.
Maybe aggregate all the user info in a wrapper (could be a fieldset), would become more friendly. And when the user goes to edit its profile, there is no information for the user, for example, where exactly he is, because the title is its own username, and bellow, follows the user edit form, with no additional information.

juampynr’s picture

Status: Active » Needs review
FileSize
500 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,827 pass(es). View

Here is a small patch to group user fields in an "Account details" fieldset.

It is visible when an authenticated user edits his account, or when an anonymous user creates an account.

Shyamala’s picture

The Patch adds "Account details" field set to the user account edit page. It enhances the user edit page, but does not fully resolve the usability issue of the forgot password section. May be the way the user section can be edited itself must be relooked at. Moving Change password to a separate TAB definitely will be a step forward.

juampynr’s picture

Assigned: Unassigned » juampynr
FileSize
5.97 KB
FAILED: [[SimpleTest]]: [MySQL] 35,941 pass(es), 22 fail(s), and 23 exception(es). View

Here is a patch in which a "Change password" tab has been added to the user account page. I have removed the change password fields from the edit account page, and verified that:

* A user can register when the email verification is disabled.
* A user can change his own password.
* The administrator can change a user's password.
* The administrator can change his own password.

I still need to review and update user tests. Will provide another patch by the end of the day, but reviews and guidance are very welcomed.

Status: Needs review » Needs work

The last submitted patch, change_password_in_separate_tab_1259892_3.patch, failed testing.

lnunesbr’s picture

Adding an additional tab might be a good idea.

droplet’s picture

@#4, not a good idea. I suppose everything could be edit under EDIT page.

#993592: Vertical Tabs on User Account Edit page

yoroy’s picture

Some screenshots would help review this.

juampynr’s picture

Version: 8.x-dev » 7.x-dev
FileSize
144.57 KB
172.84 KB
60.61 KB
24.15 KB
FAILED: [[SimpleTest]]: [MySQL] 32,892 pass(es), 7 fail(s), and 0 exception(es). View

I have seen that this issue has been discussed before. After reading the related issues the feeling I get from the conversation is that what bothers most is that "it does not look pretty". We have proven with a usability test (mentioned at the issue description at the top of this page) that the current situation is very confusing for a new user.

Here is a new patch where all fields in the user/*/edit page which are not username and password have been placed in a new page "Personal". In doing so, I have realized the following:

* Having all fields together leaded to a lot of messy checks in the code to see in which context we were. Some of them have been removed.
* Same for the functional tests. There were some assertions just to verify that there was not a collision with a field appearing where it did not have to do it.
* The forms actually look more organized. Here are some screenshots of the Account tab and the Personal tab, as opposed to the current Edit page with all the fields in one place.

juampynr’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs usability review

Changed version to 8.x-dev.

droplet’s picture

Version: 7.x-dev » 8.x-dev

suggest change "Password" to "New Password"

juampynr’s picture

Status: Needs work » Needs review
FileSize
24.15 KB
FAILED: [[SimpleTest]]: [MySQL] 32,910 pass(es), 7 fail(s), and 0 exception(es). View

Changed status to "needs revew". Plus the patch again so it passes under the testing bot.

Status: Needs review » Needs work

The last submitted patch, leave_email_and_password_fields_alone_1259892.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
26.48 KB
FAILED: [[SimpleTest]]: [MySQL] 32,885 pass(es), 15 fail(s), and 0 exception(es). View

Here is an updated patch that fixes the failing tests.

Status: Needs review » Needs work

The last submitted patch, leave_email_and_password_fields_alone_1259892.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review
FileSize
25.21 KB
PASSED: [[SimpleTest]]: [MySQL] 32,904 pass(es). View

Updated patch. User language field set will have to stay at the user account page as it is used to set the default language for new users.

klonos’s picture

...subscribing. I still like #993592: Vertical Tabs on User Account Edit page better though.

bryancasler’s picture

subscribe

karschsp’s picture

FileSize
36.13 KB
43.61 KB
820 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1259892.019.patch. Unable to apply patch. See the log in the details link for more information. View

What about simply putting the password fields in a collapsed Change Password fieldset?

juampynr’s picture

hi @karschsp, that was suggested in comment #2, but rejected because the password field at the top and the email fields are tied to the change password fields, and the usability tests demonstrated that users who tried to change their password and upload a profile picture, but failed on retyping the old password had to fill the form and select the picture again.

There is quite a bit of logic related to the password field (change, reset, ignore on first login...) and that is why it should be separated. either on hirizontal or vertical tabs.

Cheers

Bojhan’s picture

Title: UA2: Users could not find the Change password fields » Users could not find the Change password fields
Issue tags: -Needs usability review

I am honestly not sure about any if the suggestions here, can we not redesign the visual hierarchy for it to stand out without using fieldsets?

juampynr’s picture

@bojhan, can you suggest an alternative with a graphic or wireframe?

kscheirer’s picture

#19: 1259892.019.patch queued for re-testing.

kscheirer’s picture

Retesting against latest HEAD since it has been over a year.

Status: Needs review » Needs work

The last submitted patch, 1259892.019.patch, failed testing.

Cottser’s picture

Component: theme system » user.module
Issue tags: +Novice, +Needs reroll

Tagging as reroll (and novice) and moving to user.module component.

It looks like the code for this has moved to Drupal\user\AccountFormController::form() but the same change should still be possible.

Kevin Morse’s picture

Status: Needs work » Needs review
FileSize
989 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1259892-d7-27.patch. Unable to apply patch. See the log in the details link for more information. View

Here you go.

andymartha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
63.04 KB
48.17 KB

In a fresh installation of Drupal 8, the patch 1259892-d7-27.patch in #27 by Kevin Morse DOES create a fieldset with label around the change password fields. Core functionality seems to work properly. Users still have to enter their own password in the "current password" field above the "change password" fieldset to change their password. See screenshots.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

It doesn't seem like anyone has answered @Bojhan's question in #21... is there a way we can do this without a fieldset? Or is using a fieldset the right thing here?

Bojhan’s picture

We do not really use "fieldsets" to get more priority. Please keep in mind in how different this is used, basically adding a element from core sounds like a really bad idea. At the very least this should be Bartik specific. Does this have to do with position. E.g. should it always be the last on the page?

tim.plunkett’s picture

#27: 1259892-d7-27.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +D7UX usability, +Bartik, +ideup user testing, +Needs reroll

The last submitted patch, 1259892-d7-27.patch, failed testing.

aaronbauman’s picture

Doesn't sound like there's much consensus on an approach yet.

Here are the suggestions so far:
1. Change the name of the field to "New Password"
2. Move password fields to the end of the form
3. Move password fields into a fieldset
4. Move password fields into a collapsed fieldset
5. Move everything but password, email, and username fields to their own tab

FWIW, Wordpress does #1 and #2 (which incidentally would have the smallest footprint in terms of changing this form).

aaronbauman’s picture

Issue summary: View changes

Added anchor to link.

madison.major’s picture

Issue summary: View changes
Issue tags: +D8UX usability
madison.major’s picture

Issue summary: View changes
madison.major’s picture

neetu morwani’s picture

Status: Needs work » Needs review
FileSize
998 bytes
PASSED: [[SimpleTest]]: [MySQL] 64,226 pass(es). View

Patch rerolled successfully.

rob3000’s picture

Issue tags: -Needs reroll

Following on from bojhan -> https://drupal.org/comment/7396608#comment-7396608 there should be another way to do this witout a fieldset.

rob3000’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes

put back the list of proposals, but noted which were rejected, and tried to explain why.

LewisNyman’s picture

How about using a header in combination with the “new password” label? Here's a quick mockup

joshi.rohit100’s picture

FileSize
910 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,494 pass(es), 3 fail(s), and 1 exception(s). View

Recreated patch as last patch at #37 failed to apply..

Status: Needs review » Needs work

The last submitted patch, 42: user_improved-1259892-42.patch, failed testing.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,571 pass(es), 5 fail(s), and 1 exception(s). View
1022 bytes

Please review patch to fix test case failures as reported in #42.

Status: Needs review » Needs work

The last submitted patch, 44: user_improved-1259892-44.patch, failed testing.

fenstrat’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,961 pass(es), 6 fail(s), and 3,022 exception(s). View
46.24 KB
46.51 KB

Here's another approach:

1. Move new password into a "Change password" details/fieldset. A "Change password" heading as per #41 seems better but we went with a fieldset as it seem to group them semantically.
2. Change new password label from "Password" to "New password". This required editing form.inc to allow an optional 'title1' and 'title2' property to be passed into the 'password_confirm' element.
3. Move "Current password" to below "New password" and use the #states system to show it only when "Email address" or "New password" fields change.


A few of us thrashed this solution out as part of the Drupal Gold Coast sprint.

fenstrat’s picture

Issue summary: View changes

Fixing image order.

The last submitted patch, 46: 1259892-46-improve_user_password_edit.patch, failed testing.

fenstrat’s picture

Issue summary: View changes
FileSize
44.91 KB
3.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,799 pass(es), 2 fail(s), and 0 exception(s). View
4.67 KB

Here's a different approach from #46:

1. It gets rid of the details/fieldset around "New password" as well as moving Username back to be right before "New password". Doing so allows UserAccountFormFieldsTest tests to pass. Field order is important here because as that test states "Verifies that the field order in user account forms is compatible with password managers of web browsers.".

So the approach now is:

1. Change new password label from "Password" to "New password". Added a "Change password" heading to it using '#prefix', ugly, but I can't see a better way right now.
2. Move "Current password" to below "New password" and use the #states system to show it only when "Email address" or "New password" fields change.

Status: Needs review » Needs work

The last submitted patch, 49: 1259892-49-improve_user_password_edit.patch, failed testing.

LewisNyman’s picture

This design change works well for the change password task, but it looks like it will introduce a new problem when someone changes their email address. I'm not sure how to solve both. Maybe cutting down on some of the description text will make the other elements clearer?

fenstrat’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,997 pass(es). View
1.68 KB

This should fix up the failing tests.

@LewisNyman appreciate that the distance between Email and Current password is an issue. Not sure we could cut down on the descriptive text as it's all pretty important? I originally moved Email under Username but as noted in #49 this breaks UserAccountFormFieldsTest->assertFieldOrder() which specifically tests username is right before password to ensure browser password managers work with the form. I still think this patch improves the current situation a great deal.

realityloop’s picture

Based on the screenshot at #49, my main observation is that there seems to be too much space between the descriptive text and the Confirm new password field

fenstrat’s picture

@realityloop Yeah that's the same observation @LewisNyman had in #51. What about moving the Current password under Email?

realityloop’s picture

@fenstrat personally I'm ok with the moving of the current password field to the bottom of the screen, I think your logic about it only displaying if the email or password field changes makes sense..

I'd also almost ask the question should the Confirm new password field only appear after you populate the New password field..?

amit.drupal’s picture

@fenstrat Your patch are working fine.
Much Space between the descriptive text and the Confirm new password field.

I have suggestion - Use Collapsible fieldsets in change Password field .

fenstrat’s picture

Issue summary: View changes
FileSize
5.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,188 pass(es). View
656 bytes
55.94 KB

@amit.drupal a collapsible fieldset/detail for Change password is the approach we tried in #46. For the reason this was abandoned see #49. Additionally, unless it defaulted to collapsed (not very usable I'd say) it wouldn't fix the vertical space problem.

@realityloop Interesting suggestion, only show the Confirm new password if New password is populated. Gains us vertical space and it seems like a good use of the #states system. Attached implements that. Thoughts?

realityloop’s picture

Status: Needs review » Needs work

I really like the changes here, just a few thoughts...

+++ b/core/modules/user/src/AccountForm.php
@@ -115,6 +115,9 @@ public function form(array $form, array &$form_state) {
+        '#title2' => $this->t('Confirm new password'),

May I sugggest this be renamed to "New password confirmation"

+++ b/core/modules/user/src/AccountForm.php
@@ -115,6 +115,9 @@ public function form(array $form, array &$form_state) {
         '#description' => $this->t('To change the current user password, enter the new password in both fields.'),

I think this may need to be reworded if the 2nd field is initially hidden?

+++ b/core/modules/user/src/AccountForm.php
@@ -130,9 +133,9 @@ public function form(array $form, array &$form_state) {
+        $current_pass_description = $this->t('Required if you want to change the %mail or %pass above. !request_new.', array('%mail' => $protected_values['mail'], '%pass' => $protected_values['pass'], '!request_new' => $request_new));

I wonder if it may be clearer to say 'Confirm your current password to change the %mail or %pass above. !request_new.'

fenstrat’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,746 pass(es). View
1.79 KB
46.11 KB

@realityloop Thanks for the review, all good points. I've implemented them in the attached patch.

fenstrat’s picture

Issue summary: View changes
FileSize
46.11 KB
a_thakur’s picture

The patch works fine. Though, a Current Password fieldset (non-collapsible) would be better with Current Password, New Password and Confirm New Password.

fenstrat’s picture

@a_thakur Putting "Current password" into a fieldset with New password wouldn't really work as it needs to be shown if Email is edited. Or have I misunderstood you?

realityloop’s picture

Status: Needs review » Reviewed & tested by the community

I've tested #59 and am happy with the changes, I think this is ready to commit. nice work @fenstrat

fenstrat’s picture

Great, thanks @realityloop.

As noted above a few of us thrashed out this approach as part of the Drupal Gold Coast sprint.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Neat, I like this improvement a lot for passwords. The only issue is with e-mail addresses, we now have a huge visual disconnect between the field you're changing and the field that you must now fill out ("Current password" ended up "below the fold" for me so I never caught it, only after submitting the form and getting an error.)

I'm wondering if we could make the weight based on the #states system, so if you last touched the "Email" field, the Current Pass would show up just below it, versus if you last hit the "New password" field, it'd show up down there?

+++ b/core/modules/user/src/Tests/UserEditTest.php
@@ -58,7 +58,7 @@ function testUserEdit() {
+    $this->assertRaw(t("Your current password is missing or incorrect; it's required to change the %name.", array('%name' => t('New password'))));

+++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
@@ -85,7 +85,7 @@ function testUserPasswordReset() {
+    $this->assertText(t('Your current password is missing or incorrect; it\'s required to change the New password.'), 'Password needed to make profile changes.');

"New" capitalized by itself looks a bit silly. :) Maybe "New password" field?

Also, in one place we're using the translatable button names, in another place we're not. Let's make both places make those consistent. AFAIK we don't translate anything in assertion messages, so maybe check around in HEAD to figure out which one we should pick.

realityloop’s picture

Status: Needs work » Needs review
FileSize
3.33 KB
7.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,968 pass(es). View

My spidey senses start tingling at the idea of changing the weight of a form field mid edit, can we get some input from the usability team before doing this?

Patch attached that does the updating of New password, I forgot to remove the t(), will upload fixed patch shortly.

realityloop’s picture

FileSize
1.49 KB
13.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,000 pass(es). View

updated patch

fenstrat’s picture

Those "New password" and t() changes look fine, thanks @realityloop.

I'd also baulk at the idea of moving a form field around mid edit (on top of the fact that I'm pretty sure that's not supported by the #states system?). Most interested in the opinion of @Bojhan or someone else from the usability team.

YesCT’s picture

@webchick

Also, in one place we're using the translatable button names, in another place we're not.

I couldn't really see button names that in the patch.
Maybe meant the argument (the field label) to the assert string?

I'm not sure taking the t() out like in #67 is the thing to do.
In fact, if the assert string is t()'d,
Then I think the field label should be also...

+++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
@@ -85,7 +85,7 @@ function testUserPasswordReset() {
-    $this->assertText(t('Your current password is missing or incorrect; it\'s required to change the Password.'), 'Password needed to make profile changes.');
+    $this->assertText(t('Your current password is missing or incorrect; it\'s required to change the "New password" field.'), 'Password needed to make profile changes.');

So I think that should use a t()'d arg like

+++ b/core/modules/user/src/Tests/UserEditTest.php
@@ -47,7 +47,7 @@ function testUserEdit() {
-    $this->assertRaw(t("Your current password is missing or incorrect; it's required to change the %name.", array('%name' => t('Email address'))));
+    $this->assertRaw(t("Your current password is missing or incorrect; it's required to change the \"%name\" field.", array('%name' => t('Email address'))));

for: New password

-------

For sure the "message" for the assert ( 'Password needed to make profile changes.') should *not* have t(). See a child of #500866: [META] remove t() from assert message.

There are a bunch of asserts with t() in them in core:
[edit: better pattern and the number result]
ag "assert(Raw|Text)\(t\(" core/modules/*
is just some. (where some = 651)

LewisNyman’s picture

I have a new idea:

On submit, if either the email or password field has changed, a modal appears prompting the user to fill in the previous password field. It would be impossible to miss because you can't progress without filling it in, and we don't have to worry about placement of the field.

What does everyone think? I'm not sure how to implement this in Drupal, as a pure JS solution is sounds quite simple.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

This needs a reroll.

Regarding the use of the t() function in #69, rather, the String::format() method should be used.

AkshayKalose’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,139 pass(es), 354 fail(s), and 131 exception(s). View

Rerolled from #67

Status: Needs review » Needs work

The last submitted patch, 72: improve_user_password_edit-1259892-72.patch, failed testing.

AkshayKalose’s picture

Status: Needs work » Needs review
FileSize
13.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch improve_user_password_edit-1259892-74.patch. Unable to apply patch. See the log in the details link for more information. View

Updated rerolled patch.
$config['prefixes'][$options['language']->id
to
$config['prefixes'][$options['language']->getId()

YesCT’s picture

Assigned: juampynr » Unassigned

Thanks.
To show the difference between two patches that apply, interdiffs are super nice.
https://www.drupal.org/documentation/git/interdiff

Status: Needs review » Needs work

The last submitted patch, 74: improve_user_password_edit-1259892-74.patch, failed testing.

adci_contributor’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
13.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,917 pass(es). View

However these bouth is unable to apply now :P

Rerolled from #74.
Also attached 72-74-interdiff

mirie’s picture

It looks like this patch is adding 2 new form element attributes #title1 and #title2. Will this be confusing since these are not part of the Form API?

blacklight4’s picture

I'd go with:

Login

username
Specify "user@email.com in username box

password

Change password (if desired)

Current password

New password

Confirm new password

doakym’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

patch does not apply and

i cant test this patch in simpletest me http://simplytest.me/project/drupal/8.0.x?patch[]=https://www.drupal.org...

"An error occurred while patching the project."

YesCT’s picture

Issue tags: +Usability

adding the generic usability tag so this does not get lost.

ibonelli’s picture

The "simpletest me" worked for me now, so I guess something changed? (I'm referring to comment #81 which says it doesn't)

In any case it works now and I see most working. What seems missing (when I test) is the change from "Password" to "New password"... But the rest seems to work as expected as far as I can tell.

On message #59 I see the mockup as it was originally intended, then there is a different rough mock up on #80. But none is applied. I agree with the #80 suggestion of "Change password (if desired)" and will modify code so it looks like that.

ibonelli’s picture

Status: Needs work » Needs review
Issue tags: +LatinAmerica2015
FileSize
13.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,701 pass(es). View
1.59 KB

So I created the new patch that I'm attaching over here. There were two changes instead of one:

  • The "Password" box label now is "New password"
  • The "Change password" box label now is "Confirm new password"

This is consistent with the design originally proposed and comment #49. So I'm submitting for review. Thanks!

mgfong’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
69.75 KB
95.34 KB
86.25 KB

I tested the new patch at #84 and is working as proposed.

1. Original version
original version

2. Proposed changes
proposed changes

3. Changes implemented
changes implemented

webchick’s picture

Status: Reviewed & tested by the community » Needs review

So from a usability perspective in terms of changing a user's password, this makes a ton of sense. However, you also need to enter "Current password" in order to change the e-mail address (this is in the field description), and with the text field that far down, this is no longer clear. So I worry we'll get UX problems in the opposite direction now. Hm.

Could we maybe get a few people who weren't involved in this issue to test changing their email address in D8 with this patch applied and see how they fare?

YesCT’s picture

Issue summary: View changes

some img were duplicated in the summary.
took out duplicates.
also re-ordered to make more sense.

I will review this now.

YesCT’s picture

Issue tags: -Needs reroll

I think the reason for moving the current password below the email and new password fields is because we are hiding it until the user types something in either field.
If we left it at the beginning, hidden, then it would show, but the user may not notice it because they are scrolled down.
with it after, they have a chance to see it on the way to the save button.

I wonder how the aria accessibility of this is. I'll try that as well.

I also have someone new to the issue to test it.

victorkane’s picture

Issue summary: View changes
FileSize
30.5 KB

I tested patch #84. I changed my account name and saved. I saved the email and upon error (red message) telling me to add my current password, that field (it does have help text I just hadn't read it), I added my current password, and the change worked. I then went to change my password. It was easy to see where to place the new password + confirmation. Then I knew to add the current password as well and that worked too.

However, my immediate subjective impression was that my experience has always been, when changing passwords on different accounts, that there is a link somewhere to change your password. When you press it, my natural expectation was to expect to put the old (current) password first, and then the new password and confirmation of new password.

So I went to look for corroboration on some sites. I changed my yahoo account, but it did something completely different. When I clicked on change your password, it had me login again. A successful login took me to a page where there were two text inputs only, New Password and Confirm your new password.

So I went to a well-known dating site and lo and behold, it's way of doing things (both for changing email and for changing password) turned out to be exactly like the patch #84:

In conclusion, if the idea is to include many account details on the same page, and for the current password to be requested only upon attempting to change email or password, patch #84 seems to have the proper usability.

YesCT’s picture

Issue summary: View changes
Issue tags: -Novice

I used the mac voice over and listened to the form.
Having the current password after the email and new password seemed to work really well. I didn't have to worry about the hidden field being shown and me not hearing it.

YesCT’s picture

Status: Needs review » Needs work

looks like some other changes from another issue might be in this patch.
Maybe in #66 -> #67
and the changes to url routing are not in the interdiff...

I will try and take them out now.

I will also make a tests only patch to see what the fails look like on that.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,814 pass(es), 3 fail(s), and 0 exception(s). View
8.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,759 pass(es). View
4.51 KB

took out some url generator stuff.

@realityloop do you know what issue that code might belong to?

setting this to needs review to see what the bot says.

I have some other small changes to make. I will do that now. Do not need this reviewed just yet.

YesCT’s picture

FileSize
8.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,816 pass(es). View
741 bytes

just a comma for grammar was accidentally taken out earlier.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.module
@@ -393,6 +393,32 @@ function _user_role_permissions_update($roles) {
+ * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+ *   Use \Drupal\Core\Session\AccountInterface::hasPermission().
+ */
+function user_access($string, AccountInterface $account = NULL) {

looks like an earlier reroll made a mistake. we should not add back in functions that were removed.

this was removed in #2306429: Remove user_access()

which I found by doing:
git log -S "Determine whether the user has a given privilege" core/modules/user

---
I will try and fix that now.

[edit:]
Actually I dont see that function being called in here.
So I will just undo it.

The last submitted patch, 92: 1259892.92.testsonly.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,821 pass(es). View
1.21 KB
YesCT’s picture

Status: Needs review » Needs work

in #69 I had wondered if the arg should also have t()

ag "assertRaw\(t\(.*array.* t\("

looks like there are only 6 like that... and it doesn't matter since our tests run in english unless they are specifically about language or translation. so let's just leave it.

---
However, the contraction "it's" is in a line we are already changing (in response to a request from webchick to use '"Name of the field" field'. Since that line is already needing to be changed, we can fix it and say "it is" instead of the contraction. I'll do that now.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch improve_user_password_edit-1259892-98.patch. Unable to apply patch. See the log in the details link for more information. View
3.39 KB

this changes it's to it is

--

OK. I think this actually needs a full review now.

gobinathm’s picture

Assigned: Unassigned » gobinathm

Assigning to myself for a complete review

mpdonadio’s picture

OK @gobinathm said he was going to do a full review, but can someone update the IS why a fieldset around the final solution was rejected and explain how the lack of a fieldset doesn't cause a11y problems? I searched through the comments several times, but didn't see this (just earlier suggestions of putting a fieldset around what was there). I'm half tempted to set it back to Needs Work for this.

YesCT’s picture

Assigned: gobinathm » Unassigned

@gobinathm if you have any questions, or a partial review, please post it. :)
unassigning since it has been a while, so it is clear others can work on this.

YesCT’s picture

adding tag based on #100

Status: Needs review » Needs work

The last submitted patch, 98: improve_user_password_edit-1259892-98.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs reroll

Looks like this needs a reroll. Did a quick test, and there are merge conflicts, so I don't know if this is truly a novice task.

epari.siva’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,240 pass(es), 3 fail(s), and 0 exception(s). View

Patch rerolled.

Status: Needs review » Needs work

The last submitted patch, 106: improve_user_password_edit-1259892-106.patch, failed testing.

fenstrat’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,691 pass(es). View
2.34 KB

Here's a reroll of #106 that should pass tests.

I've gone through all comments again and updated the issue summary trying to account for all the feedback.

@mpdonadio Re: wrapping the solution in a fieldset: I can't see how it'd work if only the email is changed, in which case the Confirm password field needs to be shown, and including it in a fieldset with the New password (which in this case remains unchanged) doesn't seem to make any sense. Also, @Bojhan's stated he's not in favour of fieldsets: "We do not really use "fieldsets" to get more priority".

fenstrat’s picture

Issue summary: View changes
mpdonadio’s picture

#109, my comment about a fieldset didn't have to do with priority, but more with accessibility, eg http://webaim.org/techniques/forms/controls and http://www.w3.org/WAI/tutorials/forms/grouping/, especially for screenreaders and non-traditional input/output devices where the legend and being able to tab over the fieldset are important..

I think this is RTBC, but I won't set it w/o one of the Accessibility leads chiming in about this.

fenstrat’s picture

Thanks for clearing that up @mpdonadio. I can't really comment on the accessibility of a fieldset, however practically speaking I'm pretty sure one wouldn't makes sense here. If you update only Email then Current password will be shown via #states. If New password and Current password are grouped into a fieldset then Current password is sitting in a fieldset that doesn't really have bearing on what the user was trying to do (i.e. change their Email), because a New password isn't being set. Hope this makes sense.

fenstrat’s picture

Hmm, trying to display the last patch only.

Truptti’s picture

FileSize
245.18 KB

Tried verifying the issue with the patch '1259892-59-improve_user_password_edit.patch' in #112 , but the patch failed.Attached snapshot for reference.Updating status to Needs work.

Truptti’s picture

Status: Needs review » Needs work
mpdonadio’s picture

Issue tags: +Needs reroll

#113, when that happens add the Needs Reroll tag. BTW, I't usually best to use `git apply --index` with Drupal patches.

druprad’s picture

Issue tags: -Needs reroll
FileSize
7.3 KB

Patch rerolled from #108

druprad’s picture

Status: Needs work » Needs review
FileSize
7.29 KB

Corrected description single quote with /' and queued for testing

Status: Needs review » Needs work

The last submitted patch, 117: improve_user_password_edit-1259892-117.patch, failed testing.

mpdonadio’s picture

I took a quick look at #117 and I think there were problems with the reroll. @pradeep.singh, how did you approach this? I may take a stab at this tonight.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I am going to re-reroll this tonight.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
7.51 KB

Reroll of #108

$ get checkout b75b691
$ git apply --index improve_user_password_edit-1259892-108.patch
$ git rebase origin/8.0.x
First, rewinding head to replay your work on top of it...
Applying: improve_user_password_edit-1259892-108.patch
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
M	core/modules/user/src/AccountForm.php
M	core/modules/user/src/Tests/UserEditTest.php
M	core/modules/user/src/Tests/UserPasswordResetTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/user/src/Tests/UserPasswordResetTest.php
Auto-merging core/modules/user/src/Tests/UserEditTest.php
Auto-merging core/modules/user/src/AccountForm.php
CONFLICT (content): Merge conflict in core/modules/user/src/AccountForm.php
Auto-merging core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
Failed to merge in the changes.
Patch failed at 0001 improve_user_password_edit-1259892-108.patch

Someone should double check the hand merges. Also, I am not sure why, but I had to manually add `use Drupal\Core\Url;` back into AccountForm.

fenstrat’s picture

Reroll of #108.

This has been through quite a few reviews as noted in the Issue summary. It'd be nice to see if we can get sign off on this approach as it's seems like an well accepted solution outside of Drupal.

  1. The reroll in #121 added an hunk around if (!$form_state->get('user_pass_reset')) { that was not needed.
  2. The one modification I've made to #108 is that the default for '#title1' is now 'Password' and not 'New password'. Without this change when adding a user the password field was labelled 'New password' when it should have been 'Password'. Also changed this in the Usage example for PasswordConfirm.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pixelmord’s picture

Status: Needs review » Needs work
Issue tags: +DevDaysMilan, +sprint

Reviewing this I can say a few things:

  • The position of the current password filed is always wrong. Fro a usability perspective you should not have a field that is required to be filled in higher up in the document outline/form field order. You will always overlook that because you are already "past" that point visually. That is even more critical when the field "suddenly" appears. That is a problem both for changing the email AND a password change user flow.
  • Addressing the concerns that @webchick had (visual distance between the Email and Confirm password fields. See #65 and #86. ), I guess the best solution would be to bring email, password change and current PW fields closer together. Since the requirement to fill in the current PW is only valid, when you start editing the other fields, I suggest we really move the email field closer to new password field and then put the current password field after these fields that create this requirement.
  • Amending the help text for the email and change PW fields to indicate that you will need to enter your current password also would make sense, I think.
  • Lastly since we're already using the state-API we also should dynamically mark the confirm password and current password fields as required, so users get a visaul cue with the red star and we get HTML5 validation for free.

So I will create a patch that incorporates these changes and then we can review how that feels from a usability perspective.

pixelmord’s picture

So here's a patch that incorporates the suggestions from #124:

  • Moved the current password filed further down the form order. As a compromise after discussing that here with some people during the sprint, I positioned that between email and new password fields
  • Reordered the name and email fields to reverse order to get the email field closer to confirm password
  • Added info text regarding the requirement to confirm the current password to the help description of both fields that have that requirement
  • Set the confirm password and current password fields conditionally to "required" using state API

Check the screenshots for the HTML5-validation of the fields (German text, but you get the picture)

email change screenshot
email change screenshot

Status: Needs review » Needs work

The last submitted patch, 125: improve_user_password_edit-1259892-125.patch, failed testing.

pixelmord’s picture

Status: Needs work » Needs review
FileSize
15.31 KB
6.71 KB

I had to adjust the tests that assert the order of the fields in various forms so that the new order is tested correctly.
So attached there's a new patch.

pixelmord’s picture

Marked that for UX review

fenstrat’s picture

Status: Needs review » Needs work

Thanks for working on this @pixelmord.

Overall this is certainly a better approach than what exists now and really needs UX review.

I think reordering the name and email fields as well as moving current_pass under email is a decent compromise. It ensure that when current_pass is shown it is never too visually far away from the field that triggered the show.

Nice addition on marking current_pass required if name or email changes.

  1. +++ b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
    @@ -14,6 +14,7 @@
      * $form['pass'] = array(
      *   '#type' => 'password_confirm',
    + *   '#title1' => t('New password'),
      *   '#title' => $this->t('Password'),
      *   '#size' => 25,
      * );
    

    No need to keep '#title' here (as setting it does nothing), it should simply be changed to '#title1'.

  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -108,25 +96,25 @@ public function form(array $form, FormStateInterface $form_state) {
    +      '#description' => $this->t('A valid email address. All emails from the system will be sent to this address. The email address is not made public and will only be used if you wish to receive a new password or wish to receive certain news or notifications by email. In case you want to change this email address you also need to confirm your current password.'),
    
    @@ -146,13 +134,44 @@ public function form(array $form, FormStateInterface $form_state) {
    +        '#description' => $this->t('To change the current user password, enter the new password here. To be allowed to do this you also need to confirm your current password.'),
    

    There's inconsistencies in the "... you also need to confirm your password." message. One starts with "In case ..." and the other "To be allowed to ...". Now current_pass becomes required why not simply remove these from the descriptions?

  3. +++ b/core/modules/user/src/AccountForm.php
    @@ -146,13 +134,44 @@ public function form(array $form, FormStateInterface $form_state) {
    +            // Mark the field as required if mail or password has changed.
    

    Nit, should be "new password" not "password" to match code comment above.

Bojhan’s picture

pixelmord’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +String change in 8.2.0

So since this contains string changes, I put version to 8.2.x.

@fenstrat: thanks for your code review, I will see to do those corrections mentioned.

I lean towards to keeping the hint that you will have to provide your password, just to make it clear before the field will be revealed. However that should also be part of a usability review.

Bojhan’s picture

Issue tags: -Needs usability review

This kind of just feels like exchanging one problem for the other :) But its in line with Angie's review, so fine way to move forward.

pixelmord’s picture

Status: Needs work » Needs review
FileSize
16.19 KB
3.02 KB

As promised in #131 I introduced some changes

  1. I changed the example in the comment of the PasswordConfirm form element class. Since this could possibly used in other forms I decided to keep that neutral. "Password" + "Confirm password". The change for the array key makes total sense
  2. I changed the wording of the help text to be a bit more consistent, but still believe what I said before is valid: Better tell the user beforehand that he needs to provide his current password.
  3. Also used the same wording for the comments for the state settings

@Bohjan: Thank you for the usability review! I see your point that this solution is may be not the ultimate best what we can do, however I think it improves the current solution significantly.
I can imagine that we can come up with an even better solution eventually, but I have the strong feeling that that will require more drastic changes to the form and the design pattern that we use. To be able to go done that road we definitely need a conceptual and design step before. Therefore I propose to go forward with this solution and consider opening a follow up issue that opens up a discussion about we could improve that more drastically. E.g. using a modal form that concentrates the user's focus on the desired action and filling the necessary fields.

fenstrat’s picture

Status: Needs review » Needs work

Changes look good. Just one nit:

+++ b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
@@ -14,7 +14,8 @@
+ *   '#title1' => t('Password'),
+ *   '#title2' => $this->t('Confirm password'),

For this code comment '#title1' needs to be $this->t() not t().

Manjit.Singh’s picture

Status: Needs work » Needs review
FileSize
15.37 KB

Nice catch :) Here are the changes.

Manjit.Singh’s picture

FileSize
525 bytes

opppss forget to attach interdiff. So here is that.

fenstrat’s picture

Status: Needs review » Reviewed & tested by the community

Ok I think this is RTBC. This addresses @webchick's concerns from #86 and has @Bojhan's UX sign off in #132. Is it perfect? No way. However it is certainly an improvement on what is there now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 135: users_could_not_find-1259892-135.patch, failed testing.

fenstrat’s picture

Not sure what happened in #138, tests seem to still be passing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 135: users_could_not_find-1259892-135.patch, failed testing.

fenstrat’s picture

Status: Needs work » Reviewed & tested by the community

Hmm failures seem unrelated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 135: users_could_not_find-1259892-135.patch, failed testing.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

Thanks @pixelmord!

When creating screenshots for issues, please crop the image to only the relevant part of the screen. It's helpful to also annotate the screenshot to indicate the key changes. Otherwise, it's hard for reviewers to understand what the proposed UI changes are. I updated the contributor task instructions to try to clarify these key points: https://www.drupal.org/node/1859584/revisions/view/9218398/9888925

Some of the screenshots in the summary are also out of date, and others seem to be many duplicate copies (?). I've removed them. The issue summary also seems to be out of date. Finally, the final patch differs from what @Bojhan signed off on.

So, let's get some small, clear, annotated before-and-after screenshots to illustrate the proposed change, update the summary so it reflects the current status, and also indicate what has been changed since the usability signoff so we can see if we need another UX review.

Thanks everyone for your patience and ongoing work to make our administrative UIs easier to understand!

xjm’s picture

Issue summary: View changes

(Removing more out of date images and saving issue credit.)

+++ b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
@@ -14,7 +14,8 @@
+ *   '#title1' => $this->t('Password'),
+ *   '#title2' => $this->t('Confirm password'),

Also, this looks nonstandard; we do not do this anywhere else in core and these keys are not very meaningful. I see @mirie already asked about this choice in #79, but no response to that question other than "it's fine" essentially, which I'm not sure I agree with. :) At a minimum, let's use more specific key names, or better, let's look to see if there's a better pattern to use elsewhere in core.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aaronbauman’s picture

Issue summary: View changes
FileSize
94.12 KB
80.47 KB
100.31 KB
129.33 KB

Re changes between #127, where Bojhan signed off, and #135 -- there are exactly 2, both described in #133. Specifically, those 2 changes are:
1. Default "pass2" #title changed from "New Password" to "Confirm Password". AccountForm "pass" #title unchanged between patches.
2. AccountForm "pass" #description changed from:

To change the current user password, enter the new password here. To be allowed to do this you also need to confirm your current password.

to:

To change the current user password, enter the new password here. In that case you also need to provide your current password.

Taking a stab at rewriting issue summary, including screenshots, then will provide re-rolled patch and address "#title1"/"#title2" concerns.

aaronbauman’s picture

Status: Needs work » Needs review
FileSize
15.38 KB

After looking through Drupal's other core elements, I agree that using "#title1" and "#title2" is confusing. In this patch, I've changed "#title1" back to "#title", and introduced a new attribute "#title_confirm" which better explains its purpose.

There are no core form elements whose attributes make sense to use instead of "#title2", and there's no directly analogous pattern form which to borrow. However, there is plenty of precedent for introducing one-off attributes, outside of Form API, for example: Table introduces #header, #sticky, and #responsive; Pager introduces #quantity, #parameters, #route_name, #tags; MachineName introduces #machine_name, etc.

Re-rolled patch attached, otherwise equivalent to #135

Status: Needs review » Needs work

The last submitted patch, 147: drupal-improve_user_password_edit-1259892-147.patch, failed testing.

aaronbauman’s picture

Status: Needs work » Needs review
fenstrat’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thanks for picking this up @aaronbauman, this is RTBC.

The name of the #title_confirm attribute makes sense.

Again, this addresses @webchick's concerns from #86 and has @Bojhan's UX sign off in #132.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -String change in 8.2.0 +Needs usability review

Thanks for the updated screenshots @aaronbauman! That's quite helpful. The screenshots are very clear and the text and arrow annotations explain the workflow.

In the future, please provide an interdiff when you update a patch.

As a release manager, I am still asking for usability signoff on the final version of this, even though signoff was given on earlier versions. I understand that the changes may seem small, but it does actually matter that the final patch receive usability review in its final form, especially for a change specifically for usability and especially for such an important form.

I definitely agree that the problem this issue is trying to solve is a usability issue, but I also agree with @Bojhan that this feels like trading one problem for another, so it really should have more usability testing to see if we can't come up with something better. Really, the workflow illustrated in the screenshots in the summary seems way too complicated to me. I think hiding the current password field is just complicated and confusing. Also, being coy and only revealing the confirm password field after the new password is entered makes it seem like we are just moving the finish line on the user. Every website I ever remember using has the confirm password field already visible when the first one is shown.

I do agree with the idea of the "reset your password" link dynamically exposing the two fields to change it (at the same time). I also think these fields should appear directly under the current password field rather than somewhere strange and far away. Can we reduce the patch to just do those two changes? If some contributors are really convinced that all the other stuff is worthwhile, we could split that into a followup, and get the minimal improvement in to actually improve this form rather than stalling on a complicated change with multiple aspects that has at best halfhearted usability and product signoff.

So, for all these reasons, tagging for another usability review. I bet the usability team could go over this in one of their weekly meetings as a group. They can evaluate whether to implement my suggestions, or go with the current patch, or something else.

Finally, this issue is also tagged for accessibility review, which is also a core gate, and it hasn't received that yet. Thanks!

aaronbauman’s picture

Thank you for the detailed feedback, xjm. In general I agree with the sentiment that we're just trading one problem for another.

This statement, in particular, got me thinking:

Every website I ever remember using has the confirm password field already visible when the first one is shown.

I reviewed 10 of Alexa's top sites, and every single one of them offers a single, dedicated form to change one's password. The user experience of a single form with two password fields is very straightforward, so there's no reason for the shenanigans outlined in this issue.

We've been going around in circles on this issue for more than 5 years. In the meantime, the companies who employ thousands of people to focus on usability and accessibility have converged on a consistent pattern. Let's remove the "change password" fields from the user edit form, and move them to a standalone form.

In addition to providing users an experience to which they're more accustom, there are many other advantages:

  • Simpler user edit form - removes 2 fields and a bunch of extraneous text
  • Simpler password form - just 3 fields necessary without extraneous text related to changing email address
  • All fields can be required on the standalone password form
  • Unrelated validation issues do not force user to re-enter password(s)
  • Password-change form gets its own URI
wturrell’s picture

+1 for moving to a separate form.

As others have mentioned previously, one of the problems with passwords on same form as everything else is the annoyance of having to retype them all if there is an error elsewhere.

I'd also be interested to know if there's any measurable difference in usability between 'Reset your password' (our current standard) and 'Forgot password?' text.

dmsmidt’s picture

yoroy’s picture

Status: Needs review » Needs work
Issue tags: -D8UX usability, -Needs usability review

We discussed this during yesterday’s usability meeting: https://youtu.be/OaeH-dEe4GE?t=14m24s

Current situations is indeed not good. The solution this patch now arrived at adds too much "magic" with the hiding and showing of the password fields.

The ultimate proposal to move both change email and change password to separate forms is the way to go. Thank you @aaronbauman for doing that research! Creating these new pages and the links to them from the account edit page is definitely a task for 8.4 and needs some design work (what will the links to these pages look like?)

If we want to add some improvement to 8.3 we need to stay within this account edit page and look into reordering the fields into a more sensible flow.

The challenge in staying on this page is that we need the current password for two different operations: change email, change password.

What about grouping the 3 password reset fields in a fieldset? I created a short video sketching out an idea for that: https://www.youtube.com/watch?v=gcr3AMuqNaU

aaronbauman’s picture

@yoroy, i think this is a good proposal for 8.3
A couple points of feedback:

1. This doesn't address @xjm's concern about "moving the goalposts"
Personally, I think if "current password" is revealed on focus of email field (rather than on change), it would address this concern.

2. I can further anticipate feedback that having a duplicate "current password" field is not a great UI.
If there were some additional magic to make sure that the user only has to enter "current password" field one time, then I think this would be a good solution for 8.3.

I mocked up a crude prototype of this UI proposal:
https://jsfiddle.net/pufc9rrg/

and recorded a brief video of how this might work:
https://youtu.be/hS8yQlAbJlA

Maybe this is too much javascript magic for one form?

If we remove all the javascript magic, this approach would at least accomplish xjm's other suggestion:

I do agree with the idea of the "reset your password" link dynamically exposing the two fields to change it (at the same time). I also think these fields should appear directly under the current password field rather than somewhere strange and far away.

xinyuma’s picture

This design pattern could be a solution https://www.drupal.org/node/2883755

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.