feat(accessibility): Editing/adding behaviors and adding accessibility description for components by kolaps33 · Pull Request #74 · microsoft/fluent-ui-react

kolaps33 · 2018-08-09T13:53:45Z

Accessibility behaviors & descriptions

jurokapsiar · 2018-08-09T14:25:38Z

src/components/Button/Button.tsx

+ * Default behaviour name: ButtonBehavior
+ * Default behaviour description:
+ * - If element is not button, then "role='button'" is applied.
+ *


I would start a new section here, something like Accessibility consideretions. but not sure about the wording yet. The goal is to indicate that we are no longer describing behavior, but other accessibility topics related to the component

maybe it should be:

Default behavior: ButtonBehavior - adds role='button' if element type is other than 'button' Other considerations: - for disabled buttons, add 'disabled' attribute so that the state is properly recognized by the screen reader - if button includes icon only, textual representation needs to be provided by using 'title', 'aria-label', or 'aria-labelledby' attributes

jurokapsiar · 2018-08-09T14:32:05Z

src/components/Icon/Icon.tsx

 import * as React from 'react'
 import * as PropTypes from 'prop-types'
 import { customPropTypes, UIComponent, createShorthandFactory } from '../../lib'
+import { IconBehaviour } from '../../lib/accessibility/'


IconBehavior

codecov · 2018-08-10T15:33:13Z

Codecov Report

Merging #74 into master will increase coverage by 0.65%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   88.38%   89.04%   +0.65%     
==========================================
  Files          47       47              
  Lines         775      776       +1     
  Branches      110      101       -9     
==========================================
+ Hits          685      691       +6     
+ Misses         87       83       -4     
+ Partials        3        2       -1

Impacted Files	Coverage Δ
src/components/Input/Input.tsx	`78.68% <ø> (ø)`	⬆️
src/components/Header/Header.tsx	`95% <ø> (ø)`	⬆️
src/components/Button/Button.tsx	`93.93% <ø> (ø)`	⬆️
src/components/Image/Image.tsx	`100% <ø> (ø)`	⬆️
src/components/Icon/Icon.tsx	`100% <100%> (+20.83%)`	⬆️

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 4b3cd2e...c76637c. Read the comment docs.

levithomason · 2018-08-10T16:39:14Z

src/components/Button/Button.tsx

+ *
+ * Other considerations:
+ *  - for disabled buttons, add 'disabled' attribute so that the state is properly recognized by the screen reader
+ *  - if button includes icon only, textual representation needs to be provided by using 'title', 'aria-label', or 'aria-labelledby' attributes


These are really great points. They are concise, easy to understand, and easy to implement. Before merging this, I want to make sure they get first class treatment in the code base.

Here are the thoughts that come to mind when I read the @accessibility points above:

I want these to stay updated and not get overlooked with library updates.

I would like to enforce a test to be written for each point.

I would like these points to be accessible from other places.

Perhaps we should move these somewhere closer to the Accessibility classes? I can see having these in some kind of requirements JSON file where we could then enforce a test to be written for each one.

Alternatively, we could include these as JSDoc comments directly in the Accessibility classes themselves and parse them out like we do props for components.

I'm not sure Default behavior: ButtonBehavior makes sense to doc site users without including docs for the accessibility classes, which might be a great idea.

Conclusion
This material is too valuable to leave in component comments alone. We should make it more accessible, put it in the path of updates to keep it fresh, and use it for more cases.

Feedback?

I'd recommend having the separate section on the Docs site for accessibility behaviors, the similar as we have Components section. That section should contain all behaviors and description when and how to use them.

I agree, especially once the Accessibility classes will include keyboard handling, we should document them separately.

A lot of this information is however related to the component directly, see especially the 'Other considerations' part:

they include additional information to the user, not something that will be implemented in the behavior.

they are directly related to the component, less to the behavior

