Make WordPress Core

Opened 17 years ago

Closed 10 months ago

Last modified 7 months ago

#3534 closed enhancement (fixed)

Hide password in setup-config.php

Reported by: xmarcos's profile xmarcos Owned by: joedolson's profile joedolson
Milestone: 6.3 Priority: low
Severity: normal Version: 2.1
Component: Upgrade/Install Keywords: has-patch has-testing-info has-screenshots dev-feedback
Focuses: accessibility Cc:

Description

The password field in setup-config.php is set as type="text", so when you install WordPress, the password is visible as you type it. This is not only dangerous if you have someone around but it is also a possible risk if someone gets into that machine later, form fields are remembered by the browser most of the times.

In order to fix this we just need to set the field type as password, type="password".

Attachments (13)

setup-config.diff (444 bytes) - added by xmarcos 17 years ago.
patch.diff (2.2 KB) - added by r0uter 10 years ago.
Changed the input text values to placeholders, so they look tidy. changed password type to password. Added a show password link. JQuery is imported from http-must be fixed
password_show_hide.diff (2.4 KB) - added by wpnook 8 years ago.
Adds show/hide button for password obfuscation
password_show_hide_fix.diff (3.8 KB) - added by wpnook 8 years ago.
Fix for password/show hide diff
password_show_hide_fix.patch (3.7 KB) - added by wpnook 8 years ago.
Removes console log
3534 - PR 2071.gif (214.2 KB) - added by costdev 2 years ago.
install-LTR-800.png (25.6 KB) - added by sabernhardt 10 months ago.
Install: Password field and button do not expand to the full width
install-RTL-480.png (20.6 KB) - added by sabernhardt 10 months ago.
Install: the Password field needs to be full width on smaller screens, and it has extra padding on the right side in RTL
setup-config-RTL-mobile-toggle-overlap.png (18.3 KB) - added by sabernhardt 10 months ago.
Setup config: the toggle covers the password text in RTL on smaller screens
setup-config-toggle-movement.webm (107.2 KB) - added by sabernhardt 10 months ago.
Setup config: column shifts with change in width when toggling password visibility
setup-config-Show-button-Nepali.png (33.4 KB) - added by sabernhardt 10 months ago.
Setup config: wider toggle button with Nepali translation of "Show"
3534.3.diff (12.9 KB) - added by joedolson 10 months ago.
Updated patch
install.jpg (68.5 KB) - added by joedolson 10 months ago.
Two-column setup/config

Download all attachments as: .zip

Change History (114)

#1 @matt
17 years ago

  • Milestone changed from 2.1 to 2.2
  • Resolution set to wontfix
  • Status changed from new to closed

The reason we don't do this is because the people most likely to use this feature are the least able to view source or something to see the password they typed in and forgot.

The password field is not an access method, it's a setting. We wouldn't hide the title, or the categories, or anything else we feel needs to be accessible and editable by the author.

As for the "if you have someone around" argument, if the person is that paranoid they can always minimize that sidebar box.

#2 @markjaquith
17 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Matt, you seem to be talking about the post password option for blog posts... xmarcos is talking about entering the database password in setup-config.php (your answer might be the same, but I just sensed a Cool Hand Luke moment).

#3 @xmarcos
17 years ago

  • Owner changed from anonymous to ryan
  • Status changed from reopened to new

As markjaquith points, I'm talking about the database password in here, there is a description above and a diff file with the small change.

Last edited 2 years ago by xmarcos (previous) (diff)

#4 @xmarcos
17 years ago

  • Milestone changed from 2.2 to 2.1

#5 @JeremyVisser
17 years ago

-1. I always liked the fact that it shows it.

#6 follow-up: @Nazgul
17 years ago

  • Milestone 2.1 deleted

-1 for the current proposed solution.

If you use a field of type password, you should use a confirmation password field as well, because people can't see their password typos this way.

Also, because this is a enhancment and not a bugfix, I'm pushing it out of scope for 2.1.

#7 follow-up: @foolswisdom
17 years ago

  • Milestone set to 2.2

I like everything to have a milestone or be closed ;-)

#8 in reply to: ↑ 7 ; follow-up: @Nazgul
17 years ago

Replying to foolswisdom:

I like everything to have a milestone or be closed ;-)

What's keeping you:
http://trac.wordpress.org/query?status=new&status=assigned&status=reopened&milestone=
;-)

#9 in reply to: ↑ 8 @foolswisdom
17 years ago

Replying to Nazgul:

What's keeping you:
http://trac.wordpress.org/query?status=new&status=assigned&status=reopened&milestone=
;-)

Slowly but surely, I have been working through those. Many are no longer valid.

#10 in reply to: ↑ 6 ; follow-up: @xmarcos
17 years ago

  • Milestone changed from 2.2 to 2.1

