Users could not find the Change password fields

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.

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.

Adding an additional tab might be a good idea.

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

#993592: Vertical Tabs on User Account Edit page

Some screenshots would help review this.

suggest change "Password" to "New Password"

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

subscribe

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

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?

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

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

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.

Here you go.

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.

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?

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?

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

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).

Added anchor to link.

Patch rerolled successfully.

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

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

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

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

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

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.

Fixing image order.

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.

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?

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.

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

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

@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..?

@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 .

@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?

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.'

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

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

@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?

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

Great, thanks @realityloop.

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

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.

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.

updated patch

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.

@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)

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.

This needs a reroll.

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

Rerolled from #67

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

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

However these bouth is unable to apply now :P

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

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?

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

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."

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

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.

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!

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

1. Original version

2. Proposed changes

3. Changes implemented

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?

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

I will review this now.

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.

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.

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.

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.

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.

just a comma for grammar was accidentally taken out earlier.

+++ 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.

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.

this changes it's to it is

--

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

Assigning to myself for a complete review

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.

@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.

adding tag based on #100

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.

Patch rerolled.

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".

#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.

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.

Hmm, trying to display the last patch only.

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.

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

Patch rerolled from #108

Corrected description single quote with /' and queued for testing

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.

I am going to re-reroll this tonight.

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.

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.

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.

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)

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.

Marked that for UX review

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.

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.

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.

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.

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().

Nice catch :) Here are the changes.

opppss forget to attach interdiff. So here is that.

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.

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

Hmm failures seem unrelated.

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!

(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.

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.

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

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.

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!

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

+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.