Make WordPress Core

Opened 4 years ago

Closed 6 months ago

Last modified 6 months ago

#51786 closed enhancement (fixed)

Accessibility issue with the logo on the login page

Reported by: roytanck's profile roytanck Owned by: joedolson's profile joedolson
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch has-testing-info commit
Focuses: accessibility Cc:

Description

Currently, the login page (wp-login.php) contains a logo, that is created using the following HTML markup:

<h1><a href="https://wordpress.org/">Powered by WordPress</a></h1>

Both the href URL and the link text can be filtered. Through CSS, the link text is placed off-screen, and a logo is added to the link as a background.

The <h1> is required for proper a11y, but it should contain the title for the page below. Something like "Log in" would probably be ideal. Instead, it currently contains a link that takes the user off the page, with no description of the login page's content.

This is causing serious issues for client who are required to conform to WCAG standards, as required by European law.

I'd like to suggested removing the <h1> from around the logo link, and adding an actual <h1> title to the white box below, with relevant textual content.

Attachments (2)

wp-login-with-title.png (11.6 KB) - added by roytanck 4 years ago.
Quick mockup of a login page containing a proper title.
core-test-51786.jpg (41.1 KB) - added by nagpai 6 months ago.
screenshot from test

Download all attachments as: .zip

Change History (32)

@roytanck
4 years ago

Quick mockup of a login page containing a proper title.

#1 @hellofromTonya
4 years ago

  • Version trunk deleted

#2 @roytanck
4 years ago

To clarify, the issue can not be resolved using the relevant filter hooks, because the overall HTML structure is hardcoded.

<h1><a href="[filterable URL]">[filterable link text]</a></h1>

Whatever you put into the two filterable elements, it will always be an <h1> with (nothing but) a <a> in it. The two elements need to be separated to make the page properly accessible.

  • For a11y, the <h1> should contain (just) the page's title
  • The <a> can link to wp.org (or to any other place through filters), but must not be an <h1>

This will require a bit of tinkering with the page's markup and styling, and will introduce a new translation string. And perhaps a new filter for the title text.

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


4 years ago

#5 @joedolson
4 years ago

Hi, @roytanck! Can you provide a citation for why there's a problem with having a link be the content of the site's H1 heading?

In general, I'd be happy if the logo/link used the site's logo & linked to the site home page if the site's title and logo are set; but I'm not aware of any arguments that it isn't acceptable to have a link inside an H1 element. The H1 should illustrate the primary subject of the page; but having a link within that H1 is not an issue that I'm aware of.

As a note, the primary subject of this page is already fairly clear, particularly given that the entire content of the page consists of a couple of form fields and a couple of links. The topic is made clear in numerous ways, and the H1 is not particularly crucial to understand the purpose of the document.

That said, I think that the clarity could be improved if the primary branding of the screen was associated with the site, rather than with WordPress, and that WordPress was secondary branding.

#6 @roytanck
4 years ago

Hi @joedolson. Thank you for looking into this. The problem isn't with the HTML markup as such. It's perfectly fine to have an <a> inside an <h1>. The issue is with the purpose of the <h1>.

https://fae.disability.illinois.edu/rulesets/HEADING_1/

The <h1> should contain an indication of the contents of the current page. A link to another location (whether it's wp.org or the site's home page) is not a description or title for what's found on the current page.

To people using assistive technologies, the purpose of the page is less immediately obvious. They often rely on headings to navigate the page, and don't immediately "see" the form below it.

It's great that WordPress offers filters for the link's href and text, but they can only be used to modify the link. The link text should always describe the link's target (a different page), so in no case will it describe the current page's contents.

This issue came up during an accessibility audit of a client's site (a large government organization in NL). The a11y expert and I concluded that there was no way to fix it using the hooks that WP currently offers. The <h1> and the <a> serve different purposes, and the best option is to split them into two entities. A credit link for WP, and a proper page title.

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


4 years ago

#8 @ryokuhi
4 years ago

  • Milestone changed from Awaiting Review to Future Release

The ticket was discussed today during the accessibility team's weekly bug-scrub.
The team agrees that there are some aspects of the login page to improve, so the ticket will be considered for one of the next releases.
The main topic argument against changing the h1 is that it might impact backward compatibility, so some more exploration should be done.

#9 @sabernhardt
4 years ago

Backward compatibility is not really an obstacle to this change, but it could present some challenges with multiple existing filters and custom CSS.
I've used a custom stylesheet and some of the filters.

Supporting custom CSS (that's already implemented) is not as difficult I had thought with the replaceable background image and text clipping. Anyone who has replaced the logo with a logo of different dimensions probably edited the .login h1 a image and/or size without modifying the h1. It would be good to add a class to the H1 tag to facilitate any overrides, though, for plugins that continue to support older WordPress versions.

Some other considerations:

  1. The wp-login.php file is also used for registration and password-changing forms.
  2. The Powered by WordPress message could be moved to the bottom, with site branding at the top.

#10 follow-up: @sabernhardt
4 years ago

@roytanck Until there's a better option, I recommend using these two filters and replacing the logo image with CSS.

function sab_login_logo_url() {
        return home_url( '/' );
}
add_filter( 'login_headerurl', 'sab_login_logo_url' );

function sab_login_logo_text() {
        return get_bloginfo( 'name' );
}
add_filter( 'login_headertext', 'sab_login_logo_text' );

The Site Title would fit as link text for a link to the home page, and it would work well enough as a heading.

#11 in reply to: ↑ 10 @roytanck
4 years ago

Replying to sabernhardt:

Until there's a better option, I recommend using these two filters and replacing the logo image with CSS.
...
The Site Title would fit as link text for a link to the home page, and it would work well enough as a heading.

Thank you. This is almost exactly what we're currently using.

As for backwards compatibility, I'd assume that at least some plugins/tweaks that style the login screen will have the h1 as part of CSS selectors. So for some 3rd party code, styling could break. Adding more specific CSS classes seems like a good idea.

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


8 months ago

#13 @joedolson
8 months ago

  • Milestone changed from Future Release to 6.7
  • Owner set to joedolson
  • Status changed from new to accepted

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


7 months ago

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


6 months ago

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


6 months ago
#16

  • Keywords has-patch added
  • I've added a "Login" text inside the form and moved the <h1> tag to it. So now the WordPress logo is inside a div.
  • I also added a main tag to the page.

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

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


6 months ago

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


6 months ago

#20 @joedolson
6 months ago

Thanks for the patch @pamprn!

I've created a new version adapted from yours. The new PR contains the following differences:

1) Removed role="main". WordPress no longer adds these fallback alternatives, as they are almost never needed anymore.

2) Change the div that replaced the logo h1 with role="presentation" on the H1. On reviewing, I note that there are well over 300 plugins that reference the specific pattern #login h1. That makes changing this element too risky to be a good idea. However, we can change it to a presentational element so that the semantics are removed. I left the CSS changes pointing to the .logo class, however, as that allows us to write a dev note to encourage people to update their CSS and JS references so that we can remove this in the future.

3) Add h1 headings in the other views: lost password, reset password, and register. The confirm admin email password already had one.

4) Added .screen-reader-text to the headings. While a visible heading is nice, practically speaking these forms are pretty clear cut. They are single-purpose pages, with an appropriate title element, and clearly labeled input fields. The visible heading, to me, creates more labor in updating screenshots than any benefit from a visible heading.

#21 follow-up: @joedolson
6 months ago

  • Keywords needs-testing has-testing-info added

Instructions for testing:

  • Visually inspect the login, registration, lost password, and reset password screens to verify no visual changes.
  • Inspect the source code to verify that these pages have two h1 elements, one on the WP logo with role="presentation" and one in the header element with descriptive text.
  • Verify the presence of a header element and main element in the source code.

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


6 months ago

#23 in reply to: ↑ 21 @nagpai
6 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Environment

  • OS: macOS Sonoma 14.7
  • WordPress: 6.6.2 ( via Playground )
  • Browser: Vivaldi 6.9.3447.48
  • Theme: Twenty Twenty-Four
  • Active Plugins:none

Actual Results

Visually inspect the login, registration, lost password, and reset password screens to verify no visual changes.

  • ✅ Login, Registration, and Lost Password pages show no visual changes.

Inspect the source code to verify that these pages have two h1 elements, one on the WP logo with role="presentation" and one in the header element with descriptive text.

  • ✅ Found the two h1 elements as described in the testing instructions. Descriptive texts - Login, Lost Password and Registration form found on the respective pages header h1 tags.

Verify the presence of a header element and main element in the source code.

  • ✅ Found as expected

Please check attached screenshot for a sample of how it appears in the source code.

Last edited 6 months ago by nagpai (previous) (diff)

@nagpai
6 months ago

screenshot from test

#24 @nagpai
6 months ago

  • Keywords needs-testing removed

#25 @sabernhardt
6 months ago

I think I would rather remove the header and main elements from this patch. Landmarks are a best practice, but the login pages on most sites essentially contain only one region right now. I'm more concerned about the possibility of adding semantic elements when authors have already created them (for example, within the output of login_header or login_footer hooks). At worst, leaving them out of the patch is a lack of improvement to navigating the page at this time.

Then the code could omit the hidden heading for the email confirmation screen without leaving an empty header.

<?php
  if ( 'confirm_admin_email' !== $action ) :
?>
    <h1 class="screen-reader-text"><?php echo $title; ?></h1>
<?php
  endif;
?>

If the login page ever adds the admin toolbar (#48831), that would make regions and/or a skip link more helpful/necessary.

#26 @joedolson
6 months ago

@sabernhardt I'm inclined to agree. It's not the primary purpose of this ticket, and poses some risks.

I did a quick check of plugins that use the login_header and login_footer actions, and while they aren't adding landmarks, they *are* inserting HTML elements; and the first I looked at inserted an open div in the header and a close div in the footer. With this patch, that would be broken code. Any extender who is using these hooks to add an additional wrapper will have that issue.

Additionally, if we do *not* wrap the footer in a footer element, and it ever contains content, we've left orphaned content.

The value of landmarks is in being able to find the sections of the page; the default WordPress version of these forms only have one section, so it's essentially a moot point structurally. If an extender needs additional sections, it probably makes more sense for them to define that.

#27 @joedolson
6 months ago

  • Keywords commit added

With these changes, I think this is good to commit. It has no visual changes, and the only markup changes are in screen-reader-text.

I did consider whether we should move the h1 above the login_header action, but decided it was fine where it is. Even if somebody inserts content using the login_header, the h1 continues to define the primary purpose of the page and describes the content following.

And that would be a misuse of the action, since the login_message filter is better suited to that.

#28 @joedolson
6 months ago

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

In 59138:

Login and Registration: Add descriptive h1 on login screens.

Add an h1 heading with the existing login_header() text string on each view of the login screen. Mark the existing h1, used to wrap the WordPress logo, with role="presentation", to remove it from the headings hierarchy.

Props roytanck, joedolson, ryokuhi, sabernhardt, pamprn, nagpai, mukesh27.
Fixes #51786.

Note: See TracTickets for help on using tickets.