WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#32495 closed defect (bug) (fixed)

WP Admin bar multi-level menu accessibility

Reported by: joedolson Owned by: ocean90
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-patch has-unit-tests commit
Focuses: accessibility, administration Cc:

Description

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

Attachments (9)

32495.patch (739 bytes) - added by joedolson 6 years ago.
Fixes keyboard accessibility on multilevel menus in adminbar
32495.2.patch (811 bytes) - added by joedolson 6 years ago.
Updated patch: esc_attr, double quotes
32495.3.patch (1.2 KB) - added by joedolson 6 years ago.
Specific handling for search
32495.4.patch (2.3 KB) - added by joedolson 6 years ago.
Add 'wrapper' to object.
32495.5.patch (2.1 KB) - added by joedolson 5 years ago.
Fixed esc_attr usage
32495.diff (1.3 KB) - added by afercia 5 years ago.
32495.2.diff (1.3 KB) - added by afercia 4 years ago.
32495.3.diff (1.5 KB) - added by afercia 4 years ago.
32495.4.2.diff (5.2 KB) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (38)

@joedolson
6 years ago

Fixes keyboard accessibility on multilevel menus in adminbar

#1 @joedolson
6 years ago

  • Focuses accessibility administration added

#2 @joedolson
6 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Priority changed from normal to high

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


6 years ago

#4 @rianrietveld
6 years ago

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.

Last edited 6 years ago by rianrietveld (previous) (diff)

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


6 years ago

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


6 years ago

#7 @obenland
6 years ago

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
6 years ago

Updated patch: esc_attr, double quotes

#8 @joedolson
6 years ago

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

#9 @obenland
6 years ago

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.

#10 @joedolson
6 years ago

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

#12 @joedolson
6 years ago

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
6 years ago

Specific handling for search

@joedolson
6 years ago

Add 'wrapper' to object.

#13 in reply to: ↑ description @obenland
5 years ago

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.

#14 @joedolson
5 years ago

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?

#15 @ocean90
5 years ago

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.

#16 @joedolson
5 years ago

Whoops; thanks for catching that.

@joedolson
5 years ago

Fixed esc_attr usage

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


5 years ago

#18 @jamesbonham
5 years ago

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.

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


5 years ago

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


5 years ago

#21 @jorbin
5 years ago

  • Milestone changed from 4.3 to Future Release

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.

#22 @afercia
5 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Future Release to 4.6

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
5 years ago

#23 @afercia
5 years ago

  • Keywords has-patch added; needs-patch removed

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
4 years ago

#24 @afercia
4 years ago

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
4 years ago

#25 @afercia
4 years ago

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.

#26 @ocean90
4 years ago

  • Component changed from General to Toolbar
  • Keywords needs-unit-tests added
  • Priority changed from high to normal

@ocean90
4 years ago

#27 @ocean90
4 years ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed

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

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


4 years ago

#29 @ocean90
4 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.