Fleet UI: Keyboard focus enters, traps in, and restores from modals

changes/23266-modal-focus-scope

@@ -0,0 +1,3 @@
+- Improved keyboard accessibility for modals: keyboard focus enters the modal on open and returns to the previously focused control when the modal closes.
+- Fixed a bug where holding Enter on the "Manage automations" button could auto-submit the form before the modal opened.
+- Fixed a bug where pressing Escape closed both modals when one was stacked on top of another.

frontend/components/Modal/Modal.tests.tsx

@@ -1,6 +1,12 @@
-import React from "react";
+import React, { useState } from "react";
 import { noop } from "lodash";
-import { render, screen, fireEvent, act } from "@testing-library/react";
+import {
+  render,
+  screen,
+  fireEvent,
+  act,
+  waitFor,
+} from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 
 import Modal from "./Modal";
@@ -189,4 +195,142 @@ describe("Modal", () => {
     act(() => jest.runAllTimers());
     expect(onExit).not.toHaveBeenCalled();
   });
+
+  describe("focus management", () => {
+    beforeEach(() => jest.useRealTimers());
+
+    it("focuses the modal container on open, not any interactive child", async () => {
+      const { container } = render(
+        <Modal title="Confirm" onExit={noop}>
+          <>
+            <button type="button">Submit</button>
+            <button type="button">Cancel</button>
+          </>
+        </Modal>
+      );
+
+      const modalContainer = container.querySelector(".modal__modal_container");
+      await waitFor(() => {
+        expect(modalContainer).toHaveFocus();
+      });
+      expect(screen.getByRole("button", { name: "Submit" })).not.toHaveFocus();
+    });
+
+    it("does not steal focus from a child that autofocused itself", async () => {
+      const Autofocused = () => {
+        const ref = React.useRef<HTMLInputElement>(null);
+        React.useEffect(() => {
+          ref.current?.focus();
+        }, []);
+        return <input ref={ref} data-testid="autofocused" />;
+      };
+
+      render(
+        <Modal title="Confirm" onExit={noop}>
+          <Autofocused />
+        </Modal>
+      );
+
+      const autofocused = screen.getByTestId("autofocused");
+      await waitFor(() => expect(autofocused).toHaveFocus());
+    });
+
+    it("does not steal focus on mount when isHidden (stacked modal on top)", () => {
+      render(
+        <>
+          <button type="button" data-testid="stacked">
+            Stacked modal control
+          </button>
+          <Modal title="Bottom" isHidden onExit={noop}>
+            <button type="button">Submit</button>
+          </Modal>
+        </>
+      );
+
+      const stacked = screen.getByTestId("stacked");
+      stacked.focus();
+      expect(stacked).toHaveFocus();
+    });
+
+    it("does not close on Escape while isHidden", () => {
+      const onExit = jest.fn();
+      render(
+        <Modal title="Bottom" isHidden onExit={onExit}>
+          <div>content</div>
+        </Modal>
+      );
+
+      fireEvent.keyDown(document, { key: "Escape" });
+      expect(onExit).not.toHaveBeenCalled();
+    });
+
+    it("does not fire onEnter for autorepeated Enter keys", () => {
+      const onEnter = jest.fn();
+      render(
+        <Modal title="Confirm" onEnter={onEnter} onExit={noop}>
+          <div>content</div>
+        </Modal>
+      );
+
+      // The trigger button's Enter is still down when the modal mounts and
+      // this listener attaches; the browser's autorepeat fires more keydowns
+      // with event.repeat === true. Those must not fire onEnter.
+      fireEvent.keyDown(document, { code: "Enter", repeat: true });
+      expect(onEnter).not.toHaveBeenCalled();
+
+      // A fresh Enter (repeat: false) is a real user intent — fires.
+      fireEvent.keyDown(document, { code: "Enter", repeat: false });
+      expect(onEnter).toHaveBeenCalledTimes(1);
+    });
+
+    it("restores focus to the previously focused element on close", async () => {
+      const Wrapper = () => {
+        const [open, setOpen] = useState(false);
+        return (
+          <>
+            <button
+              type="button"
+              onClick={() => setOpen(true)}
+              data-testid="trigger"
+            >
+              Open
+            </button>
+            {open && (
+              <Modal title="Confirm" onExit={() => setOpen(false)}>
+                <button type="button" onClick={() => setOpen(false)}>
+                  Close from inside
+                </button>
+              </Modal>
+            )}
+          </>
+        );
+      };
+
+      const user = userEvent.setup();
+      render(<Wrapper />);
+      const trigger = screen.getByTestId("trigger");
+      trigger.focus();
+      expect(trigger).toHaveFocus();
+
+      await user.click(trigger);
+      // Container is now focused (not the interior button).
+      await waitFor(() => {
+        expect(
+          screen.getByRole("button", { name: "Close from inside" })
+        ).not.toHaveFocus();
+      });
+
+      await user.click(
+        screen.getByRole("button", { name: "Close from inside" })
+      );
+      await waitFor(() => {
+        expect(
+          screen.queryByRole("button", { name: "Close from inside" })
+        ).not.toBeInTheDocument();
+      });
+      await waitFor(() => {
+        expect(trigger).toHaveFocus();
+      });
+    });
+  });
 });

