WB-1638: Fix tabbing in popover (#2159)

## Summary:
There is a bug in the popover where tabbing out of the popover (while it
is still open) will cause the focus order to be incorrect. More
specifically, the focus will be returned to the trigger element, while
it should be returned to the next element in the tab order.

This is because the popover is not properly handling the case where the
trigger element is the last element in the tab order. This PR fixes that
by making all the focusable elements in the popover non-tabbable, and
then make them tabbable again when the popover gains focus.

For more reference on the actual error, see: https://khanacademy.atlassian.net/browse/CL-1259

https://github.com/Khan/wonder-blocks/assets/843075/ca72e266-9091-409e-9065-3754e4c681c9

Issue: WB-1638

## Test plan:

- Navigate to /?path=/story/popover-popover--keyboard-navigation
- Start tabbing through the elements in the story
- Verify that the focus order is correct
- Open the popover
- Tab through the elements in the popover and verify that the focus
  order is still correct
- Tab backwards through the elements in the popover and verify that the
  focus order is still correct (using shift+tab).
- Close the popover and verify that the focus order is still correct


https://github.com/Khan/wonder-blocks/assets/843075/a8831575-84a8-4bb3-a0f3-11a26d872076

Author: jandrade

Reviewers: lillialexis, kevinbarabash, jandrade

Required Reviewers:

Approved By: lillialexis, kevinbarabash

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ codecov/project, ✅ Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️  dependabot

Pull Request URL: #2159

.changeset/cyan-oranges-wait.md

@@ -0,0 +1,5 @@
+---
+"@khanacademy/wonder-blocks-popover": patch
+---
+
+Fix tab navigation order

__docs__/wonder-blocks-popover/popover.stories.tsx

@@ -3,6 +3,7 @@ import {StyleSheet} from "aphrodite";
 import type {Meta, StoryObj} from "@storybook/react";
 
 import Button from "@khanacademy/wonder-blocks-button";
+import Color from "@khanacademy/wonder-blocks-color";
 import {View} from "@khanacademy/wonder-blocks-core";
 import {Strut} from "@khanacademy/wonder-blocks-layout";
 import Spacing from "@khanacademy/wonder-blocks-spacing";
@@ -80,6 +81,13 @@ const styles = StyleSheet.create({
         flex: 1,
         justifyContent: "space-between",
     },
+    playground: {
+        border: `1px dashed ${Color.lightBlue}`,
+        marginTop: Spacing.large_24,
+        padding: Spacing.large_24,
+        flexDirection: "row",
+        gap: Spacing.medium_16,
+    },
 });
 
 type StoryComponentType = StoryObj<typeof Popover>;
@@ -360,6 +368,120 @@ export const CustomPopoverContent: StoryComponentType = {
         id: "custom-popover",
     } as PopoverArgs,
 };
