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

Improve block mover labels for speech recognition software #5751

Merged
merged 5 commits into from Mar 23, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Mar 22, 2018

This PR tries to improve the block movers for use with speech recognition software.

Users will voice commands based on the visually displayed tooltip, however the accessible name of the block movers (their aria label attribute) doesn't match the displayed tooltip text. For more details please refer to the related issue #5750.

By matching the tooltip text with the aria-label, speech recognition software will execute the related command, e.g.: Click Move up.

The info about the position in the set of blocks is moved to a span element targeted with an aria-describedby attribute. Screen readers will announce in sequence:

  • the aria label e.g. "Move up"
  • the description e.g. "Move "Paragraph" block from position 2 up to position 1"

notes:

  • I'm not using withInstanceId, please let me know if that's desired
  • I've tried to fix the tests passing the same SVG used in the component, please notice I'm not sure the test was 100% correct even before

Fixes #5750

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Mar 22, 2018
1,
)
}
</span>
</div>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's use withInstanceId to ensure unique IDs. since two movers can be visible at the same time (hovering and selecting)

aria-disabled={ isLast }
/>
<span id="editor-block-mover__up-description" className="editor-block-mover__description">
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have a specific className for these kind of labels? like reader-text or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there's screen-reader-text but I'd like the spans not to be read out as normal content. Hiding them with display: none prevents that and still works for aria-describedby.

<svg width="18" height="18" xmlns="http://www.w3.org/2000/svg" aria-hidden role="img" focusable="false">
<path d="M12.293 6.086L9 9.379 5.707 6.086 4.293 7.5 9 12.207 13.707 7.5z" />
</svg>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactoring

@@ -0,0 +1,14 @@
/**
* Module constants
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Not certain this comment is valid in a separate file

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.

Looks good overall, just left some minor comments, the important one is the instanceId

@afercia
Copy link
Contributor Author

afercia commented Mar 23, 2018

Note: I've removed the quotes from the strings used for the position info because JAWS (screen reader) was reading out them literally, for example:
Move quote paragraph quote block from position ...

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 👍

@afercia
Copy link
Contributor Author

afercia commented Mar 23, 2018

@youknowriad thanks 🎉

@afercia afercia merged commit 85cd6f5 into master Mar 23, 2018
@afercia afercia deleted the update/block-mover-label-description branch March 23, 2018 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block movers don't work so well with speech recognition software
2 participants