Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 18 months ago

#54483 closed defect (bug) (fixed)

On logout input fields have aria described login_error

Reported by: patrickgroot's profile patrickgroot Owned by: joedolson's profile joedolson
Milestone: 6.1 Priority: normal
Severity: trivial Version: 5.8.2
Component: Login and Registration Keywords: has-patch commit add-to-field-guide
Focuses: ui, accessibility Cc:

Description

Hi Team,

Currently i'm trying to smooth out the UI a bit while developing a plugin that add some extra functionality to the login and registration pages of WordPress.

One thing i noticed is while you're logged-in into your WordPress site is that when you're logging out the input fields from the login form have a aria-describedby="login_error" what doesn't make sense when you're just logged-out.

URL for testing: mydomain.com/wp-login.php?loggedout=true

Attachments (4)

54483.diff (3.8 MB) - added by rollybueno 2 years ago.
Created patch.
54483.2.diff (1.6 KB) - added by rollybueno 2 years ago.
Created correct patch file. The first one that I uploaded earlier was created using wrong method, thus having 4MB size. It can be disregarded.
54483.3.diff (2.3 KB) - added by afercia 2 years ago.
54483.4.diff (2.4 KB) - added by afercia 2 years ago.

Change History (21)

#1 @patrickgroot
2 years ago

Potential/proposal fix

Original file: wp-login.php

<?php
if ( $errors->has_errors() ) {
    $aria_describedby_error = ' aria-describedby="login_error"';
} else {
    $aria_describedby_error = '';
}

Changed line 1370:

<?php
if ( $errors->has_errors() && empty( $_GET['loggedout'] ) ) {
    $aria_describedby_error = ' aria-describedby="login_error"';
} else {
    $aria_describedby_error = '';
}
Last edited 2 years ago by patrickgroot (previous) (diff)

This ticket was mentioned in PR #1926 on WordPress/wordpress-develop by pgroot91.


2 years ago
#2

  • Keywords has-patch added

Adds an extra check preventing the input fields from having an aria described of login_errors while you just loggedout of your website.

Trac ticket: 54483

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


2 years ago

#4 @alexstine
2 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 6.0
  • Owner set to alexstine
  • Status changed from new to accepted

The Accessibility team thinks it may be best to have the successful logout message read to screen readers. The current success message is not connected via aria-describedby. I will work on a patch to test shortly.

Thanks.

@rollybueno
2 years ago

Created patch.

@rollybueno
2 years ago

Created correct patch file. The first one that I uploaded earlier was created using wrong method, thus having 4MB size. It can be disregarded.

#5 @rollybueno
2 years ago

  • Keywords has-patch added; needs-patch removed

Hi here,

I noticed that this ticket has no movement for 2 months so I have uploaded a new patch(54483.2.diff). I have updated the wp-login.php that:

  1. On user logged out, add specific value for the aria-describedby to the fields.
  2. On the login form message, I have applied the same value of the aria-describedby from the fields inside the message paragraph

@afercia
2 years ago

#6 @afercia
2 years ago

Given that the value of aria-describedby should point to the ID of an element present on the page, I'd like to propose a different approach in 54483.3.diff. This updated patch should cover the case of the login and logout.

Worth noting there are at least 3 more cases where input fields aren't connected to messages displayed on the page:

  • lost password: this page shows a default message and potential errors, however the input field is not connected to any description.
  • reset password: the password field is connected to the password strength meter, which is also an aria-live region.
  • register: this page shows a default message and potential errors, however the two input fields are not connected to any description.

I'm not sure these cases should be addressed in the context of this ticket though.

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


2 years ago

#8 @ryokuhi
2 years ago

This ticket was reviewed today during the Accessibility Team's weekly bug-scrub.
Adding the needs-testing label, so that this ticket gets a little exposure.

@afercia
2 years ago

#9 @afercia
2 years ago

54483.4.diff is a refresh against latest trunk.

To test:

  • Logout from the admin.
  • Check both the username and password fields have an aria-describedby attribute that points to the success notice with id login-message.
  • Try to login by entering invalid credentials.
  • Check both the username and password fields have an aria-describedby attribute that points to the error notice with id login_error.

#10 @sabernhardt
2 years ago

  • Milestone changed from 6.0 to 6.1

#11 @sabernhardt
2 years ago

  • Keywords needs-testing added

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


20 months ago

#13 @joedolson
20 months ago

  • Owner changed from alexstine to joedolson

#14 @joedolson
20 months ago

  • Keywords commit added; needs-testing removed

Tested and works as expected; marking for commit.

#15 @joedolson
20 months ago

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

In 53707:

Login: Explicitly associate errors with input fields.

Add an aria-describedby relationship between login email/password input fields and displayed login error messages.

Props patrickgroot, rollybueno, afercia.
Fixes #54483.

#16 @joedolson
20 months ago

In 53708:

Coding Standards: Missed space in code indentation in [53707].

Fixes code style issue from [53707].

Follow up to [53707].

See #54483.

#17 @milana_cap
18 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.