WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 11 months ago

#26167 closed task (blessed) (fixed)

Plugin activation links need to contain plugin name and the "Plugin" column should be marked as row header

Reported by: grahamarmfield Owned by: afercia
Milestone: 4.3 Priority: low
Severity: normal Version: 3.8
Component: Plugins Keywords: has-patch commit
Focuses: ui, accessibility Cc:

Description

In the plugins table within admin, the Activate, Deactivate, Edit and Delete links need to contain the plugin name they relate to. This is to provide context for screen reader users.

If required, the plugin name portion of the link can be hidden from sighted users with the use of the 'screen-reader-text' CSS class. An example:

<a href="">Activate<span class="screen-reader-text"> Akismet</span></a>

This shouldn't clash with internationalisation processing since the plugin name won't change in different languages (I believe).

Attachments (8)

26167.patch (5.0 KB) - added by SergeyBiryukov 3 years ago.
26167-1.patch (6.2 KB) - added by bramd 2 years ago.
This patch depends on 25459-functions.patch (see #25459). It solves the internationalization issues by using a placeholder for the plugin name and making this string translatable. I've also sneaked in another little change that marks the column containing the plugin name as row header (<th>) instead of the checkbox which is more appropriate. Probably the ticket title/description should be updated to reflect this change.
26167-2.patch (6.3 KB) - added by bramd 2 years ago.
New patch that uses aria-label for an extended label for activate/deactivate/delete links.
26167.3.patch (6.3 KB) - added by rianrietveld 19 months ago.
26167.4.patch (5.9 KB) - added by rianrietveld 19 months ago.
refresh of 26167.2 also removing the title attributes from the link
26167.5.patch (10.1 KB) - added by afercia 11 months ago.
26167.6.patch (9.9 KB) - added by afercia 11 months ago.
26167.7.patch (6.4 KB) - added by afercia 11 months ago.

Download all attachments as: .zip

Change History (44)

#1 @SergeyBiryukov
3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.8

This shouldn't clash with internationalisation processing since the plugin name won't change in different languages (I believe).

Actually, plugin headers (including title) are translatable, but I don't think that would cause an issue here either.

#2 follow-up: @ocean90
3 years ago

  • Keywords needs-patch added; has-patch removed

I think this needs some more work since there are some translation issues.

First: This needs probably a swap in RTL.
Second: This needs some context since in German "Activate" is currently "Aktivieren". So a screen reader would read "Aktivieren Akismet", that's wrong. It must be "Aktiviere Akismet" in this context - or "Akismet aktivieren".

#3 @dd32
3 years ago

Activate %s should probably be used with the span and translated plugin name injected.

#4 @MikeLittle
3 years ago

It seems from the German example that the verb needs to be translatable in two different contexts. With the plugin name (for screen readers), and without the plugin name for visual readers. That is, with the name is it "verb object", without it is "command".
That cannot be handled with a single translation.

Suggested alternates (assume plugin name is wrapped in the span):
Activate: Plugin name
Activate (plugin name)

#5 @johnbillion
3 years ago

  • Type changed from defect (bug) to enhancement

Related: #25459

#6 @matt
2 years ago

  • Priority changed from normal to low

#7 in reply to: ↑ 2 @daveshine
2 years ago

Replying to ocean90:

I think this needs some more work since there are some translation issues.

First: This needs probably a swap in RTL.
Second: This needs some context since in German "Activate" is currently "Aktivieren". So a screen reader would read "Aktivieren Akismet", that's wrong. It must be "Aktiviere Akismet" in this context - or "Akismet aktivieren".

+1 to this!
This patch needs more testing and I doubt that it is ready for 3.8 already. We should test more and maybe include with 3.9. The translation issues are very important as it directly effects the user experience. The current view should not be changed for sighting users, IMHO.

My suggestion is, to use @MikeLittle's approach with translation context here, maybe in combination with sprintf() and string placeholders.

#8 @daveshine
2 years ago

  • Cc daveshine added

#9 @ocean90
2 years ago

  • Milestone changed from 3.8 to Future Release

@bramd
2 years ago

This patch depends on 25459-functions.patch (see #25459). It solves the internationalization issues by using a placeholder for the plugin name and making this string translatable. I've also sneaked in another little change that marks the column containing the plugin name as row header (<th>) instead of the checkbox which is more appropriate. Probably the ticket title/description should be updated to reflect this change.

#10 @bramd
2 years ago

  • Cc bramd added
  • Keywords has-patch added; needs-patch removed
  • Status changed from new to closed
  • Summary changed from Plugin activation links need to contain plugin name to Plugin activation links need to contain plugin name and the "Plugin" column should be marked as row header

#11 @bramd
2 years ago

  • Status changed from closed to reopened

Whoops, didn't want to close the ticket...

#12 @nacin
2 years ago

  • Component changed from Accessibility to Plugins
  • Focuses accessibility added

@bramd
2 years ago

New patch that uses aria-label for an extended label for activate/deactivate/delete links.

#13 @GrahamArmfield
22 months ago

I think this patch may need refreshing, I don't seem to be able to apply it.

@rianrietveld
19 months ago

refresh of 26167.2 also removing the title attributes from the link

#14 @rianrietveld
19 months ago

  • Keywords needs-testing added

Added 2 patches:

26167.3: refresh of 26167.2 by @bramd
26167.4: refresh of 26167.2 also removing the title attributes from the link

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


19 months ago

#16 @arush
19 months ago

I've tested this with Jaws latest release, and NVDA latest release and this patch works perfectly, even when tabbing through the screen. Browsers tested, IE10, IE11 FF latest release.

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


19 months ago

#18 @arush
19 months ago

  • Keywords needs-testing removed

I think this patch is the perfect way to handle the plugin activation/edit/delete links on the installed plugins screen.

#19 @afercia
19 months ago

Tested patch 26167.4, please check. Seems it breaks the color highlighting (background and left border) for active plugins and updates, inverting a th with a td. Also, tested with Firefox + NVDA, since the table cell with the plugin name and row actions is not a th, it will report wrong information about the plugin. Basically, the table semantics is broken.
The cell with the checkbox should still be a th and the one with plugin name and row actions still be a td.

#20 follow-up: @afercia
19 months ago

Double checked and yes, I definitely agree with @bramd:

I've also sneaked in another little change that marks the column (cell) containing the plugin name as row header (<th>) instead of the checkbox which is more appropriate.

Patch n. 1 gets it right, the issue is in the following patches: the attribute scope="row" was moved back from the th to the td which is semantically incorrect and also not valid HTML. But I definitely agree: the relevant information here is the plugin name, so that should be used as row header. I guess the CSS issue about row colors can be fixed.

One more thing worries me here and in all the other similar tables: the "Select All" checkbox used as column header. This way, for each checkbox in each row, you will hear something like:

row 2 Select All column 1
Select {plugin name} check box not checked

which provides a wrong information, you're not going to "Select All" but to select just that single plugin.
I would propose to don't use the "Select All" as column header.

At this point, I'm wondering if moving the whole "checkboxes" column as last column in the table would be beneficial. Any thoughts more than welcome.

#21 in reply to: ↑ 20 @arush
19 months ago

Replying to afercia:

Double checked and yes, I definitely agree with @bramd:

I've also sneaked in another little change that marks the column (cell) containing the plugin name as row header (<th>) instead of the checkbox which is more appropriate.

Patch n. 1 gets it right, the issue is in the following patches: the attribute scope="row" was moved back from the th to the td which is semantically incorrect and also not valid HTML. But I definitely agree: the relevant information here is the plugin name, so that should be used as row header. I guess the CSS issue about row colors can be fixed.

One more thing worries me here and in all the other similar tables: the "Select All" checkbox used as column header. This way, for each checkbox in each row, you will hear something like:

row 2 Select All column 1
Select {plugin name} check box not checked

which provides a wrong information, you're not going to "Select All" but to select just that single plugin.
I would propose to don't use the "Select All" as column header.

At this point, I'm wondering if moving the whole "checkboxes" column as last column in the table would be beneficial. Any thoughts more than welcome.

I think moving the checkboxes to the last column would be beneficial.

#22 @afercia
11 months ago

Reviewing the ticket and the patches, I'm not so sure we should make the .plugin-title cells be the headers for the table rows. Doing this, all the text inside the plugin title cell will be read out as row header and it would be very redundant and confusing. Please consider the screenshot below.

Tested with Firefox+NVDA and default settings: when navigating vertically through the table cells and moving from one description cell to the description cell below or above, the header for that row will be read out. In this example, moving from the Admin Columns plugin description to the Hello Dolly plugin description will make NVDA read out:

Hello Dolly
Activate Hello Dolly
Edit Hello Dolly
Delete Hello Dolly
This is not just a plugin, it symbolizes etc...

Too much noise :) I'd say all that's needed here is a reference to the plugin name, we could use a screen-reader-text in the description cell to prepend the plugin name to the description and remove the row headers.
Any thoughts more than welcome.

https://cldup.com/IEiXGpXH1q.png

@afercia
11 months ago

#23 @afercia
11 months ago

  • Focuses ui added

Refreshed patch with the new approach. Would be nice to have this shipped in 4.3 with all the other improvements to the List Tables.

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


11 months ago

#25 @joedolson
11 months ago

Looks like what you're going with is just an aria-label attribute with an expanded label? That seems fine to me.

#26 @afercia
11 months ago

Yup, the aria-labels were already in the previous patches. The new thing is removing any th in a row.

#27 @afercia
11 months ago

  • Milestone changed from Future Release to 4.3
  • Owner set to afercia
  • Status changed from reopened to assigned

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


11 months ago

#29 @obenland
11 months ago

  • Type changed from enhancement to task (blessed)

Let's get this in before the weekend. Preferably before beta1.

@afercia
11 months ago

#30 @afercia
11 months ago

Refreshed patch after r33016. Also includes comments for translators. Pleass notice the plugin table would need some CSS adjustments on small screens but that should be done in the responsive table ticket, see #32395

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


11 months ago

#32 @afercia
11 months ago

  • Keywords commit added

Patch still applies, would propose for commit consideration before further changes to the CSS needed for #32395.

#33 follow-up: @obenland
11 months ago

What would the patch look like if we only focused on the links, like the ticket description describes? The checkbox issue is true for way more admin screens than just this one.

#34 in reply to: ↑ 33 @afercia
11 months ago

Replying to obenland:

The checkbox issue is true for way more admin screens than just this one.

Indeed, so maybe we should move the checkbox issue in a general, separate, ticket. See what was done for the Select All column header #31654.

Also, was thinking we could make the Plugin Name cell be the row header and use the abbr attribute, see #32170. If only this worked in all browsers :( currently working just in IE/JAWS.
http://www.w3.org/TR/html5/tabular-data.html#attr-th-abbr
https://bugzilla.mozilla.org/show_bug.cgi?id=673418
Kindly commenting on the bugzilla ticket could maybe help motivating Mozillians to fix it. :)

@afercia
11 months ago

#35 @afercia
11 months ago

Refreshed patch strips out any change to the checkbox column and focuses just on the row action links.

#36 @SergeyBiryukov
11 months ago

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

In 33087:

Plugins list table: Add an aria-label attribute with the plugin name to plugin action links to provide context for screen reader users.

props bramd, rianrietveld, afercia.
fixes #26167.

Note: See TracTickets for help on using tickets.