Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#34774 closed defect (bug) (fixed)

Accessibility problem: On update screen, missing aria-label tag from plugin and theme checkboxes

Reported by: oaron's profile oaron Owned by: afercia's profile afercia
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.9
Component: Upgrade/Install Keywords: has-patch commit
Focuses: ui, accessibility Cc:

Description

Steps to reproduce with NVDA 2015.4:

  1. Open the update screen
  2. Select plugins or themes table. (important to have new versions available).
  3. When you press the f key, it jumps to "select all checkbox" checkbox, NVDA announces correctly. But when you press f key again, NVDA announces "checkbox", but you don't get any information about its label.

Solution:
Aria-label solves it, and it doesn't cause any design problem (it appears only for the screen readers)
I've created a patch, which is solve this problem.

Attachments (5)

update-core_patch.php (1.5 KB) - added by oaron 8 years ago.
aria-label solution
34774.2.patch (1.8 KB) - added by afercia 8 years ago.
34774.3.patch (2.7 KB) - added by afercia 8 years ago.
34774.4.patch (2.8 KB) - added by afercia 8 years ago.
34774.5.patch (3.3 KB) - added by SergeyBiryukov 8 years ago.

Download all attachments as: .zip

Change History (18)

@oaron
8 years ago

aria-label solution

#1 @afercia
8 years ago

  • Focuses ui added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to afercia
  • Status changed from new to assigned

Thanks very much @oaron, good catch :)

#2 follow-up: @afercia
8 years ago

  • Milestone changed from Future Release to 4.5

While the aria-label solution works, for consistency with other similar tables in the admin screens I'd suggest to fix this using real labels, hidden with screen-reader-text. See 34774.2.patch.

I'd greatly appreciate feedback about $theme->update['theme'] used for the ID and for attribute. cc @obenland thanks!

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

#3 in reply to: ↑ 2 ; follow-up: @obenland
8 years ago

  • Version changed from 4.4 to 2.9

Replying to afercia:

I'd greatly appreciate feedback about $theme->update['theme'] used for the ID and for attribute. cc @obenland thanks!

I'd probably use esc_attr( $plugin_file ) and esc_attr( $stylesheet ) respectively. The plugin slug is not unique, the plugin file is.

#4 in reply to: ↑ 3 @afercia
8 years ago

Replying to obenland:

I'd probably use esc_attr( $plugin_file ) and esc_attr( $stylesheet )

Thanks @obenland, wondering if we should use md5() as in the plugins list table and the network themes list table (for consistency):

`$checkbox_id =  "checkbox_" . md5($plugin_data['Name']);`

`$checkbox_id = 'checkbox_' . md5( $theme->get('Name') );`

$plugin_file will give something like rtl-tester/rtl-tester.php and though they're valid, I'm not so happy to use slashes or dots in an attribute value. But maybe that's just because I'm old :)

#5 follow-up: @obenland
8 years ago

Let's go with $plugin_data->update->id like you originally suggested. It's the plugins directory's topic id for that specific plugin.

#6 @afercia
8 years ago

Refrehed patch (see 34774.3.patch), uses $plugin_data->update->id and esc_attr( $stylesheet ) for the unique IDs. Also, not sure there's still the need to specify the strings in double quotes.

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

#7 @afercia
8 years ago

Ehm just realized this ticket number is 34774 and I've uploaded patches named 34744...

@afercia
8 years ago

@afercia
8 years ago

#8 @afercia
8 years ago

Pending final review, looks good to me.

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


8 years ago

#10 in reply to: ↑ 5 @SergeyBiryukov
8 years ago

Replying to obenland:

Let's go with $plugin_data->update->id like you originally suggested. It's the plugins directory's topic id for that specific plugin.

Would it still work for plugins hosted elsewhere, with custom update handlers?

@afercia
8 years ago

#11 @afercia
8 years ago

Just needs a unique ID, whatever it is :) After all, this is just to associate labels with their controls and the worst thing that can happen would be they're not associated. At this point for simplicity and consistency I'd lean toward the md5 solution using the plugin name and the theme name.
See refreshed patch.

#12 @SergeyBiryukov
8 years ago

  • Keywords commit added

34774.5.patch adds translator comments and improves readability. Should be good to go.

#13 @afercia
8 years ago

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

In 35952:

Accessibility: add missing labels for Plugins and Themes checkboxes in the Updates screen.

Also, adds translator comments and improves readability.

Props SergeyBiryukov, afercia, oaron for the initial patch.
Fixes #34774.

Note: See TracTickets for help on using tickets.