WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 9 months ago

Last modified 5 months ago

#42852 closed defect (bug) (fixed)

Add new user: new password not generated when reopening Show Password

Reported by: afercia Owned by: adamsilverstein
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.7
Component: Users Keywords: has-screenshots has-patch commit
Focuses: javascript Cc:

Description

Noticed while working on #42749: After [38494], see also [34312], [33766], [33079], and [32589], in the "Add new user" screen a new password is not displayed when clicking "Show Password", then "Cancel", then "Show Password" again.

In WordPress 4.6:

  • go in the "Add new user" screen (user-new.php)
  • click "Show Password"
  • observe the generated password
  • click "Cancel": the form closes
  • click "Show Password" again
  • observe there's a new generated password displayed

This matched the behavior in the "Your profile" screen (profile.php.)

Repeat the above steps in WordPress 4.7 or more recent version:

  • go in the "Add new user" screen (user-new.php)
  • click "Show Password"
  • observe the generated password
  • click "Cancel": the form closes
  • click "Show Password" again
  • the password field is empty

At this point, there's no way to generate a new password other than reloading the page. Additionally:

  • a real password is actually generated but is not displayed in its "non-masked" form
  • the password field looks empty but the strength indicator says "Strong"
  • clicking "Hide" shows the real password field filled with dots: confusing for users, since the other field is empty

Following the argumentations on the related tickets #33419 and #37902, the implemented changes make sense. However, there's an unintended consequence: the real password field $pass1 is filled up while its visual representation $pass1Text is empty.

Attachments (8)

42852.diff (13.2 KB) - added by afercia 4 years ago.
42852.2.diff (13.2 KB) - added by pento 3 years ago.
42852.3.patch (11.4 KB) - added by bookdude13 18 months ago.
Refreshed patch
42852.4.diff (9.3 KB) - added by bookdude13 18 months ago.
A cleaner refresh
42852.5.diff (9.6 KB) - added by bookdude13 18 months ago.
Hiding again, and cached password gen
42852.3.diff (9.9 KB) - added by adamsilverstein 10 months ago.
42852.6.diff (9.7 KB) - added by adamsilverstein 10 months ago.
42852.7.diff (748 bytes) - added by adamsilverstein 9 months ago.

Download all attachments as: .zip

Change History (50)

#1 @afercia
4 years ago

See also #32979, with some feedback about the potentially confusing UI.

#2 @afercia
4 years ago

I think the root cause of the problem is some ambiguity in the controls names, as pointed out in #32979, together with the intent to automate some behaviors in a smart way. Sometimes trying to be smart can be confusing for users, as it often assumes one specific workflow while users might have a different intent.

Any change to this UI should try to respect the original intent of the feature, see The Plan for Passwords. Also, the ability for users to generate new passwords added in #33450 should be preserved, but I think it shouldn't happen when clicking "Cancel".

I'd propose to try to keep things simple, and make the UI more transparent to users.

  • rename the "Generate Password" and "Show password" buttons to "Set New Password"
  • make the button always generate a new password each time it gets pressed: the new button text will indicate exactly what the button does (apart from opening the form)
  • don't move focus or select the password programmatically: this is often based on an assumption on a specific workflow and might not be what users want, not to mention is confusing for assistive technologies users
  • make the "Cancel" button just... "Cancel" :) keeping the form open and just clearing the fields would make very clear to users what's going on

/cc @markjaquith @helen @adamsilverstein @SergeyBiryukov

Last edited 3 years ago by afercia (previous) (diff)

@afercia
4 years ago

#3 @afercia
4 years ago

  • Keywords has-patch added; needs-patch removed

42852.diff is a first try and incorporates accessibility enhancements from #42749 props to @alexstine.

Last edited 3 years ago by afercia (previous) (diff)

#4 @adamsilverstein
4 years ago

  • Keywords needs-screenshots added

Thanks for opening this ticket @afercia - I agree the user experience could greatly improved here and changing the wording is definitely part of that. I'll give your patch a test soon and try to provide some feedback.

At some point on a separate ticket, it would great to consider refactoring the JavaScript which is over complicated by needing to support IE8 which considers the input field type read only.

#5 @afercia
4 years ago

  • Keywords has-screenshots added; needs-screenshots removed

Screenshot:

https://cldup.com/0tJC7dsaAg.png

  • clicking "Set New Password" always generates a new password
  • clicking "Cancel" empties the fields; the form stays open, as closing it seems to be one of the root causes of some confusion for users

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

