WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 36 hours ago

#42888 assigned enhancement

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

Reported by: johnbillion Owned by: Iceable
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: good-first-bug has-patch dev-feedback has-screenshots 2nd-opinion 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 (14)

Screenshot 2017-12-13 01.56.15.png (16.0 KB) - added by johnbillion 6 months ago.
Screenshot 2017-12-13 01.56.44.png (23.5 KB) - added by johnbillion 6 months ago.
42888.diff (1.6 KB) - added by audrasjb 6 months ago.
42888-show-password-button
42888-patch-full-behaviour-on-desktop.gif (115.7 KB) - added by audrasjb 6 months ago.
42888-patch-full-behaviour-on-desktop
42888-patch-hidden-Android.png (221.0 KB) - added by audrasjb 6 months ago.
42888-patch-hidden-Android
42888-patch-visible-iOS.png (506.5 KB) - added by audrasjb 6 months ago.
42888-patch-visible-android
42888-patch-hidden-iOS.png (492.6 KB) - added by audrasjb 6 months ago.
42888-patch-hidden-iOS
42888-patch-visible-iOS.2.png (506.5 KB) - added by audrasjb 6 months ago.
42888-patch-visible-iOS
42888-2.diff (1.9 KB) - added by Iceable 5 months ago.
Second iteration based on the first patch (details below)
42888-3.diff (2.1 KB) - added by Iceable 5 months ago.
Enhanced 42888-2 with aria-label value toggling
42888-4.diff (18.7 KB) - added by Iceable 5 months ago.
New approach for all password fields + accessbiility
testing 42888-4.diff.mov (409.7 KB) - added by boemedia 5 weeks ago.
Showing testing of patch 42888-4
longer password not visible in field.png (40.2 KB) - added by boemedia 36 hours 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 36 hours ago.
On mobile, password and eye-icon overlap

Change History (36)

#1 @Iceable
6 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
6 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
6 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
6 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
6 months ago

42888-show-password-button

@audrasjb
6 months ago

42888-patch-full-behaviour-on-desktop

@audrasjb
6 months ago

42888-patch-hidden-Android

@audrasjb
6 months ago

42888-patch-visible-android

@audrasjb
6 months ago

42888-patch-hidden-iOS

@audrasjb
6 months ago

42888-patch-visible-iOS

#5 @audrasjb
6 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.


5 months ago

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


5 months ago

#8 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 5.0

#9 @joyously
5 months ago

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

#10 @afercia
5 months ago

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

@Iceable
5 months ago

Second iteration based on the first patch (details below)

#11 @Iceable
5 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 5 months ago by Iceable (previous) (diff)

@Iceable
5 months ago

Enhanced 42888-2 with aria-label value toggling

#12 @afercia
5 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
5 months ago

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

#14 @Iceable
5 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
5 months ago

@Iceable Excellent, thank you!

@Iceable
5 months ago

New approach for all password fields + accessbiility

#16 @Iceable
5 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 5 months ago by Iceable (previous) (diff)

#17 @DrewAPicture
5 months ago

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

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

#18 @boemedia
5 weeks 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 5 weeks ago by boemedia (previous) (diff)

@boemedia
5 weeks ago

Showing testing of patch 42888-4

#19 @Iceable
2 weeks 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 days 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 days ago by adamsilverstein (previous) (diff)

#21 in reply to: ↑ 20 @boemedia
4 days 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
36 hours 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 36 hours ago by boemedia (previous) (diff)

@boemedia
36 hours ago

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

@boemedia
36 hours ago

On mobile, password and eye-icon overlap

Note: See TracTickets for help on using tickets.