Skip to content

[ACCESSIBILITY] Global changes to pass AXE/Lighthouse Audits #66

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

[ACCESSIBILITY] Global changes to pass AXE/Lighthouse Audits #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
74ef2ab
chore(release): publish
Jul 26, 2018
3da9842
feat(accessibility): assorted accessibility upgrades
Jul 24, 2018
f5197df
fix(examples): update examples to pass axe audit
Jul 24, 2018
bfb2fb4
fix(docs): fix all accessibility audit failures in react-styleguidist
Jul 24, 2018
dcb155d
Addressed Ryan feedback
Jul 25, 2018
be32908
Addressed JZ feedback
Jul 26, 2018
ba8394e
Fix typo
Jul 26, 2018
45026d8
Merge branch 'master' into agreen/accessibility
Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions packages/autocomplete/src/containers/AutocompleteContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,12 +317,10 @@ class AutocompleteContainer extends ControlledComponent {
};

getItemId = key =>
typeof key === 'undefined' ? '' : `${this.getControlledState().id}--item-${key}`;
typeof key === 'undefined' ? null : `${this.getControlledState().id}--item-${key}`;

getTagId = key =>
typeof key === 'undefined' ? '' : `${this.getControlledState().id}--tag-${key}`;

getMenuId = () => `${this.getControlledState().id}--menu`;
typeof key === 'undefined' ? null : `${this.getControlledState().id}--tag-${key}`;