#7 @johnbillion
3 years ago

  • Milestone changed from 5.0 to 5.1

@pento
3 years ago

#8 @pento
3 years ago

  • Keywords needs-design added

42852.2.diff refreshes the patch to apply cleanly to trunk.

Having the password field just be blanked by the Cancel button feels kind of weird. It doesn't really give an indication of what that means for the password that this new account will have.

Perhaps it needs some inline help text to explain what the options are for setting a password? Tagging design for their thoughts.

#9 @afercia
3 years ago

If / when committing this change please don't forget props to @alexstine who originally proposed some of these changes in #42749.

#10 @pento
3 years ago

  • Milestone changed from 5.1 to 5.2

Moving to 5.2, as this needs design feedback, and patch updates based on that.

#11 @desrosj
2 years ago

  • Milestone changed from 5.2 to Future Release

This has not been touched since being punted to 5.3, and still needs design. Moving to future-release.

#12 @adamsilverstein
21 months ago

  • Milestone changed from Future Release to 5.4

@bookdude13
18 months ago

Refreshed patch

#13 follow-up: @bookdude13
18 months ago

Patch refreshed. A few suggestions to improve (needs design review):

  • When editing a user (or your own profile) the "Set New Password" button text could change to "Generate New Password" once the field appears.
  • When editing a user (or your own profile) the "Cancel" button could hide all of the fields, making the "Set New Password" button act as a toggle for their appearance
  • There is not a way to generate new strong passwords during installation
  • It should be checked that #42853 didn't get reverted in this. The code that was changed from there were removed from the original patch here.
  • Related: #42766

#14 in reply to: ↑ 13 @bookdude13
18 months ago

My last refresh was more focused on code porting than functionality...so here's another attempt :)

  • This should match the four bullets mentioned by @afercia
  • For creating new users, changed it so the fields are never hidden. This also allows for a value to always be set and visible for the password when creating users. Since it's always visible I have the text set to "Generate New Password"
  • It still needs to be tested that passwords can't be set even if they were never visible, e.g. an edit profile/user page updating the password by accident when something else updates
  • I have not tested on other browsers or mobile, just Chrome.

This still needs design review for the question of how to handle the Cancel button and where to put the Set New Password button (above or below)

Replying to bookdude13:

Patch refreshed. A few suggestions to improve (needs design review):

  • When editing a user (or your own profile) the "Set New Password" button text could change to "Generate New Password" once the field appears.
  • When editing a user (or your own profile) the "Cancel" button could hide all of the fields, making the "Set New Password" button act as a toggle for their appearance
  • There is not a way to generate new strong passwords during installation
  • It should be checked that #42853 didn't get reverted in this. The code that was changed from there were removed from the original patch here.
  • Related: #42766

@bookdude13
18 months ago

A cleaner refresh

This ticket was mentioned in PR #163 on WordPress/wordpress-develop by adamsilverstein.


18 months ago

#16 @adamsilverstein
18 months ago

  • Milestone changed from 5.4 to 5.5

Thanks for the patch @bookdude13 - this looks pretty nice, however I feel like somehow this lost some of the original intent of "Cancel" which was to close the "Show Password" section. I think a design review would be worthwhile given how much changed here.

I captured a screencast of how this looks after your patch.

https://p10.f2.n0.cdn.getcloudapp.com/items/JruWX0EQ/Screen+Recording+2020-02-21+at+01.59+PM.gif



@afercia is the issue you described still happening? When I tried to reproduce the field was not cleared when I clicked cancel. If you can still reproduce, what browser/version are you using? Screencast:

https://p10.f2.n0.cdn.getcloudapp.com/items/04uKrpWL/Screen+Recording+2020-02-21+at+01.56+PM.gif


In the mean time since this needs design review and we are approaching 5.4 beta 3, I'm punting this to 5.5.

#17 @bookdude13
18 months ago

@adamsilverstein sounds good to punt it out.

I would be fine having the Cancel button close everything up again. Updated patch added to see what that functionality would look like. That latest patch also caches the generated random password value like it used to be, so it doesn't have a delay before showing up. Awaiting design.

Another question when this gets checked: why is there a Cancel button when creating a new user? A password needs to be set, so that seems unintuitive. I'd think it should be like the install page, where you can generate again but can't "cancel" anything.

@bookdude13
18 months ago

Hiding again, and cached password gen

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


14 months ago

#19 @michaelarestad
13 months ago

  • Keywords needs-design removed

There are two flows here:

