Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#37513 closed defect (bug) (fixed)

Admin bar sub menu items dashicon and screen readers

Reported by: afercia's profile afercia Owned by: afercia's profile afercia
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-screenshots has-patch has-dev-note
Focuses: ui, accessibility Cc:

Description

Some screen readers may try to read out CSS generated content. For example, Safari and VoicOver get the admin bar menu sub-items dashicons-arrow-right as a text element and try to read it. There's nothing to be announced so VoiceOver just says "You are currently on a text element".

https://cldup.com/xOaq2xSczn.png

A minor but nice improvement could be refactoring these arrows, either using CSS borders or some other CSS technique, or using a <span aria-hidden="true"></span> and targeting the span for the generated pseudo element.

Either ways, it could be a good opportunity to improve a bit also the visual, maybe these arrows look a bit "old style".
Designers welcome!

Attachments (6)

37513.diff (1.9 KB) - added by afercia 8 years ago.
37513.2.diff (2.6 KB) - added by afercia 8 years ago.
37513.3.patch (4.1 KB) - added by keraweb 8 years ago.
Fix top-secondary + node tree functions
37513.3.diff (2.5 KB) - added by afercia 8 years ago.
37513.4.diff (2.8 KB) - added by afercia 6 years ago.
37513 after.jpg (94.4 KB) - added by afercia 6 years ago.
Patch applied with plugins making use of right and left arrows for their sub-menus items.

Download all attachments as: .zip

Change History (55)

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


8 years ago

#2 @rianrietveld
8 years ago

  • Keywords 4.7-early added

Discussed this in the wpa11y bug scrub:
This needs some research on how screen readers handle this.

@antotrifiroconaccento proposed: aria-haspopup with aria-expanded maybe could work well
@afercia: https://www.w3.org/TR/css-content-3/#accessibility

Generated content should be searchable, selectable, and available to assistive technologies. The content property applies to speech and generated content must be rendered for speech output.

Last edited 8 years ago by afercia (previous) (diff)

#3 @rianrietveld
8 years ago

  • Milestone changed from Awaiting Review to Future Release

#4 @afercia
8 years ago

Just wanted to clarify what the spec says (https://www.w3.org/TR/css-content-3/#accessibility) is that something is probably going to change in the next future and CSS generated content, including dashicons, will be probably announced as content by assistive technologies. We should monitor this as a general issue for all the CSS generated content used in WordPress

#6 @afercia
8 years ago

  • Keywords 4.7-early removed
  • Milestone changed from Future Release to 4.7

Some interesting data about browser and screen reader support (i.e. which ones announce CSS generated content) here: http://tink.uk/accessibility-support-for-css-generated-content/

Maybe in the future it will be possible to use a CSS-only solution, see:
Alternative Text for Speech
https://www.w3.org/TR/css-content-3/#alt
which will hopefully introduce the ability to specify alternative text for the CSS generated content property, after a slash:

content: "\25BA" / ""; 

No browser support for now. Worth also considering these arrows dashicons get "swapped" during the generation of .rtl files so things get a bit more complicated. The only options I can think of are: using a different CSS technique or use a <span> with aria-hidden. Any thoughts welcome.

#7 @jorbin
8 years ago

@afercia @rianrietveld Is this still on the a11y plan for 4.7? I want to make sure it doesn't get lost if it is.

#8 @afercia
8 years ago

@jorbin we'll discuss this in the next a11y meeting on Slack. Everybody's welcome, meetings are on Mondays 18:00 UTC :)

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


8 years ago

#10 @afercia
8 years ago

Discussed a bit this ticket at today's accessibility weekly meeting. Given the fact CSS generated content will be treated as actual content in the near future (VoiceOver already does that) our recommendation is to make everyone aware of this issue and be prepared. When CSS generated content is not intended to be available for speech output, for example for the several icons used in core, then it should always use an element (for example a <span>) with aria-hidden="true". At the moment, this is the only reliable way to hide CSS generated font icons to assistive technologies.

#11 @afercia
8 years ago

  • Owner set to afercia
  • Status changed from new to assigned

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


8 years ago

@afercia
8 years ago

#13 @afercia
8 years ago

  • Keywords has-patch needs-testing added

The only, reliable, way to hide CSS generated content from screen readers is to wrap it in an element with aria-hidden="true". The proposed patch uses a <span> element for this, which isn't particularly elegant because it gets printed out for all the menu parent items. Seems it doesn't harm anything though.
Any feedback welcome and some testing, especially in IE and Edge, would be nice.

@afercia
8 years ago

#14 @afercia
8 years ago

  • Keywords needs-testing removed

The refreshed patch introduces a couple more checks to print out the icon <span> only when needed. Tested using a menu added by a plugin:

https://cldup.com/EB1KjA6HHf.png

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


8 years ago

#16 @Cheffheid
8 years ago

Looks fine in IE11 and Edge to me. NVDA doesn't announce the arrow (though it doesn't without patch either), but should get someone on JAWS to test too.

http://i.imgur.com/aMvU3mu.png

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


8 years ago

#18 @afercia
8 years ago

Tested with IE11 and JAWS, the arrows don't get announced, as expected, so it's OK. From an a11y perspective looks good. Some more eyes for a review (variable names, edge cases, etc.) would be appreciated.

Last edited 8 years ago by afercia (previous) (diff)

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


8 years ago

#20 @afercia
8 years ago

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

In 38984:

Accessibility: Hide the Toolbar sub-menu icons from assistive technologies.

CSS generated content is going to be rendered for speech output more and more in
the next future. When it's not intended to be available for speech output, for
example with font icons, then special care should be used to hide it from
assistive technologies. At the moment, the only reliable way to do this is making
use of a wrapper element and set aria-hidden="true" on it.

Fixes #37513.

#21 @keraweb
8 years ago

Didn't noticed this change untill I just patched to the latest dev version of WP. So sorry for this late reply.

Two problems

  • On the right side the arrows should be left.
  • I'd say a dev should be able to filter the arrow results.

I'd really like to re-open this and fix the above issues.

Example of a large dropdown on the right side:
As you can see there is also some leftover code for the old-css-style arrows on the right side.

http://i.imgur.com/6elKs1W.png

Last edited 8 years ago by keraweb (previous) (diff)

#22 @keraweb
8 years ago

Just checked the actual changes and the problem is with this line:

<?php
$is_top_secondary_item = 'top-secondary' === $node->parent;

This only check the current items parent, not it's tree.

Maybe add a method to get the current item tree. Could be handy in more cases.

#23 @afercia
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

@keraweb thanks very much, yep this needs to take into account also menus on the right side, missed that. Actually, my concern was to exclude top-secondary, to avoid to print out the new <span> in too many places. Turns out I was wrong :) So since the new <span> just wraps the CSS generated content, do you think it would be OK to print it out also for menus in top-secondary and adjust the CSS accordingly?

#24 @keraweb
8 years ago

@afercia

Already busy with a new patch! Allmost done.

@keraweb
8 years ago

Fix top-secondary + node tree functions

#25 @keraweb
8 years ago

Also, this other ticked I've made crosses the same file for changes:
https://core.trac.wordpress.org/ticket/38636

Let me know if I need to make a new patch for that or that merging won't be a problem.

Version 0, edited 8 years ago by keraweb (next)

#26 @afercia
8 years ago

@keraweb interesting :)
I'm not sure there would be consensus about introducing two new methods at this point of the release (we're almost in Beta 2) and they would need tests too. Before going into that, worth considering there are rules during the release cycle and Beta time is just for bug fixes, not enhancements. So there are probably just two options:

1
Go with a simpler fix.
Not sure we need to check for the tree, could you please expand on that? Tried your patch and the arrows are printed out, they're just hidden below your plugin custom icons:

https://cldup.com/8DHQGReRQl.png

After all, any parent item (except the top level items) should get the arrow, unless I'm missing something. Also, I'd say there's no need to print two <span> elements (right and left): just one generic span and then adjusting the CSS could work. As far as I see, this would replicate the current situation, for example in your plugin you're already overriding the CSS with
#wpadminbar #wp-admin-bar-view-as .vaa-has-icon > .ab-item::before {display: none;}
to hide the default arrow. Of course the plugin CSS selectors should be updated, so this could cause some minor visual breakage in other plugins too and I guess we're a bit late to notify plugin authors with a make core post.

2
Revert this change :) Not a big deal since it's a minor accessibility improvement and can be done better in the next release cycle.

Hint: if you pass 'tabindex' => '0' to the meta of your menu items without links, they become focusable and operable with a keyboard. Small a11y improvement :) See [38035].

#27 @keraweb
8 years ago

@afercia Ah I understand that.

I know my plugin is also not compatible with this patch but that is easily fixed before the release of 4.7 (I already fixed it in local dev).

But I think more plugins use the top-secondary. That's the reason I think it's better to align them correctly in this phase already.

