Make WordPress Core

Opened 15 years ago

Closed 13 months ago

#9883 closed enhancement (fixed)

Password shows under Settings / Writing

Reported by: mastrup's profile mastrup Owned by: joedolson's profile joedolson
Milestone: 6.3 Priority: low
Severity: trivial Version: 2.7.1
Component: Administration Keywords: has-patch has-testing-info
Focuses: ui, accessibility Cc:

Description (last modified by Denis-de-Bernardy)

The password box in the section "Post via e-mail" on Settings -> Writing should hide the password, not show it.

Attachments (7)

9883.diff (617 bytes) - added by wojtek.szkutnik 14 years ago.
9883 - PR 2070.gif (911.1 KB) - added by costdev 3 years ago.
9883.2.diff (584 bytes) - added by sabernhardt 13 months ago.
makes the input cover the full width and corrects left padding in RTL on smaller screens; removes redundant direction
settings-writing-RTL-before.png (15.3 KB) - added by sabernhardt 13 months ago.
before patch, RTL languages would have a shorter input with excess left padding on smaller screens
settings-writing-RTL-with-patch-2.png (15.1 KB) - added by sabernhardt 13 months ago.
with patch, RTL has full-width input with 10px left padding on smaller screens
settings-writing-LTR-before.png (15.8 KB) - added by sabernhardt 13 months ago.
before patch, LTR languages have the narrower input
settings-writing-LTR-with-patch-2.png (15.7 KB) - added by sabernhardt 13 months ago.
with patch, LTR also has full-width input

Download all attachments as: .zip

Change History (68)

#1 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to Future Release
  • Severity changed from normal to trivial

don't you mean the password over at settings / writing? If not, this is invalid -- that would have been added by a plugin.

#2 @ryan
15 years ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

I cant find the string 'Topics via e-mail' anywhere in WP. Please reopen with more information.

#3 @ryan
15 years ago

  • Milestone set to Future Release
  • Resolution invalid deleted
  • Status changed from closed to reopened

Ah, probably "Post via e-mail" on Settings -> Writing. Overlooked comment from Denis-de-Bernardy.

#4 @Denis-de-Bernardy
15 years ago

  • Description modified (diff)
  • Summary changed from Password to Password shows under Settings / Writing

#5 @Denis-de-Bernardy
15 years ago

  • Component changed from Security to UI
  • Milestone changed from Future Release to 2.9
  • Type changed from feature request to enhancement

#6 @bandonrandon
15 years ago

  • Cc bandonrandon@… added

This should be as simple as changing the input type from "text" to "password" which I can easily do and upload a patch. My question is because this is an admin only page and the user is theoretically setting the password for there "post via e-mail" do we want to either

a) enable an option to allow them to see the password if the checkbox is checked (ie,change the input type using jquary)

b) make the user confirm their password to make sure they typed the same thing twice and know their password

c) just change the input type and trust the user to not mess up their password and if they do they can always go back and type it again.

#7 @janeforshort
15 years ago

  • Milestone changed from 2.9 to Future Release

Punting due to schedule.

#8 @wojtek.szkutnik
14 years ago

  • Cc wojtek.szkutnik@… added

#9 @wojtek.szkutnik
14 years ago

a) is not very user-friendly for me
b) in my opinion password confirmations should be used for registration purposes only, when there's a bad user authentication data threat
I'd go with c), which is the best option out of the three simple solutions. However, one would expect a "check authentication data" button to make sure the login and password are valid.

#10 @wojtek.szkutnik
14 years ago

  • Cc wojtek.szkutnik@… removed
  • Keywords has-patch gsoc added; needs-patch removed

#11 @nacin
14 years ago

  • Keywords ux-feedback added

Seems sane to me.

#13 @dd32
14 years ago

I'm not sure emiting the password in the HTML and setting it as type="password" is going to be the best, I'm certain we'd see a "Password shown to hackers!11!1" type report in that case...

If anything, I'd like to see the field shown blank/greyed down with a message explaining it. Upon option save, it'll be possible to check to see if the value submitted is non-empty and different from the current one.

Of course, the other option is to do type=password & echo * and just prevent updating the password to that value..

#14 @dd32
14 years ago

at the same time, the password would be visible on options.php as well i believe.

#15 @wojtek.szkutnik
14 years ago

  • Cc wojtek.szkutnik@… added

#16 @sabreuse
12 years ago

  • Component changed from UI to Administration
  • Keywords ui-focus added; ux-feedback removed

See #22942 for discussion on deprecating and then removing Post By Email altogether: if that goes ahead, I don't think there will be much point in updating an interface that the majority of users won't see, and that hasn't caused complaints since this issue was raised. Until then, moving it as part of the UI cleanup.

#17 @SergeyBiryukov
12 years ago

Similar discussions for the installation screen: #3534, #5529.

The consensus was that the visible password allows the user to make sure it's typed correctly.

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#18 @ryan
10 years ago

  • Owner ryan deleted
  • Status changed from reopened to assigned

#19 @chriscct7
9 years ago

  • Keywords gsoc removed

#20 @SergeyBiryukov
7 years ago

#40402 was marked as a duplicate.

#21 @costdev
3 years ago

#54664 was marked as a duplicate.

#22 @costdev
3 years ago

Consensus on the open related ticket #3534 moved in the direction of a "Show/Hide" toggle for the password.

  • There are numerous examples of this in Core already, so it will not be a new experience for users.
  • As the screen in question is not for creating a password, but rather for entering a password that already exists an external service, there is no need to provide "Generate Password" functionality.
  • A "Show/Hide" toggle negates the need for a password confirmation field, as they can simply review the password by toggling the visibility.

#24 @costdev
3 years ago

  • Focuses accessibility added
  • Keywords needs-testing has-testing-info added
  • Marking as needs-testing.
  • Adding accessibility focus to verify that there are no regressions.

Testing instructions

  • Checkout PR 2070.
  • Build with npm run build or npm run build:dev, depending on your preferred test environment.
  • Navigate to Settings > Writing.
  • The password should be masked.
  • Click the "Show" button.
  • The password should be revealed and the "Show" button should now read "Hide".
  • Change the password.
  • Click "Save Changes".
  • The password should now be hidden.
  • Click the "Show" button.
  • The new password should now be visible.

#25 @costdev
3 years ago

  • Milestone changed from Future Release to 6.0

Milestoning for 6.0.

Similar tickets have consensus towards a solution and this ticket has been in the queue for way too long without resolution.

#26 @zgrkaralar
2 years ago

I also test it with Checkout PR 2070 is working.

#27 @Clorith
2 years ago

Commented similarly on a different ticket, but just to keep the tickets moving, and the conversations grouped to their respective tickets, I'm also including some details here;

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 between the button, and it's input field, and maintaining consistency throughout core whenever possible?

#28 @renyot
2 years ago

Tested with Checkout PR 2070 and here is the result
https://cdn-std.droplr.net/files/acc_1232551/OlzWAO

#29 @costdev
2 years ago

Thanks for testing @renyot!

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

#30 @costdev
2 years ago

@Clorith, what do you think of the implementation as shown in @renyot's comment?

#31 @Clorith
2 years ago

This is looking much better!

Code-wise, I'm tempted to say we should unify all these password toggle codes, there's currently one in core, two open tickets to add two new ones, so it makes sense if this had a reusable markup, code, and styles, so we don't need to create a new identifier for each one (this would also make it easier for future uses, or even plugins and themes to add it, for example to API key fields and similar).

#32 @sabernhardt
2 years ago

The updated patch has a problem with password managers, just as the login screen does (#48222). The previous version of the PR used code from the Profile screen's New Password visibility button instead of the login's icon-only style.

I also have these concerns with the latest version:

  1. When the page loads, it automatically focuses on the password field, but there are important fields before that.
  2. The ltr class should not be removed from the text/password input. See #54915.

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


2 years ago

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

#36 @sabernhardt
2 years ago

  • Milestone changed from 6.0 to Future Release

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


22 months ago

#38 @Boniu91
22 months ago

We were testing this ticket today on test scrub. I was able to see:

  1. That the password field is focused and active after visiting the page
  2. When saving settings, browser asks whether it should save the password. Unfortunately, if we agree, the password will be used by the browser for the wp-login.php access. I believe we should not allow saving this password as it can later cause confusion.

#39 @costdev
22 months ago

  • Keywords dev-feedback added

Regarding the "Save password?" dialog in browsers, this was intended to be fixed by this line that already existed in the form:

<input class="hidden" type="password" value=" " /><!-- #24364 workaround -->

However, it seems that when a password input exists, some browsers ignore this hack, as well as ignoring autocomplete set to off, new-password, one-time-code, etc.

There would be an option of using input[type="text"] and a text-security: disc; CSS rule, with vendor prefixed versions too, but unfortunately, Firefox deprecated the -moz-text-security property and doesn't recognise unprefixed, or other vendor prefixes versions of the property.

I'm not sure how to proceed on the "Save Password?" issue.


  • I'll push a change to the PR which refreshes against trunk and also forces LTR on the password field, per this comment and the linked ticket #54915.
  • Regarding password managers, I tested with LastPass. While the icon showed on the login form, it didn't show for the mailserver password in Options > Writing. @sabernhardt, what password manager showed this issue for you?
  • I'll look into the focus issue too.

#40 @ironprogrammer
22 months ago

I understand that this patch needs a refresh against trunk, but supplying results when using 1Password 7.

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2070.diff

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6
  • Browser: Safari 16.0
  • Server: nginx/1.23.1
  • PHP: 7.4.32
  • WordPress: 6.0-alpha-52448-src (PR needs refresh; see #comment:39)
  • Password Manager: 1Password 7 v7.9.6

Actual Results

  • ✅ Password is hidden on page load, and clicking "eye" icon toggles visibility.
  • ❌ "Eye" icon is covered by 1Password 7 suggestion toggle icon.
  • ❌ Password field is focused on page load, which toggles on the 1Password 7 suggestion.

Supplemental Artifacts

Figure 1: Password field auto-focused on page load
https://cldup.com/MUYgOXgIMq.png

#41 @costdev
22 months ago

Thanks for the test report @ironprogrammer!

I've now updated the PR with a fix for the auto-focus and for password managers. When you have time, could you run a quick test to confirm whether this fixes both issues? (Remember to rebuild and hard refresh! 😅)

#42 @ironprogrammer
22 months ago

@costdev, the patch addresses the previous items. However, testing further, when the password field is set to visible, the 1Password icon still covers the "crossed-out eye".

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2070.diff (after update via comment:41)

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6
  • Browser: Safari 16.0
  • Server: nginx/1.23.1
  • PHP: 7.4.32
  • WordPress: 6.1-RC1-54503-src
  • Password Manager: 1Password 7 v7.9.6

Actual Results

  • ✅ Password is hidden on page load, and clicking "eye" icon toggles visibility.
  • ✅ "Eye" icon is not covered by 1Password 7 suggestion toggle icon.
  • ✅ Password field is not focused on page load.
  • ❌ "Eye (crossed-out)" icon is covered by 1Password 7 suggestion toggle icon when password is toggled visible.

Supplemental Artifacts

Figure 1: Results after patch update; includes password visible toggle
https://cldup.com/b0lZJKe_0m.gif

#43 @costdev
22 months ago

Nicely spotted @ironprogrammer! I've pushed an update to the PR which should handle this. Another test to verify this would be much appreciated!

If you have time, could you also test with an RTL language to make sure the alignment of the text and icons is the same?

#44 @ironprogrammer
22 months ago

The last PR update appears to have resolved the display issues, @costdev 👍🏻

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2070.diff (after update via comment:43)

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6
  • Browser: Safari 16.0
  • Server: nginx/1.23.1
  • PHP: 7.4.32
  • WordPress: 6.1-RC2-54640-src
  • Password Manager: 1Password 7 v7.9.6

Actual Results

  • ✅ Password is hidden on page load, and clicking "eye" icon toggles visibility.
  • ✅ "Eye" icon is not covered by 1Password 7 suggestion toggle icon.
  • ✅ Password field is not focused on page load.
  • ✅ "Eye (crossed-out)" icon is not covered by 1Password 7 suggestion toggle icon when password is toggled visible.
  • ✅ For RTL, the eye icons are not covered by the 1Password 7 toggle icon.

Additional Information

For the RTL language tested, 1Password 7 misinterpreted the password field as a login name, rather than a password field, despite no change to the markup except for the ltr<->rtl field class difference.

Supplemental Artifacts

Figure 1: After patch, icons remain separate when password is visible
https://cldup.com/0lmZTaMJuN.png

Figure 2: Behavior is consistent when site configured with RTL language
https://cldup.com/sKJQD8Xxx9.png

#45 @bgoewert
20 months ago

Wanted to note here that PR 3725 for #3534 utilizes some styles from @costdev's PR 2070 and thus .mailserver-pass-wrap added here was renamed to be more generalized. This was done with the intention of attempting to stay consistent as @Clorith suggested.

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

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


18 months ago

#47 @joedolson
17 months ago

  • Milestone changed from Future Release to 6.3

#48 @joedolson
17 months ago

  • Owner set to joedolson
  • Status changed from assigned to accepted

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


14 months ago

#50 @chaion07
14 months ago

Hello @mastrup and thank you for reporting this. We discussed this during a recent bug-scrub session. A few of the team members have agreed to look into this later this week.

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


13 months ago

#52 @sabernhardt
13 months ago

#58578 was marked as a duplicate.

#53 @joedolson
13 months ago

  • Keywords needs-testing dev-feedback removed

#54 @joedolson
13 months ago

  • Keywords commit added

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


13 months ago
#55

Pro forma recheck on updated patch.

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

#56 @joedolson
13 months ago

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

In 55974:

Administration: Hide password in options/writing.

Hide the password for the "Post via e-mail" settings in writing options. Use the same password hiding mechanisms in use elsewhere in core.

Props mastrup, denis-de-bernardy, ryan, brookedot, wojtek.szkutnik, dd32, sabreuse, sergeybiryukov, costdev, peterwilsoncc, zgrkaralar, clorith, renyot, sabernhardt, boniu91, ironprogrammer, bgoewert.
Fixes #9883.

@joedolson commented on PR #4667:


13 months ago
#57

Fixed in r55974

#58 @sabernhardt
13 months ago

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

Reopening for style adjustments (mobile and RTL).

#59 @joedolson
13 months ago

Sounds good!

@sabernhardt
13 months ago

makes the input cover the full width and corrects left padding in RTL on smaller screens; removes redundant direction

@sabernhardt
13 months ago

before patch, RTL languages would have a shorter input with excess left padding on smaller screens

@sabernhardt
13 months ago

with patch, RTL has full-width input with 10px left padding on smaller screens

@sabernhardt
13 months ago

before patch, LTR languages have the narrower input

@sabernhardt
13 months ago

with patch, LTR also has full-width input

#60 @sabernhardt
13 months ago

Updates in 9883.2.diff:

  1. Setting the direction in the stylesheet is not necessary when the input has an ltr class, so the property is removed.
  2. The inline-block style did not fill the width on mobile, so it switches to block at 782px.
  3. The input had a 5rem left padding on smaller screens, and the patch puts it back to the same 10px as other inputs on mobile.

#61 @joedolson
13 months ago

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

In 55989:

Administration: Fix password layout for RTL and mobile.

Adjust hide password field to be full width on mobile and remove 5rem left padding inside input.

Props sabernhardt.
Fixes #9883.

Note: See TracTickets for help on using tickets.