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

Add aria-label to describe action of featured image update button #10869

Merged
merged 3 commits into from
Oct 30, 2018

Conversation

ocean90
Copy link
Member

@ocean90 ocean90 commented Oct 21, 2018

Description

Fixes #10867.

How has this been tested?

Activate screen reader text and verify that the label "Click to edit or update the image" is announced.

Currently unsure if "Click to" is necessary or "Edit or update the image" would be enough.

@ocean90 ocean90 added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 21, 2018
@danielbachhuber danielbachhuber added this to the 4.2 milestone Oct 21, 2018
@danielbachhuber danielbachhuber added the [Feature] Document Settings Document settings experience label Oct 21, 2018
@ocean90
Copy link
Member Author

ocean90 commented Oct 21, 2018

After looking at some of the other label, I removed the prefix in 68ae546 so it's consistent with other labels.

@youknowriad
Copy link
Contributor

In case you didn't know about it and something we forget often (me included) is to add an entry in the CHANGELOG of the modified packages each time we do a change. This helps the person releasing the npm packages later. Thanks :)

@afercia
Copy link
Contributor

afercia commented Oct 21, 2018

Nice improvement @ocean90 👍

See also the related #1520 / #10482 for further improvements. Rationale: in an editing context like an editor, users need to know which is the image actually set. This is different from the purpose the image has in the front-end. #10482 tries to add a reference to the image alt (if set) or the image file with a wording along the lines of This image has an empty alt attribute; its file name is %s. Details still to refine.
In this case, I'd tend to think the same thing should be done for the button aria-label but it I guess it can be addressed in #10482 in a next iteration.

@ocean90
Copy link
Member Author

ocean90 commented Oct 21, 2018

@afercia Thanks for the feedback and the links to the issues!

Just to get it right, the label should mention the alt/name of the current selected image? I have set the actual alt attribute of the image to an empty since it was ignored by the screen reader (testing with macOS). If that's right, what about using sprintf( __( 'Edit or update the image "%s"' ), alt || filename ); for the label?

@afercia
Copy link
Contributor

afercia commented Oct 21, 2018

it was ignored by the screen reader

Yes because the aria-label on the wrapping button overrides the button content (which is the image alt)

what about using sprintf( __( 'Edit or update the image "%s"' ), alt || filename ); for the label?

If the image has an alt empty attribute (which is perfectly OK in most of the cases) I'd rather consider to inform users explicitly, using some wording similar to the one proposed in #10482. Maybe consider some coordination between the two PRs?

@ocean90
Copy link
Member Author

ocean90 commented Oct 22, 2018

I'm just worried that the button's primary action gets lost when the string is too long.

@youknowriad youknowriad added the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Oct 29, 2018
@youknowriad
Copy link
Contributor

What do we need to unblock this as it's state for 4.2. It's not technically API freeze, we can move for later but if It can be unblocked quickly, I'm fine keeping it.

@ocean90
Copy link
Member Author

ocean90 commented Oct 29, 2018

I think this can go in. Once #10960 is done I'm happy to improve the string if necessary.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Code wise

@youknowriad youknowriad merged commit d44a1bf into master Oct 30, 2018
@youknowriad youknowriad deleted the add/label-featured-image-button branch October 30, 2018 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Document Settings Document settings experience [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Copy Review Needs review of user-facing copy (language, phrasing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants