WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 5 days 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 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 (12)

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 5 months ago.
42888-show-password-button
42888-patch-full-behaviour-on-desktop.gif (115.7 KB) - added by audrasjb 5 months ago.
42888-patch-full-behaviour-on-desktop
42888-patch-hidden-Android.png (221.0 KB) - added by audrasjb 5 months ago.
42888-patch-hidden-Android
42888-patch-visible-iOS.png (506.5 KB) - added by audrasjb 5 months ago.
42888-patch-visible-android
42888-patch-hidden-iOS.png (492.6 KB) - added by audrasjb 5 months ago.
42888-patch-hidden-iOS
42888-patch-visible-iOS.2.png (506.5 KB) - added by audrasjb 5 months ago.
42888-patch-visible-iOS
42888-2.diff (1.9 KB) - added by Iceable 4 months ago.
Second iteration based on the first patch (details below)
42888-3.diff (2.1 KB) - added by Iceable 4 months ago.
Enhanced 42888-2 with aria-label value toggling
42888-4.diff (18.7 KB) - added by Iceable 4 months ago.
New approach for all password fields + accessbiility
testing 42888-4.diff.mov (409.7 KB) - added by boemedia 5 days ago.
Showing testing of patch 42888-4

Change History (30)

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

42888-show-password-button

@audrasjb
5 months ago

42888-patch-full-behaviour-on-desktop

@audrasjb
5 months ago

42888-patch-hidden-Android

@audrasjb
5 months ago

42888-patch-visible-android

@audrasjb
5 months ago

42888-patch-hidden-iOS

@audrasjb
5 months ago

42888-patch-visible-iOS

#5 @audrasjb
5 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
4 months ago

Second iteration based on the first patch (details below)

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

@Iceable
4 months ago

Enhanced 42888-2 with aria-label value toggling

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

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

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

@Iceable Excellent, thank you!

@Iceable
4 months ago

New approach for all password fields + accessbiility

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

#17 @DrewAPicture
4 months ago

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

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

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

@boemedia
5 days ago

Showing testing of patch 42888-4

Note: See TracTickets for help on using tickets.