WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 4 weeks ago

#42888 assigned enhancement

Add a "Show" button next to password fields on mobile

Reported by: johnbillion Owned by: shadyvb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch has-screenshots has-ux-feedback
Focuses: accessibility, javascript Cc:

Description

It's become quite common for web services to display a Show button next to password fields when the user is using a mobile device. Several mobile apps do this too. The intent is to help the user ensure the password they've entered is correct when using the onscreen keyboard.

One example is on https://m.facebook.com (the Show button actually appears on any device when using that site). The Show button simply toggles the input type between password and text.

WordPress should add this functionality to the login screen, the user profile screen, and anywhere else where a password gets entered. This functionality already exists when adding a new user, if the user first clicks the Show password button.

Attachments (17)

Screenshot 2017-12-13 01.56.15.png (16.0 KB) - added by johnbillion 19 months ago.
Screenshot 2017-12-13 01.56.44.png (23.5 KB) - added by johnbillion 19 months ago.
42888.diff (1.6 KB) - added by audrasjb 19 months ago.
42888-show-password-button
42888-patch-full-behaviour-on-desktop.gif (115.7 KB) - added by audrasjb 19 months ago.
42888-patch-full-behaviour-on-desktop
42888-patch-hidden-Android.png (221.0 KB) - added by audrasjb 19 months ago.
42888-patch-hidden-Android
42888-patch-visible-iOS.png (506.5 KB) - added by audrasjb 19 months ago.
42888-patch-visible-android
42888-patch-hidden-iOS.png (492.6 KB) - added by audrasjb 19 months ago.
42888-patch-hidden-iOS
42888-patch-visible-iOS.2.png (506.5 KB) - added by audrasjb 19 months ago.
42888-patch-visible-iOS
42888-2.diff (1.9 KB) - added by Iceable 18 months ago.
Second iteration based on the first patch (details below)
42888-3.diff (2.1 KB) - added by Iceable 18 months ago.
Enhanced 42888-2 with aria-label value toggling
42888-4.diff (18.7 KB) - added by Iceable 18 months ago.
New approach for all password fields + accessbiility
testing 42888-4.diff.mov (409.7 KB) - added by boemedia 14 months ago.
Showing testing of patch 42888-4
longer password not visible in field.png (40.2 KB) - added by boemedia 13 months ago.
Long passwords are partly hidden due to small field, large font and 'eye' taking some space
password on mobile - overlap with eye.png (41.5 KB) - added by boemedia 13 months ago.
On mobile, password and eye-icon overlap
48222-5.diff (19.2 KB) - added by Iceable 11 months ago.
icon & tooltip instead of text + fixed overlap
42888.2.diff (18.6 KB) - added by shadyvb 4 weeks ago.
42888.3.diff (22.4 KB) - added by afercia 4 weeks ago.

Change History (58)

#1 @Iceable
19 months ago

The Show button simply toggles the input type between password and text.

This sounds like the most straightforward way to do it.

The Show button on the "add new user" page is doing it in a more convoluted way: there is one password input and one text input (hidden with display: none;), each one mirrors the value of the other.

Clicking the Show button toggles a show-password class on the wrapper around these two inputs, which in turns hides the password one and shows the text one.

I wonder if there is a good reason to do it that way as opposed to what Facebook does.

https://i.imgur.com/oBR05JC.png

#2 @Junaidkbr
19 months ago

@Iceable is right, I also don't get the practice followed in /wp-admin/user-new.php to Show/Hide password. But this practice is followed on most of the web and most of the tutorials available for the practice teach the same practice as WordPress is using.

I dug into both /wp-admin/user-new.php and /wp-login.php but I don't think we can follow the current the practice in /wp-login.php which is used already in /wp-admin/user-new.php. Reasons why

  1. The current approach uses jQuery while /wp-login.php use/load jQuery. So, either we have to include jQuery in login screen or rewrite the approach in vanilla JS.
  2. The login screen uses <p> tags to as wrapper to the input elements. Now, I am not deeply aware of the standards but following the approach in /wp-admin/user-new.php will break HTML standards and again we'll have to rewrite the structure.

I think the best option here is to just create a simple input type toggle i.e. type="text/password" on login screen.

#3 @adamsilverstein
19 months ago

