Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#50702 closed enhancement (fixed)

Invalid tabindex in screen-reader-shortcut

Reported by: erikjandelange's profile erikjandelange Owned by: audrasjb's profile audrasjb
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: good-first-bug has-patch commit
Focuses: accessibility Cc:

Description

I'm trying to update a website to be 100% accessible. Till so far the bugs in the theme are solved but now i have (according to Google Chrome - Lighthouse) an bug in the tabindex of the screen reader shortcut.

In line 451 of the file wp-includes/class-wp-admin-bar.php there is a tabindex set to 1. Which isn't allowed.

The confusing part, that the code is valid, but Lighthouse marks it as invalid because users can expirience an abnormal behaviour of the tab walk-through.

The message Lighthouse returns is:

Some elements have a [tabindex] value greater than 0
A value greater than 0 implies an explicit navigation ordering. Although technically valid, this often creates frustrating experiences for users who rely on assistive technologies. Learn more.

Change History (12)

#1 @afercia
5 years ago

  • Component changed from General to Toolbar
  • Keywords accessibility tabindex screen-reader removed
  • Type changed from defect (bug) to enhancement
  • Version 5.4.2 deleted

Thanks for your report @erikjandelange and welcome to Trac!

Automated checking tools shouldn't be fully trusted. While they cover a wide range of issues that can be automatically detected, some issues always require evaluation from a human.

In this case, the tabindex 1 is legitimate and intentional. Its goal is to make the "Skip to toolbar" link the first focusable thing in the page.

This was definitely necessary when the WordPress admin bar was visually placed at the top but actually rendered at the bottom of the DOM.

Since WordPress 5.4, the admin bar placement was fixed but only for themes that support the new hook wp_body_open. Old themes still render the admin bar in the footer hook.

For more details, see "Accessibility: the Admin Bar is now loaded with wp_body_open when available" in the miscellaneous dev notes for WordPress 5.4: https://make.wordpress.org/core/2020/02/26/miscellaneous-developer-focused-changes-in-wordpress-5-4/

Related ticket: #47053

Glad to hear you're willing to make a website 100% accessible. In this case, I'd say Lighthouse's take on this rule is a bit too strict. If it's reporting it as an error, that's a wrong take. It should be reported as a warning that needs manual investigation.

On the other hand, it is worth considering to remove the tabindex attribute altogether but only for themes that use the new wp_body_open hook because in that case it's unnecessary. However, the tabindex attribute needs to stay for all the other themes.

#2 @afercia
5 years ago

  • Keywords good-first-bug added

In wp-includes/class-wp-admin-bar.php there's a check to conditionally render the "Skip to toolbar" link:

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-admin-bar.php?rev=47771&marks=439-441#L438

I think the best approach would be to prevent this link to be printed out when the admin bar has been rendered on the wp_body_open action. Actually, the link is pointless when the admin bar is printed out as first thing within the body element.

Instead, for all the themes that don't implement the wp_body_open action yet, the admin bar is still printed out in the footer so the skip link needs to stay.

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


5 years ago

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


4 years ago

#5 @audrasjb
4 years ago

  • Milestone changed from Awaiting Review to 5.6

#6 @audrasjb
4 years ago

  • Keywords needs-patch added

See @afercia’s last comment :)

#7 @audrasjb
4 years ago

  • Owner set to audrasjb
  • Status changed from new to accepted

This ticket was mentioned in PR #485 on WordPress/wordpress-develop by sarahricker.


4 years ago
#8

  • Keywords has-patch added; needs-patch removed

Prevent loading of Skip to Toolbar on frontend for modern themes. It is unnecessary there, as the toolbar is the first thing in the DOM. Continue to keep Skip to Toolbar link with tabindex="1" for themes that don't use the new wp_body_open hook.

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

#9 follow-up: @sarahricker
4 years ago

  • Keywords needs-testing added

I implemented @afercia's suggestion as my first good-first-bug. Needs testing, let me know if any changes are needed. Thanks!

#10 in reply to: ↑ 9 @SergeyBiryukov
4 years ago

  • Keywords commit added; needs-testing removed

Replying to sarahricker:

I implemented @afercia's suggestion as my first good-first-bug. Needs testing, let me know if any changes are needed. Thanks!

Thanks for the patch! It works as expected in my testing, and is exactly what's needed here.

#11 @SergeyBiryukov
4 years ago

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

In 48812:

Accessibility: Toolbar: Don't output the "Skip to toolbar" link in modern themes that support the wp_body_open action.

The links is unnecessary there, as the toolbar is the first thing in the DOM within the <body> element.

For themes that don't implement the wp_body_open action yet and render the admin bar in the footer, the "Skip to toolbar" link with tabindex="1" is still necessary, to ensure it's the first focusable element in the page.

Props sarahricker, afercia, erikjandelange, audrasjb.
Fixes #50702.

sarahricker commented on PR #485:


4 years ago
#12

Commited in Trac: https://core.trac.wordpress.org/changeset/48812, closing PR.

Note: See TracTickets for help on using tickets.