fix(action-popover): ensure that opening using the up arrow focuses l…

…ast element in the menu Currently we have a bug whereby when a user presses the up arrow key, the first focusable item in the menu is focused. This appears to be a bug as we have code that specifically handles this behaviour but is overridden elsewhere in the code. This fix ensures that the last focusable item is focused. fixes #6826
Sage · Oct 18, 2024 · 38aaed9 · 38aaed9
1 parent e5d2ff5
commit 38aaed9
Show file tree

Hide file tree

Showing 9 changed files with 367 additions and 70 deletions.
diff --git a/src/components/action-popover/__internal__/action-popover-utils.tsx b/src/components/action-popover/__internal__/action-popover-utils.tsx
@@ -0,0 +1,30 @@
+import React from "react";
+import { ActionPopoverItem } from "../action-popover-item/action-popover-item.component";
+
+// Reusable type alias for item types
+type ReactItem = React.ReactChild | React.ReactFragment | React.ReactPortal;
+
+export const getItems = (
+  children: React.ReactNode | React.ReactNode[]
+): ReactItem[] =>
+  React.Children.toArray(children).filter(
+    (child) => React.isValidElement(child) && child.type === ActionPopoverItem
+  );
+
+export const isItemDisabled = (item: ReactItem) =>
+  React.isValidElement(item) && !!item.props?.disabled;
+
+export const findFirstFocusableItem = (items: ReactItem[]): number =>
+  items.findIndex((_, index) => !isItemDisabled(items[index]));
+
+// FIX-ME: FE-6248
+// Once we no longer support Node 16, this function can be removed and `findLastIndex()` can be used in its place.
+// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findLastIndex
+export const findLastFocusableItem = (items: ReactItem[]): number => {
+  for (let i = items.length - 1; i >= 0; i--) {
+    if (!isItemDisabled(items[i])) {
+      return i;
+    }
+  }
+  return -1;
+};
diff --git a/src/components/action-popover/action-popover-item/action-popover-item.component.tsx b/src/components/action-popover/action-popover-item/action-popover-item.component.tsx
@@ -216,10 +216,13 @@ export const ActionPopoverItem = ({
     }
   }, [alignSubmenu, submenu]);
 
-  // focuses item on opening of actionPopover submenu
+  // Focuses item on opening of actionPopover submenu, but we want to do this once the Popover has finished opening
+  // We always want the focused item to be in the user's view for accessibility purposes, and without the initial unexpected scroll to top of page when used in a table.
   useEffect(() => {
     if (focusItem) {
-      ref.current?.focus({ preventScroll: true });
+      setTimeout(() => {
+        ref.current?.focus();
+      }, 0);
     }
   }, [focusItem]);
 

diff --git a/src/components/action-popover/action-popover-menu/action-popover-menu.component.tsx b/src/components/action-popover/action-popover-menu/action-popover-menu.component.tsx
@@ -1,10 +1,4 @@
-import React, {
-  useCallback,
-  useMemo,
-  useContext,
-  useState,
-  useEffect,
-} from "react";
+import React, { useCallback, useMemo, useContext, useState } from "react";
 import invariant from "invariant";
 
 import { Menu } from "../action-popover.style";
@@ -16,6 +10,12 @@ import ActionPopoverDivider from "../action-popover-divider/action-popover-divid
 import ActionPopoverContext, {
   Alignment,
 } from "../__internal__/action-popover.context";
+import {
+  findFirstFocusableItem,
+  findLastFocusableItem,
+  getItems,
+  isItemDisabled,
+} from "../__internal__/action-popover-utils";
 
 export interface ActionPopoverMenuBaseProps {
   /** Children for the menu */
@@ -120,46 +120,16 @@ const ActionPopoverMenu = React.forwardRef<
         ` and \`${ActionPopoverDivider.displayName}\`.`
     );
 