Great idea @johnbillion - I agree we should have a show button wherever we have a password entry field. I could see some users not wanting password fields ever visible - especially on the login screen - for security reasons (someone could easily see your password or photograph it) Do you think we should have some sort of hook to disable?

@Junaidkbr, @Iceable - As a bit of background, the current code that uses a hidden mirrored field and toggles visibility instead of toggling the field type from password to text was introduced for internet explorer 8 compatibility, see https://core.trac.wordpress.org/ticket/32886#comment:2. Sadly, IE8 considered the field type property read only. I was responsible for writing most of that code and I can only cringe apologetically and say it was required at the time.

Fortunately, we no longer need to support IE8 in core and can proceed with simpler code that uses the text/password type swap approach. We really should consider refactoring the existing code as well, the swapping fields approach works but creates some edge cases we have had to code around.

#4 @audrasjb
19 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

Hi,
I tried a small workaround for this issue.
Here is my small first patch for it, a very first first workaround to see if my approach is good or not.
I will test it with differents configs now (I only tested it in Chrome/MacOS and Safari/iOS for the moment).

Cheers :)
Jb

@audrasjb
19 months ago

42888-show-password-button

@audrasjb
19 months ago

42888-patch-full-behaviour-on-desktop

@audrasjb
19 months ago

42888-patch-hidden-Android

@audrasjb
19 months ago

42888-patch-visible-android

@audrasjb
19 months ago

42888-patch-hidden-iOS

@audrasjb
19 months ago

42888-patch-visible-iOS

#5 @audrasjb
19 months ago

  • Keywords has-screenshots ux-feedback added

Tested on iOS and Android.

This ticket was mentioned in Slack in #design by audrasjb. View the logs.


19 months ago

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


18 months ago

#8 @SergeyBiryukov
18 months ago

  • Milestone changed from Awaiting Review to 5.0

#9 @joyously
18 months ago

This patch only covers wp-login? What about the other places that passwords are entered?

#10 @afercia
18 months ago

For some related changes to the user pages (not the login one) see #42749 and #42852.

@Iceable
18 months ago

Second iteration based on the first patch (details below)

#11 @Iceable
18 months ago

42888-2.diff 42888-3.diff is based on 42888.diff, with the following changes:

  • Swapped dashicons-visibility and dashicons-hidden icons: the first patch was using icons to describe the current state of the field ("dashicon-hidden" when the password is hidden). Since the icon is a clickable button, I think it makes more sense to describe the action (i.e. click the "dashicons-visibility" icon to make the password visible). This is also consistent with the buttons that already exist in other places.
  • Changed id="wp-show-login-pass" to class="wp-hide-pw", for consistency with the already existing "show/hide" buttons in other places.
  • Moved inline styles from first patch to /wp-admin/css/login.css.
  • Added some padding to the right password field. This prevents the password value and the icon from overlapping, in case of very long passwords.
  • [EDIT] My bad, I forgot to have the aria-label attribute for the button to toggle between Show password and Hide password. This is included in 42888-3.diff.

About other places where passwords are entered, as @audrasjb mentioned, this was a first iteration to test and validate the approach.
If I'm not mistaken, the only other places where passwords are entered are the ones below and they already have a show/hide button:

  • user-edit.php (User profile page)
  • user-new.php (Add New User page)
  • install.php (Step 1 of the "5-minutes install")

Though these are using the convoluted "IE8 friendly" implementation as @adamsilverstein explained in #3 (thank you for providing background by the way, this makes more sense now!).

So if the approach we are using here for wp-login.php is the way to go, we could take this opportunity to refactor them.

Last edited 18 months ago by Iceable (previous) (diff)

@Iceable
18 months ago

Enhanced 42888-2 with aria-label value toggling

#12 @afercia
18 months ago

  • Focuses accessibility added

Noticed a few things:

  • The current patch wraps both the input field and the button in a <label> element; this should be avoided as the label "Password" should be associated just to the input field.
  • On the other hand, some of the labels in wp-login.php wrap the associated control and also use a for attribute; this should be avoided: either use a wrapping label without for attribute or a not wrapping label with a for attribute. Never do both. As per the WordPress accessibility standards, a not wrapping label with a for attribute is preferred. Wondering if this would be a good opportunity to adjust all these labels.
  • The login screen uses <p> tags to as wrapper to the input elements. Yeah, that's not a great example of good semantics. Paragraphs should be used to markup units of text separable from the rest of the content, not to wrap form elements. I guess the paragraphs were used form some styling purposes but I don't see a good reason to keep them.
  • Worth noting a similar button is already used in the Reset Password page and in the Installation pages (see screenshots below); these pages already use jQuery because it's loaded as dependency of the user-profile script (which also makes some useful strings available in userProfileL10n); I don't see why jQuery shouldn't be used in the login page too, unless I'm missing something