Replying to Nazgul:

-1 for the current proposed solution.

If you use a field of type password, you should use a confirmation password field as well, because people can't see their password typos this way.

Also, because this is a enhancment and not a bugfix, I'm pushing it out of scope for 2.1.

For the third time i think, this is the DATABASE PASSWORD, ok?

You won´t forgot it because you already have it, otherwise you won't get Wordpress running, trust me, no database password, no Wordpress.

Regarding the confirmation, it is NOT A USER-SET PASSWORD. Again you have the password already, if you mistype it, you get and sql connection error.

Now, I remember why I avoid posting tickets, people just don't take the time to read descriptions.

#11 @akbigdog
17 years ago

For what it's worth, I agree that the field *should* be a password field and *should not* have a password confirmation field.

I install WordPress about twice a month, and I don't particularly like seeing all my previous database passwords pop up when the database password field receives focus, especially if I have a client looking over my shoulder.

For the other point, the person installing WP will know immediately whether they typed the password incorrectly or not when they go to the next installation page because WP will tell them it can't connect to the database. I usually just copy and paste anyway. And if people are using the one-click Fantastico installations--as I'm guessing many who would be prone to mistyping a DB password would be--they never see the field, so they can't mess it up with a wrong value.

#12 follow-up: @intoxination
17 years ago

Perhaps the best solution would be to change it to a password field. Once that is submitted, a test connection is made. If it succeeds, the installation proceeds. If it fails then it returns to the previous form alerting the end user of the error (numerous other systems utilize something very similar).

#13 in reply to: ↑ 12 ; follow-up: @foolswisdom
17 years ago

Replying to intoxination:

Once that is submitted, a test connection is made.

That sounds good. Setting milestone to 2.2 awaiting a patch.

#14 @foolswisdom
17 years ago

  • Milestone changed from 2.1 to 2.2

#15 in reply to: ↑ 10 @rob1n
17 years ago

  • Keywords dev-feedback 2nd-opinion added; password setup removed
  • Milestone changed from 2.2 to 2.3

Replying to xmarcos:

Replying to Nazgul:

-1 for the current proposed solution.

If you use a field of type password, you should use a confirmation password field as well, because people can't see their password typos this way.

Also, because this is a enhancment and not a bugfix, I'm pushing it out of scope for 2.1.

For the third time i think, this is the DATABASE PASSWORD, ok?

You won´t forgot it because you already have it, otherwise you won't get Wordpress running, trust me, no database password, no Wordpress.

Regarding the confirmation, it is NOT A USER-SET PASSWORD. Again you have the password already, if you mistype it, you get and sql connection error.

Now, I remember why I avoid posting tickets, people just don't take the time to read descriptions.

Number one, being rude isn't going to get you anywhere. Whatsoever.

Secondly, Nazgul IS referencing the DATABASE password. If it's a passworded field, you can't see what you type in, and thus is more liable to typos. I am -1 for including this. It's not a defect -- it's a feature.

#16 in reply to: ↑ 13 @rob1n
17 years ago

Replying to foolswisdom:

Replying to intoxination:

Once that is submitted, a test connection is made.

That sounds good. Setting milestone to 2.2 awaiting a patch.

It would probably be prudent to set up a second confirmation field, too, then.

#17 @rob1n
17 years ago

  • Milestone changed from 2.3 to 2.2
  • Owner changed from ryan to rob1n
  • Status changed from new to assigned

#18 @MichaelH
17 years ago

Just an observation that two MySQL database creation processes, GoDaddy and cPanel, use different methods. GoDaddy hides the password and requires you to type the password twice. cPanel displays the password and only requires the password once.

#19 @rob1n
17 years ago

  • Keywords dev-feedback 2nd-opinion removed
  • Milestone 2.2 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

I suppose it's up to personal choice. I think we should keep it the way it is right now, as having it in cleartext DOES limit typos (and eliminates, if the user double checks). It's not as though you enter this password in every day, like the user password.

If one of the developers want to change this, please reopen and assign to me, as I will gladly cook up a patch.

#20 @SergeyBiryukov
11 years ago

#22443 was marked as a duplicate.

#21 @empireoflight
11 years ago

It's been years since this was discussed, and I think it warrants discussion. There's something just wrong about legible passwords in text inputs. Make them type it twice.

#22 @rmccue
11 years ago

-1 on changing it. Keeping it as a text field seems like better UX to me. For similar types of interfaces, see the wifi password input interfaces in most OSes, which have a show password checkbox to ensure that you don't mistype it.

#23 @markoheijnen
11 years ago

So +1 for having that checkbox and default hidden ;)

#24 @matt
11 years ago

It's interesting that some modern services are starting to go the other direction:

http://www.lukew.com/ff/entry.asp?1653

