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

New: output block position for screen readers. Update: refactor the way the screen reader information on block button is delivered. #29060

Closed
wants to merge 46 commits into from

Conversation

alexstine
Copy link
Contributor

@alexstine alexstine commented Feb 17, 2021

Description

Fixes #24556

  • Refactored the block button to be more screen reader accessible.
  • Changed the block title to append "Block." to the title for screen reader users.
  • Fix the Button component to use the provided id for aria-describedby and the description element.
  • Add total blocks announcement.
  • Add position announcement out of total.
  • Add a message to let screen reader users know the parent block title of child blocks.

How has this been tested?

I tested this using NV Access on Windows 7 using Firefox.

Screenshots

Types of changes

Bug Fix and New Feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 17, 2021
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alexstine! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@alexstine alexstine self-assigned this Feb 17, 2021
@alexstine alexstine added [a11y] Epic [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility labels Feb 17, 2021
@tikifez
Copy link

tikifez commented Feb 22, 2021

Absolutely. Should have to begin with.

I want to prefix this by saying this is not something I think needs to be fixed, but visually looks odd. Which clearly doesn't have precedence here. But I don't know, which is why i bring it up.

First, a bit about VoiceOver. When VoiceOver is running it shows a floating text box with the text that it is reading aloud. When VoiceOver's settings are set to "high verbosity" or "medium verbosity" the punctuation and letter-casing given don't follow grammatical rules with the label as it stands in this patch. VoiceOver defaults to high verbosity.

What happens is In the document outline on a post or page, VoiceOver adds text to the end of the label. The text is written like the label text doesn't have ending punctuation, adding things like a comma immediately after or additional text in parenthesis. The important bit is that VoiceOver reads it fine. It's only the phrase shown by VoiceOver in the text box which has anything "wrong" with it, with something like "Paragraph Block, period, comma,
more text" or "Quote Block, period, open parenthesis, text in lowercase, close parenthesis, comma, more text"

To reproduce (if it's even needed), start VoiceFlow with the verbosity set to Medium or High. In WordPress, navigate to a new page or post then tab to the document outline and press space to open, and navigate down the items in the list. When VoiceFlow processes each item in the list it sounds fine, e.g. "paragraph block, selected block" but shows "Paragraph block, period, open parenthesis, (text in lowercase) selected block, close parenthesis, comma, button.

Now that I've gone on for too long for something which is probably a moot point and I don't feel strongly about, if action is needed, probably removing the period at the end would be fine. Thanks for your patience!

@alexstine
Copy link
Contributor Author

Hmm, I tested with similar settings on Mac Voiceover and could not replicate. How weird. Maybe there is something I disabled because Voiceover was talking too much for me.

Anyway, I appreciate the report. Let's see if anyone else can figure out how to replicate.

I still have my doubts due to how Voiceover handles the reading of aria-describedby, but at the same time, I do not know a better way to pull this off. :(

Thanks.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I tested with VoiceOver/Safari (default verbosity settings, I never changed them at all) and the descriptions are read out correctly after the labels, with a slight pause in between.

The code looks good, apart from a couple of issues I left comments about. Would be good to get this tested by someone experienced with speech-to-text software; is there anyone in the accessibility team that can help out with that?

packages/block-editor/src/components/block-title/index.js Outdated Show resolved Hide resolved
packages/blocks/src/api/utils.js Outdated Show resolved Hide resolved
Base automatically changed from master to trunk March 1, 2021 15:45
@talldan
Copy link
Contributor

talldan commented Mar 4, 2021

@tikifez You seem to be testing List View. This PR modifies the descriptions in Navigation Mode, which can be entered by pressing the 'escape' key when you have a block focused.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @alexstine!

I left a couple of comments, but nothing major. So far its looking good.

There are some end to end tests that need to be fixed—previously these just tested the label which contained the positional information. I think it'd be good to expand these to test the described-by element now so that there's not a loss in test coverage. Is that something you're able to tackle @alexstine? If not I may be able to help with that next week.

packages/block-editor/src/components/block-title/index.js Outdated Show resolved Hide resolved
@@ -161,19 +161,24 @@ export function getBlockLabel( blockType, attributes, context = 'visual' ) {
* @param {Object} attributes The values of the block's attributes.
* @param {?number} position The position of the block in the block list.
* @param {string} [direction='vertical'] The direction of the block layout.
* @param {number} total The total number of blocks.
* @param {string} parentTitle In the event of a child block, this param gets the title of the parent block.
*
* @return {string} The block label.
*/
export function getAccessibleBlockLabel(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little surprised there aren't unit tests for this function. Probably my fault as I think I initially wrote it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, just realise there are unit tests. So these will need to be updated, with new test cases added for the additional branches of code. Happy to help with that, either directly or by providing guidance, let me know.

There are docs for testing, but not sure how good they are.

@talldan
Copy link
Contributor

talldan commented Mar 5, 2021

There's another implication of this change I just noticed. getAccessibleBlockLabel is used by the react native code base. That this function no longer returns the block title might be an issue for native mobile. I'll ping some folks on the mobile side to see if we can get some feedback.

I think we could also consider renaming getAccessibleBlockLabel to getAccessibleBlockDescription, but happy to make that a separate change so that we can avoid expanding the scope of this.

Comment on lines 173 to 175
direction = 'vertical',
total,
parentTitle
Copy link
Contributor

@guarani guarani Mar 5, 2021

Choose a reason for hiding this comment

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

This function is used in a React Native file, which will need updating as well:

const accessibilityLabel = getAccessibleBlockLabel(
blockType,
attributes,
order + 1
);

The new total parameter here I understand corresponds to the block count, which comes from:

export const getGlobalBlockCount = createSelector(

@carolinan
Copy link
Contributor

carolinan commented Mar 26, 2021

When I apply the PR and try to run npm build I get the following error:

SyntaxError: gutenberg\packages\block-editor\src\components\block-list\block-selection-button.js:
Identifier 'blockInformation' has already been declared (268:7)

I did run npm install first just in case.

@alexstine
Copy link
Contributor Author

@carolinan I fixed it. Looks like a small regression/bug from when I merged trunk in to my branch, it created another variable with the same name conflicting constants. All fixed up now.

@alexstine
Copy link
Contributor Author

@talldan Removing the title change seemed to be the best idea for this at least for the moment. Could always re-visit it in the future for a smaller change. I believe what I've done here has addressed most concerns. The only issue still making the tests fail is unit tests. As much as I would like to take on unit tests for this, I just don't have the time to learn about JS unit tests at the moment. Can you help me find someone who may be interested in fixing a few issues with the unit tests?

Much appreciated, we're almost there with this.

I will work on adding the changes to the .native file later this week.

Thanks.

@alexstine
Copy link
Contributor Author

So much has changed since I opened this PR. I'm gonna close it out for now and revisit this later down the road. I learned a lot with the first implementation but I think a bigger problem needs to be tackled first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add screen reader announcement to last block in list
9 participants