As per the button content, from an accessibility perspective, some visible text would be preferable, as done in the Installation pages:

https://cldup.com/Ec8TjBPTkA.png

https://cldup.com/qrtxmkwQsO.png

Note: in the Reset Password page, the button is actually a <span> element and should be changed to a button.

#13 @adamsilverstein
18 months ago

Thanks for the review @afercia: could you work on the requested changes @Iceable ?

#14 @Iceable
18 months ago

Thanks @afercia for the review and pointers, these all make a lot of sense.
@adamsilverstein sure, not right now but I can try to work on this within a few days.

#15 @adamsilverstein
18 months ago

@Iceable Excellent, thank you!

@Iceable
18 months ago

New approach for all password fields + accessbiility

#16 @Iceable
18 months ago

I restarted from scratch for 42888-4.diff with a more global approach to work on all password fields, and included more accessibility considerations thanks to @afercia 's comments:

  • Used an actual <button> instead of a <span> in wp-login.php (login form and password reset form)
  • Added text (Show / Hide) next to the icon on login et password reset forms.

Note: this makes the button larger and the field smaller. It will be even larger for languages in which the words for "Show" and "Hide" are longer. Not sure whether this will be an issue or not, but if it is then maybe we can move the button below the field as opposed to on the right.

  • Changed the <p> tags around inputs with <div>.

I could have removed some of them completely (which actually makes no visual difference), but I thought some wrappers wouldn't hurt and may be useful in some cases.

  • Used non wrapping <label>'s' with for attributes for input fields.
  • While I was at it, I also got rid of <p> tags and adjusted <label>'s' in other forms in wp-login.php (lost password, register)
  • Enqueued user-profile.js on the login form page, and modified the password fields related functions so that:
    • They also work for the login page so we don't need to write additional functions
    • They use the "new approach" (toggling the field's type between text and password instead of using mirrored fields) for all password fields: login, password reset, add new user, edit user

Note: jQuery was not loaded on the login page before. It is now since it is a dependency of user-profile.js. I don't see it as an issue, but worth noting.

  • Login and Password reset pages: added the .no-js class to <body>, so we can use .hide-if-no-js to hide the toggle button when JS is not available.

[edit] Just realized after uploading the patch that the button doesn't have the right height in Firefox. Changing line 94 in login.css from height: 100%; to height: 40px; fixes it.

Last edited 18 months ago by Iceable (previous) (diff)

#17 @DrewAPicture
18 months ago

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

Assigning to mark the good-first-bug as "claimed".

#18 @boemedia
14 months ago

  • Keywords has-ux-feedback added; ux-feedback removed

I applied the patch using Grunt. It's my first time, so I may have done something wrong. If I tested the wrong way, please forget my comment here asap (I'll keep learning ;))

I used 42888-4.diff by @Iceable. See my short video for visual support.

I tested in Chrome on a macbook, version 66.0.3359.181.

  • The text show is there, but totally at the right side of the browser screen.
  • When I click 'show', the text changes to 'hide', but nothing happens in the password field, I still see dots instead of text.

I suppose the show/hide part was meant to be next to the password field instead of al the way to the right. But showing the password to check typing is a great improvement from a usability perspective, so I'd love to see this implemented.

https://cl.ly/302F1t3X2b1o

Last edited 14 months ago by boemedia (previous) (diff)

@boemedia
14 months ago

Showing testing of patch 42888-4

#19 @Iceable
14 months ago

  • Keywords 2nd-opinion ux-feedback added

Thank you very much @boemedia for taking the time to try it!

I thought this patch would need a refresh as it is already 4 months old, though it still seems to apply and work correctly on my end. I just re-tested it on a Debian machine and on a MacOS one, both with a brand new VVV install.

Steps taken:

Here is a screencap of wp-login.php in Chromium 66.0.3359.117 (Developer Build) on Debian 9.4.

https://i.imgur.com/NLLAzeq.gif

It also looks and works the same for me in Chrome 66.0.3359.181 (Official build) on MacOS.

Not sure whether there is something wrong with this patch that doesn't show on my end, or if something went wrong during your test.

Additional tests and/or 2nd opinion are most welcome.

#20 follow-up: @adamsilverstein
13 months ago

@boemedia can you please give this another test following the steps @Iceable outlined? notably, use grunt patch and grunt build to make sure the patch applies to the correct code and is built into the build directory.. Also, since https://core.trac.wordpress.org/changeset/43309 you'll need to be testing by running WordPress from the build directory as well.

Thanks!

Last edited 13 months ago by adamsilverstein (previous) (diff)

#21 in reply to: ↑ 20 @boemedia
13 months ago

  • Keywords has-ux-feedback removed

Me & @Iceable briefly sat together during WCEU contributor day. We checked this on both mine (MAMP) and his (VVV) local environment, but there was a difference in display. We couldn't quite replicate what caused the display being wrong on my install (we did use grunt build) because of time and wifi issues, I have to see if I can get VVV installed on my computer and test again. Maybe someone else can test on MAMP and see if they can replicate my issue?

Replying to adamsilverstein:

@boemedia can you please give this another test following the steps @Iceable outlined? notably, use grunt patch and grunt build to make sure the patch applies to the correct code and is built into the build directory.. Also, since https://core.trac.wordpress.org/changeset/43309 you'll need to be testing by running WordPress from the build directory as well.

Thanks!

#22 @boemedia
13 months ago

  • Keywords has-ux-feedback added; ux-feedback removed

With the help of @adamsilverstein (thanks a lot!), I installed a clean local dev environment and tested the patch again. It now works fine, and I think it would be a great improvement for usability.

Two things to take in consideration:

  • with super long passwords (that seem to be trendy these days ;)), the box is not showing the full length of characters. The font size is rather large compared to the field size. Do we need to restrict the login and password box to this size, or could it be slightly bigger? That, combined with a smaller font size maybe?
  • on mobile view, the 'eye' overlaps with the password field. Either give the 'eye' a white background, but ideally, here a bigger field size would be better too.

See my screenshots for clarification.

Additonial: with other languages, show/hide may be longer words and will take up even more space.

Last edited 13 months ago by boemedia (previous) (diff)

@boemedia
13 months ago

Long passwords are partly hidden due to small field, large font and 'eye' taking some space

@boemedia
13 months ago

On mobile, password and eye-icon overlap

@Iceable
11 months ago

icon & tooltip instead of text + fixed overlap

#23 @Iceable
11 months ago

I have to apologize for the time it took me to get back to this - and thank you very much @adamsilverstein for your help and @boemedia for your feedback!

  • I was also concerned with "show/hide" being long and even longer in other languages (comment:16), now that you mention it too I think it is better to remove it altogether and have just the icon, to reclaim some of the field's width.
  • The text (show/hide icon) can instead be added as a title attribute of the <button> so it shows as a tooltip on mouse-hover.
  • I would not be inclined to change the field width: this would require to enlarge the whole form which everyone is used to - is this password field width a good enough reason to introduce such a change?
  • Reducing font size would allow to show more characters, but would it still be ok for accessibility, especially on mobile?

(the two previous are actual questions, would appreciate feedback from designer/a11y expert)

  • Good catch about the overlap! Some padding-right added to the field (equal to the width of the icon + clearance) will prevent this.

Patch 48222-5.diff slightly improves on the previous iteration by taking this into consideration:

  • Icon only, also on desktop, text shows as a tooltip on mousehover.
  • This also fixes the odd height issue of the button in some browsers.
  • No overlapping with the icon if the password is longer than the field.

https://i.imgur.com/I7HSN2H.gif

Last edited 11 months ago by Iceable (previous) (diff)

#24 @birgire
11 months ago

Tested 48222-5.diff and it seems to work well on my install.

Noticed few things though:

In Chrome (Win10) the reset password screen looks like:

https://i.imgur.com/PRemO1u.jpg

where the eye-icon is pushed down.

It seems to come from height: 100% of .login .button.button-secondary, but I didn't dig deep into this.