#25 @nacin
11 years ago

#25630 was marked as a duplicate.

This ticket was mentioned in IRC in #wordpress-dev by tonythomas. View the logs.


10 years ago

@r0uter
10 years ago

Changed the input text values to placeholders, so they look tidy. changed password type to password. Added a show password link. JQuery is imported from http-must be fixed

#27 @ocean90
10 years ago

#28996 was marked as a duplicate.

#28 follow-up: @amansurov
10 years ago

Let me give you a user's viewpoint: when I installed WP couple hours ago and saw password field to be plaintext, I didn't think for a second it's a feature, just a horrible typo someone made. Now I wonder if at some other place a whole DB access is made as transparent as a password was here. Passwords are expected to be entered into password fields. That is standard behaviour. If you insist on having it plaintext, at least put a memo nearby to explain it to user.
You can verify password with query or put a checkbox to make text visible, what gives user an option to opt-in for a non-standard behaviour

#29 in reply to: ↑ 28 @bi0xid
10 years ago

Replying to amansurov:

You can verify password with query or put a checkbox to make text visible, what gives user an option to opt-in for a non-standard behaviour

+1 for the checkbox to show/hide the password.

#30 @DrewAPicture
10 years ago

  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Related: There has been some discussion over in #24633 about using an "eye" icon or similar concept for revealing passsword fields in the user-edit/profile screen. Maybe something to consider in 4.1 or later.

#31 @wonderboymusic
9 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Resolution set to fixed
  • Status changed from reopened to closed

#32 @ocean90
9 years ago

  • Milestone changed from 4.3 to Awaiting Review
  • Resolution fixed deleted
  • Status changed from closed to reopened

This is about the MySQL password.

#33 @Narthur
8 years ago

+1 for obfuscating input unless an "eye" icon or similar is checked.

Just had a very embarrassing moment installing WordPress while screen sharing with a friend. Muscle memory ensured my password was entirely outputted in plaintext before I even realized what was happening.

An "eye" icon with default obfuscation should make everyone happy.

@wpnook
8 years ago

Adds show/hide button for password obfuscation

#34 @wpnook
8 years ago

  • Keywords has-patch added
  • Resolution set to invalid
  • Status changed from reopened to closed

I just added password_show_hide.diff which hopefully makes everyone happy.

Since the majority of the debate on this issue took place ~9 years ago, many users have argued for a clean show/hide option for displaying the password.

This patch includes internationalization for the aria labels and outputted button text.

I also removed the default value from the password and used a placeholder instead, to avoid obfuscation of the initial "password" value.

#35 @wpnook
8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

@wpnook
8 years ago

Fix for password/show hide diff

#36 @wpnook
8 years ago

Whoops. My original diff did not include the new setup-config.js file. New patch added.

@wpnook
8 years ago

Removes console log

#37 @mattheweppelsheimer
5 years ago

  • Keywords needs-unit-tests needs-testing added

#38 @markparnell
4 years ago

  • Component changed from General to Upgrade/Install
  • Keywords needs-refresh added
  • Severity changed from minor to normal

I'd love to see this resolved - entering passwords in plain-text should be considered a security issue. Using the same interface that is used elsewhere (a password-type field with a toggle to view the entered text should the user choose to do so) makes the most sense to me. I wasn't able to get the existing patch to apply to trunk though, so marking for refresh.

This ticket was mentioned in PR #2071 on WordPress/wordpress-develop by costdev.


2 years ago
#39

  • Keywords needs-refresh removed

#40 @costdev
2 years ago

  • Focuses accessibility added
  • Keywords has-testing-info added; needs-unit-tests removed
  • Marking as has-testing-info and removing needs-unit-tests as manual testing should suffice.
  • Adding accessibility focus to verify that there are no regressions.

Testing instructions

  • Checkout PR 2071.
  • Build with npm run build or npm run build:dev, depending on your preferred test environment.
  • Rename your wp-config.php file to wp-config.php1.
  • Navigate to /wp-admin/.
  • Click "Continue".
  • Click "Let's go!".
  • The "Password" field should show the "password" placeholder.
  • Type a password into the "Password" field.
  • The password should be masked.
  • Click the "Show" button.
  • The password should be revealed and the "Show" button should now read "Hide".
  • Click the "Hide" button.
  • The password should now be masked again.

Cleanup

  • Delete the newly generated wp-config.php file.
  • Rename your wp-config.php1 file back to wp-config.php.

#41 @costdev
2 years ago

  • Milestone changed from Awaiting Review to 6.0

Milestoning for 6.0.

#42 @Clorith
2 years ago

I noticed this ticket, and it's resolution, being set as the baseline for #9883 as well.

I'd like to make a counter-proposal, these both suggest adding a button outside the input-flow, which in the case of this ticket, even ends up on a newline under the input field, so not properly being attached to its related input area.

