Make WordPress Core

Opened 9 years ago

Closed 8 months ago

Last modified 7 months ago

#30685 closed enhancement (fixed)

Better Login Error&Message Displaying

Reported by: extendwings's profile extendwings Owned by: sabernhardt's profile sabernhardt
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch has-dev-note
Focuses: ui, accessibility Cc:

Description

Current way of displaying errors and messages of wp-login.php is not best, I think.

Because:

  • Should not join messages with <br />\n. Each messages has independent meanings. So, this is far from optimal HTML markup.
  • <p> is used to enclose messages. But errors are using <div>. This should be unified.

This patch fixes these 2 problems and includes its related changes.

Attachments (11)

WordPress master...shield-9 login_form.diff (3.2 KB) - added by extendwings 9 years ago.
before.png (56.5 KB) - added by extendwings 9 years ago.
Before: Two messages are displayed as if they are one message!
after.png (56.7 KB) - added by extendwings 9 years ago.
After: Two messages are displayed with better looks
30685.patch (11.7 KB) - added by afercia 9 years ago.
30685.2.patch (10.6 KB) - added by afercia 9 years ago.
30685.3.diff (8.4 KB) - added by lukecavanagh 8 years ago.
Patch Refreshed
30685.4.patch (4.7 KB) - added by rianrietveld 8 years ago.
30685.5.patch (7.0 KB) - added by rianrietveld 8 years ago.
2023-09-18_08-35-56.png (74.7 KB) - added by oglekler 8 months ago.
Login form
2023-09-18_08-38-04.png (69.6 KB) - added by oglekler 8 months ago.
Registration form
2023-09-18_08-44-58.png (46.6 KB) - added by oglekler 8 months ago.
Lost password form

Download all attachments as: .zip

Change History (70)

@extendwings
9 years ago

Before: Two messages are displayed as if they are one message!

@extendwings
9 years ago

After: Two messages are displayed with better looks

#1 @extendwings
9 years ago

  • Component changed from Administration to Login and Registration

#2 @extendwings
9 years ago

  • Keywords has-patch added

#3 @johnbillion
9 years ago

  • Version trunk deleted

#4 @joedolson
9 years ago

From an accessibility perspective, while it's a slight improvement to go from using <br> to <p>, it would be a more meaningful step to use an unordered list with list items. The advantage is that screen reader users would be notified how many errors are in the list, and would know that there's more to read. With the <p> method, it's hypothetically possible that the user would read just the first error, then jump directly into the form to correct it, without realizing there was another error needing to be fixed.

Redoing this to produce list items would be a more accessible solution.

#5 follow-ups: @afercia
9 years ago

I'm sorry maybe I'm missing something, but which steps should be followed to reproduce this and have more than one error displayed at a time? Whatever I do, I get just on error at a time.
See screenshot, dumping $wp_error->get_error_codes()

https://cldup.com/TDARSJGZca.png

Last edited 9 years ago by afercia (previous) (diff)

#6 in reply to: ↑ 5 @SergeyBiryukov
9 years ago

Replying to afercia:

I'm sorry maybe I'm missing something, but which steps should be followed to reproduce this and have more than one error displayed at a time?

I could not reproduce it with the login form, but here are the steps for the registration form:

  1. Enable registration and go to the registration form.
  2. Submit it with both fields empty.
  3. You'll get "Please enter a username" and "Please type your e-mail address".

#7 @afercia
9 years ago

Thanks Sergey.
Seems the proposed patch has some issues...

