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.

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(multi-action-button, split-button): ensure screen reader commands can navigate menu popup #7074

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@ export const MultiActionButton = ({
const handleClick = (
ev: React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>,
) => {
// ensure button is focused when clicked (Safari)
buttonRef.current?.focus();
showButtons();
handleInsideClick();
if (onClick) {
Expand All @@ -81,6 +79,7 @@ export const MultiActionButton = ({

const renderAdditionalButtons = () => (
<Popover
disablePortal
placement={
position === "left"
? "bottom-start"
Expand Down
63 changes: 37 additions & 26 deletions src/components/multi-action-button/multi-action-button.pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ test.describe("Prop tests", () => {

(
[
["left", 200],
["right", 153],
["left", 0],
["right", -46],
] as [MultiActionButtonProps["position"], number][]
).forEach(([position, value]) => {
test(`should render with menu position to the ${position}`, async ({
Expand All @@ -121,7 +121,7 @@ test.describe("Prop tests", () => {
await actionButton.click();
const listContainer = getDataElementByValue(page, "additional-buttons");
await expect(listContainer).toHaveCSS("position", "absolute");
await assertCssValueIsApproximately(listContainer, "top", 45);
await assertCssValueIsApproximately(listContainer, "top", 46);
await assertCssValueIsApproximately(listContainer, "left", value);
});
});
Expand Down Expand Up @@ -193,7 +193,7 @@ test.describe("Prop tests", () => {
});

test.describe("Functional tests", () => {
test(`should verify that clicking the main button opens the additional buttons`, async ({
test(`should verify that clicking the main button opens the additional buttons and focuses the first button`, async ({
mount,
page,
}) => {
Expand All @@ -203,9 +203,10 @@ test.describe("Functional tests", () => {
.first()
.locator("button")
.click();
await expect(getDataElementByValue(page, "additional-buttons")).toHaveCount(
1,
);
const additionalButtons = getDataElementByValue(page, "additional-buttons");

await expect(additionalButtons).toBeVisible();
await expect(additionalButtons.getByRole("button").first()).toBeFocused();
});

[...keysToTrigger].forEach((key) => {
Expand All @@ -220,11 +221,13 @@ test.describe("Functional tests", () => {
);
await actionButton.focus();
await page.keyboard.press(key);
await expect(
getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.first(),
).toBeFocused();
const additionalButtons = getDataElementByValue(
page,
"additional-buttons",
);

await expect(additionalButtons).toBeVisible();
await expect(additionalButtons.getByRole("button").first()).toBeFocused();
});
});

Expand Down Expand Up @@ -271,7 +274,7 @@ test.describe("Functional tests", () => {
await expect(listButton1).toBeFocused();
});

test(`should verify pressing shift + tab moves focus to previous child buttons and to the MultiActionButton if first child button is focused`, async ({
test(`should verify pressing Shift+Tab moves focus to previous child button, then the main button and closes the list`, async ({
mount,
page,
}) => {
Expand All @@ -295,6 +298,8 @@ test.describe("Functional tests", () => {
await expect(
getComponent(page, "multi-action-button").locator("button"),
).toBeFocused();
await expect(listButton1).not.toBeVisible();
await expect(listButton2).not.toBeVisible();
});

test(`should verify pressing ArrowDown key does not loop focus to top`, async ({
Expand All @@ -316,7 +321,7 @@ test.describe("Functional tests", () => {
await expect(listButton3).toBeFocused();
});

test(`should verify pressing tab moves focus to next child buttons and to second MultiActionButton if last child button is focused`, async ({
test(`should verify pressing Tab moves focus to next child button, then closes the list and focuses the next element on the page`, async ({
mount,
page,
}) => {
Expand All @@ -336,7 +341,6 @@ test.describe("Functional tests", () => {
.getByRole("button")
.nth(2);

await page.keyboard.press("Tab");
await expect(listButton1).toBeFocused();
await page.keyboard.press("Tab");
await expect(listButton2).toBeFocused();
Expand All @@ -346,6 +350,9 @@ test.describe("Functional tests", () => {
await expect(
getComponent(page, "multi-action-button").nth(1).locator("button"),
).toBeFocused();
await expect(listButton1).not.toBeVisible();
await expect(listButton2).not.toBeVisible();
await expect(listButton3).not.toBeVisible();
});

test(`should verify that pressing metaKey + ArrowUp moves focus to first child button`, async ({
Expand Down Expand Up @@ -434,7 +441,7 @@ test.describe("Functional tests", () => {
const listButton3 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.nth(2);
await page.keyboard.press("Tab");

await expect(listButton1).toBeFocused();
await page.keyboard.down("Meta");
await page.keyboard.press("ArrowDown");
Expand All @@ -458,7 +465,7 @@ test.describe("Functional tests", () => {
const listButton3 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.nth(2);
await page.keyboard.press("Tab");

await expect(listButton1).toBeFocused();
await page.keyboard.down("Control");
await page.keyboard.press("ArrowDown");
Expand All @@ -482,7 +489,7 @@ test.describe("Functional tests", () => {
const listButton3 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.nth(2);
await page.keyboard.press("Tab");

await expect(listButton1).toBeFocused();
await page.keyboard.press("End");
await expect(listButton3).toBeFocused();
Expand Down Expand Up @@ -553,15 +560,15 @@ test.describe("Functional tests with child buttons wrapped in a custom component
await expect(listButton1).toBeFocused();
});

test(`should verify pressing shift + tab moves focus to previous child buttons and to the MultiActionButton if first child button is focused`, async ({
test(`should verify pressing Shift+Tab moves focus to previous child button, then the main button and closes the list`, async ({
mount,
page,
}) => {
await mount(<WithWrapper />);

const actionButton = getComponent(page, "multi-action-button")
.locator("button")
.first();
.first()
.locator("button");
await actionButton.click();
const listButton1 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
Expand All @@ -577,6 +584,8 @@ test.describe("Functional tests with child buttons wrapped in a custom component
await expect(
getComponent(page, "multi-action-button").locator("button"),
).toBeFocused();
await expect(listButton1).not.toBeVisible();
await expect(listButton2).not.toBeVisible();
});

test(`should verify pressing ArrowDown key does not loop focus to top`, async ({
Expand All @@ -598,7 +607,7 @@ test.describe("Functional tests with child buttons wrapped in a custom component
await expect(listButton3).toBeFocused();
});

test(`should verify pressing tab moves focus to next child buttons and to second MultiActionButton if last child button is focused`, async ({
test(`should verify pressing Tab moves focus to next child button, then closes the list and focuses the next element on the page`, async ({
mount,
page,
}) => {
Expand All @@ -618,7 +627,6 @@ test.describe("Functional tests with child buttons wrapped in a custom component
.getByRole("button")
.nth(2);

await page.keyboard.press("Tab");
await expect(listButton1).toBeFocused();
await page.keyboard.press("Tab");
await expect(listButton2).toBeFocused();
Expand All @@ -628,6 +636,9 @@ test.describe("Functional tests with child buttons wrapped in a custom component
await expect(
getComponent(page, "multi-action-button").nth(1).locator("button"),
).toBeFocused();
await expect(listButton1).not.toBeVisible();
await expect(listButton2).not.toBeVisible();
await expect(listButton3).not.toBeVisible();
});

test(`should verify that pressing metaKey + ArrowUp moves focus to first child button`, async ({
Expand Down Expand Up @@ -716,7 +727,7 @@ test.describe("Functional tests with child buttons wrapped in a custom component
const listButton3 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.nth(2);
await page.keyboard.press("Tab");

await expect(listButton1).toBeFocused();
await page.keyboard.down("Meta");
await page.keyboard.press("ArrowDown");
Expand All @@ -740,7 +751,7 @@ test.describe("Functional tests with child buttons wrapped in a custom component
const listButton3 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.nth(2);
await page.keyboard.press("Tab");

await expect(listButton1).toBeFocused();
await page.keyboard.down("Control");
await page.keyboard.press("ArrowDown");
Expand All @@ -764,7 +775,7 @@ test.describe("Functional tests with child buttons wrapped in a custom component
const listButton3 = getDataElementByValue(page, "additional-buttons")
.getByRole("button")
.nth(2);
await page.keyboard.press("Tab");

await expect(listButton1).toBeFocused();
await page.keyboard.press("End");
await expect(listButton3).toBeFocused();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ const meta: Meta<typeof MultiActionButton> = {
argTypes: {
...styledSystemProps,
},
decorators: [
(Story) => (
<Box mb="150px">
<Story />
</Box>
),
],
};

export default meta;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,21 @@ test("should call 'onClick' when the main button is clicked", async () => {
expect(onClick).toHaveBeenCalledTimes(1);
});

test("should open additional buttons when the main button is clicked", async () => {
test("should open additional buttons when the main button is clicked and focus on the first child", async () => {
const user = userEvent.setup();
render(
<MultiActionButton text="Main Button">
<Button>First</Button>
<Button>Second</Button>
</MultiActionButton>,
);

await user.click(screen.getByRole("button", { name: "Main Button" }));

expect(await screen.findByRole("button", { name: "First" })).toBeVisible();
const button = screen.getByRole("button", { name: "First" });

expect(button).toBeVisible();
expect(button).toHaveFocus();
});

test("should close additional buttons when a child button is clicked", async () => {
Expand Down
3 changes: 1 addition & 2 deletions src/components/split-button/split-button.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,6 @@ export const SplitButton = ({
};

const handleToggleClick = () => {
// ensure button is focused when clicked (Safari)
toggleButton.current?.focus();
showButtons();
};

Expand Down Expand Up @@ -178,6 +176,7 @@ export const SplitButton = ({

return (
<Popover
disablePortal
placement={
position === "left"
? /* istanbul ignore next */ "bottom-start"
Expand Down
Loading
Loading