Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/issue 1659 give editable fields better labels #12473

Conversation

@timwright12
Copy link
Contributor

timwright12 commented Nov 30, 2018

Description

Fixes #1659

How has this been tested?

In browser tests with the inspector
Ran local unit tests
Ran local e2e tests
Regenerated snapshot

Types of changes

Added aria-labels to fields outlined in the issue so they differ from the placeholder attributes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
Tim Wright added 28 commits Oct 30, 2018
Copy link
Member

jorgefilipecosta left a comment

This changes tested well and the code changes look correct.
Before:
screenshot 2018-12-05 at 13 25 33
After:
screenshot 2018-12-05 at 13 26 50

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 5, 2019

@timwright12 would you mind refreshing this PR? There is one file that got updated in the master branch and might have some conflicts to resolve. There is also one failing e2e test which was rather a temporary thing which we shouldn't be worried about. Does it need sign of from the accessibility team or can we decide based on the description from #1659?

…-labels
@timwright12

This comment has been minimized.

Copy link
Contributor Author

timwright12 commented Feb 5, 2019

@gziolo I fixed the conflict, but I'm getting some troubling build and npm install permission errors since the latest merge in from master, so I'm not able to regen the snapshots for or run the e2e tests right now. I'll work that out, but if someone has it working locally and can run that command before me, that would be great as well.

@timwright12

This comment has been minimized.

Copy link
Contributor Author

timwright12 commented Feb 5, 2019

@gziolo Looks like all checks are passing now

Copy link
Contributor

afercia left a comment

Thanks for working on this! These changes are an improvement for screen reader users but I'm afraid they won't solve all the accessibility concerns.

A quick example to clarify: quote and paragraph:

screenshot 2019-02-05 at 21 08 21

The quote visible placeholder is "Add quote". Note this is not a real HTML placeholder attribute so it doesn't get announced by screen readers. It's just text which disappears as soon as the field gets focused.

The quote aria-label is "Quote". This gives the filed its accessible name which is announced by screen readers and clearly identifies what this field is about. Perfect.

However, the visible text mismatches the accessible name.

Speech input users see a visible text "Add quote" and will try to voice a command like "Click add quote" to focus the field. That wouldn't have any effect because the actual accessible name of the field is "Quote" instead.

This is one of the reasons why, as accessibility specialists, we always stress that visible labels are always preferred. In this case, the root cause of the issue is a design that didn't take into account accessibility requirements.

The only way I see as a viable option without design changes is to make the placeholder match the aria-label, e.g.: Quote…. This applies to all the blocks.

The paragraph is a special case.
For the aria-label, I'd suggest to always use Paragraph…. Not sure using Empty paragraph block when the block is empty is really helpful.

The visible text Add text or type forward slash to choose a new block type totally mismatches the accessible name. Also, this text is not announced by screen readers. The only compromise I can think of would be trying the following:

When the paragraph has no content:

  • aria-label: Paragraph
  • the placeholder text Add text or type forward slash to choose a new block type should be targeted by aria-describedby set on the editable field (unique IDs would be required)
  • when the empty field gets focused, the placeholder should not be removed from the DOM: it should just be hidden and still be targeted by aria-describedby

When the paragraph has content:

  • aria-label: Paragraph
  • remove the aria-describedby and the placeholder

I now it's getting complicated, but it's because of the design constraints.

@gziolo gziolo requested a review from mapk Feb 6, 2019
@@ -177,7 +177,8 @@ class AudioEdit extends Component {
{ ( ! RichText.isEmpty( caption ) || isSelected ) && (
<RichText
tagName="figcaption"
placeholder={ __( 'Write caption…' ) }
placeholder={ __( 'Add audio caption' ) }

This comment has been minimized.

Copy link
@gziolo

gziolo Feb 6, 2019

Member

I'm seeing that there is some discussion about unifying placeholder and aria-label. If that happens then we might want to revisit the part which removes using placeholder value also as aria-label.

Another thing to consider is using screen-reader-text class name with an HTML element included inside RichText component out of the box when placeholder is provided. A similar solution for the post title:

<label htmlFor={ `post-title-${ instanceId }` } className="screen-reader-text">
{ decodedPlaceholder || __( 'Add title' ) }
</label>

I don't think label would be correct in such context but we could use aria-labelledby somehow as presented on MDN:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/textbox_role#Description

This comment has been minimized.

Copy link
@afercia

afercia Feb 6, 2019

Contributor

Note the hidden <label> element on the title was added in #5669 to support speech input software (Dragon). We should take into consideration Dragon doesn't fully supports ARIA.

packages/editor/src/components/block-list/style.scss Outdated Show resolved Hide resolved
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Apr 3, 2019

@timwright12 - this PR needs to be rebased with the latest changes in the master branch. In general, most of the test files were removed, so hopefully, this shouldn't be a big issue.

Otherwise, this PR should be good to land assuming that proposed changes were reviewed by the Accessibility team.

I now see the comment from @afercia, it still needs some work.

aria-label={ content ? __( 'Paragraph block' ) : __( 'Empty block; start writing or type forward slash to choose a block' ) }
placeholder={ placeholder || __( 'Start writing or type / to choose a block' ) }
aria-label={ content ? __( 'Paragraph' ) : __( 'Empty paragraph block' ) }
placeholder={ placeholder || __( 'Add text or type forward slash to choose a new block type' ) }

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 3, 2019

Member

Is there a corresponding ticket opened in WordPress core to update placeholder text set on PHP side?

@timwright12

This comment has been minimized.

Copy link
Contributor Author

timwright12 commented Aug 27, 2019

@timwright12 pinging myself on this one

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Dec 17, 2019

Hi @timwright12, thank you for the effort in this PR. It seems the PR needs some updates/rebase. Is there any difficulty you are having or anything we could do to help move the PR forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.