getInputProps = ({
tabIndex = 0,
Expand All @@ -340,7 +338,6 @@ class AutocompleteContainer extends ControlledComponent {
role,
'aria-autocomplete': 'list',
'aria-haspopup': 'true',
'aria-owns': this.getMenuId(),
Copy link
Contributor

@ryanseddon ryanseddon Jul 25, 2018

Choose a reason for hiding this comment

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

Does this apply when the menu is open? I can't see that anywhere.

Copy link
Contributor Author

@austingreendev austingreendev Jul 25, 2018

Choose a reason for hiding this comment

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

No it does not. Since we are moving focus to the menu it seems to not be required? I ran AXE across all of the states in the menu and the only thing it picked up was us appending it to the body in a "non-landmark region" which would be something a consumer would customize.

'aria-expanded': isOpen,
'aria-activedescendant': isOpen ? this.getItemId(focusedKey) : this.getTagId(tagFocusedKey),
autoComplete: 'off',
Expand Down Expand Up @@ -387,15 +384,8 @@ class AutocompleteContainer extends ControlledComponent {
};
};

getMenuProps = ({
id = this.getMenuId(),
role = 'listbox',
onMouseDown,
onMouseUp,
...other
} = {}) => {
getMenuProps = ({ role = 'listbox', onMouseDown, onMouseUp, ...other } = {}) => {
return {
id,
role,
onMouseDown: composeEventHandlers(onMouseDown, () => {
this.menuMousedDown = true;
Expand Down
2 changes: 0 additions & 2 deletions packages/autocomplete/src/containers/AutocompleteContainer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ describe('AutocompleteContainer', () => {
expect(input).toHaveProp('role', 'combobox');
expect(input).toHaveProp('aria-autocomplete', 'list');
expect(input).toHaveProp('aria-haspopup', 'true');
expect(input).toHaveProp('aria-owns', 'test--menu');
expect(input).toHaveProp('autoComplete', 'off');
});

Expand Down Expand Up @@ -492,7 +491,6 @@ describe('AutocompleteContainer', () => {
it('applies accessibility attributes correctly', () => {
const menu = findMenu(wrapper);

expect(menu).toHaveProp('id', 'test--menu');
expect(menu).toHaveProp('role', 'listbox');
});

Expand Down
37 changes: 20 additions & 17 deletions packages/autocomplete/src/examples/autocomplete.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,27 @@ initialState = {
{!isOpen && <span>{state.value}</span>}
<Input
{...getInputProps(
getFieldInputProps({
bare: true,
innerRef: inputRef,
value: state.inputValue,
onChange: e => {
setState({ inputValue: e.target.value });
getFieldInputProps(
{
bare: true,
innerRef: inputRef,
value: state.inputValue,
onChange: e => {
setState({ inputValue: e.target.value });
},
placeholder: state.value,
onFocus: () => {
setState({ isFocused: true });
},
onBlur: () => {
setState({ isFocused: false });
},
style: !isOpen
? { opacity: 0, height: 0, minHeight: 0, width: 0, minWidth: 0 }
: {}
},
placeholder: state.value,
onFocus: () => {
setState({ isFocused: true });
},
onBlur: () => {
setState({ isFocused: false });
},
style: !isOpen
? { opacity: 0, height: 0, minHeight: 0, width: 0, minWidth: 0 }
: {}
})
{ isDescribed: false }
)
)}
/>
</FauxInput>
Expand Down
91 changes: 47 additions & 44 deletions packages/autocomplete/src/examples/multiselect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ the `input` and it's collection of tags.
This example also includes an example of "copy-to-clipboard" functionality when
a tag is selected.

#### Accessibility Warning
### Accessibility Warning

When implementing a MultiSelect with the ability to delete Tags, be aware of
users that navigate primarily with a keyboard and how your features may affect
Expand Down Expand Up @@ -228,54 +228,57 @@ const MoreAnchor = styled(Anchor)`

<Input
{...getInputProps(
getFieldInputProps({
bare: true,
innerRef: inputRef,
value: state.inputValue,
onChange: e => {
setState({ inputValue: e.target.value });
},
onKeyDown: e => {
if (
e.keyCode === KEY_CODES.DELETE ||
e.keyCode === KEY_CODES.BACKSPACE
) {
if (tagFocusedKey !== undefined) {
deleteTag(tagFocusedKey);
}
getFieldInputProps(
{
bare: true,
innerRef: inputRef,
value: state.inputValue,
onChange: e => {
setState({ inputValue: e.target.value });
},
onKeyDown: e => {
if (
e.keyCode === KEY_CODES.DELETE ||
e.keyCode === KEY_CODES.BACKSPACE
) {
if (tagFocusedKey !== undefined) {
deleteTag(tagFocusedKey);
}

if (e.target.value === '') {
const tags = Object.keys(state.selectedKeys);
deleteTag(tags[tags.length - 1]);
if (e.target.value === '') {
const tags = Object.keys(state.selectedKeys);
deleteTag(tags[tags.length - 1]);
}
}
}

if (tagFocusedKey !== undefined) {
// copy-to-clipboard functionality
if (e.keyCode === 67 && e.metaKey) {
alert(`"${tagFocusedKey}" copied`);
if (tagFocusedKey !== undefined) {
// copy-to-clipboard functionality
if (e.keyCode === 67 && e.metaKey) {
alert(`"${tagFocusedKey}" copied`);
}
}
}
},
placeholder:
Object.keys(state.selectedKeys).length === 0
? 'Enter some content'
: undefined,
onFocus: () => {
setState({ isFocused: true });
},
onBlur: () => {
setState({ isFocused: false });
},
placeholder:
Object.keys(state.selectedKeys).length === 0
? 'Enter some content'
: undefined,
onFocus: () => {
setState({ isFocused: true });
},
onBlur: () => {
setState({ isFocused: false });
},
style: Object.assign(
{ margin: '0 2px', flexGrow: 1, width: 60 },
Object.keys(state.selectedKeys).length !== 0 &&
(!state.isFocused || tagFocusedKey !== undefined) &&
!isOpen
? { opacity: 0, height: 0, minHeight: 0 }
: {}
)
},
style: Object.assign(
{ margin: '0 2px', flexGrow: 1, width: 60 },
Object.keys(state.selectedKeys).length !== 0 &&
(!state.isFocused || tagFocusedKey !== undefined) &&
!isOpen
? { opacity: 0, height: 0, minHeight: 0 }
: {}
)
})
{ isDescribed: false }
)
)}
/>
</FauxInput>
Expand Down
10 changes: 9 additions & 1 deletion packages/checkboxes/src/elements/Checkbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,17 @@ export default class Checkbox extends ControlledComponent {
*/
const { onMouseDown: onFocusMouseDown, ...checkboxProps } = getFocusProps(checkboxInputProps);

let isDescribed = false;

Children.forEach(children, child => {
if (hasType(child, Hint)) {
isDescribed = true;
}
});

return (
<CheckboxView {...wrapperProps}>
<Input {...getInputProps(checkboxProps)} />
<Input {...getInputProps(checkboxProps, { isDescribed })} />
{Children.map(children, child => {
if (!isValidElement(child)) {
return child;
Expand Down
2 changes: 1 addition & 1 deletion packages/checkboxes/src/elements/Checkbox.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('Checkbox', () => {
});

it('applies container props to Message', () => {
expect(wrapper.find(Message)).toHaveProp('id', `${CHECKBOX_ID}--message`);
expect(wrapper.find(Message)).toHaveProp('role', 'alert');
});

it('applies no props to any other element', () => {
Expand Down
13 changes: 6 additions & 7 deletions packages/menus/src/containers/MenuContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,6 @@ class MenuContainer extends ControlledComponent {
}
};

getMenuId = () => `${this.getControlledState().id}--container`;

toggleMenuVisibility = ({ defaultFocusedIndex, focusOnOpen, closedByBlur } = {}) => {
const { isOpen } = this.getControlledState();

Expand All @@ -235,7 +233,6 @@ class MenuContainer extends ControlledComponent {
return {
tabIndex,
'aria-haspopup': true,
'aria-controls': this.getMenuId(),
'aria-expanded': isOpen,
onClick: composeEventHandlers(onClick, () => this.toggleMenuVisibility()),
onKeyDown: composeEventHandlers(onKeyDown, event => {
Expand Down Expand Up @@ -285,13 +282,12 @@ class MenuContainer extends ControlledComponent {
* Props to be applied to the menu container
*/
getMenuProps = (
{ id = this.getMenuId(), tabIndex = -1, role = 'menu', onKeyDown, onFocus, ...other } = {},
{ tabIndex = -1, role = 'menu', onKeyDown, onFocus, ...other } = {},
focusSelectionModel
) => {
const { focusOnOpen } = this.getControlledState();

return {
id,
role,
tabIndex,
onFocus: composeEventHandlers(onFocus, event => {
Expand Down Expand Up @@ -382,7 +378,7 @@ class MenuContainer extends ControlledComponent {
/**
* Props to be applied to each selectable menu item
**/
getItemProps = ({ key, textValue, ...other }) => {
getItemProps = ({ key, role = 'menuitemcheckbox', textValue, ...other }) => {
const { focusedKey } = this.getControlledState();

if (typeof textValue === 'string' && textValue.length > 0) {
Expand All @@ -409,6 +405,7 @@ class MenuContainer extends ControlledComponent {

return {
key,
role,
...other
};
};
Expand Down Expand Up @@ -556,7 +553,9 @@ class MenuContainer extends ControlledComponent {
getContainerProps(this.getMenuProps(props, focusSelectionModel))
),
getItemProps: props =>
getSelectionItemProps(this.getItemProps(props)),
getSelectionItemProps(this.getItemProps(props), {
selectedAriaKey: 'aria-checked'
}),
getNextItemProps: props =>
getSelectionItemProps(
this.getItemProps(this.getNextItemProps(props))
Expand Down
7 changes: 6 additions & 1 deletion packages/menus/src/containers/MenuContainer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,6 @@ describe('MenuContainer', () => {

expect(trigger).toHaveProp('tabIndex', 0);
expect(trigger).toHaveProp('aria-haspopup', true);
expect(trigger).toHaveProp('aria-controls');
expect(trigger).toHaveProp('aria-expanded', false);
});

Expand Down Expand Up @@ -464,6 +463,12 @@ describe('MenuContainer', () => {
});
});

describe('getItemProps', () => {
it('applies correct accessibility role', () => {
expect(findMenuItems(wrapper).first()).toHaveProp('role', 'menuitemcheckbox');
});
});

describe('getNextItemProps()', () => {
beforeEach(() => {
wrapper = mountWithTheme(basicExample({ onChange: onChangeSpy }));
Expand Down
Loading