Is there any reason we can't use the established pattern introduced to the login screen in [46256], which has a password reveal-toggle that "overlays" the input area, so that a clear connection is kept, and the visual flow isn't broken up?

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


2 years ago

#44 @ryokuhi
2 years ago

We reviewed this ticekt today during the accessibiltiy team's bug-scrub.

The team suggests to first test that @costdev's patch works, and then make a decision about how to make the interfaces consistent, which is also important for accessibility.

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


2 years ago

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


2 years ago

#47 @sabernhardt
2 years ago

  • Milestone changed from 6.0 to Future Release

#49 @bgoewert
17 months ago

Completed a test report using @costdev's PR 2071. Although, I wanted to also throw in that I agree with what @Clorith suggested about a consistent password reveal toggle that aligns with #9883.

I've since submitted an additional PR 3725 that combines changes from PR 2070 & PR 2071. I also completed a test report for this as well (not sure if self reports are okay). If someone else could test to verify that would be cool.

Test Report 1

Using "old" patch.

Patch tested: PR 2071

Environment

  • OS: Pop_OS! 22.04
  • Localhost: wordpress-develop & Docker Desktop
  • WordPress: trunk
  • Browser: Firefox 107, GNOME Web 43 (WebKit), Chrome 108
  • Theme: N/A
  • Active Plugins: N/A

Actual Results

  • ✅ "Password" field shows the "password" placeholder.
  • ✅️ Typing a password into the "Password" field is masked.
  • ✅️ Clicking the "Show" button reveals the un-masked password.
  • ✅️ "Show" button now reads "Hide".
  • ✅️ Clicking the "Hide" button masks the password again.

Additional Notes

  • My first test report!
  • This is an old PR. I don't know if this is correct, but before testing I locally merged trunk against the PR.

Test Report 2

Using the patch I submitted.

Patch tested: PR 3725

Environment

  • OS: Pop_OS! 22.04
  • Localhost: wordpress-develop & Docker Desktop
  • WordPress: trunk
  • Browser: Firefox 107, GNOME Web 43 (WebKit), Chrome 108
  • Theme: N/A
  • Active Plugins: N/A

Steps to test

  1. (Optional) Enable a password manager browser extension.
  2. (Optional) The password manager icon should not overlap the password visibility toggle.
  3. The "Password" field should display the "password" placeholder.
  4. Typing a password into the "Password" field should be masked.
  5. Clicking the password toggle should reveal the password and change the icon from dashicon-visibilty to dashicon-hidden.
  6. Clicking the toggle again should mask the password and change the icon back to dashicon-visibilty.

Actual Results

  • ✅️ Password manager icon does not overlap.
  • ✅️ "Password" field shows a "password" placeholder.
  • ✅️ The password is masked when entered.
  • ✅️ Clicking the toggle un-masks the password and changes the icon.
  • ✅️ Clicking the toggle again masks the password and changes the icon back.

Supplemental Artifacts

Image showing that a password manager icon does not overlap the visibility toggle and that the password is properly masked.
https://i.imgur.com/I6tZotf.png

Image showing un-masked password and the different icon.
https://i.imgur.com/WafQiKJ.png

Last edited 17 months ago by bgoewert (previous) (diff)

@ironprogrammer commented on PR #3725:


17 months ago
#50

@bgoewert, I like the direction this is headed, toward a shared implementation for the toggle feature 👍🏻

Once the above feedback is integrated, I'd love to give this a go. It should allow for #2070 to be simplified as well.

@bgoewert commented on PR #3725:


17 months ago
#51

@bgoewert, I like the direction this is headed, toward a shared implementation for the toggle feature 👍🏻

Once the above feedback is integrated, I'd love to give this a go. It should allow for #2070 to be simplified as well.

@mukeshpanchal27's suggested change has been committed. Waiting to be reviewed again.

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


16 months ago

#53 @SergeyBiryukov
16 months ago

  • Milestone changed from Future Release to 6.2

#54 @ironprogrammer
16 months ago

  • Keywords has-screenshots added

Thanks @bgoewert! I think this is almost there.

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/3725.diff (applied to trunk, testing based on instructions from comment:40)

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6.1
  • Browser: Safari 16.1, Google Chrome 108.0.5359.124, Mozilla Firefox 107.0.1
  • Server: nginx/1.23.2
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Password Manager: 1Password 7 v7.9.8

