WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 11 hours ago

#26167 assigned task (blessed)

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 (7)

26167.patch (5.0 KB) - added by SergeyBiryukov 20 months ago.
26167-1.patch (6.2 KB) - added by bramd 19 months 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 14 months 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 8 months ago.
26167.4.patch (5.9 KB) - added by rianrietveld 8 months ago.
refresh of 26167.2 also removing the title attributes from the link
26167.5.patch (10.1 KB) - added by afercia 4 days ago.
26167.6.patch (9.9 KB) - added by afercia 3 days ago.

Download all attachments as: .zip

Change History (40)

@SergeyBiryukov20 months ago

comment:1 @SergeyBiryukov20 months 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.

comment:2 follow-up: @ocean9020 months 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".

comment:3 @dd3220 months ago

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

comment:4 @MikeLittle20 months 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)

comment:5 @johnbillion20 months ago

  • Type changed from defect (bug) to enhancement

Related: #25459

comment:6 @matt20 months ago

  • Priority changed from normal to low

comment:7 in reply to: ↑ 2 @daveshine19 months 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.

comment:8 @daveshine19 months ago

  • Cc daveshine added

comment:9 @ocean9019 months ago

  • Milestone changed from 3.8 to Future Release

@bramd19 months 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.

comment:10 @bramd19 months 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

comment:11 @bramd19 months ago

  • Status changed from closed to reopened

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

comment:12 @nacin18 months ago

  • Component changed from Accessibility to Plugins
  • Focuses accessibility added

@bramd14 months ago

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

comment:13 @GrahamArmfield12 months ago

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

@rianrietveld8 months ago

@rianrietveld8 months ago

refresh of 26167.2 also removing the title attributes from the link

comment:14 @rianrietveld8 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

comment:15 @slackbot8 months ago

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

comment:16 @arush8 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.

comment:17 @slackbot8 months ago

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

comment:18 @arush8 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.

comment:19 @afercia8 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.

comment:20 follow-up: @afercia8 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.

comment:21 in reply to: ↑ 20 @arush8 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.

comment:22 @afercia4 days 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

@afercia4 days ago

comment:23 @afercia4 days 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.

comment:24 @slackbot4 days ago

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

comment:25 @joedolson4 days ago

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

comment:26 @afercia4 days ago

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

comment:27 @afercia4 days ago

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

comment:28 @slackbot4 days ago

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

comment:29 @obenland4 days ago

  • Type changed from enhancement to task (blessed)

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

@afercia3 days ago

comment:30 @afercia3 days 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

comment:31 @slackbot3 days ago

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

comment:32 @afercia41 hours ago

  • Keywords commit added

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

comment:33 @obenland11 hours 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.

Note: See TracTickets for help on using tickets.