Replace confirm password element with a new element that allows toggling to view the typed password

It would be really cool if the Must contain values got crossed out as soon as you filled them in.

That being said, there will be a need to test this if we adopt this new pattern.

I notice in the example that the descriptive text is also above the input form. Would this rely on #314385: Make position of #description configurable via the API?

There's a contrib module that implements the show/hide password functionality: https://www.drupal.org/project/password_toggle

However, I don't think that removing the confirmation element is a good idea. Showing the password is a great usability improvement, but only if you are somewhere where you're not overlooked! If you're sitting in an office, or in a café, or doing a presentation, then you can't do that, so the confirmation element is necessary as a fallback.

I have written an implementation for this (see Redesign password strength indicator so it's less fragile). Screenshot is attached (Bartik as admin theme). This 'show/hide' field is currently not inside the input element but outside (as you can see).

I think it's a good idea, especially on mobile. Having to type the same thing twice feels like a bad user experience to me. I've never really liked that. Anyway, even if we don't agree on that; I hope that we can agree that a show/hide field would be a nice improvement?

I like the idea of the issue.

About #6, I like that implementation is outside of the box, so it doesn't conflict with LastPass, or other chrome extensions that put their stuff inside of password boxes

Also, notice that Internet Explorer 10 and 11, has this functionality already inside the password box:
http://www.askvg.com/how-to-disable-show-password-button-in-windows-8-wi...

I'll try to post the patch here so we could experiment with this.

I'll try to get this working again :)

As a way to get started, I've included (copy/paste) the behavior from the password_toggle module.

Works that needs to be done with the javascript:
- Change the checkbox to a text/link (after the input field).
- Remove all the IE overhead that's currently in the behavior.

As soon as that part is working, we can start removing the confirm password field.

Hah, did you check #2247049: Redesign password strength indicator so it's less fragile

@Bojhan: What do you mean? I was involved in that issue you're mentioning. In comment #36, this issue was created as a follow-up? That's why it's marked as a related issue ;-)

Please don't forget the JavaScript tag when working on JS issues :)

The JS needs work. We use .find explicitly instead of context selector, we only use .on, not the shortcut versions, Ideally we'd detect IE10+ and don't run this if the show password feature exists in the user's browser (hopefully we can detect this without sniffing…).

Basically, we can't just copy/paste the contrib JS here.

@_nod: Would it be okay to add a button and whenever the button is clicked, the password property changes: type = 'text';. Since we've dropped support for IE8, this could be a short solution? All browsers support changing the input field type, and IE8+ supports this as well.

The script would then:
- Check if the browsers already has show/hide password functionality. If not:
- Add a link (or button) after the input field with a text.
- Whenever the link is clicked: change the text (show - hide) / change the type (text - password).

Sorry 'bout the simple copy/paste. ;-)

If changing password input type isn't a problem anymore, that's cool with me. Simpler is better.

As for the detection it's too bad they give only the pseudo-element. I don't have IE around anymore so I can't check it out.

quick question, do we want that on all password fields? including custom FAPI forms and all?

The screenshots arn't as subtle as in the OP. I am not sure the current direction makes sense.

@Bojhan: The screenshots with the checkbox were just to demonstrate the way the contrib module handles with this. There's a new patch #19, were a button is added instead of a checkbox. This patch follows the image from the issue summary.
As said in #7, we are placing a button or text outside of the password field, to overcome issues with browsers extensions and IE.

Reviewing the JS from #17, (nice work!) I saw that we could avoid querying the whole document twice (once for :password, another one for data-show-password-trigger)

Other fixes:
* Adding comment
* Adding $context = $(context) as I see is the pattern in other Drupal behaviors in core.
* Not creating the element showPassword unless is needed. (I was thinking to .clone() it if you don't like it inside of the closure)
* Moving the data-state="hidden" as part of the html string
* Name-space for the click event

Questions:
* should we just use parent() instead of .closest('.form-type-password') ? Which one should be more reliable ?
* Should we use an "a" element instead of "button" ?

Demo: http://jsfiddle.net/corbacho/6eQZX/

Sorry, no interdiff because output wasn't any useful.

Since we're talking about replacing the confirm with this button (no empty <a>) I removed the code related to the confirm functionality so if we're going this way we'll need a change notice.

Simplified the code a little. Rely on a data-drupal-revealpass attribute to do it's thing. Works pretty well so far.

We could probably create the button element from the PHP directly (in the form_process_password_confirm function) and add a "for" attribute or something so that we can do event delegation without relying on .parent() or .closest(). Anyway that's what I got so far.

fail fix.

The order of the elements could present accessibility challenges like identified in #1811240: Improve "password matches" and "password strength" accessibility so just addign this as a related issue.

Fun to run across this pattern here:
https://designpatterns.hackpad.com/Passwords-4dSSBUhCYjj

This is the hackpad for the UK Gov's https://www.gov.uk/service-manual/

Posting a reroll against 64de978a08663904ba8231f20d2f26c8f5a135e8 @ 8.0.x.

There are coding standard issues, Tabs are used in some places, it should be 2 spaces.

It's neat to see this pattern. The input form needs a label. It's fine to have that label be invisible, but screen readers need to have it announced to them what the form is.

That being said, it's better laid out for screen readers, so think it's something that will a big usability & accessibility improvement when we get it in.

Removed the coding standard issues.

Moving this forward has a couple of issues, changed the Issue summary to reflect these issue (API changes).

1. This restructures the DOM around a rendered password_confirm field. This breaks the handling of the password_confrm DOM in the user module (user.js) It expects a certain layout to attach password strength and password hint elements. (and possibly affects other modules)
2. This breaks drush install because it expect a pass1 and pass2 field for setting admin password
3. All password_confirm elements would now require a #title to be set in the form definition. In the original version a title was set when splitting the element into two fields, so no titles are currently set by modules using password_confirm

In view of these issues it seems unlikely that this can still be added to D8 and should maybe be moved to contrib.

Marking this as 'needs review' to run the testbot, but this still needs work.

This looks sweet!

We could re-use some of the JS/CSS from #1811240: Improve "password matches" and "password strength" accessibility. And add Drupal.announce too.

Reading #36, I feel like this is to late to add this functionality to Drupal 8. Any other thoughts? Marking as postponed meanwhile.

This needs a reroll again. I would say this would be a pretty damn cool feature to bring into D8, but I have a hard time not seeing it as a feature. In which case we could bump it to 8.1.

Any other thoughts?

+++ b/core/misc/revealpass.js
@@ -0,0 +1,38 @@
\ No newline at end of file

Small issue ;) I'll try to look into the rest of the patch again later today.

Luke has published a new interesting research
Showing Passwords on Log-In Screens http://www.lukew.com/ff/entry.asp?1941

Rerolled #36, 1) and 3) are taken care of, about 2) and drush, they're releasing same time as D8 betas so it shouldn't be a huge issue. NR for testbot.

Rerolled #53 for #2348321: Upgrade to jQuery Once 2.x

Set issue to the right version so patch gets applied to 8.0.x

One more thing I'd like to add to my list in #36:
4. Name of the form element should be changed to something else then 'password confirm', because that is no longer what this field does. 'password reveal' or something along those lines? Or should we keep the name for ease of migration....

Let's see if this applies...

Hopefully this fixes most of the fails...

Sweet, good to go for me. Let's hope someone can RTBC this.

Need to ping drush maintainers and check what needs to be done on that end.

Bit of nitpicking...

+++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
@@ -177,6 +177,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
diff --git a/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php

Docblocks needed to be updated.

Some screenshots from the last patch. Would be so nice to have this in D8.

Looking good for me !!!
Some screenshots from #59 patch !!

Also Button link is collapsed with password input field in LTR and RTL, So Adding some margin for link button :)