as they are suggestions, we cannot really test them (it could be however great to come up with a way of automatically notifying user, that once he uses a button with icon only, he should provide a textual representation as well - haven't figured out yet how to do this taking the flexibility of StarDust into account)

I agree that at least big part of the documentation should be inside of the Accessibility objects code - we should generate doc pages based on that and link the component page to its default behavior page.

I would prefer this to be done in a separate PR - this PR already includes valuable information to the users. We can improve both the user facing side of it as well as the way how it is generated, but that should be a separate PR.

jurokapsiar · 2018-08-14T15:43:48Z

src/components/Header/Header.tsx

+ *
+ *
+ * Other considerations:
+ *  - when description is used in header, then reader narrate both as header text.


I would change this line to:
when description property is used in header, readers will narrate both header content and description within the element. In addition to that, both will be displayed in the list of headings.

jurokapsiar · 2018-08-14T15:46:23Z

src/components/Image/Image.tsx

+ * Other considerations:
+ *  - when alt attribute is empty, then Narrator in scan mode navigate to image and narrate it as empty paragraph
+ *  - when image has role='presentation' then screen readers navigate to the element in scan/virtual mode. To avoid it the attribute "aria-hidden='true'" should be used
+ *  - when image is visible for screen reader and contains another attribute as aria-label/arialabbeledby/title, then verify different screen readers narrations


why should the users test/verify this? let's try to provide more concrete information here

hughreeling · 2018-08-21T11:55:16Z

src/components/Input/Input.tsx

+ *
+ *
+ * Other considerations:
+ *  - if input is search, then user "role='search'"


typo - use not user

hughreeling · 2018-08-21T11:56:25Z

src/components/Header/Header.tsx

+ *
+ *
+ * Other considerations:
+ *  - when description property is used in header, readers will narrate both header content and description within the element.


'when the description property' is better

hughreeling · 2018-08-21T11:59:30Z

src/components/Divider/Divider.tsx

+ *
+ *
+ * Other considerations:
+ *  - if separator contains text, then "role='separator'" cannnot be used because text is not narrated by VoiceOver (even with aria-label)


This might be confusing - it tells the user what happens, but not how to fix. Perhaps 'if the separator does contain text, then the aria-label / aria-labelledby should indicate it's a separator as well as the associated text'

kuzhelov · 2018-08-22T14:37:59Z

test/specs/components/Icon/Icon-test.tsx

+      overriddenRootRole: 'menu',
+    })
+
+    describe('aria hidden', () => {


it is better to introduce test for SVG icon as well, as the problem with unassigned aria-hidden attribute has not been caught by tests now

kuzhelov

Very useful information provided 👍

Couple of notes:

for svg variant of Icon component aria-hidden attribute seems to be applied to wrong element
also, there are no tests provided to verify aria-hidden attribute for svg icon - it is better to do that as those icon's use different rendering trees
Button component's disabled state is not reflected in aria attributes now (but it was before) - is it intentional change?

miroslavstastny · 2018-08-23T12:49:02Z

CHANGELOG.md

You are editing a changelog of a version which is already released

kuzhelov

please, take a look on the comments

kuzhelov · 2018-08-23T18:46:47Z

src/lib/accessibility/Behaviors/Button/ToggleButtonBehavior.ts

-      role: 'button',
+      role: props.as === 'button' ? undefined : 'button',
      'aria-pressed': !!props['active'],
-      'aria-disabled': !!props['disabled'],


so, there is no aria-disabled support for Toggle variant anymore?

kuzhelov · 2018-08-23T18:51:29Z

test/specs/components/Button/Button-test.tsx

-    defaultRootRole: 'button',
-    accessibilityOverride: MenuBehavior,
-    overriddenRootRole: 'menu',
+  describe('Button accessibility', () => {


lets place accessibility under one single section in tests:

describe('accessibility', () => { describe('general', () => { .... }) describe('as div', () => { .... }) describe('disabled', () => { .... }) })

kuzhelov · 2018-08-23T18:52:40Z

test/specs/components/Icon/Icon-test.tsx

+    })
+
+    describe('aria hidden', () => {
+      it('icon - set to true by default', () => {


better to replace 'icon' word on 'font-based' - it will be more expressive in terms of what is the difference from 'svg'

kuzhelov · 2018-08-23T18:53:10Z

test/specs/components/Icon/Icon-test.tsx

+        expect(getRenderedAttribute(renderedComponent, 'aria-hidden', '')).toBe('true')
+      })
+
+      it('svg - set to true by default', () => {


for the sake of consistency, lets use test instead of it

kuzhelov · 2018-08-23T18:54:11Z

test/specs/components/Image/Image-test.tsx

+      overriddenRootRole: 'menu',
+    })
+
+    test('set to true if alt is not defined', () => {


from test case's text it is unclear what is set to true - only from test's implementation code there is a hint that it is about aria-hidden attribute

kuzhelov · 2018-08-24T12:12:37Z

test/specs/components/Button/Button-test.tsx

+        })
+      })
+
+      describe('aria disabled', () => {


test is duplicated

kuzhelov · 2018-08-24T12:13:47Z

test/specs/components/Button/Button-test.tsx

+      })
+    })
+
+    describe('toggleButton behavior', () => {


better to write as two separate words: toggle Button

kuzhelov · 2018-08-24T12:15:02Z

test/specs/components/Button/Button-test.tsx

-    defaultRootRole: 'button',
-    accessibilityOverride: MenuBehavior,
-    overriddenRootRole: 'menu',
+  describe('accessibility', () => {


tests for 'icon only' Button's accessibility features are missed - although those are described

it seems that there is no generic logic that could ensure this fact, as aria-label (or its alternative versions) should be provided by client. The only thing that we could try here is to enforce mandatory coexistence of iconOnly prop and aria attribute in component props' TS types, which seems to be an overcomplex solution

editing,adding behaviours and adding acc. description for components

8ad5ae9

kolaps33 requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 9, 2018 13:53

kolaps33 changed the title ~~Editing/adding behaviors and adding accessibility description for components~~ Draft(not ready for team review) > Editing/adding behaviors and adding accessibility description for components Aug 9, 2018

kuzhelov changed the title ~~Draft(not ready for team review) > Editing/adding behaviors and adding accessibility description for components~~ [WIP] Editing/adding behaviors and adding accessibility description for components Aug 9, 2018

jurokapsiar reviewed Aug 9, 2018

View reviewed changes

mituron added 4 commits August 9, 2018 16:33

removing divider role as it was not correctly implemented

65003f9

returning back divider file, which deleted by mistake

7669fd8

adding tests

83fca4f

adding test, modify tests, change format of description

0309b1f

another changes in descriptions

96aa2e9

levithomason reviewed Aug 10, 2018

View reviewed changes

jurokapsiar reviewed Aug 14, 2018

View reviewed changes

mituron and others added 2 commits August 17, 2018 11:17

changing description based on PR proposal

aa73aa5

minor changes

4031a53

jurokapsiar changed the title ~~[WIP] Editing/adding behaviors and adding accessibility description for components~~ Editing/adding behaviors and adding accessibility description for components Aug 17, 2018

jurokapsiar added the 🚀 ready for review label Aug 17, 2018

jurokapsiar changed the title ~~Editing/adding behaviors and adding accessibility description for components~~ feat(accessibility): Editing/adding behaviors and adding accessibility description for components Aug 20, 2018

hughreeling reviewed Aug 21, 2018

View reviewed changes

changes after PR, merged with latest develop

6a08214

kolaps33 requested a review from miroslavstastny as a code owner August 21, 2018 12:57

mituron added 2 commits August 21, 2018 14:58

reverting back divider to origin state

f8e00fc

adding changelog

97ad26c

kuzhelov reviewed Aug 22, 2018

View reviewed changes

kuzhelov suggested changes Aug 22, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Aug 22, 2018

changes after PR, added unit tests

0e4ee5a

jurokapsiar added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Aug 23, 2018

merge latest master, solve conflicts

9860d3f

miroslavstastny reviewed Aug 23, 2018

View reviewed changes

CHANGELOG.md

Copy link

Member

miroslavstastny Aug 23, 2018

Choose a reason for hiding this comment

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

You are editing a changelog of a version which is already released

mituron added 2 commits August 23, 2018 17:25

moving coment in changelog to correct place

bf801d6

moving comment in changelog file

eff83f6

kuzhelov suggested changes Aug 23, 2018

View reviewed changes

kuzhelov added needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Aug 23, 2018

mituron added 2 commits August 24, 2018 10:19

changes after PR, renaming tests, adding tests for toggleButton behavior

472e747

merge with latest master

5ec91c4

kolaps33 added 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Aug 24, 2018

kuzhelov reviewed Aug 24, 2018

View reviewed changes

changing it keyword to test

e252971

kuzhelov approved these changes Aug 24, 2018

View reviewed changes

capital letter for behavior

641d61a

kuzhelov added ready for merge and removed 🚀 ready for review labels Aug 24, 2018

mituron and others added 4 commits August 24, 2018 15:04

changing aria disabled to aria-disabled

1c23a5c

Merge branch 'master' into feat/acc-behaviours-descriptions

237521f

introduce missed dashes into test names

8106bef

Merge branch 'master' into feat/acc-behaviours-descriptions

c76637c

kuzhelov merged commit 7fbe0ff into master Aug 24, 2018

kuzhelov deleted the feat/acc-behaviours-descriptions branch August 29, 2018 21:06

Conversation

kolaps33 commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Accessibility behaviors & descriptions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jurokapsiar Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kolaps33 commented Aug 9, 2018 •

edited

Loading

jurokapsiar Aug 9, 2018 •

edited

Loading

codecov bot commented Aug 10, 2018 •

edited

Loading