frontend/components/Modal/Modal.tsx

@@ -1,4 +1,10 @@
-import React, { useCallback, useEffect, useRef, useState } from "react";
+import React, {
+  useCallback,
+  useEffect,
+  useLayoutEffect,
+  useRef,
+  useState,
+} from "react";
 import classnames from "classnames";
 import Button from "components/buttons/Button/Button";
 import Icon from "components/Icon/Icon";
@@ -56,11 +62,45 @@ const Modal = ({
   disableClosingModal = false,
   className,
 }: IModalProps): JSX.Element => {
+  const containerRef = useRef<HTMLDivElement>(null);
   const isDownOnBackgroundRef = useRef(false);
   const isFormDirtyRef = useRef(false);
   const [isClosing, setIsClosing] = useState(false);
   const isClosingRef = useRef(false);
 
+  // Latest-value ref for document-level handlers to consult isHidden without
+  // re-running their mount effects. isHidden signals a sibling modal is
+  // stacked on top of this one; that modal owns focus and keyboard input.
+  const isHiddenRef = useRef(isHidden);
+  useLayoutEffect(() => {
+    isHiddenRef.current = isHidden;
+  }, [isHidden]);
+
+  useEffect(() => {
+    const container = containerRef.current;
+    if (!container) return undefined;
+
+    const previouslyFocused = document.activeElement as HTMLElement | null;
+    // Skip if a stacked modal is on top or if a child already claimed focus
+    // (e.g. an autoFocused InputField). Restore on unmount only when we took
+    // focus and are not hidden at close time, so a stacked-then-unmount flow
+    // doesn't yank focus away from the top modal.
+    const tookFocus =
+      !isHiddenRef.current && !container.contains(document.activeElement);
+    if (tookFocus) container.focus();
+
+    return () => {
+      if (
+        tookFocus &&
+        !isHiddenRef.current &&
+        previouslyFocused &&
+        document.body.contains(previouslyFocused)
+      ) {
+        previouslyFocused.focus();
+      }
+    };
+  }, []);
+
   const handleClose = useCallback(() => {
     if (isClosingRef.current) return;
     isClosingRef.current = true;
@@ -72,6 +112,7 @@ const Modal = ({
 
   useEffect(() => {
     const closeWithEscapeKey = (e: KeyboardEvent) => {
+      if (isHiddenRef.current) return;
       if (e.key === "Escape") {
         handleClose();
       }
@@ -91,6 +132,11 @@ const Modal = ({
   useEffect(() => {
     if (onEnter) {
       const closeOrSaveWithEnterKey = (event: KeyboardEvent) => {
+        // Ignore Enter while a stacked modal is on top of this one.
+        if (isHiddenRef.current) return;
+        // Skip autorepeated keys: the trigger button's Enter may still be
+        // held when the modal mounts and this listener attaches.
+        if (event.repeat) return;
         if (event.code === "Enter" || event.code === "NumpadEnter") {
           event.preventDefault();
           onEnter();
@@ -194,8 +240,9 @@ const Modal = ({
       onMouseUp={handleBackgroundMouseUp}
     >
       <div
+        ref={containerRef}
         className={modalContainerClasses}
-        tabIndex={-1} // Make focusable
+        tabIndex={-1}
         onMouseDown={handleContainerMouseDown}
         onMouseUp={handleContainerMouseUp}
         onInput={handleContainerInput}
@@ -205,12 +252,7 @@ const Modal = ({
           <span>{title}</span>
           {!disableClosingModal && (
             <div className={`${baseClass}__ex`}>
-              <Button
-                variant="icon"
-                onClick={handleClose}
-                iconStroke
-                autofocus={isContentDisabled}
-              >
+              <Button variant="icon" onClick={handleClose} iconStroke>
                 <Icon name="close" color="core-fleet-black" size="medium" />
               </Button>
             </div>

frontend/components/Modal/_styles.scss

@@ -71,6 +71,14 @@
     border-radius: 8px;
     animation: scale-up 150ms ease-out;
 
+    // Container receives focus only as a landing target for keyboard users
+    // (tabIndex=-1). It's not an interactive control, so suppress the
+    // browser's focus ring on the whole modal.
+    &:focus,
+    &:focus-visible {
+      outline: none;
+    }
+
     &.modal__closing {
       animation: scale-down var(--modal-close-duration) ease-in forwards;
     }

frontend/pages/queries/ManageQueriesPage/components/ManageQueryAutomationsModal/ManageQueryAutomationsModal.tsx

@@ -124,38 +124,16 @@ const ManageQueryAutomationsModal = ({
     );
   };
 
-  const onSubmitQueryAutomations = (
-    evt: React.MouseEvent<HTMLFormElement> | KeyboardEvent
-  ) => {
+  const onSubmitQueryAutomations = (evt: React.FormEvent<HTMLFormElement>) => {
     evt.preventDefault();
-
-    const newQueryIds: number[] = [];
-    queryItems?.forEach((p) => p.isChecked && newQueryIds.push(p.id));
-
     onSubmit({
-      newAutomatedQueryIds: newQueryIds,
+      newAutomatedQueryIds: queryItems
+        .filter((p) => p.isChecked)
+        .map((p) => p.id),
       previousAutomatedQueryIds: automatedQueryIds,
     });
   };
 
-  useEffect(() => {
-    const listener = (event: KeyboardEvent) => {
-      if (event.code === "Enter" || event.code === "NumpadEnter") {
-        event.preventDefault();
-        onSubmit({
-          newAutomatedQueryIds: queryItems
-            .filter((p) => p.isChecked)
-            .map((p) => p.id),
-          previousAutomatedQueryIds: automatedQueryIds,
-        });
-      }
-    };
-    document.addEventListener("keydown", listener);
-    return () => {
-      document.removeEventListener("keydown", listener);
-    };
-  });
-
   return (
     <Modal
       title="Manage automations"
@@ -164,7 +142,7 @@ const ManageQueryAutomationsModal = ({
       width="large"
       isHidden={isShowingPreviewDataModal}
     >
-      <div className={`${baseClass} form`}>
+      <form className={`${baseClass} form`} onSubmit={onSubmitQueryAutomations}>
         <div className={`${baseClass}__heading`}>
           Report automations let you send data gathered from macOS, Windows, and
           Linux hosts to a log destination. Data is sent according to a
@@ -238,7 +216,6 @@ const ManageQueryAutomationsModal = ({
             renderChildren={(disableChildren) => (
               <Button
                 type="submit"
-                onClick={onSubmitQueryAutomations}
                 className="save-loading"
                 isLoading={isUpdatingAutomations}
                 disabled={disableChildren}
@@ -251,7 +228,7 @@ const ManageQueryAutomationsModal = ({
             Cancel
           </Button>
         </div>
-      </div>
+      </form>
     </Modal>
   );
 };