+
+/**
+ * This example shows how the focus is managed when a popover is opened. If the
+ * popover is closed, the focus flows naturally. However, if the popover is
+ * opened, the focus is managed internally by the `Popover` component.
+ *
+ * The focus is managed in the following way:
+ * - When the popover is opened, the focus is set on the first focusable element
+ *  inside the popover.
+ * - When the popover is closed, the focus is returned to the element that
+ * triggered the popover.
+ * - If the popover is opened and the focus reaches the last focusable element
+ * inside the popover, the next tab will set focus on the next focusable
+ * element that exists after the PopoverAnchor (or trigger element).
+ * - If the focus is set to the first focusable element inside the popover, the
+ * next shift + tab will set focus on the PopoverAnchor element.
+ *
+ * **NOTE:** You can add/remove buttons after the trigger element by using the
+ * buttons at the top of the example.
+ */
+export const KeyboardNavigation: StoryComponentType = {
+    render: function Render() {
+        const [numButtonsAfter, setNumButtonsAfter] = React.useState(0);
+        const [numButtonsInside, setNumButtonsInside] = React.useState(1);
+
+        return (
+            <View>
+                <View style={[styles.row, {gap: Spacing.medium_16}]}>
+                    <Button
+                        kind="secondary"
+                        onClick={() => {
+                            setNumButtonsAfter(numButtonsAfter + 1);
+                        }}
+                    >
+                        Add button after trigger element
+                    </Button>
+                    <Button
+                        kind="secondary"
+                        color="destructive"
+                        onClick={() => {
+                            if (numButtonsAfter > 0) {
+                                setNumButtonsAfter(numButtonsAfter - 1);
+                            }
+                        }}
+                    >
+                        Remove button after trigger element
+                    </Button>
+                    <Button
+                        kind="secondary"
+                        onClick={() => {
+                            setNumButtonsInside(numButtonsInside + 1);
+                        }}
+                    >
+                        Add button inside popover
+                    </Button>
+                    <Button
+                        kind="secondary"
+                        color="destructive"
+                        onClick={() => {
+                            if (numButtonsAfter > 0) {
+                                setNumButtonsInside(numButtonsInside - 1);
+                            }
+                        }}
+                    >
+                        Remove button inside popover
+                    </Button>
+                </View>
+                <View style={styles.playground}>
+                    <Button>First button</Button>
+                    <Popover
+                        content={({close}) => (
+                            <PopoverContent
+                                closeButtonVisible
+                                title="Keyboard navigation"
+                                content="This example shows how the focus is managed when a popover is opened."
+                                actions={
+                                    <View style={[styles.row, styles.actions]}>
+                                        {Array.from(
+                                            {length: numButtonsInside},
+                                            (_, index) => (
+                                                <Button
+                                                    onClick={() => {}}
+                                                    key={index}
+                                                    kind="tertiary"
+                                                >
+                                                    {`Button ${index + 1}`}
+                                                </Button>
+                                            ),
+                                        )}
+                                    </View>
+                                }
+                            />
+                        )}
+                        placement="top"
+                    >
+                        <Button>Open popover (trigger element)</Button>
+                    </Popover>
+                    {Array.from({length: numButtonsAfter}, (_, index) => (
+                        <Button onClick={() => {}} key={index}>
+                            {`Button ${index + 1}`}
+                        </Button>
+                    ))}
+                </View>
+            </View>
+        );
+    },
+    parameters: {
+        // This example is behavior based, not visual.
+        chromatic: {
+            disableSnapshot: true,
+        },
+    },
+};
+
 /**
  * Alignment example
  */

packages/wonder-blocks-popover/src/components/__tests__/focus-manager.test.tsx

@@ -1,31 +1,19 @@
 import * as React from "react";
-import * as ReactDOM from "react-dom";
 import {render, screen} from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 
 import FocusManager from "../focus-manager";
