-
Notifications
You must be signed in to change notification settings - Fork 55
feat(accessibility): Editing/adding behaviors and adding accessibility description for components #74
Conversation
src/components/Button/Button.tsx
Outdated
* Default behaviour name: ButtonBehavior | ||
* Default behaviour description: | ||
* - If element is not button, then "role='button'" is applied. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/components/Icon/Icon.tsx
Outdated
@@ -1,13 +1,22 @@ | |||
import * as React from 'react' | |||
import * as PropTypes from 'prop-types' | |||
import { customPropTypes, UIComponent, createShorthandFactory } from '../../lib' | |||
import { IconBehaviour } from '../../lib/accessibility/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IconBehavior
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
* | ||
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/components/Header/Header.tsx
Outdated
* | ||
* | ||
* Other considerations: | ||
* - when description is used in header, then reader narrate both as header text. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/components/Image/Image.tsx
Outdated
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why should the users test/verify this? let's try to provide more concrete information here
src/components/Input/Input.tsx
Outdated
* | ||
* | ||
* Other considerations: | ||
* - if input is search, then user "role='search'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo - use not user
src/components/Header/Header.tsx
Outdated
* | ||
* | ||
* Other considerations: | ||
* - when description property is used in header, readers will narrate both header content and description within the element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'when the description property' is better
src/components/Divider/Divider.tsx
Outdated
* | ||
* | ||
* Other considerations: | ||
* - if separator contains text, then "role='separator'" cannnot be used because text is not narrated by VoiceOver (even with aria-label) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
overriddenRootRole: 'menu', | ||
}) | ||
|
||
describe('aria hidden', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very useful information provided 👍
Couple of notes:
- for
svg
variant of Icon componentaria-hidden
attribute seems to be applied to wrong element - also, there are no tests provided to verify
aria-hidden
attribute forsvg
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?
CHANGELOG.md
Outdated
@@ -35,6 +35,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
- Fix components generation script @kuzhelov ([#105](https://github.com/stardust-ui/react/pull/105)) | |||
- Reactivate tests for `Text` @kuzhelov ([#104](https://github.com/stardust-ui/react/pull/104)) | |||
- Fix Button icon color @levithomason ([#102](https://github.com/stardust-ui/react/pull/102)) | |||
- Fix `icon` shorthand property for Button @kuzhelov ([#112](https://github.com/stardust-ui/react/pull/112)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, take a look on the comments
'aria-pressed': !!props['active'], | ||
'aria-disabled': !!props['disabled'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, there is no aria-disabled
support for Toggle variant anymore?
defaultRootRole: 'button', | ||
accessibilityOverride: MenuBehavior, | ||
overriddenRootRole: 'menu', | ||
describe('Button accessibility', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets place accessibility under one single section in tests:
describe('accessibility', () => {
describe('general', () => {
....
})
describe('as div', () => {
....
})
describe('disabled', () => {
....
})
})
}) | ||
|
||
describe('aria hidden', () => { | ||
it('icon - set to true by default', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to replace 'icon' word on 'font-based' - it will be more expressive in terms of what is the difference from 'svg'
expect(getRenderedAttribute(renderedComponent, 'aria-hidden', '')).toBe('true') | ||
}) | ||
|
||
it('svg - set to true by default', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the sake of consistency, lets use test
instead of it
overriddenRootRole: 'menu', | ||
}) | ||
|
||
test('set to true if alt is not defined', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}) | ||
}) | ||
|
||
describe('aria disabled', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test is duplicated
}) | ||
}) | ||
|
||
describe('toggleButton behavior', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to write as two separate words: toggle Button
defaultRootRole: 'button', | ||
accessibilityOverride: MenuBehavior, | ||
overriddenRootRole: 'menu', | ||
describe('accessibility', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests for 'icon only' Button's accessibility features are missed - although those are described
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Accessibility behaviors & descriptions