WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#38185 closed defect (bug) (fixed)

Improve the post "locked indicator" accessibility

Reported by: afercia Owned by: afercia
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility Cc:

Description

When a post is "locked" because someone else is editing it, an icon is displayed instead of the checkbox to visually indicate the locked status:

https://cldup.com/Yo0_EoWBSL.png

This is just a <div> element with an icon and doesn't have any text to convey the locked status to assistive technologies. An aria-label or some hidden text would solve the issue, needs some testing since some screen readers ignore CSS generated content and skip this element because it's just an "empty div".

Attachments (5)

38185.patch (875 bytes) - added by mariovalney 4 years ago.
Fix: added screen-reader-text tag
38185.2.patch (4.2 KB) - added by mariovalney 4 years ago.
38185.3.patch (4.1 KB) - added by mariovalney 4 years ago.
Making things simple
38185.diff (3.3 KB) - added by afercia 4 years ago.
38185.2.diff (3.3 KB) - added by afercia 4 years ago.

Download all attachments as: .zip

Change History (30)

@mariovalney
4 years ago

Fix: added screen-reader-text tag

#1 @mariovalney
4 years ago

  • Keywords has-patch added; needs-patch removed

#2 follow-up: @mattking5000
4 years ago

@afercia

In the supplied patch, this text gets applied to the div even when the post is not locked.

As far as what is going on in the html, that div always exists but the icon gets shown when the post is being edited (and then goes away when it's not).

I think what *should* happen here is that the same ajax calls that make the icon appear/disappear should also apply the correct aria-label. Will screen readers know when the page is updated via ajax and this div changes state? Wondering how to easily indicate to a screen reader when a post gets "unlocked" while they have the post admin page still open.

Last edited 4 years ago by mattking5000 (previous) (diff)

#3 @mattking5000
4 years ago

  • Focuses administration added

#4 in reply to: ↑ 2 ; follow-up: @afercia
4 years ago

Replying to mattking5000:

In the supplied patch, this text gets applied to the div even when the post is not locked.

Yep, right. I think the best option would be keeping it simple and rely on the current wp-locked CSS class. In list-tables.css use display: none by default on the locked indicator and set to display block when the table row has the wp-locked class. Screen readers don't announce element hidden with display: none.

Wondering how to easily indicate to a screen reader when a post gets "unlocked" while they have the post admin page still open.

This could be done with a wp.a11y.speak() message when the locked status is checked and set/unset (see inline-edit-post.js) but then the same should be done when a post gets "locked". Considering there could be multiple posts changing state at the same time (or in a short amount of time), this could lead to multiple messages being announced repeatedly and could be potentially confusing. I'd rather keep it simple, at least for now :)

@mariovalney
4 years ago

#5 in reply to: ↑ 4 @mariovalney
4 years ago

Replying to mattking5000:

In the supplied patch, this text gets applied to the div even when the post is not locked.

Thank you for to point it @mattking5000.

Replying to afercia:

Screen readers don't announce element hidden with display: none.

Thank you for the suggestion @afercia.

Considering there could be multiple posts changing state at the same time (or in a short amount of time), this could lead to multiple messages being announced repeatedly and could be potentially confusing.

To make it less confusing I grouped all posts. So we will have 2 messages at maximum and just in the first locked/unlocked change.

About translate support I'm not sure this is the best approach, but the objective was give the most friendly message to user.

#6 @afercia
4 years ago

@mariovalney interesting :) The patch works nicely, see screenshots below. As you also pointed out, I'm not sure the strings are ideal for translation purposes though. Having translatable strings that are made of several parts makes them hard, if not impossible, to translate. This could be a nice accessibility improvement and I'd like to see it in core. Pinging @SergeyBiryukov for some recommendations here, as he has a better insight into l10n/i18n.

https://cldup.com/AT7z3Ji4vU.png

https://cldup.com/nQAT17pkn6.png

#7 @mariovalney
4 years ago

Thank you @afercia
I guess there is another approach: create that message in back-end, so we can make the translatable strings in one only part (with params, of course).

We can pass that string through heartbeat and make the JS just talk.

#8 follow-up: @afercia
4 years ago

Or maybe simplify the strings: replacement in the JS strings are OK, and there are other cases where WordPress is doing it, but the string should be unique and easy to translate. Also, putting the post title in a JS string makes me wonder about other languages characters set and escaping.

#9 in reply to: ↑ 8 @mariovalney
4 years ago

You are right, I didn't think about other languages characters.

Maybe change the patch to just do a general announcement like posts quantity?

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


4 years ago

#11 @rianrietveld
4 years ago

  • Milestone changed from Awaiting Review to 4.7

We discussed this in the accessibility bug scrub and agree this should be done. If a good patch comes to life shortly we could consider it for 4.7, if not move to future release.

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


4 years ago

@mariovalney
4 years ago

Making things simple

#13 follow-up: @mariusvetrici
4 years ago

I've installed and tested 38185.3.patch and it adds the Span with the text notification.

However, when the indicator is hidden, it is not set to display: none
http://screencast.com/t/IBl4RhEU

rather the height is set to 0:
http://screencast.com/t/lkCRAcbUp5ei

I didn't test this with a screen reader, but as per the comments from above looks like we need to have it display: none to prevent screen readers from reading this text.

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


4 years ago

#15 in reply to: ↑ 13 @mariovalney
4 years ago

@mariusvetrici

It's OK here https://s21.postimg.org/wc7aumufr/display_none.png .
Maybe you should use SCRIPT_DEBUG to wp-admin/css/list-tables.css be used.

#16 in reply to: ↑ 14 @mariovalney
4 years ago

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

@afercia Should I do a patch with just locked-indicator's screen-reader?

#17 @afercia
4 years ago

@mariovalney hi, let's see if @SergeyBiryukov has a chance to help us find a solution for the translatable strings. If not, yep maybe a simpler patch with just hidden text for the icon would be enough, I think.

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


4 years ago

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


4 years ago

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


4 years ago

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


4 years ago

#22 @afercia
4 years ago

We've discussed a bit this ticket in the latest accessibility weekly meeting on Slack and, given the time constraint and the issues about translations, we'd recommend to keep it simple for now. Further iterations can happen in new tickets in the next release cycle.

For now, I'd say to ensure the icon is hidden from assistive technologies and to add a hidden text. the shorter the better. Also, moving the "locked info" with the user gravatar and the related text to the top would be a nice improvement. This way the combination of the two text strings would be announced, for example, as:
Hello world! is locked ... admin is currently editing

@afercia
4 years ago

#23 @afercia
4 years ago

  • Focuses administration removed
  • Keywords commit added; good-first-bug removed

Refreshed patch. Screenshot before:

https://cldup.com/TyTX90-w4j.png

and after:

https://cldup.com/0EeTsBS6_m.png

@afercia
4 years ago

#24 @afercia
4 years ago

Refreshed patch to fix a couple typos.

#25 @afercia
4 years ago

  • Owner set to afercia
  • Resolution set to fixed
  • Status changed from new to closed

In 38965:

Administration: Better accessibility for the "Post locked" indicator.

  • hides the locked icon from assistive technologies
  • adds hidden text to indicate the post is locked
  • moves the info about who's currently editing to the top of the title table cell

Props mariovalney.
Fixes #38185.

Note: See TracTickets for help on using tickets.