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

Collapse: fix a11y violation (interactive nested elements) #4553

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+17 −16
Open

Collapse: fix a11y violation (interactive nested elements) #4553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions packages/orbit-components/src/Collapse/Collapse.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,12 @@ export const Uncontrolled: Story = {
args: {
expanded: undefined,
},

parameters: {
controls: {
exclude: "expanded",
},
},
};

export const Rtl: Story = {
Expand Down
17 changes: 9 additions & 8 deletions packages/orbit-components/src/Collapse/__tests__/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ import userEvent from "@testing-library/user-event";
import { render, screen } from "../../test-utils";
import Collapse from "..";

const toggleButtons = [
[0, "label"],
[1, "icon button"],
];
const toggleTriggers = ["group", "button"];

describe("Collapse", () => {
const user = userEvent.setup();
Expand All @@ -31,26 +28,30 @@ describe("Collapse", () => {
expect(screen.getByText("customLabel")).toBeInTheDocument();
});

it.each(toggleButtons)("should trigger click handler when clicking on %s", async buttonIndex => {
it.each(toggleTriggers)("should trigger click handler when clicking on %s", async trigger => {
const onClick = jest.fn();
render(
<Collapse label="Collapse" onClick={onClick}>
<div>children</div>
</Collapse>,
);
const toggleButton = screen.getAllByRole("button", { name: "Collapse" })[buttonIndex];

const toggleButton = screen.getByRole(trigger);

await user.click(toggleButton);
expect(onClick).toHaveBeenCalled();
});

describe("uncontrolled", () => {
it.each(toggleButtons)("should expand/collapse when clicking on %s", async buttonIndex => {
it.each(toggleTriggers)("should expand/collapse when clicking on %s", async trigger => {
render(
<Collapse label="Collapse">
<article>children</article>
</Collapse>,
);
const toggleButton = screen.getAllByRole("button", { name: "Collapse" })[buttonIndex];

const toggleButton = screen.getByRole(trigger);

// with ByRole we can test whether the content is visible because of aria-hidden
expect(screen.queryByRole("article")).not.toBeInTheDocument();
await user.click(toggleButton);
Expand Down
10 changes: 2 additions & 8 deletions packages/orbit-components/src/Collapse/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,8 @@ const Collapse = ({
data-test={dataTest}
id={id}
>
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events */}
<div
className="block w-full cursor-pointer"
onClick={handleClick}
role="button"
tabIndex={-1}
id={labelID}
>
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions */}
<div className="block w-full cursor-pointer" id={labelID} onClick={handleClick} role="group">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain why we're using the group role here now?
In the end, the div is acting as a button, isn't it? It even has an onClick handler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, the div should be kept as a button and the inner ButtonLink should be rendered as a div

Copy link
Collaborator

Choose a reason for hiding this comment

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

the inner ButtonLink should be rendered as a div

I assume this would remove the role="button" quality, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, because the role="button" is only applied if the element is natively a button or if it has an onClick, which it doesn't, since the onClick is on the outer div

<Stack justify="between" align="center">
{label && !customLabel && <Heading type="title4">{label}</Heading>}
{customLabel}
Expand Down
Loading