foreach ( $errors ) {
...
foreach( $messages ) {
...

etc.

#8 in reply to: ↑ 5 @extendwings
9 years ago

Replying to afercia:

I'm sorry maybe I'm missing something, but which steps should be followed to reproduce this and have more than one error displayed at a time? Whatever I do, I get just on error at a time.
See screenshot, dumping $wp_error->get_error_codes()

"more than one error" is not displayed at the login form by default. But it's possible when we use some plugin.

Last edited 9 years ago by extendwings (previous) (diff)

#9 @afercia
9 years ago

@extendwings thanks very much for the clarification.

With the new proposed patch, error messages will use a list. Together with the proposed use of aria-describedby to make errors be announced automatically by screen readers, this could be a very nice improvement for accessibility.
In the attached patch:

  • errors are listed with a <ul>, messages will still use a <p>
  • both use now the "notice" CSS classes
  • messages are all blue, see ticket:27418#comment:9
  • added focus style for links
  • changed link color #999 in #6e6e6e in order to have a sufficient color contrast for WCAG level AA
  • simplified CSS (e.g. removed transitions on links, avoided things like .login *)
  • removed a few title attributes see #24766
  • removed two <br class="clear" /> because screen readers will read out "blank"
  • removed <div class="clear"></div> before </body> didn't see any reason for that
  • added CSS class modal-open for the body in wp-auth-check.js for consistency with modals

I may have missed something, please let me know. Tried to test all the possible actions, a few example screenshots:
https://cloudup.com/cNxU3BhyLL1

Aside:
many CSS rules here are duplicated, for example the "notice" style are already in common.css.
Was thinking to include common.css in the login screen and clean out login.css but maybe better to wait for the CSS roadmap to sanity

@afercia
9 years ago

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


9 years ago

@afercia
9 years ago

#11 @afercia
9 years ago

Patch was a bit stale, refreshed.

#12 @rianrietveld
9 years ago

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

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


8 years ago

#14 @rianrietveld
8 years ago

  • Milestone changed from Awaiting Review to 4.6

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


8 years ago

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


8 years ago

#17 @voldemortensen
8 years ago

  • Milestone changed from 4.6 to Future Release

Punting from 4.6. Please move back if an updated patch is produced soon.

#18 @lukecavanagh
8 years ago

  • Keywords needs-refresh added

@lukecavanagh
8 years ago

Patch Refreshed

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


8 years ago

#20 follow-ups: @rianrietveld
8 years ago

  • Keywords needs-refresh removed

Simplified the changes and limited them to the core issue only: put the error messages in a list and adjusted the CSS for that issue only.
I could not understand why

$( 'body' ).addClass( 'modal-open' );

needed to be added to wp-includes/js/wp-auth-check.js because it's already there in the code and the class is added to the body when the model is open. So I left the changes to wp-auth-check.js out.

#21 @lukecavanagh
8 years ago

Patch applies cleanly and seems to work well.

#22 in reply to: ↑ 20 @afercia
8 years ago

Replying to rianrietveld:

I could not understand why

$( 'body' ).addClass( 'modal-open' );

needed to be added to wp-includes/js/wp-auth-check.js because it's already there

Good catch. It was added in a separate commit, see [31945].

#23 in reply to: ↑ 20 ; follow-up: @afercia
8 years ago

Replying to rianrietveld:

adjusted the CSS for that issue only.

The new CSS is necessary to properly display the error list, without that it will look like this:

https://cldup.com/lyne1ewXAO.png

#24 in reply to: ↑ 23 @rianrietveld
8 years ago

Replying to afercia:

Replying to rianrietveld:

adjusted the CSS for that issue only.

The new CSS is necessary to properly display the error list, without that it will look like this:

Yes, and I thought that looked rather pretty ;)

#25 @rianrietveld
8 years ago

  • Milestone changed from Future Release to 4.6

Discussed this ticket on the contributor day in WordCamp Europe last week with @afercia @jipmoors and @webmandesign. And we came up with the following (patch 30685.5)

CSS class notice:
The designers in the discussion preferred the login_error block separated from the notice-info.
Andrea prefers to use the "notice" CSS classes they should be used this way: "notice notice-type".

aria-describedby:
In the login screen:aria-describedby="login_error will only be added to the input fields when there are actually error messages.
To ensure the aria-describedby attribute is used only when there are errors, the issue is everything here is technically a WP_Error but some of them are just informative messages,
for example when:
?loggedout=true
?action=register
The only way we found is to check for the error_data associated to the errors.

The aria-describedby should also be added for the registration screen, but we will open a separate ticket for this.

Last edited 8 years ago by rianrietveld (previous) (diff)

#26 @ocean90
8 years ago

  • Milestone changed from 4.6 to Future Release

Trunk is closed to all new features and enhancements.

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


7 years ago

#28 @afercia
7 years ago

Related: #39971

#29 @audrasjb
5 years ago

  • Milestone changed from Future Release to 5.1

#30 @pento
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to Future Release

Patch needs refreshing.

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


5 years ago

#32 @afercia
5 years ago

Discussed during today's accessibility bug-scrub. This issue would probably need some research on modern best practices. Reference:

https://www.w3.org/TR/WCAG21/#error-identification

Rationale and suggested techniques:
Understanding Success Criterion 3.3.1: Error Identification
https://www.w3.org/WAI/WCAG21/Understanding/error-identification.html

#33 @rianrietveld
5 years ago

  • Owner rianrietveld deleted

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


17 months ago

#35 @joedolson
17 months ago

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

#36 @sabernhardt
17 months ago

  • Owner changed from joedolson to sabernhardt

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


14 months ago

#38 @sabernhardt
14 months ago

  • Milestone changed from Future Release to 6.3

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


14 months ago

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


12 months ago

#41 @oglekler
11 months ago

  • Milestone changed from 6.3 to 6.4

Due to lack of activity, I am moving this ticket to 6.4

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


10 months ago

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


9 months ago

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


9 months ago

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


9 months ago

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


9 months ago
#46

  • Keywords needs-refresh removed
  • Uses an unordered list for multiple errors, or a paragraph for only one.
  • Removes the bullet styling from the error list and adds a small margin between multiple error messages.
  • Uses .notice and .notice-error classes on div containers, and updates styles.
  • Wraps message paragraphs with a div in login_header() functions and retains the message, reset-pass and register classes on the div instead of the paragraph.
  • Removes .clear elements, and adds a 16px bottom margin for the password strength indicator and the registration form mail message to replace the br elements.
  • Edits margin and padding for Reset Password fields:
  1. The zero bottom margin that applies to the rp action did not apply to resetpass.
  2. The 2.5rem right padding for password inputs applies to both the text and password types with the selector .js.login input.password-input, so the others would be unnecessary.

Trac 30685

#47 @sabernhardt
9 months ago

Backward compatibility note: the PR retains the message, reset-pass and register classes for any special styles plugins use for the messages now. However, the classes are moved to the div, so some login-related plugins that are among the plugins targeting a `.message` paragraph element would need to update with this change.

@oglekler
8 months ago

Login form

@oglekler
8 months ago

Registration form

@oglekler
8 months ago

Lost password form

#48 @oglekler
8 months ago

From the code it looks like we still can have two error messages, because of different severity, but I didn't manage to make it. I assume with custom additional fields to registration form it can be possible. But I don't know what we need this different things for if they have the same classes and cannot be differentiated 🤔

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


8 months ago

@joedolson commented on PR #5178:


8 months ago
#50

@costdev Looks like we need to load wp_admin_notices() earlier so we can use the function at login. Any thoughts on that?

@costdev commented on PR #5178:


8 months ago
#51

@joedolson We _could_ move the definitions of wp_get_admin_notice() and wp_admin_notice() to wp-includes/functions.php.

This file is loaded on the frontend and backend.

This is an example of how we can implement wp_admin_notice() and work with the login page's stylesheet, as it currently doesn't have access to the admin notice styles. This styling difference can be rectified later, so I don't think that's a blocker for this PR.

if ( ! empty( $messages ) ) {
        /**
         * Filters instructional messages displayed above the login form.
         *
         * @since 2.5.0
         *
         * @param string $messages Login messages.
         */
        $messages = apply_filters( 'login_messages', $messages );
        wp_admin_notice(
                $messages,
                array(
                        'id'                 => 'login-message',
                        'additional_classes' => array( 'message' ),
                        'paragraph_wrap'     => false,
                )
        );
}

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/79332690/02e2785c-b8e9-4337-a2d6-0f7d52796322

@joedolson commented on PR #5178:


8 months ago
#52

I'd like to look at abstracting styles in a later iteration; I could easily see having a dedicated notices.css that could hold all of those - and I think the time to work on that is when we go through and migrate non-conforming markup.

I think there would be benefits to having wp_admin_notice() and wp_get_admin_notice() available on the front-end for extenders; not with the styles, because any independent front-end usage should provide its own styling, but for this usage I think it makes sense.

@costdev commented on PR #5178:


8 months ago
#53

Thanks for the updates @sabernhardt!

Did a little testing with the following to check that the logic was preserved and just for a general comparison with trunk.

The extra spacing between each error message is a nice result! 🙂

add_filter(
    'wp_login_errors',
    function( $errors ) {
        $errors->add( 'message_1', '<strong>Test 1:</strong> One message with a severity.',           'message' );
        $errors->add( 'message_2', '<strong>Test 2:</strong> 1/2 messages with a severity.',          'message' );
        $errors->add( 'message_2', '<strong>Test 2:</strong> 2/2 messages with a severity.',          'message' );
        $errors->add( 'message_3', '<strong>Test 3:</strong> 1/2 messages, first with a severity.',   'message' );
        $errors->add( 'message_3', '<strong>Test 3:</strong> 2/2 messages, first with a severity.',             );
        $errors->add( 'message_4', '<strong>Test 4:</strong> 1/2 messages, second with a severity.'             );
        $errors->add( 'message_4', '<strong>Test 4:</strong> 2/2 messages, second with a severity.',  'message' );
        $errors->add( 'error_1',   '<strong>Test 5:</strong> One error with a severity.',             'error'   );
        $errors->add( 'error_2',   '<strong>Test 6:</strong> One error with no severity.'                       );
        $errors->add( 'error_3',   '<strong>Test 7:</strong> 1/2 errors with a severity.',            'error'   );
        $errors->add( 'error_3',   '<strong>Test 7:</strong> 2/2 errors with a severity.',            'error'   );
        $errors->add( 'error_4',   '<strong>Test 8:</strong> 1/2 errors, first with a severity.',     'error'   );
        $errors->add( 'error_4',   '<strong>Test 8:</strong> 2/2 errors, first with a severity.'                );
        $errors->add( 'error_5',   '<strong>Test 9:</strong> 1/2 errors, last with a severity.'                 );
        $errors->add( 'error_5',   '<strong>Test 9:</strong> 2/2 errors, last with a severity.',      'error'   );
        $errors->add( 'mixed_1',   '<strong>Test 10:</strong> 1/2 errors with different severities.', 'message' );
        $errors->add( 'mixed_1',   '<strong>Test 10:</strong> 2/2 errors with different severities.', 'error'   );
        return $errors;
    }
);

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/79332690/c949178c-1e30-499d-8b1b-68c45288fbc7

#54 @joedolson
8 months ago

  • Keywords commit added

PR includes moving wp_admin_notice() and wp_get_admin_notice() function definitions to wp-includes/functions.php, so that those wrapper functions can be used in Login/Registration contexts. Since this changes the location of new functions in 6.4., there are no backwards compatibility concerns.

#55 @joedolson
8 months ago

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

In 56654:

Login and Registration: Improve HTML for errors and notices.

Improve markup on Login and Registration errors. Use list markup for multiple issues, paragraph when only one to reduce semantic burden in the most common case. Normalize classes and markup for wrapper using wp_admin_notice() and wp_get_admin_notice() functions. Move definition of those functions from wp-admin\includes\misc.php to wp-includes\functions.php. Move tests to functions group.

Props extendwings, sabernhardt, afercia, lukecavanagh, rianrietveld, oglekler, sergeybiryukov, costdev, joedolson.
Fixes #30685.

#57 @joedolson
8 months ago

  • Keywords needs-dev-note added

Plugin usage of p.message is moderate (https://wpdirectory.net/search/01H8YXGNVRC89KQNBW2Q1GFMH7), probably should get a dev note. I don't think it needs a dedicated note, but can be documented along with #57590.

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


8 months ago

Note: See TracTickets for help on using tickets.