.password-parent button.link {
  margin: 0 10px;
}

Patch is missing the revealpass.js file.

Also we can't rely on password-parent class to style revealpass because it's a class added by the user module and we don't want to tie this feature to user module. We need some CSS help, don't like using px much for this, ideally we'd add the space inside the CSS with before and content or something. Like we did for admin links (the edit in machine name).

We need to add a span around that button to add the space in CSS. Need to have someone tell us which name we should give the class on the span.

Adding revealpass.js in patch.

replaced px with em

.password-parent button.link {
 margin: 0 0.75em;
}

@nod_ Would form-type-password-confirm Or form-item-pass work ? rather than password-parent.

forget to attach patch :)

Lets kick the testbot into action....

Updated after #2457887: Use Utility\SafeMarkup class instead of Utility\String for placeholder(), checkPlain(),format() functions changed some unrelated things.

Rerolled.

@lewisnyman Found some issue on installation screen.

1. margin is not consistent of the link. (Show password / Hide password)
2. undefined text under password feild

These issues are only on installation screen, Please find screenshot.

We missed a place, changed it let's see how it goes.
didn't touch the style.

Seems like issues are still there ? Can you Please check the screenshot and comment posted in #79.

@nod_ you found it first :-) Nice one.

@Manjit.Singh can't reproduce the issues you show in #79 with the patch in #80 in Safari or Chrome, what browser are you using?

