WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#27555 closed defect (bug) (fixed)

Make tag post meta box more accessible

Reported by: joedolson Owned by: joedolson
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.0
Component: Editor Keywords: has-patch commit
Focuses: accessibility Cc:

Description

The tag post meta box has a couple of issues:

1) Current tags are not keyboard-navigable, so can't be deleted.
2) If a tag is added from quicklinks, no confirmation of adding is read.

This patch makes the tag delete link focusable, by adding an href attribute value; adds a :focus state equivalent to the existing :hover state for keyboard users; adds aria-live=assertive to the container where tags are injected, and replaces the 'X' currently used for the link text with off-screen text stating "Delete [tag name]".

Known issues with this patch:

  • Adding the href value causes the usual moving window problem; I couldn't find where to attach the default event prevention in post.js
  • 'Delete val' is not translatable as is; do we use wp_localize_script with core? Not sure what to do with that.

Attachments (9)

27555.patch (3.1 KB) - added by joedolson 5 years ago.
First pass at addressing issues with tag meta box
27555.2.patch (2.8 KB) - added by afercia 5 years ago.
27555.diff (2.9 KB) - added by cgrymala 3 years ago.
Refresh of 27555.2
27555.2.diff (10.2 KB) - added by afercia 3 years ago.
27555.3.diff (10.2 KB) - added by afercia 3 years ago.
27555.4.diff (15.8 KB) - added by afercia 3 years ago.
27555.5.diff (19.5 KB) - added by azaozz 3 years ago.
27555.6.diff (19.5 KB) - added by afercia 3 years ago.
with blue icons
27555.7.diff (19.5 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (46)

@joedolson
5 years ago

First pass at addressing issues with tag meta box

#1 @joedolson
5 years ago

  • Keywords has-patch needs-testing dev-feedback added

#2 @johnbillion
5 years ago

  • Version changed from trunk to 3.0

#3 @joedolson
5 years ago

So, there may just be something I'm not understanding, but why was this changed to version 3.0? I'm not sure I get the logic.

#4 @johnbillion
5 years ago

The 'Version' field indicates the first version in which the issue was present (unlike the 'Milestone' field which indicates the target version for getting it fixed).

In this particular case the first affected version isn't really relevant (unlike more severe bugs when we try to identify exactly when they were introduced) but we do it anyway as part of the bug management workflow.

#5 @joedolson
5 years ago

Interesting. Good to know! I've always wondered what that represented; is there an easier way to track that back other than to just start browsing tags? I can start adding that info when I create reports; but it would be nice to do it somehow other than totally manually.

#6 @afercia
5 years ago

Refreshed patch:

  • adds e.preventDefault()
  • makes "Delete" translatable
  • rebuilt from the SVN install root

Main outstanding issues:

  • after a new tag is added, focus should not be moved back to the input field
  • in some circumstances, screen readers may read out something like "Delete tagname tagname" (2 times)

@joedolson, Accessibility Team, anyone: maybe we should avoid to introduce new non-links with href="#", what about buttons instead? Will end up with too many buttons?

@afercia
5 years ago

#7 follow-up: @helen
5 years ago

What would feelings be about holding on this (along with other changes to the tags metabox) for a release and seeing about a switch to a different JS library for the UI? I would love to use Select2 for it instead, assuming we'd bring it in 4.2 for some other things we're working on in the admin: http://ivaynberg.github.io/select2/select2-latest.html#tags

If Select2 needs upstream fixes for a11y, would also love to know about that.

#8 in reply to: ↑ 7 @afercia
5 years ago

Replying to helen:

If Select2 needs upstream fixes for a11y, would also love to know about that.

Just started looking into Select2, where should we report accessibility considerations for the dropdown replacement and for the tagging thing? they're 2 separate cases. New tickets, github, upstream?

#9 @joedolson
5 years ago

Select2 issues should probably be reported upstream to the Select2 developers; but I don't know where they handle it.

I generally agree that if we're going to make significant changes to this area in a near release, it doesn't make sense to work on the current iteration now.

#10 @joedolson
5 years ago

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

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


3 years ago

#12 @joedolson
3 years ago

  • Milestone changed from Awaiting Review to 4.6

#13 @joedolson
3 years ago

  • Status changed from assigned to accepted

#14 @joedolson
3 years ago

Not going to wait on Select2 issues at this point; those may eventually be resolved, but this is simple enough that we can improve it significantly now without continuing to wait.

@afercia - Yes, I agree, probably going with a button; possibly using text-styling will be a better choice.

Will get this patch refreshed soon.

@cgrymala
3 years ago

Refresh of 27555.2

#15 @cgrymala
3 years ago

Refreshed the 27555.2 patch so that it applies properly to the current source and attached it to this ticket.

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


3 years ago

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


3 years ago

#18 @afercia
3 years ago

  • Keywords early added
  • Milestone changed from 4.6 to Future Release

We're almost in 4.6 beta 3, sorry to punt this to future release but definitely something that should be done early in the next release cycle.

#19 @afercia
3 years ago

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

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


3 years ago

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


3 years ago

@afercia
3 years ago

#22 @afercia
3 years ago

  • Keywords needs-testing dev-feedback removed

Refreshed patch. Please notice the tags box should be tested also in Press This.

  • the remove tag links are now buttons
  • they use the circular focus style (see screenshot below)
  • they use a screen reader text "Remove item:" + tagname
  • wp.a11y.speak() is used to give feedback to screen reader users when adding/removing tags

Note: tags can be also custom taxonomies terms so the strings can't refer to "tags" but they should use a generic wording. Alternatively, new taxonomy labels should be introduced and there should be a way to pass them to the JS part. I've opted to keep things simple and use "item". Any feedback welcome.

https://cldup.com/mVfcSAZ-hL.png

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


3 years ago

@afercia
3 years ago

#24 @afercia
3 years ago

  • Keywords commit added

Refreshed patch. As per today's discussion during the accessibility weekly meeting on Slack, changes the term "Item" used in the new strings in "Term". Also, some renaming to better reflect the terms are not "deleted" and instead they're "removed". Looks good to me and any testing (also on Press This) is very welcome!

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


3 years ago

#26 follow-up: @afercia
3 years ago

This should now be coordinated with #33902.

#27 in reply to: ↑ 26 @azaozz
3 years ago

Replying to afercia:

Yes, 27555.3.diff needs a (small) refresh after [38797], sorry. Also, keep in mind that everything that's needed for the Tags metaboxes is also probably needed for Quick/Bulk Edit :)

@afercia
3 years ago

#28 follow-up: @afercia
3 years ago

Refreshed patch after [38797].

Replying to azaozz:

Yes, 27555.3.diff needs a (small) refresh after [38797], sorry. Also, keep in mind that everything that's needed for the Tags metaboxes is also probably needed for Quick/Bulk Edit :)

No problem :) Yep, I've considered what happens on Quick/Bulk Edit and the interaction is a bit different since the tags are "selected" but not actually "added" until the form is submitted. Also, tags removal is a manual process. So I'd say it's fine to have just the Term selected. feedback message there.
Instead, on the Tags meta box there are specific controls and actions to add/remove (even though one could argue that terms are actually added just when the post is saved). Thus, these actions need some additional feedback.