The reason to check the correct tree is because the method needs to know what the root item is in order to know which icon to show and on what location. Not sure this can be fixed without checking the tree. If the current node is a level 4 node it's parent would be a level 3 node and this won't validate against root-default or top-secondary.

Also, the is_tree() function can even be used for more purposes since it checks all parents, not just the root.

Option 1:
One solution without any new methods would be to do the check within the code instead of in a separate method. Not sure on how to do this without large code blocks...

Option 2:
Although I can patch a fix easily for my plugin so the release won't really hurt it, I personally think it should work 100% for a major version release right?
Even though it's a minor accessibility improvement it's quite a difference for the UI and gives a weird view on all parent items that are nested under top-secondary.

And thanks for the hint! I'll look into that :).

#28 follow-up: @keraweb
8 years ago

Oh, another solution would be a CSS change.

If .wp-admin-bar-arrow-right is a child of #wp-admin-bar-top-secondary then align it left and change the icon.

Example

#wpadminbar #wp-admin-bar-top-secondary .menupop .menupop > .ab-item .wp-admin-bar-arrow-right:before {
	top: 1px;
	left: 6px;
	right: auto; /* Overwrite */
	padding: 4px 0;
	content: "\f141";
	color: inherit;
}

This ticket was mentioned in Slack in #core-customize by afercia. View the logs.


8 years ago

@afercia
8 years ago

#30 in reply to: ↑ 28 @afercia
8 years ago

Replying to keraweb:

Oh, another solution would be a CSS change.

Yep, 37513.3.diff is what I had in mind as a possible, simpler approach. It works but would require some CSS adjustments on the plugins side. I'm inclined to revert this change, if a better solution doesn't come up in the next couple days. Not a big deal, I guess it can be done better in the next release cycle :)

#31 @keraweb
8 years ago

@afercia
Ah that looks good aswell!

I think with any change related to this issue any plugins that modify the admin bar icons will have to create a patch for that right? Seems a bit inevitable unless I'm missing something.

#32 @afercia
8 years ago

@keraweb yep I can't think of a way to avoid plugins to have to adjust their custom admin bar menus if they use custom icons so I'm going to revert this change. It needs to be communicated in time, hopefully at the beginning of the next release cycle. It would be also a good opportunity to explain better why and when CSS generated content should be hidden to assistive technologies.

#33 @afercia
8 years ago

In 39147:

Accessibility: Revert [38984] as it needs to be better communicated to plugin authors.

See #37513.

#34 @afercia
8 years ago

  • Milestone changed from 4.7 to Future Release

We can do this better in 4.8, hopefully :)

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


8 years ago

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


7 years ago

#37 @afercia
7 years ago

  • Keywords needs-refresh needs-dev-note added; needs-patch removed
  • Milestone changed from Future Release to 5.0

#38 @afercia
6 years ago

  • Owner afercia deleted
  • Status changed from reopened to assigned

#39 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

@afercia
6 years ago

#40 @afercia
6 years ago

  • Keywords has-patch added; needs-refresh removed
  • Owner set to afercia

37513.4.diff refreshes the patch against current trunk, and merges the previous (reverted) commit with the simpler approach illustrated in 37513.3.diff.

As mentioned previously, this change should be communicated to plugin authors at the beginning of a new release cycle. Being a relatively simple change, I'd propose to consider it for 5.1.

@afercia
6 years ago

Patch applied with plugins making use of right and left arrows for their sub-menus items.

#41 @afercia
6 years ago

  • Milestone changed from 5.1 to 5.2

WordPress 5.1 Beta 1 is going to happen tomorrow January 10, and this change needs to be communicated with a make post at the beginning of a release cycle. Punting to 5.2.

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


6 years ago

#43 @afercia
6 years ago

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

In 44793:

Accessibility: Hide the Toolbar sub-menu CSS generated icons from assistive technologies.

CSS generated content is rendered for speech output. When it's not meant to be announced by assistive technologies, for example with font icons, special care should be used to hide it. At the moment, the only reliable way to do this is making use of a wrapper element and set aria-hidden="true" on it.

Fixes #37513.

#44 @afercia
6 years ago

@keraweb hello. FYI, just merged this patch into trunk. If all goes well, it will be released with WordPress 5.2. A dev note post will be published on the Make WordPress blog.

#45 @keraweb
6 years ago

Thanks for the heads up @afercia ! I'll update to the latest trunk version a.s.a.p. and take a look.

#46 @keraweb
6 years ago

@afercia Works great!

#47 @afercia
6 years ago

@keraweb thanks for testing it! I've seen you've already updated your plugin, super fast :)

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


6 years ago

#49 @desrosj
6 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.