Problem/Motivation

It's more useful to show the user what they are typing instead of asking them to type it twice:
http://www.lukew.com/ff/entry.asp?1653= (2012)
http://www.lukew.com/ff/entry.asp?1941 (2015)

Proposed resolution

Add a password field with show/hide functionality and make it an option to use this instead of the password_confirm field

Remaining tasks

  1. Agree the usability reasons are the correct course of action
  2. Write a patch
  3. Rewrite all the tests

User interface changes

The password confirmation field can be removed and replaced with a show/hide link

API changes

  1. Programmatic password submits for user create/update now require one value instead of 'pass1' and 'pass2' depending on config

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Feature because usability improvement
Issue priority Not critical because usability improvement
Unfrozen changes Unfrozen because it only changes CSS/markup
Prioritized changes The main goal of this issue is usability
Files: 

Comments

LewisNyman’s picture

Bojhan’s picture

Title: Replace confirm password field with show/hide functionality » Add show/hide functionality to password field
Bojhan’s picture

Title: Add show/hide functionality to password field » Replace confirm password field with show/hide functionality
mgifford’s picture

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?

joachim’s picture

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.

sqndr’s picture

FileSize
19.17 KB

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?

corbacho’s picture

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

sqndr’s picture

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

sqndr’s picture

Assigned: Unassigned » sqndr

I'll try to get this working again :)

sqndr’s picture

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.

Bojhan’s picture

sqndr’s picture

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

nod_’s picture

Status: Active » Needs work
Issue tags: +JavaScript

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.

LewisNyman’s picture

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

There seems to be very little documentation of this feature compared to the caps lock warning. It looks like we can style it using a pseudo selector so at least we can remove it so we don't have two showing up at the same time?

I'll work on this patch a little today.

sqndr’s picture

@_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. ;-)

nod_’s picture

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.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
1.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,998 pass(es). View

I rewrote the JS, posting it for review before I do any styling/design work.

nod_’s picture

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

LewisNyman’s picture

hmm, the only use cases I can think of for password fields are logging in and setting your password. It feels correct to keep this functionality in the same place as password strength,

Bojhan’s picture

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

sqndr’s picture

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

corbacho’s picture

FileSize
1.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,994 pass(es). View

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.

nod_’s picture

FileSize
16.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,312 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 23: core-password-reveal-2293803-23.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
15.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,310 pass(es), 0 fail(s), and 5 exception(s). View

fail fix.

Status: Needs review » Needs work

The last submitted patch, 25: core-password-reveal-2293803-25.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
17.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-password-reveal-2293803-27.patch. Unable to apply patch. See the log in the details link for more information. View
mgifford’s picture

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.

mgifford’s picture

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/

Status: Needs review » Needs work

The last submitted patch, 27: core-password-reveal-2293803-27.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll
mauzeh’s picture

FileSize
17.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Posting a reroll against 64de978a08663904ba8231f20d2f26c8f5a135e8 @ 8.0.x.

nod_’s picture

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

mgifford’s picture

FileSize
99.55 KB

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.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Amsterdam2014
FileSize
17.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,907 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, 36: core-password-reveal-2293803-35.patch, failed testing.

BarisW’s picture

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.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: core-password-reveal-2293803-35.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 33: core-password-reveal-2293803-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 36: core-password-reveal-2293803-35.patch, failed testing.

emma.maria’s picture

Issue tags: -Needs reroll

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

Setting to 'needs work' based on the comments in #36. Surprised it passes tests.

sqndr’s picture

Status: Needs work » Postponed

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

mgifford’s picture

Issue tags: +Needs reroll

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?

LewisNyman’s picture

Sounds like a good call

LewisNyman’s picture

Version: 8.0.x-dev » 8.1.x-dev
sqndr’s picture

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

corbacho’s picture

Issue summary: View changes
Issue tags: -Amsterdam2014

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

nod_’s picture

Status: Postponed » Needs review
Issue tags: -Needs reroll
FileSize
6.89 KB
24.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-password-reveal-2293803-53.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 53: core-password-reveal-2293803-53.patch, failed testing.

Lendude’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review
FileSize
24.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 26,754 pass(es), 878 fail(s), and 2,428 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 55: core-password-reveal-2293803-55.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
697 bytes
24.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,128 pass(es). View

Hopefully this fixes most of the fails...

nod_’s picture

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.

Lendude’s picture

FileSize
24.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,147 pass(es). View

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.

mgifford’s picture

Issue summary: View changes
FileSize
11.99 KB
12.65 KB

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

Manjit.Singh’s picture

FileSize
24.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,439 pass(es), 0 fail(s), and 9 exception(s). View
14.59 KB
18.65 KB

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;
}

Status: Needs review » Needs work

The last submitted patch, 61: core-password-reveal-2293803-61.patch, failed testing.

nod_’s picture

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.

Manjit.Singh’s picture

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.

Manjit.Singh’s picture

FileSize
25.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-password-reveal-2293803-64.patch. Unable to apply patch. See the log in the details link for more information. View

forget to attach patch :)

Lendude’s picture

Status: Needs work » Needs review

Lets kick the testbot into action....

Status: Needs review » Needs work