Patch looks good to me, would appreciate a code review. Any feedback's welcome. Trying to recap what the patch does:

  • changes the "X" remove tag links in buttons
  • uses the circular focus style on the buttons
  • adds a screen reader text "Remove item:" + tagname
  • uses wp.a11y.speak() to give screen reader users feedback when adding/removing tags
  • makes the tagcloud-link toggle a button, with an aria-expanded attribute to indicate the tag cloud collapsed/expanded state
  • changes colors for the autocomplete highlighted option in order to have a sufficient contrast ratio, doesn't touch the wplink colors (will open a separate ticket)
  • reduces the font size for the autocomplete on Press This, it was a bit too large (see screenshot)
  • removes CSS related to the old suggest.js (.ac_results) from Press This but keeps it in core for backwards compatibility

Highlighted option accessible colors and X button circular focus:

https://cldup.com/m2wUaWK8Bv.png

Font size in Press This was a bit too large:

https://cldup.com/-jg0xOjvI4.png

@azaozz
3 years ago

#29 in reply to: ↑ 28 @azaozz
3 years ago

Replying to afercia:

27555.4.diff looks pretty good. Tweaked tags-suggest.js a bit in 27555.5.diff and added some inline docs.

The only thing I'm wondering about is the 'circular focus style' on the X buttons. It seems the change in color from light grey to red is pretty strong indication which one is focused. If there is not enough contrast, perhaps can make the red a bit darker?