Found this issue in chrome while installing new site.

Steps to reproduce.

1. Install site from scratch.
2. when you came across Configure sitepage just apply patch #80.
3. Clear cache from database manually.
4. Refresh Configure site page.

I have done these steps and facing this issue. Can you please try it at your end. Hopefully you will get same issue.

@Manjit.Singh nope, still just works for me. Can you try patching first and then doing the install from the beginning, that is after all how it will work in the end.

It will cause Password Manager doesn't work.

Needs a bit of polishing. Can someone please finish this awesome patch? Would make every dev his live less miserable who has to deal with UX designers ;)

I attached some screenshots to show that it sometimes brakes a bit on different screen sizes.
I tried the installer and the user/edit page.

#78 has some starting instructions.

+ // @todo remove element?

And we need to take care of this.

Rerolled the latest patch

Removed some more pass1/pass2 references that have snuck in and the Confirm test.

Not sure where that extra file came from.

So passes tests:) but the strength indicator seems to be gone at the moment.

Oh that was easy, hopefully I attached it in the right place

Nice to see this moving again!

1. Missed a spot in stable theme to remove .password-confirm-match styling.
2. Issues raised in #78, #86 and #87 still need to be addressed (tested it and those issues still exist)

3. +++ b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php
   @@ -28,94 +27,26 @@
   +class PasswordConfirm extends Password {
   

   We should really rename the element to something like PasswordReveal (and change all references in classes and such from confirm to reveal), the current naming makes little sense except from a BC stand point I feel. But maybe that should be a follow up (cause I realise that would make this a much bigger change)?

Manually tested this in Bartik, Seven, Stark and during install and the reveal works and password strength and hints show up in all.

#99.3: We could create new class called PasswordReveal and PasswordConfirm would extend that. How ever we cannot change our existing usages to use that new element since we have to maintain BC.

Removed the leftover comfirm selector
Added media query for margin
changed selector to new class
removed element in test

+++ b/sites/development.services.yml
@@ -5,3 +5,6 @@
+parameters:
+  twig.config:
+    debug: true
 

this snuck into the patch

Oops.

And oops again, that last patch is incorrect, hence the 15KB growth.

Fixed some eslint warnings caused by the new JavaScript.

I applied patch replace_confirm-2293803-105.patch locally on a clean Drupal install and was able to test the hide password / show password functionality without receiving any errors. I tested the patch by running drush uli to generate a link to create a new password. I then entered the new password and clicked the show password / hide password link.

1. +++ b/core/misc/revealpass.js
   @@ -0,0 +1,51 @@
   +          var $trigger = $('<button type="button" class="link toggle-password">' + showPass + '</button>');
   

   we should not loop this inside each.

2. +++ b/core/misc/revealpass.js
   @@ -0,0 +1,51 @@
   +          $trigger.on('click', {password: element}, revealClickHandle);
   +          $trigger.insertAfter(element);
   

   IMO, it should be init from revealClickHandle function and provide a way to override default actions.

The script is pretty simple, I would expect people to override Drupal.behaviors.revealPass if they want override the behavior.

They can still register a delegated event listener is they want to do something on the click.

The #107.1 makes sense but could you please elaborate #107.2 since it's over my head :)

Edit: Cross posted with nod_. Anyway if that is wanted to be done, I need some more help to get it done!

- * Formats as a pair of password fields, which do not validate unless the two
- * entered passwords match.
+ * Password form element has hidden text that may be revealed by the user.
  *
  * Usage example:
  * @code
@@ -28,94 +27,26 @@
  *
  * @FormElement("password_confirm")
  */
-class PasswordConfirm extends FormElement {
+class PasswordConfirm extends Password {

Why is this called "PasswordConfirm" if it doesn't involve confirming the password?

And related to that, why is it modifying the existing PHP class + form element rather than creating a new one, called e.g. PasswordReveal?

#99 raised some of the above points as well.

See also #90 - for backwards compatibility shouldn't this be an opt-in feature (not necessarily a new module, but opt-in for existing sites in some way)?

+++ b/core/misc/revealpass.js
@@ -21,16 +32,20 @@
+        .each(function revealLink(index, element) {

This should be an anonymous function when it was moved in the each() in #105

As part of SprintWeekend2016 Chuchunaku and I re-rolled the patch using the comment in #111. Other questions remain in comment #110.

So, rewrite per #90, #99 and #110.

Don't replace the current confirm functionality but add a new element and allow sites to opt-in using it on user edit pages.

Since it's an opt-in after install, it won't be used on the install pages (so no breaking of drush).

The new element would still need tests if this is the direction we want to take this.

Did some manual testing and the strength indicator and hints show up and in the right place as far as I could tell.

Think this should fix most (all?) of the fails.

Few things slipped from the last time I checked. All good on my end now.

Looks very sweet!

I think we'll probably need an update hook in user.install to set the property in user.settings - but not 100% sure though.
(note: if it does, I think an upgrade path test would be very much over the top though)

Thanks for the great JS rewrite @nod_!

Didn't look into #117, but thanks for pointing that out @swentel. I have no idea if this is needed. If somebody knows, let us know!

Added some tests for the new element and for using it in the user edit form. Plus removed some residual confirm references in reveal.

Great to see that configurable, otoh looking on that from entity api side (see related) that should be configurable with widget.
Also introduction of new setting should have upgrade for existing sites and covered with tests

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -170,6 +170,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $form['registration_cancellation']['user_password_type_reveal'] = array(
...
+      '#default_value' => $config->get('password_type_reveal'),

this needs upgrade path with test

1. +++ b/core/lib/Drupal/Core/Render/Element/PasswordReveal.php
   @@ -0,0 +1,55 @@
   +    $info['#process'][] = [get_class($this), 'processPasswordReveal'];
   

   Note: you can replace get_class($this) by static::class.

2. +++ b/core/lib/Drupal/Core/Render/Element/PasswordReveal.php
   @@ -0,0 +1,55 @@
   +    $element['#attributes']['class'][] = 'password-field';
   +    $element['#attributes']['class'][] = 'js-password-field';
   

   I'm curious, is this something we could do in a template layer or does our JS require those classes?

3. +++ b/core/misc/revealpass.js
   index 8372ccd..7e12609 100644
   --- a/core/modules/user/config/install/user.settings.yml
   
   --- a/core/modules/user/config/install/user.settings.yml
   +++ b/core/modules/user/config/install/user.settings.yml
   
   +++ b/core/modules/user/config/install/user.settings.yml
   +++ b/core/modules/user/config/install/user.settings.yml
   @@ -13,4 +13,5 @@ register: visitors
   
   @@ -13,4 +13,5 @@ register: visitors
    cancel_method: user_cancel_block
    password_reset_timeout: 86400
    password_strength: true
   +password_type_reveal: false
    langcode: en
   diff --git a/core/modules/user/config/schema/user.schema.yml b/core/modules/user/config/schema/user.schema.yml
   
   diff --git a/core/modules/user/config/schema/user.schema.yml b/core/modules/user/config/schema/user.schema.yml
   index 627d8a6..81f224b 100644
   
   index 627d8a6..81f224b 100644
   --- a/core/modules/user/config/schema/user.schema.yml
   
   --- a/core/modules/user/config/schema/user.schema.yml
   +++ b/core/modules/user/config/schema/user.schema.yml
   
   +++ b/core/modules/user/config/schema/user.schema.yml
   +++ b/core/modules/user/config/schema/user.schema.yml
   @@ -50,6 +50,9 @@ user.settings:
   
   @@ -50,6 +50,9 @@ user.settings:
        password_strength:
          type: boolean
          label: 'Enable password strength indicator'
   +    password_type_reveal:
   +      type: boolean
   +      label: 'Use password reveal on account edit page'
    
    user.mail:
   

   We need an update hook for that

4. +++ b/core/modules/user/src/Tests/UserEditTest.php
   @@ -106,6 +106,19 @@ function testUserEdit() {
   +
   +    // Test editing the user with a password_reveal field.
   +    $config->set('password_type_reveal', TRUE)->save();
   +    $config->set('password_strength', TRUE)->save();
   +
   +    $this->drupalGet("user/" . $admin_user->id() . "/edit");
   +    $this->assertRaw('Password strength:', 'The password strength indicator is displayed.');
   +
   +    $edit = array();
   +    $edit['pass'] = $this->randomMachineName();
   +    $edit['current_pass'] = $admin_user->pass_raw;
   +    $this->drupalPostForm("user/" . $admin_user->id() . "/edit", $edit, t('Save'));
   +    $this->assertRaw(t("The changes have been saved."));
   

   Isn't type_reveal the default now, so we should also add a test which tests the other case?

Thanks so much! Looks really great. A few small tidbits.

$info['#process'][] = [get_class($this), 'processPasswordReveal'];

Now Drupal 8 went with PHP 5.5 so you can just do (check https://3v4l.org/1HEea)

$info['#process'][] = [static::class, 'processPasswordReveal'];

+    $element['#attached']['library'][] = 'core/drupal.revealpass';

Can we move this up a few lines as it's looking rather odd in the middle of #attributes modifications?

Finally, the issue summary needs an update as the API changes listed say "Programmatic password_confirm submits now require one value instead of 'pass1' and 'pass2'" but it seems this is now config dependent.

Thanks for the great feedback all.

Updated the issue summary per @chx

I'm not to clear on the whole update path (let alone a test for it), so took a stab at it, but you might be referring to something else.

120.2 Yeah the JS uses those classes.
120.4 Currently type_reveal = FALSE as a default, so I don't think we need any tests for that (all user edit tests are run with that setting, so that should be enough right?)

Added relationship to #1259892

Users could not find the Change password fields

Bug : If i try to apply a patch then the existing functionality of 'password match' is not working.

Fixing confirmed password status issue.

Adding the correct patch file.

@Prashant.c thanks for looking at this! The patch seems to be missing all the new files from #122

And since we now have javascript testing, this should get some javascript tests. And the update path also still needs a test.

@Prashant.c The only change that i found in the latest patch is in /core/modules/user/user.js except the missing files.
Can you please create a fresh patch with your changes ?

@Lendude , @Manjit.Singh
Thanks, made the suggested changes into the new patch file, same needs to be reviewed.

this seems like not created with current head :( fails to apply.
Also please attach interdiff also in future. It help reviewers.

Here is a re-roll.

And here is the interdiff

@BarisW thanks for the reroll! Manually tested this again and still works.

Bit of nitpicking:

+++ b/core/lib/Drupal/Core/Render/Element/PasswordReveal.php
@@ -0,0 +1,55 @@
+/**
+ * @file
+ * Contains \Drupal\Core\Render\Element\PasswordConfirm.
+ */

this needs to go

+++ b/core/modules/user/user.module
@@ -1271,6 +1274,44 @@ function user_form_process_password_confirm($element) {
+ * elements to add the JavaScript and string translations for dynamic password
+ * validation.

The copy/paste 'dynamic password validation' comment probably needs a change.

And we still need a Javascript test for this. Setting to needs work mostly for the javascript test.

Like I said, needs work...

Added Js tests and fixed comments from #139.

This new patch checks that two reveal_passwords fields can work together.

This needs an update on the tests now that #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits was committed.

Hm, after reading #2851541: [policy, no patch] No new WebTestBase tests in Drupal 8 after mass-conversion...

If a WTB already exists, and additional coverage is appropriate in that class, then add in new tests methods or assertions as needed

It seems we don't need a modification of the patch. Because the WebTestBase already exists in the user module. Back to NR.

Rerolled after array > [] conversion. As an interesting note, I used the php-short-array-syntax-converter like other devs are doing but for this particular patch, some ); need to be converted by hand.

I re-rolled the patch because it wasn't applying cleanly anymore. I also fixed 3 minor coding style issues:

core/misc/revealpass.js
33	Missing JSDoc parameter description for 'element'. (valid-jsdoc)
33	Missing JSDoc parameter description for 'index'. (valid-jsdoc)
core/tests/Drupal/Tests/Core/Render/Element/PasswordRevealTest.php
3	Namespaced classes, interfaces and traits should not begin with a file doc comment

Currently testing the patch and doing some further code review.

It works very well. I didn't provide text saying what's going on in the attached PDF, but it basically goes through the whole process: running update.php, configuring it on, testing it out, logging on to make sure it works, then changing it back.

What is the reason to make this an optional setting instead of the new default? I wonder why core itself should provide two different ways of handling this.

Making such change require strong reasons in issue summary.

There's no agreement on usability reasons, at least they not explained in comments!

new form element introduced, but why it's not integrated to existing?

I pointed #2227381: Apply formatters and widgets to User base fields 'name' and 'email' and I'm sure proper widget is a way to go.
but what about install form and tests that supposes there should be 2 fields (all over contrib).
And it requre product manager review + maybe postpone til 8.5

but what about install form and tests that supposes there should be 2 fields (all over contrib).

If I remember right that was the reason why it was not set as default. At least it broke drush, probably other things.

( edit ) also, 3 years ago. time flies.

Existing sites that are "themed" could be ugly if we swap the password widget. But we could/should enforce it for new installs and deprecate the old widget.

It broke the PasswordManager #85.

By the way,

Why the ref articles talk about LOGIN SCREEN, but here used in a Change Password screen. Does it correct?

Just reroll of #148 for core version 8.5.x

I checked this patch on real projects, patch works good for me

Thank you for rerolling @vprocessor. We need peer review on this and as per #151 it needs work.

whoops

Although the patch works within the change password area the functionality that I believe was desired is the ability to do a reveal password on the login page so that the user can see what they are typing in. Lord knows there are enough uneducated users out there that don't remember the caps lock.

My two cents

I could see the usefulness of allowing people to turn the strength indicator on and off in certain instances. Here's an updated version of #156 but allowing the data-drupal-password-strength value to be set at the form level and passed on through to JS.

I think this one should require an accessibility maintainer's sign-off before committing. @mgifford and I are both following this. I like the ideas here though, it would be good to have this.

Is this just about replacing the dual-field widget for creating passwords (profile edit, site installation) or will this be used on the user login form?

Depending how it gets implemented, there are potential security issues that impact visually impaired users. If a screen reader announces it as a password field, users may assume what they type is masked out, and be unaware that it's on screen and readable over their shoulder. And vice versa, it may be visually masked out, and a screen reader announces the keys being pressed.

I think patch #162 lost some stuff from #156 that is still needed. It no longer has the JS function revealClickHandle(), for example. I'm attaching an interdiff for someone else to do a re-roll.

I went scanning though the patches for important semantics like role or type attributes. I can see the approach of revealClickHandle() in #156 is toggling between <input type="text"> and <input type="password">. That's good to see, and is probably sufficient to answer the security concern I put in #164. I had been worried by some approaches I'd heard about where a text field is laid on top of a password field using CSS. I'll do some manual accessibility testing on this soon hopefully.