WP Admin bar multi-level menu accessibility

[joedolson]

If an admin toolbar dropdown is added with multiple level dropdowns and the intermediary parent is not linked, the descendant dropdowns are not accessible.

This is because the adminbar switches to a <div> for the parent container if there's no link, and it can't receive focus.

Patch switches this to an anchor link that points to the list item container ID for a target value. This could also be solved by adding tabindex="0" to the div, but that would require duplicating the focus and hover styles.

Example: See adminbar navigation implementation in Yoast SEO

[joedolson]

Fixes keyboard accessibility on multilevel menus in adminbar

[rianrietveld]

Tested 32495.patch, access by keyboard works now for multiple level drop downs.

Better would be if this was a valid menu structure altogether (like the adminmenu), without the div ab-sub-wrapper.
The behavious is different, you have to press enter to show a submenu.
Now a keyboard only user has no clue there is a submenu, because the sub menu does not open on focus.

This is a quick bug fix to make the multiple level drop downs work for keyboard only, the same as the other menu items, but I think this whole menu structure should be in review.

[obenland]

The patch looks fairly straight forward.

Any particular reason you used single quotes instead of double quotes for the href attribute? Do you think $node->id would escaping before it's echoed?

[joedolson]

Updated patch: esc_attr, double quotes

[joedolson]

I also removed the if else for $has_link, since it now appends the same value either way.

[obenland]

With the patch applied the search input doesn't stay open, when clicked. This could be an indicator for a back-compat concern if plugins make assumptions like the search-bar does.

[joedolson]

Interesting. I'll take a look at that and see what can be done about that. That's probably a JS assumption somewhere.

[obenland]

Yes, the class gets toggled on/off.

[joedolson]

So, I have two methods for solving this problem. The first is simple; add an exception to class-wp-admin-bar.php so that the node ID 'search' uses a div. (32495.3.patch)

The second is to modify the admin bar node object to have a 'wrapper' parameter with default value 'a', that is set to 'div' for the search object in admin-bar.php. (32495.4.patch)

The latter is more elegant to me, and shouldn't break anything in core; but obviously needs some testing.

If there are plug-ins or themes adding content to the adminbar that depends on a <div> wrapper, the latter solution allows them to keep this while still providing accessibility support for more common circumstances.

[joedolson]

Specific handling for search

[joedolson]

Add 'wrapper' to object.

[obenland]

Replying to joedolson:

This could also be solved by adding tabindex="0" to the div, but that would require duplicating the focus and hover styles.

Maybe worth exploring?

I'm not sure the latest two patches address the back compat concern sufficiently. While core makes that assumption about the search form, plugins might make that same assumption about other parts of the toolbar, including their custom nodes.

[joedolson]

To me, it's the difference between doing a hack to make this work and implementing something more robust. I'd rather not implement a hack to make this work, on the whole.

It should be reasonably easy to identify plugins that add custom nodes to the toolbar without links; can somebody grep the repository to find out what kind of impact this would have?

[ocean90]

esc_attr_e( $node->id ); is used wrong in 32495.4.patch​. It should only be used for translating the attribute which is not the case here.

[joedolson]

Whoops; thanks for catching that.

[joedolson]

Fixed esc_attr usage

[jamesbonham]

This does work fine, but it does have some side effects which should be considered.

1. For mouse users, the top level in the dropdown now behaves as if it is clickable, going blue on hover.
2. For mouse users, if you click the top level item, nothing happens as far as the user is concerned, but the color stays blue even if you move the mouse away (because it is active).

So my thought on this is that focus and hover styles should deviate here, with mouse hover styles behaving much like they do without this patch, but still retaining the blue focus style, so you know where you are when navigating by keyboard.

[jorbin]

I share the same backcompat concerns as Obenland. As the admin bar is looking like it will have some larger changes in the futurer (#32678), then this should happen with that.

[afercia]

Looking back at this, I'd propose first of all to allow a proper use of the tabindex attribute. For accessibility, the only important tabindex values to set on the empty menu items (the ones rendered with a <div> element) are 0 or -1. By the way in _render_item() these two lines don't allow to use '0' to make an item focusable:

$tabindex = isset( $node->meta['tabindex'] ) ? (int) $node->meta['tabindex'] : '';
$aria_attributes = $tabindex ? 'tabindex="' . $tabindex . '"' : '';

So say I've set on my menu item meta a tabindex => '0' then it is set and casted to integer. In the following line is converted to boolean to be evaluated and it is false so no luck. I can't make my div focusable.
I'd propose to change this in order to only allow 0 and -1. Any other value shouldn't be allowed.

This way, diligent plugin authors could build keyboard accessible multi-level menus. The change would be simple enough to allow further improvements and I don't see backcompat concerns.

[afercia]

Alternative approach allowing tabindex="0" on the div. This would work when plugin authors explicitly set a tabindex meta on the menu item. I'm unsure about the opportunity to go further and introduce some check to force a tabindex attribute. Something like "if is parent and has no link and has no tabindex then set a tabindex" could work but it's very close to an assumption. Any thoughts more than welcome.

Worth noting there are other outstanding accessibility issues on the admin bar menu that should probably be addressed in separate tickets.

[afercia]

Refreshed and simplified patch. This just allows a tabindex value of 0 which is what's needed for accessibility to make menu items without links focusable with a keyboard. Maybe using min() and max() we could force the values and allow just 0 and -1 but I guess this should be discussed a bit before.

[afercia]

As @ocean90 pointed out 32495.2.diff​ would change the current behaviour if someone is defining a tabindex with an empty string as value. So 32495.3.diff​ takes a different approach that should preserve the current behavior and allow a tabindex with a value of 0, which is the value we need for accessibility.

Wondering if this could be simplified and made more readable, not sure there's really the need to keep the two ternary expressions. Any thoughts welcome.

[ocean90]

32495.4.2.diff​ adds unit tests and changes is_int( $tabindex ) to '' !== $tabindex.

[ocean90]

In 38035:

Toolbar: Allow 0 as a value for the tabindex property of a menu item.

To enhance accessibility for items without a link you can now define tabindex="0", which makes descendant dropdowns accessible.

Props joedolson, afercia, ocean90.
Fixes #32495.