#30 @afercia
3 years ago

Open to suggestions about colors. We should consider also the hover state, so currently the colors are:

https://cldup.com/bJdQHdfPxI.png

The red is the one already used for the "dismiss" X icons, for example:

https://cldup.com/pYD2sSLuYJ.png

But, just realized the light grey for the normal state doesn't have a sufficient color contrast ratio for accessibility, should be at least 4.5:1. One option could be making the icons use the same blue used for links:

https://cldup.com/YoLXDcz-ew.png

alternatively, they should use a darker grey.

@afercia
3 years ago

with blue icons

#31 @afercia
3 years ago

Refreshed patch to test the icons in blue :)

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


3 years ago

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


3 years ago

@afercia
3 years ago

#34 @afercia
3 years ago

Refreshed patch. Discussed a bit on Slack a few days ago, looks good to go. https://wordpress.slack.com/archives/accessibility/p1476728057002403

#35 @afercia
3 years ago

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

In 38880:

Accessibility: Improve the Tags meta box accessibility.

  • changes the "X" links in buttons, improves their color contrast ratio and focus style
  • adds screen reader text "Remove item: + tagname"
  • uses wp.a11y.speak() to give screen reader users feedback when adding/removing tags
  • makes the tagcloud-link toggle a button, with an aria-expanded attribute to indicate the tag cloud collapsed/expanded state
  • changes colors for the autocomplete highlighted option in order to have a better color contrast ratio
  • reduces the font size for the autocomplete on Press This
  • removes CSS related to the old suggest.js from Press This

Props joedolson, cgrymala, azaozz, afercia.
Fixes #27555.

#36 @cyclic
3 years ago

This change includes removing "return false" from the click handler on $( '.tagcloud-link' ):

https://github.com/WordPress/WordPress/commit/5c555e5d10d57c69b41c2eb5e5fae383805cef88#diff-9d47a87c240c1d10701cd6a02b28aa1bL199

Previously an anchor tag with that class could have any href and it wouldn't make a difference. Now the href must be "javascript:;" to accommodate the fact that the handler no longer returns false. Is that a bug or is there a reason it was removed?

The specific case I'm looking at is in the bp-event-organiser plugin which relies on the tags-box library:

wp_enqueue_script:
https://github.com/cuny-academic-commons/bp-event-organiser/blob/master/includes/wp-frontend-admin-screen/wp-frontend-admin-screen.php#L448

the markup which includes the class "tagcloud" with an href of "#titlediv", which the browser obeys after upgrading WP to include the new tags-box (previously the href was ignored due to "return false"):
https://github.com/cuny-academic-commons/bp-event-organiser/blob/master/includes/wp-frontend-admin-screen/abstraction-metabox.php#L476

Last edited 3 years ago by cyclic (previous) (diff)

#37 @afercia
3 years ago

@cyclic thanks for your comment. In WordPress, that link has been changed in a button element with a type="button"attribute and as such, it doesn't have any default action to prevent. Previously, it was an anchor tag and thus it needed a return false to prevent the link default action.

For accessibility reasons, links that are not real links and instead perform an action should be buttons. In the last months, WordPress has changed many non-links in buttons and still there are many other ones to change. Ideally, the plugin you mentioned should follow core's best practices and change its link in a button :)

Note: See TracTickets for help on using tickets.