WordPress.org

Make WordPress Core

Opened 14 months ago

Last modified 11 months ago

#37513 reopened defect (bug)

Admin bar sub menu items dashicon and screen readers

Reported by: afercia Owned by: afercia
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-screenshots needs-patch
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 (4)

37513.diff (1.9 KB) - added by afercia 12 months ago.
37513.2.diff (2.6 KB) - added by afercia 12 months ago.
37513.3.patch (4.1 KB) - added by keraweb 11 months ago.
Fix top-secondary + node tree functions
37513.3.diff (2.5 KB) - added by afercia 11 months ago.

Download all attachments as: .zip

Change History (39)

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


14 months ago

#2 @rianrietveld
14 months 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 14 months ago by afercia (previous) (diff)

#3 @rianrietveld
14 months ago

  • Milestone changed from Awaiting Review to Future Release

#4 @afercia
14 months 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
13 months 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
13 months 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
13 months 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.


12 months ago

#10 @afercia
12 months 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
12 months ago

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

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


12 months ago

@afercia
12 months ago

#13 @afercia
12 months 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
12 months ago

#14 @afercia
12 months 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.


11 months ago

#16 @Cheffheid
11 months 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.


11 months ago

#18 @afercia
11 months 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 11 months ago by afercia (previous) (diff)

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


11 months ago

#20 @afercia
11 months 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
11 months 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 11 months ago by keraweb (previous) (diff)

#22 @keraweb
11 months 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
11 months 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
11 months ago

@afercia

Already busy with a new patch! Allmost done.

@keraweb
11 months ago

Fix top-secondary + node tree functions

#25 @keraweb
11 months ago

Also, this other ticket 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.

Last edited 11 months ago by keraweb (previous) (diff)

#26 @afercia
11 months 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
11 months 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
11 months 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.


11 months ago

@afercia
11 months ago

#30 in reply to: ↑ 28 @afercia
11 months 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
11 months 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
11 months 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
11 months ago

In 39147:

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

See #37513.

#34 @afercia
11 months 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.


11 months ago

Note: See TracTickets for help on using tickets.