WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 3 weeks ago

#47053 assigned defect (bug)

Accessibility: Need to set proper 'tabindex' in 'Skip To Toolbar' HTML

Reported by: jankimoradiya Owned by: joedolson
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-screenshots has-patch needs-testing early dev-feedback 2nd-opinion
Focuses: ui, accessibility Cc:
PR Number:

Description

I have found that, There is a set tabindex="1" in the "Skip to toolbar" html.

<a class="screen-reader-shortcut" href="#wp-toolbar" tabindex="1">Skip to toolbar</a>

I assume that, tabindex need to set "0" or "-1" for the Accessibility standard.

Attachments (5)

skip-to-toolbar.png (227.1 KB) - added by jankimoradiya 7 months ago.
47053.diff (700 bytes) - added by audrasjb 2 months ago.
Workaround: including the admin bar on wp_body_open
47053.2.patch (752 bytes) - added by joedolson 7 weeks ago.
Updated patch using did_action
47053.3.patch (1.2 KB) - added by joedolson 7 weeks ago.
Removes body attachment of adminbar in admin-bar.js
47053.4.patch (1.6 KB) - added by joedolson 7 weeks ago.
Adds adminbar action to wp_footer if wp_body_open not executed.

Download all attachments as: .zip

Change History (32)

#1 follow-up: @afercia
7 months ago

  • Keywords needs-patch dev-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

@jankimoradiya thanks for your report. There's a reason for this: on the front end, the Toolbar needs to use one of the hooks that are available in a theme. Traditionally, it has been hooked to wp_footer because this hook is widely used by themes. This way, the Toolbar is rendered in the markup at the end of the page, before the closing </body> tag.

Worth noting on the front end the Toolbar markup includes the "Skip to toolbar" link. There's the need to make this skip link the first tabbable element in the page. The only way to do that is by using a tabindex="1" attribute. I'd agree it's a compromise and, as far as I can tell, is the only case in WordPress where a tabindex attribute has a positive value.

The position of the Toolbar in the markup isn't ideal in the first place. Ideally, it should be at the top of the markup right after the start <body> tag so it would match the visual order and there wouldn't be the need for a Skip link.

WordPress 5.2 is going to introduce a new wp_body_open hook meant to be used after the start <body> tag: this could be used to render the Toolbar at the top. However, there's the need to wait for all the themes out there to use the new hook though. I'm afraid improvements in this area will need to wait a while.

#2 in reply to: ↑ 1 ; follow-up: @SergeyBiryukov
6 months ago

Replying to afercia:

WordPress 5.2 is going to introduce a new wp_body_open hook meant to be used after the start <body> tag: this could be used to render the Toolbar at the top. However, there's the need to wait for all the themes out there to use the new hook though. I'm afraid improvements in this area will need to wait a while.

We could use wp_body_open as the main hook, then check did_action( 'wp_body_open' ) and use wp_footer as a fallback, as suggested in #46743. This doesn't require theme updates and seems feasible for 5.3.

#3 @SergeyBiryukov
6 months ago

  • Component changed from General to Toolbar

#4 in reply to: ↑ 2 @afercia
6 months ago

Replying to SergeyBiryukov:

This doesn't require theme updates and seems feasible for 5.3.

Thanks @SergeyBiryukov yep, good point! The "Skip links" should be then adjusted a bit, as they should be the first focusable element in the page. In the front tend, this could be a bit complicated, as that's a theme responsibility.

#5 @SergeyBiryukov
6 months ago

  • Milestone set to Awaiting Review
  • Resolution maybelater deleted
  • Status changed from closed to reopened

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


6 months ago

#7 @afercia
6 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3

Discussed during today's accessibility bug scrub. Agreed to try this for the next major release, 5.3.

#8 @joedolson
3 months ago

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

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


3 months ago

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


2 months ago

#11 @afercia
2 months ago

