Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Accessibility): Remove role="presentation" from chatMessageBehavior and FocusZone #530

Merged
merged 6 commits into from
Nov 28, 2018

Conversation

sophieH29
Copy link
Contributor

After testing chat with multiple screen readers, we figured out that no role should be assigned to Chat message. Since FocusZone sets role=presentation to container by default, I override it in chatMessageBehavior by setting role=undefined

@codecov
Copy link

codecov bot commented Nov 27, 2018

Codecov Report

Merging #530 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #530   +/-   ##
=======================================
  Coverage   88.17%   88.17%           
=======================================
  Files          42       42           
  Lines        1455     1455           
  Branches      212      187   -25     
=======================================
  Hits         1283     1283           
  Misses        167      167           
  Partials        5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1e5df6...802e81d. Read the comment docs.

* Sets the message to be a focusable element.
* Adds a vertical circular focus zone navigation where a user navigates using a Tab key.
* Adds a key action which prevents up and down arrow keys from navigating in FocusZone, we only want a Tab key to navigate.
*/
const chatMessageBehavior: Accessibility = (props: any) => ({
attributes: {
root: {
role: 'presentation',
role: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be removed altogether then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good question, I was also considering that.
@jurokapsiar, do you think this role=presentation to be set by default in FocusZone? We always can add in behavior in cases where we need it

Copy link
Contributor Author

@sophieH29 sophieH29 Nov 28, 2018

Choose a reason for hiding this comment

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

After discussion with @jurokapsiar, we decided to remove that attribute from FocusZone. Thank you Roman, for raising this question, it always good to have someone to look at the situation with a fresh eye :)
So this attribute is added in the original FocusZone in fabric-ui, and they have some reasons for that, but we don't see the need for it right now, as in most cases we control role in behaviors.
Will update the PR

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 27, 2018
@sophieH29 sophieH29 changed the title fix(Accessibility): Change role="presentation" to role=undefined for chatMessageBehavior fix(Accessibility): Remove role="presentation" from chatMessageBehavior and FocusZone Nov 28, 2018
@sophieH29 sophieH29 added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Nov 28, 2018
@sophieH29 sophieH29 merged commit ca9e14f into master Nov 28, 2018
@sophieH29 sophieH29 deleted the fix/chat-message-behavior-remove-role branch November 28, 2018 12:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants