fix(multi-action-button, split-button): ensure screen reader commands…

… can navigate menu popup Fixes issue where screen reader users using VoiceOver could not navigate into the menu buttons using commands VO+Space to open the menu then VO+right to navigate. The first option will now gain focus on open using any interaction method, including onClick, keyboard navigation and screen reader controls. fix #7054
Sage · Nov 14, 2024 · 5127910 · 5127910
1 parent c810d5c
commit 5127910
Show file tree

Hide file tree

Showing 12 changed files with 149 additions and 220 deletions.
diff --git a/src/components/multi-action-button/multi-action-button.component.tsx b/src/components/multi-action-button/multi-action-button.component.tsx
@@ -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) {
@@ -81,6 +79,7 @@ export const MultiActionButton = ({
 
   const renderAdditionalButtons = () => (
     <Popover
+      disablePortal
       placement={
         position === "left"
           ? "bottom-start"

diff --git a/src/components/multi-action-button/multi-action-button.pw.tsx b/src/components/multi-action-button/multi-action-button.pw.tsx
@@ -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 ({
@@ -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);
     });
   });
@@ -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,
   }) => {
@@ -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) => {
@@ -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();
     });
   });
 
@@ -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,
   }) => {
@@ -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 ({
@@ -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,
   }) => {
@@ -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();
@@ -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 ({
@@ -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");
@@ -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");
@@ -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();
@@ -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")
@@ -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 ({
@@ -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,
   }) => {
@@ -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();
@@ -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 ({
@@ -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");
@@ -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");
@@ -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();

diff --git a/src/components/multi-action-button/multi-action-button.stories.tsx b/src/components/multi-action-button/multi-action-button.stories.tsx
@@ -19,6 +19,13 @@ const meta: Meta<typeof MultiActionButton> = {
   argTypes: {
     ...styledSystemProps,
   },
+  decorators: [
+    (Story) => (
+      <Box mb="150px">
+        <Story />
+      </Box>
+    ),
+  ],
 };
 
 export default meta;

diff --git a/src/components/multi-action-button/multi-action-button.test.tsx b/src/components/multi-action-button/multi-action-button.test.tsx
@@ -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 () => {

diff --git a/src/components/split-button/split-button.component.tsx b/src/components/split-button/split-button.component.tsx
@@ -111,8 +111,6 @@ export const SplitButton = ({
   };
 
   const handleToggleClick = () => {
-    // ensure button is focused when clicked (Safari)
-    toggleButton.current?.focus();
     showButtons();
   };
 
@@ -178,6 +176,7 @@ export const SplitButton = ({
 
     return (
       <Popover
+        disablePortal
         placement={
           position === "left"
             ? /* istanbul ignore next */ "bottom-start"