Worth reminding there's some JavaScript in admin-bar.js that moves the admin bar but only when jQuery is not used on the front end. This is the case with Twenty Nineteen, which doesn't use jQuery.

		addEvent(w, 'load', function() {
			aB = d.getElementById('wpadminbar');

			if ( d.body && aB ) {
				d.body.appendChild( aB );

@audrasjb
2 months ago

Workaround: including the admin bar on wp_body_open

#12 @audrasjb
2 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

47053.diff works fine on my side but needs testing.
By the way, I don't really understand why we should check did_action before using wp_body_open hook :)

#13 @audrasjb
2 months ago

  • Keywords needs-refresh added

Ok sorry for the misunderstanding in the comment above. We _do need to check if wp_body_open is available for old themes backward compatibility :)

#14 @joedolson
7 weeks ago

Can somebody clarify to me why the adminbar is being moved to the footer in that context? Is this because there's a problem running the adminbar at the top of the body when jQuery isn't available?

@joedolson
7 weeks ago

Updated patch using did_action

#15 @joedolson
7 weeks ago

This seems too simple to be right, honestly. I haven't done anything regarding the admin-bar.js, as I'm not clear what needs to be done there, if anything.

#16 @joedolson
7 weeks ago

  • Keywords needs-refresh removed

#17 @afercia
7 weeks ago

I think the admin bar is at the bottom of the source for historical reasons. wp_footer is the only reliable hook that is in place since years and the admin bar was always hooked there.

#18 @joedolson
7 weeks ago

That part I understand; what I don't get is why admin-bar.js has scripting that specifically moves the adminbar to the bottom of the body even if it's initially rendered elsewhere. If it's only for historical reasons, then we should just remove that.

#19 @afercia
7 weeks ago

The scripting part is very old and basically not used as the vast majority of themes do use jQuery on the front end. Except Twenty Nineteen :) So this moving-via-JS doesn't really happen since years on most of the themes. It happens on Twenty Nineteen, even if the admin bar is _already_ printed out at the bottom of the source.

After some software archeology, going through relevant commits:
[17263]
[16070]
[16038]

I landed on this ticket:
https://core.trac.wordpress.org/ticket/14772#comment:71

On the non-debug menu, JS does the following:

  • Append the admin bar as a child of the body element.

and I really don't know what the "non-debug menu" is / was. Maybe someone with good historical knowledge may know. @azaozz? :)

Personally, I think it can be safely removed.

Last edited 3 weeks ago by SergeyBiryukov (previous) (diff)

#20 @joedolson
7 weeks ago

Non-debug menu doesn't mean anything to me, as well. Let's go ahead and remove it; this is a beta, so if that removal has consequences, this is a good time to find out.

@joedolson
7 weeks ago

Removes body attachment of adminbar in admin-bar.js

#21 @ocean90
7 weeks ago

Looking at 47053.3.patch, did_action( 'wp_body_open' ) will always return false as default-filters.php is loaded before any actions are run. Also, if it should be the first thing to load the prio shouldn't be 1000.

I don't think that moving the rendering action of the toolbar is actually a good idea as it comes with some back-compat and performance concerns. Plugins which extend the toolbar may register their extensions in wp_footer or require that everything but the toolbar is already rendered. The primary focus should be set to the site content not the toolbar.

#22 @joedolson
7 weeks ago

Thought that seemed too easy to be true!

I don't know why the priority is 1000; that's not a change in this patch.

#23 @afercia
7 weeks ago

@ocean90 thanks for your feedback. I do understand the backwards compatibility issue.

However, the order of the admin bar in the source is far from ideal and has been an accessibility issue for years. I'd strongly recommend to find a way to solve this problem. We are all here to prioritize user experience over developers convenience :)

The new wp_body_open hook offers a great opportunity to finally fix this issue. I hope we can find together a good way to address backwards compatibility and improve users experience.

@joedolson
7 weeks ago

Adds adminbar action to wp_footer if wp_body_open not executed.

#24 @joedolson
7 weeks ago

This patch will actually work. ;) Whether this is safe to do is a separate question, and may require further exploration.

I'm not sure what the best way is to identify examples of plugins that could trigger problems here. The hooks that might be used in such a case would be very generic, and most examples wouldn't be relevant.

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


6 weeks ago

#26 @audrasjb
6 weeks ago

  • Keywords early dev-feedback 2nd-opinion added
  • Milestone changed from 5.3 to 5.4

This ticket still needs some work and improvements. Moving it to 5.4 with early keyword since the patch needs wider testing.

#27 @dinhtungdu
4 weeks ago

We can skip the admin-bar.js file because we're refactoring it at #47069.

One more thing we need to care about is when the admin bar is hooked to wp_body_open, the skip link shouldn't be rendered.

Note: See TracTickets for help on using tickets.