For the "New Password" flow, I think what is proposed makes sense.

For the "New user" flow, I would suggest showing this:

https://cldup.com/KQEH0gbgd_-2000x2000.png

This would remove the buttons to display the password and the cancel button.

As for the "Generate" button, I don't think that's necessary. I don't know how many folks are opinionated about their generated random string. If so, they can customize it.

What do you think?

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


13 months ago

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


12 months ago

#22 @whyisjake
12 months ago

  • Milestone changed from 5.5 to 5.6

Looks like there is some more work needed here ahead of the 5.5 release. Going to punt to 5.6.

#23 follow-up: @adamsilverstein
10 months ago

@michaelarestad thanks for the feedback!

As for the "Generate" button, I don't think that's necessary. I don't know how many folks are opinionated about their generated random string. If so, they can customize it.

What do you think?

That seems fine to me and simplifies the UI. Any problems with removing the from an a11y standpoint @afercia ?

#24 @adamsilverstein
10 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#26 in reply to: ↑ 23 @afercia
10 months ago

Replying to adamsilverstein:

Any problems with removing the from an a11y standpoint @afercia ?

From an a11y perspective, this would be a drastic simplification of the flow to set a new password to it would be an improvement.

However, I'm not sure removing the UI to generate the password would be in line with the original purpose of this feature: encouraging the use of strong passwords. I'd suggest to carefully read the reasoning behind the introduction of this feature in the related ticket #32589 and, more importantly, read the related post on Make:

The Plan for Passwords
https://make.wordpress.org/core/2015/05/11/the-plan-for-passwords/

#27 @adamsilverstein
10 months ago

@afercia Ah yes, I remember that ticket. Thanks for reminding me to review the comments there.

I now see why we need to leave the Generate Password button - if the user deletes the password, they have no way to generate a new one. (See https://core.trac.wordpress.org/ticket/32589#comment:20).

#28 @adamsilverstein
10 months ago

New user:
https://p201.p0.n0.cdn.getcloudapp.com/items/WnurbmlW/Image%202020-10-09%20at%2011.03.23%20AM.png

Edit user:
https://p201.p0.n0.cdn.getcloudapp.com/items/JrugLy5J/Screen%20Recording%202020-10-09%20at%2010.50.09%20AM.gif

One more place to check the form works as expected: the initial WordPress setup screen (fresh install). We should also make sure to carefully test this change in our supported browsers.

#29 @adamsilverstein
10 months ago

https://p201.p0.n0.cdn.getcloudapp.com/items/geuqgBkz/Image%202020-10-16%20at%204.43.11%20PM.png

Setup looks unchanged, password is generated. If I clear the password I have to refresh to get a new one.

#30 @adamsilverstein
9 months ago

  • Keywords commit added

Verified this works well in ie 11, going to commit this.

#31 @adamsilverstein
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49248:

Users: improve password generation feature.

On the user edit screen improve handling and clarify language: rename the "Generate Password" and "Show password" buttons to "Set New Password". Clicking it always generates a password. Also: improve inline code comments and descriptions.

Props afercia, bookdude13, michaelarestad, pento.
Fixes #42852.

#32 @adamsilverstein
9 months ago

In 49251:

Coding Standards: Fix WPCS issues in [49248].

See #42852.

#33 @SergeyBiryukov
9 months ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The aria-label attribute for the "Cancel" button was changed from "Cancel password change" to just "Cancel", which duplicates the button text. That doesn't seem right, I think aria-label is supposed to be more descriptive.

#35 @adamsilverstein
9 months ago

The aria-label attribute for the "Cancel" button was changed from "Cancel password change" to just "Cancel", which duplicates the button text. That doesn't seem right, I think aria-label is supposed to be more descriptive.

I'll fix that, thanks for the feedback.

#36 @adamsilverstein
9 months ago

@SergeyBiryukov aria fix in 42852.7.diff

This ticket was mentioned in PR #673 on WordPress/wordpress-develop by adamsilverstein.


9 months ago

  • Keywords has-patch added

#38 @adamsilverstein
9 months ago

  • Keywords commit added

#39 @adamsilverstein
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 49392:

Users: Password generation - restore "Cancel" aria label after r49248.

Restore the "Cancel" button aria label to "Cancel password change" after it was inadvertently changed in r49248.

Props SergeyBiryukov.
Fixes #42852.

#42 @desrosj
5 months ago

#42749 was marked as a duplicate.

Note: See TracTickets for help on using tickets.