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.

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

fix(List, ListItem): Improve accessibility for List and migrate tests #949

Merged
merged 13 commits into from
May 15, 2023
Merged

fix(List, ListItem): Improve accessibility for List and migrate tests #949

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cda68f2
feat: migrate List tests
PavelKirvelTextkernel Apr 20, 2023
d648951
feat: update role listItem
PavelKirvelTextkernel Apr 21, 2023
42d76dc
feat: update list test
PavelKirvelTextkernel Apr 21, 2023
97375ff
test: changes after code review
PavelKirvelTextkernel Apr 24, 2023
e254d79
test: changes after code review
PavelKirvelTextkernel Apr 27, 2023
82d37ea
test: add userEvent setUp
PavelKirvelTextkernel Apr 27, 2023
6063445
test: add userEvent setup
PavelKirvelTextkernel Apr 27, 2023
1acfb15
Merge branch 'master' into ONEUI-364_Migrate_List_tests
PavelKirvelTextkernel Apr 28, 2023
294cfee
test: changes after master merged
PavelKirvelTextkernel Apr 28, 2023
38126a0
refactor List tests
eszthoff Apr 28, 2023
a05f784
test: change keyDown function
PavelKirvelTextkernel May 1, 2023
d2b8c74
Improving List component accessibility (#962)
eszthoff May 10, 2023
7aec95e
test: code refactoring
PavelKirvelTextkernel May 15, 2023
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
27 changes: 18 additions & 9 deletions ...mponents/AutosuggestDeprecated/__tests__/__snapshots__/AutosuggestDeprecated.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,10 @@ exports[`AutosuggestDeprecated rendering should render all suggestions from the
className="ListItem ListItem--isHighlighted ListItem--clickable List__item"
id="downshift-5-item-0"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -272,9 +273,10 @@ exports[`AutosuggestDeprecated rendering should render all suggestions from the
className="ListItem ListItem--clickable List__item"
id="downshift-5-item-1"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -307,9 +309,10 @@ exports[`AutosuggestDeprecated rendering should render all suggestions from the
className="ListItem ListItem--clickable List__item"
id="downshift-5-item-2"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -342,9 +345,10 @@ exports[`AutosuggestDeprecated rendering should render all suggestions from the
className="ListItem ListItem--clickable List__item"
id="downshift-5-item-3"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -654,8 +658,9 @@ exports[`AutosuggestDeprecated rendering should render noSuggestions placeholder
key=".0"
>
<li
aria-selected={false}
className="ListItem ListItem--disabled List__item"
role="presentation"
role="option"
>
<Text
context="muted"
Expand Down Expand Up @@ -815,9 +820,10 @@ exports[`AutosuggestDeprecated rendering should render suggestions also if it pa
className="ListItem ListItem--isHighlighted ListItem--clickable List__item"
id="downshift-6-item-0"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -850,9 +856,10 @@ exports[`AutosuggestDeprecated rendering should render suggestions also if it pa
className="ListItem ListItem--clickable List__item"
id="downshift-6-item-1"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -885,9 +892,10 @@ exports[`AutosuggestDeprecated rendering should render suggestions also if it pa
className="ListItem ListItem--clickable List__item"
id="downshift-6-item-2"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down Expand Up @@ -920,9 +928,10 @@ exports[`AutosuggestDeprecated rendering should render suggestions also if it pa
className="ListItem ListItem--clickable List__item"
id="downshift-6-item-3"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseMove={[Function]}
role="presentation"
role="option"
>
<MarkedText
inline={true}
Expand Down
22 changes: 12 additions & 10 deletions src/components/Dropdown/__tests__/Dropdown.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ describe('Dropdown', () => {
const mockOnDropdownStateChange = jest.fn();
let view;

const getListItemAt = (index) => screen.getAllByRole('option')[index];

beforeEach(() => {
view = render(
<Dropdown
Expand All @@ -39,14 +41,14 @@ describe('Dropdown', () => {

it('should render correctly closed', () => {
expect(view.asFragment()).toMatchSnapshot();
expect(screen.queryAllByRole('presentation')).toHaveLength(0);
expect(screen.queryAllByRole('option')).toHaveLength(0);
});

it('should render correctly opened', async () => {
await userEvent.click(screen.getByRole('button', { name: 'Click me!' }));
expect(view.asFragment()).toMatchSnapshot();
expect(screen.getAllByRole('listbox')).toHaveLength(1);
expect(screen.getAllByRole('presentation')).toHaveLength(2);
expect(screen.getAllByRole('option')).toHaveLength(2);
});
it('should downshift only by enabled items with value', async () => {
const { asFragment } = render(
Expand Down Expand Up @@ -76,16 +78,16 @@ describe('Dropdown', () => {

// 1 keydown
keyDown();
expect(screen.getAllByRole('presentation')[1].getAttribute('aria-selected')).toBeTruthy();
expect(screen.getAllByRole('presentation')[1]).toHaveTextContent('With value 1');
expect(getListItemAt(1).getAttribute('aria-selected')).toBeTruthy();
expect(getListItemAt(1)).toHaveTextContent('With value 1');
// 2 keydown
keyDown();
expect(screen.getAllByRole('presentation')[3].getAttribute('aria-selected')).toBeTruthy();
expect(screen.getAllByRole('presentation')[3]).toHaveTextContent('With value 2');
expect(getListItemAt(3).getAttribute('aria-selected')).toBeTruthy();
expect(getListItemAt(3)).toHaveTextContent('With value 2');
// 3 keydown. Should be again last not disabled with value item => 'With value 2'
keyDown();
expect(screen.getAllByRole('presentation')[3].getAttribute('aria-selected')).toBeTruthy();
expect(screen.getAllByRole('presentation')[3]).toHaveTextContent('With value 2');
expect(getListItemAt(3).getAttribute('aria-selected')).toBeTruthy();
expect(getListItemAt(3)).toHaveTextContent('With value 2');
});

it('onChange should return passed value', async () => {
Expand Down Expand Up @@ -130,7 +132,7 @@ describe('Dropdown', () => {
);
await userEvent.click(screen.getAllByRole('button', { name: 'Click me!' })[1]);
expect(asFragment()).toMatchSnapshot();
expect(screen.getAllByRole('presentation')).toHaveLength(3);
expect(screen.getAllByRole('option')).toHaveLength(3);
});

it('should call cb when button is clicked', async () => {
Expand Down Expand Up @@ -200,7 +202,7 @@ describe('Dropdown', () => {
</Dropdown>
);

expect(screen.getAllByRole('presentation')).toHaveLength(3);
expect(screen.getAllByRole('option')).toHaveLength(3);
});

it('should allow for conditional rendering of items', async () => {
Expand Down
24 changes: 14 additions & 10 deletions src/components/Dropdown/__tests__/__snapshots__/Dropdown.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ exports[`Dropdown should allow for conditional rendering of items 1`] = `
aria-selected="false"
class="ListItem ListItem--clickable List__item"
id="downshift-103-item-0"
role="presentation"
role="option"
>
<span>
With value
Expand Down Expand Up @@ -61,8 +61,9 @@ exports[`Dropdown should downshift only by enabled items with value 1`] = `
tabindex="-1"
>
<li
aria-selected="false"
class="ListItem ListItem--disabled List__item"
role="presentation"
role="option"
>
<span>
Disabled
Expand All @@ -72,15 +73,16 @@ exports[`Dropdown should downshift only by enabled items with value 1`] = `
aria-selected="false"
class="ListItem ListItem--clickable List__item"
id="downshift-14-item-0"
role="presentation"
role="option"
>
<span>
With value 1
</span>
</li>
<li
aria-selected="false"
class="ListItem List__item"
role="presentation"
role="option"
>
<span>
Without value
Expand All @@ -95,7 +97,7 @@ exports[`Dropdown should downshift only by enabled items with value 1`] = `
aria-selected="false"
class="ListItem ListItem--clickable List__item"
id="downshift-14-item-1"
role="presentation"
role="option"
>
<span>
With value 2
Expand Down Expand Up @@ -151,8 +153,9 @@ exports[`Dropdown should render correctly opened 1`] = `
tabindex="-1"
>
<li
aria-selected="false"
class="ListItem ListItem--disabled List__item"
role="presentation"
role="option"
>
<span>
Disabled
Expand All @@ -162,7 +165,7 @@ exports[`Dropdown should render correctly opened 1`] = `
aria-selected="false"
class="ListItem ListItem--clickable List__item"
id="downshift-3-item-0"
role="presentation"
role="option"
>
<span>
With value
Expand Down Expand Up @@ -196,8 +199,9 @@ exports[`Dropdown should render correctly with mixed children: array and single
tabindex="-1"
>
<li
aria-selected="false"
class="ListItem ListItem--disabled List__item"
role="presentation"
role="option"
>
<span>
Disabled
Expand All @@ -207,7 +211,7 @@ exports[`Dropdown should render correctly with mixed children: array and single
aria-selected="false"
class="ListItem ListItem--clickable List__item"
id="downshift-41-item-0"
role="presentation"
role="option"
>
<span>
one
Expand All @@ -217,7 +221,7 @@ exports[`Dropdown should render correctly with mixed children: array and single
aria-selected="false"
class="ListItem ListItem--clickable List__item"
id="downshift-41-item-1"
role="presentation"
role="option"
>
<span>
two
Expand Down
4 changes: 2 additions & 2 deletions src/components/Field/__tests__/Field.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import '@testing-library/jest-dom';
describe('<Field> that renders an input field', () => {
it('should render a component with a label properly', () => {
const labelText = 'labelText';
const wrapper = render(
const view = render(
<Field labelText={labelText} className="customClass">
<input />
</Field>
);
const button = screen.getByRole('textbox', { name: labelText });

expect(button).toBeInTheDocument();
expect(wrapper.asFragment()).toMatchSnapshot();
expect(view.asFragment()).toMatchSnapshot();
});
});
3 changes: 2 additions & 1 deletion src/components/List/List.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export const List = React.forwardRef<HTMLUListElement, Props>(
};

return isControlledNavigation ? (
<ul {...rest} ref={ref} {...block({ isDivided, ...rest })}>
<ul {...rest} ref={ref} {...block({ isDivided, ...rest })} role="listbox">
{React.Children.map(children, (child) => {
if (child) {
return child.props[NOT_LIST_CHILD]
Expand All @@ -151,6 +151,7 @@ export const List = React.forwardRef<HTMLUListElement, Props>(
tabIndex="0"
onKeyDown={handleKeyDown}
{...block({ isDivided, ...rest })}
role="listbox"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@eszthoff eszthoff Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. Role list is for static lists, while listbox is for selectable lists, which is what this component is about. Since we have onKeyDown and tabindex we should use listbox here, but not in the first render block.

>
{React.Children.map(children, (child, index) => {
if (child) {
Expand Down
10 changes: 0 additions & 10 deletions src/components/List/ListActions/__tests__/ListActions.spec.js
View file
Open in desktop

This file was deleted.

13 changes: 13 additions & 0 deletions src/components/List/ListActions/__tests__/ListActions.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import '@testing-library/jest-dom';
import { ListActions } from '../ListActions';

describe('<ListActions>', () => {
it('should render ListActions', () => {
const view = render(<ListActions>action</ListActions>);
expect(view).toMatchSnapshot();

expect(screen.getByText('action')).toBeInTheDocument();
});
});
9 changes: 0 additions & 9 deletions src/components/List/ListActions/__tests__/__snapshots__/ListActions.spec.js.snap
View file
Open in desktop

This file was deleted.

Loading