-    const items = useMemo(() => {
-      return React.Children.toArray(children).filter((child) => {
-        return React.isValidElement(child) && child.type === ActionPopoverItem;
-      });
-    }, [children]);
+    const items = useMemo(() => getItems(children), [children]);
 
-    const isItemDisabled = useCallback(
-      (value: number) => {
-        const item = items[value];
-        // The invariant will be triggered before this else path can be explored, hence the ignore else.
-        // istanbul ignore else
-        return React.isValidElement(item) && item.props.disabled;
-      },
+    const checkItemDisabled = useCallback(
+      (value) => isItemDisabled(items[value]),
       [items]
     );
 
-    const firstFocusableItem = items.findIndex(
-      (_, index) => !isItemDisabled(index)
-    );
-
-    // FIX-ME: FE-6248
-    // Once we no longer support Node 16, this function can be removed and `findLastIndex()` can be used in it's place.
-    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findLastIndex
-    function findLastFocusableItem() {
-      let lastFocusableItem = -1;
-      for (let i = items.length - 1; i >= 0; i--) {
-        if (!isItemDisabled(i)) {
-          lastFocusableItem = i;
-          break;
-        }
-      }
-      return lastFocusableItem;
-    }
-
-    const lastFocusableItem = findLastFocusableItem();
+    const firstFocusableItem = findFirstFocusableItem(items);
 
-    useEffect(() => {
-      if (isOpen && firstFocusableItem !== -1)
-        setFocusIndex(firstFocusableItem);
-    }, [isOpen, firstFocusableItem, setFocusIndex]);
+    const lastFocusableItem = findLastFocusableItem(items);
 
     const onKeyDown = useCallback(
       (e) => {
@@ -173,7 +143,7 @@ const ActionPopoverMenu = React.forwardRef<
           e.preventDefault();
           e.stopPropagation();
           let indexValue = focusIndex + 1;
-          while (indexValue < items.length && isItemDisabled(indexValue)) {
+          while (indexValue < items.length && checkItemDisabled(indexValue)) {
             indexValue += 1;
           }
           if (indexValue >= items.length) {
@@ -187,7 +157,7 @@ const ActionPopoverMenu = React.forwardRef<
           let indexValue = focusIndex - 1;
           while (
             indexValue >= firstFocusableItem &&
-            isItemDisabled(indexValue)
+            checkItemDisabled(indexValue)
           ) {
             indexValue -= 1;
           }
@@ -216,7 +186,7 @@ const ActionPopoverMenu = React.forwardRef<
           items.forEach((item, index) => {
             if (
               React.isValidElement(item) &&
-              !isItemDisabled(index) &&
+              !checkItemDisabled(index) &&
               item.props.children.toLowerCase().startsWith(e.key.toLowerCase())
             ) {
               // istanbul ignore else
@@ -241,7 +211,7 @@ const ActionPopoverMenu = React.forwardRef<
         setOpen,
         focusIndex,
         items,
-        isItemDisabled,
+        checkItemDisabled,
         setFocusIndex,
         firstFocusableItem,
         lastFocusableItem,

diff --git a/src/components/action-popover/action-popover-test.stories.tsx b/src/components/action-popover/action-popover-test.stories.tsx
@@ -17,10 +17,11 @@ import {
   FlatTableHeader,
   FlatTableCell,
 } from "../flat-table";
+import Box from "../box";
 
 export default {
   title: "Action Popover/Test",
-  includeStories: ["Default"],
+  includeStories: ["Default", "LongMenuExample"],
   parameters: {
     info: { disable: true },
     chromatic: {
@@ -706,3 +707,96 @@ export const ActionPopoverPropsComponentAllDisabled = (
     </ActionPopover>
   );
 };
+
+export const LongMenuExample = () => {
+  const submenu = (
+    <ActionPopoverMenu>
+      <ActionPopoverItem onClick={() => {}}>Sub Menu 1</ActionPopoverItem>
+      <ActionPopoverItem onClick={() => {}}>Sub Menu 2</ActionPopoverItem>
+      <ActionPopoverItem disabled onClick={() => {}}>
+        Sub Menu 3
+      </ActionPopoverItem>
+    </ActionPopoverMenu>
+  );
+  const submenuWithIcons = (
+    <ActionPopoverMenu>
+      <ActionPopoverItem icon="graph" onClick={() => {}}>
+        Sub Menu 1
+      </ActionPopoverItem>
+      <ActionPopoverItem icon="add" onClick={() => {}}>
+        Sub Menu 2
+      </ActionPopoverItem>
+      <ActionPopoverItem icon="print" disabled onClick={() => {}}>
+        Sub Menu 3
+      </ActionPopoverItem>
+    </ActionPopoverMenu>
+  );
+  return (
+    <Box mt={60} height={275}>
+      <ActionPopover onOpen={() => {}} onClose={() => {}}>
+        <ActionPopoverItem
+          disabled
+          icon="graph"
+          submenu={submenu}
+          onClick={() => {}}
+        >
+          Business
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="email" onClick={() => {}}>
+          Email Invoiceee
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="print" onClick={() => {}} submenu={submenu}>
+          Print Invoice
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="pdf" submenu={submenu} onClick={() => {}}>
+          Download PDF
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="csv" onClick={() => {}}>
+          Download CSV
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="email" onClick={() => {}}>
+          Email Invoiceee
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="print" onClick={() => {}} submenu={submenu}>
+          Print Invoice
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="pdf" submenu={submenu} onClick={() => {}}>
+          Download PDF
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="csv" onClick={() => {}}>
+          Download CSV
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="email" onClick={() => {}}>
+          Email Invoiceee
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="print" onClick={() => {}} submenu={submenu}>
+          Print Invoice
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="pdf" submenu={submenu} onClick={() => {}}>
+          Download PDF
+        </ActionPopoverItem>
+        <ActionPopoverItem icon="csv" onClick={() => {}}>
+          Download CSV
+        </ActionPopoverItem>
+        <ActionPopoverDivider />
+        <ActionPopoverItem disabled icon="delete" onClick={() => {}}>
+          Delete
+        </ActionPopoverItem>
+      </ActionPopover>
+      <ActionPopover>
+        <ActionPopoverItem icon="csv" onClick={() => {}}>
+          Download CSV
+        </ActionPopoverItem>
+      </ActionPopover>
+      <ActionPopover>
+        <ActionPopoverItem
+          icon="csv"
+          submenu={submenuWithIcons}
+          onClick={() => {}}
+        >
+          Download CSV
+        </ActionPopoverItem>
+      </ActionPopover>
+    </Box>
+  );
+};
diff --git a/src/components/action-popover/action-popover.component.tsx b/src/components/action-popover/action-popover.component.tsx
@@ -24,6 +24,11 @@ import ActionPopoverContext, {
   Alignment,
 } from "./__internal__/action-popover.context";
 import useModalManager from "../../hooks/__internal__/useModalManager";
+import {
+  findFirstFocusableItem,
+  findLastFocusableItem,
+  getItems,
+} from "./__internal__/action-popover-utils";
 
 export interface RenderButtonProps {
   tabIndex: number;
@@ -82,12 +87,6 @@ export const ActionPopover = ({
   const buttonRef = useRef<HTMLDivElement>(null);
   const menu = useRef<HTMLUListElement>(null);
 
-  const itemCount = useMemo(() => {
-    return React.Children.toArray(children).filter((child) => {
-      return React.isValidElement(child) && child.type === ActionPopoverItem;
-    }).length;
-  }, [children]);
-
   const hasProperChildren = useMemo(() => {
     const incorrectChild = React.Children.toArray(children).find(
       (child: React.ReactNode) => {
@@ -105,6 +104,12 @@ export const ActionPopover = ({
     return !incorrectChild;
   }, [children]);
 
+  const items = useMemo(() => getItems(children), [children]);
+
+  const firstFocusableItem = findFirstFocusableItem(items);
+
+  const lastFocusableItem = findLastFocusableItem(items);
+
   invariant(
     hasProperChildren,
     `ActionPopover only accepts children of type \`${ActionPopoverItem.displayName}\`` +
@@ -152,13 +157,14 @@ export const ActionPopover = ({
     (e) => {
       e.stopPropagation();
       const isOpening = !isOpen;
+      setFocusIndex(firstFocusableItem);
       setOpen(isOpening);
       if (!isOpening) {
         // Closing the menu should focus the MenuButton
         focusButton();
       }
     },
-    [isOpen, setOpen, focusButton]
+    [isOpen, firstFocusableItem, setOpen, focusButton]
   );
 
   // Keyboard commands implemented as recommended by WAI-ARIA best practices
@@ -169,16 +175,16 @@ export const ActionPopover = ({
       if (Events.isSpaceKey(e) || Events.isDownKey(e) || Events.isEnterKey(e)) {
         e.preventDefault();
         e.stopPropagation();
-        setFocusIndex(0);
+        setFocusIndex(firstFocusableItem);
         setOpen(true);
       } else if (Events.isUpKey(e)) {
         e.preventDefault();
         e.stopPropagation();
-        setFocusIndex(itemCount - 1);
+        setFocusIndex(lastFocusableItem);
         setOpen(true);
       }
     },
-    [itemCount, setOpen]
+    [firstFocusableItem, lastFocusableItem, setOpen]
   );
 
   const handleEscapeKey = useCallback(
@@ -200,7 +206,7 @@ export const ActionPopover = ({
 
   useEffect(() => {
     const handler = ({ target }: MouseEvent) => {
-      // If the event didn't came from part of this component, close the menu.
+      // If the event didn't come from part of this component, close the menu.
       // There will be multiple document click listeners but we cant prevent propagation because it will interfere with
       // other instances on the same page
 

diff --git a/src/components/action-popover/action-popover.pw.tsx b/src/components/action-popover/action-popover.pw.tsx
@@ -65,6 +65,7 @@ import {
   Submenu,
   SubmenuPositionedRight,
   ActionPopoverNestedInDialog,
+  LongMenuExample,
 } from "../../../src/components/action-popover/components.test-pw";
 
 const keyToTrigger = ["Enter", " ", "End", "ArrowDown", "ArrowUp"] as const;
@@ -103,6 +104,21 @@ test.describe("check functionality for ActionPopover component", () => {
     });
   });
 
+  test("should open and focus last element when up keyboard key is used to open menu", async ({
+    mount,
+    page,
+  }) => {
+    await mount(<LongMenuExample />);
+    await page.setViewportSize({ width: 600, height: 600 });
+
+    const actionPopoverButtonElement = actionPopoverButton(page).first();
+    await actionPopoverButtonElement.press("ArrowUp");
+    const focusedElement = await page.locator("*:focus");
+
+    await expect(focusedElement).toContainText("Download CSV");
+    await expect(focusedElement).toBeVisible();
+  });
+
   [keyToTrigger[0], keyToTrigger[1], keyToTrigger[3]].forEach((key) => {
     // TODO: Skipped due to flaky focus behaviour. To review in FE-6428
     test.skip(`should open using ${key} keyboard key`, async ({

diff --git a/src/components/action-popover/action-popover.stories.tsx b/src/components/action-popover/action-popover.stories.tsx
@@ -86,7 +86,7 @@ export const Default: Story = () => {
           Download CSV
         </ActionPopoverItem>
         <ActionPopoverDivider />
-        <ActionPopoverItem icon="delete" onClick={() => {}}>
+        <ActionPopoverItem disabled icon="delete" onClick={() => {}}>
           Delete
         </ActionPopoverItem>
       </ActionPopover>