Actual Results

  • ✅ Password field shows "password" placeholder, defaults to input type="password", and "open eye" icon is displayed.
  • ✅ "Eye" icon is not covered by 1Password 7 suggestion toggle icon in either the hidden or visible state.
  • ✅ Toggle functions as expected, hiding or showing the password.
  • ⚠️ In Chrome and Firefox on macOS, the toggle button's :focus outline is activated when clicked, which is visually different from Safari. See Figure 4.
  • ⚠️ In RTL languages, the "eye" toggle shifts to the left of the field, separate from the 1Password toggle (compare this to RTL results in https://core.trac.wordpress.org/ticket/9883#comment:44 where the toggles remain grouped). See Figure 5.

Supplemental Artifacts

https://cldup.com/ki9Fy_Dpfg.png
Figure 1: Default Password field state.

https://cldup.com/1_eLv8G6lh.png
Figure 2: Field focused with password text entered.

https://cldup.com/j-Iv1XKM40.png
Figure 3: Field focused with password visibility toggled on (Safari).

https://cldup.com/qclOblawBW.png
Figure 4: Toggle's :focus is activated when clicked (macOS Chrome and Firefox).

https://cldup.com/g-rnFLDQU7.png
Figure 5: RTL toggle placement separated from password app icon.

#55 @bgoewert
16 months ago

Thanks for testing this on macOS @ironprogrammer!

It took me some time to figure out how RTL was working in WP and to research your ⚠️ results.

⚠️ In Chrome and Firefox on macOS, the toggle button's :focus outline is activated when clicked, which is visually different from Safari. See Figure 4.

Safari does this on purpose. See https://bugs.webkit.org/show_bug.cgi?id=22261

If you try this on other buttons around WP, it currently behaves in the same manner. Note though I only tested the login page.

Login page on Safari 16: https://imgur.com/eL35rzR.mp4
Compared to Chrome 108 : https://imgur.com/9npcBLU.mp4

Do we remove the border for all the other browsers so that it visually aligns with Safari?

⚠️ In RTL languages, the "eye" toggle shifts to the left of the field, separate from the 1Password toggle (compare this to RTL results in https://core.trac.wordpress.org/ticket/9883#comment:44 where the toggles remain grouped). See Figure 5.

This is also currently default behavior around WP. Note this image of the login page, the password toggle is on the left and password manager on the right.

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

I've tried to test this by changing the primary language on Chrome. And it did change the language for the extension, but not the position of the icons (image proof). Testing on an OS with a RTL language, rather than simply changing the WP language, may change the behavior of the browser as well as password manager extensions and yield a different result (but not likely).

Is the plan to change all the other password visibility toggles to align to the right so these icons are grouped?

Version 1, edited 16 months ago by bgoewert (previous) (next) (diff)

#56 @bgoewert
16 months ago

Is the plan to change all the other password visibility toggles to align to the right so these icons are grouped?

I guess this is a yes per these: #9883:39, #9883:32, #54915

I'll push a fix for the visibility button grouping when I can.

Just need some feedback on this:

Do we remove the visibility button border for all other browsers so that it visually aligns with Safari?

#57 @sabernhardt
16 months ago

The RTL issue mentioned in #9883:39, #9883:32 and #54915 is about the input field, not the toggle button. An earlier version of that PR had removed the ltr class, which correctly sets the password input to LTR direction in all languages. The setup-config password input already sets the #pwd direction properly, too.

The focus outline/border is necessary on the toggle button itself to show when the element is focused. If Safari does not allow that outline (and/or access to the button via keyboard), that's a serious bug in the browser that they hopefully will fix.

I'll add more comments on the PR.

@bgoewert commented on PR #3725:


16 months ago
#58

Thank you @sabernhardt for the thorough review. I tried to address all your comments. Another change I added towards a standardized password-toggle.js is this fix for performing the toggle to the correct parent input.

However, while the icon-only button is consistent with the Login page, it is not consistent with other visibility toggle buttons in .form-table elements at larger screen sizes (on Profile and Installation pages). Andrea acknowledged that icon-only controls are an accessibility anti-pattern before committing r46256 as a "self-contained" improvement that ideally would have a different design later.

I noticed the profile page has an icon and text. If it helps with accessibility I can change the icon to text to maintain consistency with the other toggles. Would it be preferable to still keep the button as "part of" the password input? Or do we go back separating the button from the input? Do we then also change the login page to follow suit?

Also, the icon-only button on setup-config has the overlap with some password managers, as seen on the Login page (ticket:48222).

Separating the button from the input would address this.

This ticket was mentioned in PR #3827 on WordPress/wordpress-develop by @bgoewert.


16 months ago
#59

https://core.trac.wordpress.org/ticket/3534

This a text variation of #3725 to address accessibility concerns and add support for various password manager implementations. The button is separated from the input while still keeping it inline — allowing for password managers which add icons directly to the input, in contrast to those that insert another element to add their respective icon. I did not include the icon, which is inconsistent with the other toggle buttons, so as to provide maximum input space on the setup-config.php form. But the icon can easily be added back to remain consistent.

https://i0.wp.com/i.imgur.com/IM3Alya.png
_Figure 1: Separated text password toggle w/ LastPass extension - inactive_

https://i0.wp.com/i.imgur.com/MoDH3EV.png
_Figure 2: Separated text password toggle w/ LastPass extension - active_

As a consequence of adding display: flex to .wp-pwd, the layout for install.php and user-new.php was also affected. I've also added changes to address this. For user-edit.php, user-profile.js#L232 applies display: block directly to .wp-pwd and was not affected.

https://i0.wp.com/i.imgur.com/Ub3imZV.png
_Figure 3: install.php before._

https://i0.wp.com/i.imgur.com/3h0p8ct.png
_Figure 4: install.php after change._

https://i0.wp.com/i.imgur.com/KQRCHTp.png
_Figure 5: install.php after fix._

#60 @bgoewert
16 months ago

  • Keywords dev-feedback added

Also meant to note on pr:3827 that password-toggle.js does not add/remove the dashicon. This is because the icon was left out of this text variation for setup-config.php. At an attempt to keep the button inline, my initial concern was that the icon would take space away from the input.

As I noted already though, the icon can easily be re-added to remain consistent with the other password toggles. However, if password-toggle.js is to be used commonly, either the icons would need to be removed from the other areas or we re-add the icon here and the password input is consequently smaller.

Awaiting feedback for which direction to go.

Edit: This is what it might appear like if we re-add the icon and kept it inline.

https://i.imgur.com/Z9izc9l.png
https://i.imgur.com/Kp9qSzA.png

Last edited 16 months ago by bgoewert (previous) (diff)

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


15 months ago

#62 @bgoewert
15 months ago

Added two new changes to pr:3827. One that re-adds the icon to the toggle button to remain consistent with the other existing password toggles — except for the login page which is currently icon-only. And another change that slightly increases the input size on install.css to allow for the bigger button as well as fix a few orphans in the descriptions on both setup-config.php and install.php.

Figure 1: Original setup-config.php with a relatively small password input and orphaned words on the last two descriptions.
https://i.imgur.com/52UUQEL.png

Figure 2: New setup-config.php after input size adjustment.
https://i.imgur.com/9FOW6NI.png

Figure 3: Original install.php with an orphaned word on the password input description.
https://i.imgur.com/dEMtkkY.png

Figure 4: New install.php after input size adjustment.
https://i.imgur.com/XCQ9EAa.png

Last edited 15 months ago by bgoewert (previous) (diff)

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


15 months ago

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


15 months ago

#65 @bgoewert
15 months ago

Tried to look into the RTL note on comment:54 some more because I am unfamiliar with such things. From what I can gather (Apple, Material Design), controls like this should be mirrored. The password manager extensions may have their own reasons or limitations for not doing this. It may be a non-issue now though since the toggle, at least on pr:3827, is now a traditional button and not an inline icon.

Last edited 15 months ago by bgoewert (previous) (diff)

#66 @bgoewert
15 months ago

Test Report

Patch tested: PR:3827

Environment

  • OS: Pop!_OS 22.04
  • Web Server: Docker-Desktop & wordpress-develop
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Browser: Firefox 108, Chrome 109
  • Theme: Twenty Twenty-Three
  • Active Plugins: None
  • OS: macOS Monterey (via LambdaTest)
  • Browser: Safari 16

Actual Results

  • ✅ Password field shows "password" placeholder.
  • ✅ Field is masked by default.
  • ✅ Toggle functions as expected, hiding or showing the password.
  • ✅ Works with password manager extension icons inserted via an element or background image.
  • ✅ Button is mirrored for RTL languages.
  • ✅ ARIA label changes on toggle.
  • ❌ Input is not full width on narrower screens. See figure 6.

Supplemental Artifacts

Figure 1: Default state with "password" placeholder and Keeper Security icon. (Chrome)
https://i.imgur.com/w6b6Tpz.png

Figure 2: Password field focused with LastPass icon. (Chrome)
https://i.imgur.com/tN49IGy.png

Figure 3: Password visibility toggled on and focused with LassPass icon. (Chrome)
https://i.imgur.com/Yf3XAFj.png

Figure 4: Password visibility toggled on. (Safari)
https://i.imgur.com/twYwSzI.png

Figure 5: RTL language with toggle button focused.
https://i.imgur.com/g8Rw6ZU.png

Figure 6: Narrower screen (768px) with toggle button focused.
https://i.imgur.com/zVYtjTp.png

Last edited 15 months ago by bgoewert (previous) (diff)

#67 @bgoewert
15 months ago

Updated PR:3827 to fix the input not being full width on narrow screens which was noted in comment:66. Also reverted the custom width/padding that was being applied to .pwd-toggle so it defaults to the regular button width/padding.

Figure 1: Password input on narrow screen with toggle button enabled and focused.
https://i.imgur.com/4oHwz7n.png

Figure 2: Toggle button before width/padding reversion.
https://i.imgur.com/uzjr2CD.png

Figure 3: Toggle with default button width/padding.
https://i.imgur.com/jMxqrt3.png

This ticket was mentioned in Slack in #core-upgrade-install by bgoewert. View the logs.


15 months ago

@audrasjb commented on PR #3827:


15 months ago
#69

Hello, there is a small conflict in steup-config.php since I committed [55110]

@bgoewert commented on PR #3827:


15 months ago
#70

Thanks @audrasjb. I just merged trunk to address the conflict.

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


15 months ago

#72 @mukesh27
15 months ago

Thanks @xmarcos for the ticket.

This ticket was discussed in the recent bug scrub.

Can someone from the testing team (@adeltahri or @robinwpdeveloper) take a look and share the feedback?

Props to @costdev

#73 follow-up: @adeltahri
15 months ago

Test Report

Patch tested: 3827

Environment

  • OS: Mac Mini 2020 M1
  • Web Server: Docker-Desktop & wordpress-develop
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Browser: Arc, Firefox 108, Chrome 109, Safari 16
  • Theme: Twenty Twenty-Three
  • Active Plugins: None

Actual Results

✅ Password field shows "password" placeholder.
✅ Field is masked by default.
✅ Toggle functions as expected, hiding or showing the password.
✅ Works with password manager extension icons inserted via an element or background image.
✅ Button is mirrored for RTL languages.
✅ ARIA label changes on the toggle.
❌ Button show/hide on setup-config.php?step=1 not showing.
❌ Button show/hide on wp-login.php not inside the input, password filed not 100% width.

Supplemental Artifacts

Figure 1:
https://d.pr/i/WAwW5Z
https://cdn-std.droplr.net/files/acc_610439/WAwW5Z

Figure 2:
https://d.pr/SL00Tv
https://cdn-std.droplr.net/files/acc_610439/SL00Tv

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


15 months ago

#75 in reply to: ↑ 73 @bgoewert
15 months ago

Replying to adeltahri:

❌ Button show/hide on setup-config.php?step=1 not showing.

This may be the hide-if-no-js class. I was unable to reproduce without blocking JavaScript. I'm surprised that toggling worked if the button was not displayed.

Can you verify what browser this was in, if not all of them? Maybe my testing environment is not correct? Although, I tested with npm run build:dev as well as npm run build and wp-env mapped to ./build. both of which display the button.

Maybe someone else can give thoughts on this?

❌ Button show/hide on wp-login.php not inside the input, password filed not 100% width.

I failed to test wp-login.php on comment:66. Thank you for catching this issue. I know what is causing this and I'll get it fixed.

#76 @bgoewert
15 months ago

Updated PR 3827 to address the login toggle button and other small things pointed out on the PR. Still unable to reproduce the button not showing at all without disabling JavaScript.

If someone, @adeltahri or @robinwpdeveloper, could test again.

#77 @adeltahri
15 months ago

Hello @bgoewert, I'll re-test it and npm run build:dev and back with the full report.

#78 @adeltahri
15 months ago

Test Report

Patch tested: PR:3827

Environment

  • OS: Mac Mini 2020 M1
  • Web Server: Docker-Desktop & wordpress-develop
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Browser: Arc, Firefox 108, Chrome 109, Safari 16
  • Theme: Twenty Twenty-Three
  • Active Plugins: None

Actual Results

✅ Button show/hide on setup-config.php?step=1 (GIF 1).
✅ Works with password manager extension Lastpass (GIF 2).
✅ Button show/hide on wp-login.php (GIF 2).
✅ Tested on install.php (GIF 3).

Supplemental Artifacts

GIF 1:
https://cdn-std.droplr.net/files/acc_610439/bhNpFb
GIF 2:
https://cdn-std.droplr.net/files/acc_610439/SbU2wJ
GIF 3:
https://cdn-std.droplr.net/files/acc_610439/9KZNi7

I have tested with npm run build:dev and found no issues with the patch. So I think we are good to go.

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


15 months ago

#80 @sabernhardt
15 months ago

  • Milestone changed from 6.2 to 6.3

#81 @SergeyBiryukov
13 months ago

#58029 was marked as a duplicate.

This ticket was mentioned in Slack in #core-upgrade-install by pbiron. View the logs.


11 months ago

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


11 months ago

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


10 months ago

#85 @joedolson
10 months ago

  • Owner changed from rob1n to joedolson
  • Status changed from reopened to reviewing

#86 @sabernhardt
10 months ago

  • Keywords changes-requested added; needs-testing removed

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


10 months ago

This ticket was mentioned in PR #4677 on WordPress/wordpress-develop by @joedolson.


10 months ago
#88

Show/hide password toggle for config; updates wrappers;

Trac ticket: https://core.trac.wordpress.org/ticket/3534

#89 @joedolson
10 months ago

  • Keywords changes-requested removed

@sabernhardt I've added a new PR with the requested changes; if you can give it a look over.

@sabernhardt
10 months ago

Install: Password field and button do not expand to the full width

@sabernhardt
10 months ago

Install: the Password field needs to be full width on smaller screens, and it has extra padding on the right side in RTL

@sabernhardt
10 months ago

Setup config: the toggle covers the password text in RTL on smaller screens

@sabernhardt
10 months ago

Setup config: column shifts with change in width when toggling password visibility

@sabernhardt
10 months ago

Setup config: wider toggle button with Nepali translation of "Show"

#90 @sabernhardt
10 months ago

  1. On the Install page, the Password field and button do not go to the opposite edge to match the other fields. Using 100% might help a little at larger screen widths, and it needs to be that wide on small screens.
    .password-input-wrapper {
    	display: inline-block;
    	width: 100%;
    }
    
  1. In RTL on smaller screens, setup-config positions the button above the input with no left padding.
    	.form-table td input[type="password"] {
    		/* rtl:ignore */
    		padding-right: 2.5rem;
    	}
    
    could be changed to
    	#pwd {
    		padding-right: 2.5rem;
    	}
    
    (The button is on the left on larger screens, so it should stay on the same side for smaller screens.)
  1. Consider giving the middle cells for setup-config a fixed width of about 300 pixels, and then reset it to auto (or 100%) for smaller screens. I don't like how the inputs expand and the description column shifts when clicking the toggle button. The Password field can also be quite narrow in languages with wide translations of Show and/or Hide.
    .setup-config-fields th + td {
    	width: 300px;
    }
    @media screen and (max-width: 782px) {
    	.setup-config-fields th + td {
    		width: 100%;
    	}
    }
    

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


10 months ago

#92 @joedolson
10 months ago

Making .password-input-wrapper 100% width at desktop sizes breaks the layout of these forms in user-new and user-edit, so not a great option. Went with making an exception when in .form-table.

Also noticed that the structure of these controls was updated in user-new.php for this patch, but not in user-edit.php, so made those updates as well.

Switched the setup config page to two columns. This matches the layout used on install.php, so I think this actually makes a more consistent interface through the install and setup process.

Noticed while I was in here that the description fields on install.php aren't explicitly associated with their respective inputs, so I'll be opening a new issue to fix that issue shortly.

@joedolson
10 months ago

Updated patch

#93 @joedolson
10 months ago

(Patch is missing the gruntfile changes; PR isn't.)

#94 @joedolson
10 months ago

I'm wondering whether we should shift all instances of the password toggle to use the new dedicated file, as well. If we're introducing this file, we should really remove the duplicate behaviors.

Although perhaps that should be a follow-up.

Last edited 10 months ago by joedolson (previous) (diff)

#95 @joedolson
10 months ago

@sabernhardt I'm not going to map this script into profile pages in this ticket; it makes more sense for you to do it in #57157 post commit.

#96 @joedolson
10 months ago

  • Keywords commit added

I'm satisfied. If additional issues crop up, they can be fixed during beta. Marking for commit.

@joedolson
10 months ago

Two-column setup/config

#97 @joedolson
10 months ago

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

In 56008:

Upgrade/Install: Show/hide toggle on password fields.

Add a show/hide toggle for new passwords in initial user creation and database access during install and setup process using the same model as on user profiles. Add a new password toggle script. Change setup config table to two columns, matching the install table layout.

Props xmarcos, matt, markjaquith, nazgul, akbigdog, intoxination, rob1n, MichaelH, empireoflight, rmccue, markoheijnen, r0uter, amansurov, bi0xid, DrewAPicture, Narthur, wpnook, markparnell, costdev, clorith, ryokuhi, sabernhardt, bgoewert, ironprogrammer, adeltahri, joedolson, mukesh27, audrasjb, sergeybiryukov.
Fixes #3534.

@joedolson commented on PR #4677:


10 months ago
#98

Closed in r56008

#99 @sabernhardt
10 months ago

  • Keywords commit removed

The visible text of "Show" and "Hide" from the script is not translated with WP6.3 beta2. I thought this might be fixed automatically closer to the release, but I did not have those translations for the toggle button on install.php either (versions 5.5 to 6.3b2). That needs a new ticket to fix separately: #58696. (The login and profile button translations are fine.)

Last edited 10 months ago by sabernhardt (previous) (diff)

@bgoewert commented on PR #3725:


7 months ago
#100

Closed in r56008

@bgoewert commented on PR #3827:


7 months ago
#101

Closed in r56008

Note: See TracTickets for help on using tickets.