-import {findFocusableNodes} from "../../util/util";
 
 describe("FocusManager", () => {
     it("should focus on the first focusable element inside the popover", async () => {
         // Arrange
-        const ref = await new Promise((resolve: any) => {
-            const nodes = (
-                <div ref={resolve}>
-                    <button>Open popover</button>
-                    <button>Next focusable element outside</button>
-                </div>
-            );
-            render(nodes);
-        });
-        // @ts-expect-error [FEI-5019] - TS2345 - Argument of type 'unknown' is not assignable to parameter of type 'ReactInstance | null | undefined'.
-        const domNode = ReactDOM.findDOMNode(ref) as HTMLElement;
-
-        // mock focusable elements in document
-        // eslint-disable-next-line testing-library/no-node-access
-        global.document.querySelectorAll = jest
-            .fn()
-            .mockImplementation(() => findFocusableNodes(domNode));
+        const externalNodes = (
+            <div>
+                <button>Open popover</button>
+                <button>Next focusable element outside</button>
+            </div>
+        );
+        render(externalNodes);
 
         // get the anchor reference to be able pass it to the FocusManager
         const anchorElementNode = screen.getByRole("button", {
@@ -58,23 +46,13 @@ describe("FocusManager", () => {
 
     it("should focus on the last focusable element inside the popover", async () => {
         // Arrange
-        const ref = await new Promise((resolve: any) => {
-            const nodes = (
-                <div ref={resolve}>
-                    <button>Open popover</button>
-                    <button>Next focusable element outside</button>
-                </div>
-            );
-            render(nodes);
-        });
-        // @ts-expect-error [FEI-5019] - TS2345 - Argument of type 'unknown' is not assignable to parameter of type 'ReactInstance | null | undefined'.
-        const domNode = ReactDOM.findDOMNode(ref) as HTMLElement;
-
-        // mock focusable elements in document
-        // eslint-disable-next-line testing-library/no-node-access
-        global.document.querySelectorAll = jest
-            .fn()
-            .mockImplementation(() => findFocusableNodes(domNode));
+        const externalNodes = (
+            <div>
+                <button>Open popover</button>
+                <button>Next focusable element outside</button>
+            </div>
+        );
+        render(externalNodes);
 
         // get the anchor reference to be able pass it to the FocusManager
         const anchorElementNode = screen.getByRole("button", {
@@ -109,4 +87,105 @@ describe("FocusManager", () => {
         // Assert
         expect(lastFocusableElementInside).toHaveFocus();
     });
+
+    it("should allow flowing the focus correctly", async () => {
+        // Arrange
+        const externalNodes = (
+            <div>
+                <button>Prev focusable element outside</button>
+                <button>Open popover</button>
+                <button>Next focusable element outside</button>
+            </div>
+        );
+        render(externalNodes);
+
+        // get the anchor reference to be able pass it to the FocusManager
+        const anchorElementNode = screen.getByRole("button", {
+            name: "Open popover",
+        });
+
+        render(
+            <FocusManager anchorElement={anchorElementNode}>
+                <div>
+                    <button>first focusable element inside</button>
+                </div>
+            </FocusManager>,
+        );
+
+        // Act
+        // 1. focus on the previous element before the popover
+        userEvent.tab();
+
+        // 2. focus on the anchor element
+        userEvent.tab();
+
+        // 3. focus on focusable element inside the popover
+        userEvent.tab();
+
+        // 4. focus on the next focusable element outside the popover (this will
+        //    be the first focusable element outside the popover)
+        userEvent.tab();
+
+        // NOTE: At this point, the focus moves to the document body, so we need
+        // to press tab again to move the focus to the next focusable element.
+        userEvent.tab();
+
+        // 5. Finally focus on the first element in the document
+        userEvent.tab();
+
+        // find previous focusable element outside the popover
+        const prevFocusableElementOutside = screen.getByRole("button", {
+            name: "Prev focusable element outside",
+        });
+
+        // Assert
+        expect(prevFocusableElementOutside).toHaveFocus();
+    });
+
+    it("should disallow focusability on internal elements if the user focus out of the focus manager", async () => {
+        // Arrange
+        const externalNodes = (
+            <div>
+                <button>Prev focusable element outside</button>
+                <button>Open popover</button>
+                <button>Next focusable element outside</button>
+            </div>
+        );
+        render(externalNodes);
+
+        // get the anchor reference to be able pass it to the FocusManager
+        const anchorElementNode = screen.getByRole("button", {
+            name: "Open popover",
+        });
+
+        render(
+            <FocusManager anchorElement={anchorElementNode}>
+                <div>
+                    <button>first focusable element inside</button>
+                </div>
+            </FocusManager>,
+        );
+
+        // Act
+        // 1. focus on the previous element before the popover
+        userEvent.tab();
+
+        // 2. focus on the anchor element
+        userEvent.tab();
+
+        // 3. focus on focusable element inside the popover
+        userEvent.tab();
+
+        // 4. focus on the next focusable element outside the popover (this will
+        //    be the first focusable element outside the popover)
+        userEvent.tab();
+
+        // The elements inside the focus manager should not be focusable anymore.
+        const focusableElementInside = screen.getByRole("button", {
+            name: "first focusable element inside",
+        });
+
+        // Assert
+        expect(focusableElementInside).toHaveAttribute("tabIndex", "-1");
+    });
 });