Also just for reference, the login screen with LastPass (password manager) enabled:

https://i.imgur.com/kn3sqsH.jpg

Noticed an empty style attribute in 48222-5.diff:

<div class="wp-hide-pw-wrap hide-if-no-js" style="
">

ps: the eye-icon is also available on desktop view, but the title of the ticket suggests it should only be on mobile.

Last edited 11 months ago by birgire (previous) (diff)

#25 @afercia
11 months ago

Quickly looking at 48222-5.diff, one quick consideration:
in the last years, WordPress has been progressively removing title attributes from its codebase. It's important to not introduce new title attributes, as they serve no useful purpose, they're available only to mouse users, and thus they're not accessible. Also, it's important to avoid to use title and aria-label attributes with the same value, as the same information may be announced twice. All the title attributes should be stripped out from the patch.

#26 @birgire
11 months ago

I noticed that the button toggle state is different from e.g. the Google Account login (only visible on mobile):

The hidden icon shows, when password is hidden:
https://i.imgur.com/qLmRFnh.jpg

The visible icon shows, when password is visible:
https://i.imgur.com/HJuOcKH.jpg

This is vice versa compared to the toggle state in 48222-5.diff.

#27 follow-up: @joyously
11 months ago

Is it an icon indicating state, or a button to change state?

#28 in reply to: ↑ 27 @birgire
11 months ago

Replying to joyously:

Is it an icon indicating state, or a button to change state?

On Google Account mobile login page, it's a button ( a div with role="button" + svg ) that we can click on to toggle it.

#29 @joyously
11 months ago

OK, but what is this one supposed to be?
It should be obvious. If it's not, make it so.

#30 @afercia
10 months ago

  • Keywords needs-refresh added

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


10 months ago

#32 @peterwilsoncc
9 months ago

  • Milestone changed from 5.0 to 5.1

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

#33 @pento
6 months ago

  • Milestone changed from 5.1 to Future Release

I like this, but it looks like the patch needs some work before it's ready. Feel free to re-milestone it once it's close.

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


6 months ago

#35 @boemedia
4 months ago

Pushing this ticket up as I wished I could see the password I entered a few times over the past few weeks. Gary mentioned this patch needs some extra work. Is there anyone available to give this some TLC? Happy to test once a new patch is up.

#36 @afercia
4 months ago

Related: #42853.

Worth noting when users reset their password, they get a link to the "new password" page. The password field in that page already has a "hide/show" button. #42853 is going to make a few improvements to that button, see screenshot on https://core.trac.wordpress.org/ticket/42853#comment:12

#37 @adamsilverstein
3 months ago

48222-5.diff no longer applies cleanly :( @Iceable do you have time to work on refreshing your patch?

#38 @Iceable
3 months ago

I'm afraid I have very limited time available at the moment. I'll try to look at this in the next few weeks but cannot promise anything. If someone wants to take over in the meantime they are very welcome.

#39 @shadyvb
4 weeks ago

I'm going to work on refreshing the patch here.

@shadyvb
4 weeks ago

#40 @shadyvb
4 weeks ago

Updated the patch in 42888.2 while also fixing a few issues:

  • Password generation was broken after applying the patch, now fixed
  • Mobile view didn't have proper padding, fixed as per #42853
  • Mobile view had hide/show titles on the button, removed them, keeping only the icon
  • There was a bug on clicking cancel in profile page, the password didn't clear and that created unexpected behavior

Tested using Chrome/Safari on macOS, both mobile and desktop views.

cc @afercia @boemedia can you please retest ?

@afercia
4 weeks ago

#41 @afercia
4 weeks ago

  • Keywords good-first-bug dev-feedback 2nd-opinion needs-refresh removed
  • Milestone changed from Future Release to 5.3
  • Owner changed from Iceable to shadyvb

Thanks @shadyvb! I reviewed a bit the patch. In 42888.3.diff:

  • phpcs fixes (indentation, yoda conditions, translators comment)
  • general clean up
  • restored the focus style on the toggle button
  • added styling to the toggle button in the install page
  • removed some wrappers: seems to me they're unnecessary
  • made the "Remember me" label font size bigger

Question:
when the field switches to type=text, should we add autocomplete=off?

Some testing would be nice :) Please test also the install page, also in the responsive view.

Note: See TracTickets for help on using tickets.