Opened 11 months ago
Closed 11 days ago
#47053 closed defect (bug) (fixed)
Accessibility: Need to set proper 'tabindex' in 'Skip To Toolbar' HTML
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (51)
#1
follow-up:
↓ 2
@
11 months ago
- Keywords needs-patch dev-feedback removed
- Milestone Awaiting Review deleted
- Resolution set to maybelater
- Status changed from new to closed
#2
in reply to:
↑ 1
;
follow-up:
↓ 4
@
11 months ago
Replying to afercia:
WordPress 5.2 is going to introduce a new
wp_body_openhook 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.
#4
in reply to:
↑ 2
@
11 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
@
11 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.
11 months ago
#7
@
11 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.
This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.
7 months ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
7 months ago
#11
@
7 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 );
#12
@
7 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
@
7 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
@
6 months 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?
#15
@
6 months 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.
#17
@
6 months 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
@
6 months 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
@
6 months 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.
#20
@
6 months 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.
#21
@
6 months 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
@
6 months 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
@
6 months 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.
#24
@
6 months 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 months ago
#26
@
6 months 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
@
6 months 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.
2 months ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
2 months ago
#30
@
2 months 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.
#31
@
2 months 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.
2 months ago
#34
@
7 weeks ago
47053.5.diff looks good to me. 👍
#37
@
4 weeks ago
- Keywords has-dev-note added; needs-dev-note removed
Documentation added in the miscellaneous changes dev note: https://make.wordpress.org/core/2020/02/26/miscellaneous-developer-focused-changes-in-wordpress-5-4/
#38
@
2 weeks 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
@
2 weeks 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].
@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_footerbecause 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_openhook 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.