The last submitted patch, 65: core-password-reveal-2293803-64.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
25.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-password-reveal-2293803-69.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 69: core-password-reveal-2293803-69.patch, failed testing.

nod_’s picture

Issue tags: +Need reroll
Lendude’s picture

Issue tags: -Need reroll
FileSize
25.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,436 pass(es), 3 fail(s), and 0 exception(s). View

Rerolled.

Lendude’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 73: core-password-reveal-2293803-73.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 73: core-password-reveal-2293803-73.patch, failed testing.

LewisNyman’s picture

Issue summary: View changes
FileSize
63.92 KB

Nice work everyone! I think that we are almost there, I noticed that we still have the desktop styling once the show password link wraps so it still looks indented. We need a min-width media query around that styling.

+++ b/core/modules/system/css/system.theme.css
@@ -179,6 +179,9 @@ abbr.ajax-changed {
+.password-parent button.link {
+  margin: 0 0.75em;
+}

Also this selector is wrong, we should add a new class called .toggle-password.

Manjit.Singh’s picture

FileSize
269.29 KB

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

nod_’s picture

Status: Needs work » Needs review
FileSize
26.03 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch core-password-reveal-2293803-80.patch. Unable to apply patch. See the log in the details link for more information. View
1.5 KB

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

Manjit.Singh’s picture

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

Lendude’s picture

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

Manjit.Singh’s picture

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.

Lendude’s picture

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

droplet’s picture

It will cause Password Manager doesn't work.

aspilicious’s picture

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.

aspilicious’s picture

+ // @todo remove element?

And we need to take care of this.

The last submitted patch, 80: core-password-reveal-2293803-80.patch, failed testing.

LewisNyman’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

We should be able to add this behind an experimental module/checkbox in 8.1, it would be great to have this option.

mgifford’s picture

Status: Postponed » Needs review
lauriii’s picture

Rerolled the latest patch

Status: Needs review » Needs work

The last submitted patch, 92: replace_confirm-2293803-92.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.12 KB
32.45 KB

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

Status: Needs review » Needs work

The last submitted patch, 94: replace_confirm-2293803-94.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
29.4 KB

Not sure where that extra file came from.

joelpittet’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
29.63 KB

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

Lendude’s picture

Status: Needs review » Needs work

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.

lauriii’s picture

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

mallezie’s picture

Status: Needs work » Needs review
FileSize
30.25 KB
2.21 KB

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

Lendude’s picture

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

this snuck into the patch

mallezie’s picture

Status: Needs work » Needs review
FileSize
45.86 KB
326 bytes

Oops.

mallezie’s picture

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

lauriii’s picture

Fixed some eslint warnings caused by the new JavaScript.

ChuChuNaKu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +SprintWeekend2016, +SprintWeekendBOS

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.

droplet’s picture

Status: Reviewed & tested by the community » Needs review
  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.

nod_’s picture

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.

lauriii’s picture

Status: Needs review » Needs work

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!

David_Rothstein’s picture

- * 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)?

joelpittet’s picture

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

leslieg’s picture

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
34.32 KB
11.45 KB

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.

Status: Needs review » Needs work

The last submitted patch, 113: password_reveal-2293803-113.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
950 bytes
12.38 KB

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

nod_’s picture

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

swentel’s picture

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)

Lendude’s picture

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.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +D8 upgrade path
Related issues: +#2227381: Apply formatters and widgets to User base fields 'name' and 'email'

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

dawehner’s picture

  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?

chx’s picture

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.

Lendude’s picture

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

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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

Added relationship to #1259892

Users could not find the Change password fields

Amruta_Nadgouda’s picture

Status: Needs review » Needs work
FileSize
506.84 KB

bug

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

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.

Prashant.c’s picture

Fixing confirmed password status issue.

Prashant.c’s picture

Adding the correct patch file.

Lendude’s picture

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

The last submitted patch, 127: core-password-reveal-2293803-127.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 128: core-password-reveal-2293803-127.patch, failed testing.

Manjit.Singh’s picture

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

Prashant.c’s picture

Status: Needs work » Needs review
FileSize
18.95 KB

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

Status: Needs review » Needs work

The last submitted patch, 133: core-password-reveal-2293803-133.patch, failed testing.

Manjit.Singh’s picture

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

BarisW’s picture

Status: Needs work » Needs review
FileSize
15.43 KB

Here is a re-roll.

BarisW’s picture

FileSize
1.04 KB

And here is the interdiff

BarisW’s picture

Issue tags: +Dublin2016
Lendude’s picture

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

Lendude’s picture

Status: Needs review » Needs work

Like I said, needs work...

dagmar’s picture

Status: Needs work » Needs review
FileSize
18.71 KB
4.7 KB

Added Js tests and fixed comments from #139.

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.

dagmar’s picture

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

dagmar’s picture

Status: Needs review » Needs work
dagmar’s picture

Status: Needs work » Needs review

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.

The last submitted patch, 112: issue-2293803-112.patch, failed testing.

dagmar’s picture

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.

jludwig’s picture

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.

jludwig’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.06 MB

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.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

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.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

nod_’s picture

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.

aspilicious’s picture

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.

droplet’s picture

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?