Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#47053 closed defect (bug) (fixed)

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

Reported by: jankimoradiya's profile jankimoradiya Owned by: joedolson's profile joedolson
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-screenshots has-patch early commit has-dev-note fixed-major dev-reviewed
Focuses: ui, accessibility Cc:

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 (8)

skip-to-toolbar.png (227.1 KB) - added by jankimoradiya 5 years ago.
47053.diff (700 bytes) - added by audrasjb 5 years ago.
Workaround: including the admin bar on wp_body_open
47053.2.patch (752 bytes) - added by joedolson 5 years ago.
Updated patch using did_action
47053.3.patch (1.2 KB) - added by joedolson 5 years ago.
Removes body attachment of adminbar in admin-bar.js
47053.4.patch (1.6 KB) - added by joedolson 5 years ago.
Adds adminbar action to wp_footer if wp_body_open not executed.
47053.5.diff (1.3 KB) - added by xkon 4 years ago.
47053.6.diff (1.7 KB) - added by SergeyBiryukov 4 years ago.
47053.7.diff (2.8 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (54)

#1 follow-up: @afercia
5 years 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
5 years 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
5 years ago

  • Component changed from General to Toolbar

#4 in reply to: ↑ 2 @afercia
5 years 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
5 years 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.


5 years ago

#7 @afercia
5 years 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
5 years ago

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

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


5 years ago

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


5 years ago

#11 @afercia
5 years 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
5 years ago

Workaround: including the admin bar on wp_body_open

#12 @audrasjb
5 years 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
5 years 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
5 years 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
5 years ago

Updated patch using did_action

#15 @joedolson
5 years 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
5 years ago

  • Keywords needs-refresh removed

#17 @afercia
5 years 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
5 years 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
5 years 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:
https://core.trac.wordpress.org/changeset/17263
https://core.trac.wordpress.org/changeset/16070
https://core.trac.wordpress.org/changeset/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.

Version 0, edited 5 years ago by afercia (next)

#20 @joedolson
5 years 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
5 years ago

Removes body attachment of adminbar in admin-bar.js

#21 @ocean90
5 years 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
5 years 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
5 years 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
5 years ago

Adds adminbar action to wp_footer if wp_body_open not executed.

#24 @joedolson
5 years 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.


4 years ago

#26 @audrasjb
4 years 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 years 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.

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


4 years ago

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


4 years ago

#30 @afercia
4 years ago

  • Keywords needs-dev-note added; 2nd-opinion removed

This ticket was discussed during today's accessibility bug-scrub: we're aiming to try it for 5.4 and in that case it will need a dev note.

@xkon
4 years ago

#31 @xkon
4 years ago

47053.5.diff makes a refresh to apply cleanly since admin-bar.js has changed as noted in comment 27.

Also changes the priority on wp_body_open hook to 0 to ensure that the admin bar will have the earliest priority on rendering for a correct DOM order.

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


4 years ago

#33 @audrasjb
4 years ago

  • Keywords commit added; needs-testing dev-feedback removed

#34 @SergeyBiryukov
4 years ago

47053.5.diff looks good to me. 👍

#35 @audrasjb
4 years ago

Great, thanks for the double check 🙌

#36 @joedolson
4 years ago

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

In 47221:

Toolbar: Load toolbar in wp_body_open when available.

For accessibility, the visual appearance and source order should match. Moving the toolbar to load in the new hook wp_body_open (5.2) fixes a long-standing source order problem.

Props jankimoradiya, afercia, SergeyBiryukov, audrasjb, ocean90, xkon, dinhtungdu.
Fixes #47053.

#37 @audrasjb
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#38 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Having second thoughts about the approach in [47221].

The usage of DocBlock syntax is not quite correct there, as it's neither a function nor a hook definition, just an if condition. WordPress core does not generally have DocBlocks for random pieces of code like this.

More importantly, this logic is specific to wp_admin_bar_render() and should be handled in that function, it does not belong in wp_footer().

47053.6.diff addresses this. No functional difference here, just more appropriate logic.

#39 @SergeyBiryukov
4 years ago

47053.7.diff further simplifies things by making the function action-agnostic and just using a static variable. This is consistent with a similar approach in some other core functions, e.g. wp_load_translations_early().

The patch also updates the function documentation, which is currently not quite correct after [47221].

#40 @SergeyBiryukov
4 years ago

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

In 47455:

Toolbar: Move the logic for rendering the admin bar on wp_footer to wp_admin_bar_render().

Clarify in the function documentation that it is now called on wp_body_open action first, with wp_footer as a fallback.

Follow-up to [47221].

Fixes #47053.

#41 @SergeyBiryukov
4 years ago

  • Keywords fixed-major dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.4 branch after a second committer's review.

#42 @whyisjake
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#43 @SergeyBiryukov
4 years ago

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

In 47467:

Toolbar: Move the logic for rendering the admin bar on wp_footer to wp_admin_bar_render().

Clarify in the function documentation that it is now called on wp_body_open action first, with wp_footer as a fallback.

Follow-up to [47221].

Reviewed by whyisjake, SergeyBiryukov.
Merges [47455] to the 5.4 branch.
Fixes #47053.

#44 follow-up: @jadpm
4 years ago

Not sure if this would trigger a reopen for this ticket, but changes merged and released here about how (and when) the admin bar is printed do affect the functionality for some plugins using that admin bar. This happens in at least 3 of the premium plugins I manage.

A shot description of the usage and the problem:

  • A plugin inserts some object into a post editor, be it a block, a shortcode, or anything else.
  • That object, when processed during the post content rendering, will generate an entry in the admin bar.

We use this to offer edit links to some of our advanced elements when they get printed in post or page content. Imagine a map, or a list, or a form. Before this change, we could keep track of all such elements as they were requested. After this change, for themes using the "wp_body_open" hook, all our data is collected after the admin bar is already printed, hence useless, and the admin bar entry can not be included.

I do not want to open a discussion about whether usability or developers safety should be considered first. Just leaving here a note to stress that changes in the order of execution of things have wild effects in the ecosystem. I would call this a backwards compatibility problem, at least.

Last edited 4 years ago by jadpm (previous) (diff)

#45 in reply to: ↑ 44 @SergeyBiryukov
4 years ago

Replying to jadpm:

Just leaving here a note to stress that changes in the order of execution of things have wild effects in the ecosystem. I would call this a backwards compatibility problem, at least.

Thanks for highlighting that! If it's crucial for a plugin to render the admin bar in the footer, that should be trivial to achieve by using remove_action( 'wp_body_open', 'wp_admin_bar_render', 0 ). I agree this could be considered a back compat issue, that's why it was included in the Miscellaneous Developer Focused Changes in WordPress 5.4 dev note.

#46 @a4jp.com
3 years ago

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

The tab index is 1 but can this be 0?

Note: See TracTickets for help on using tickets.