From 22a5e249cf6e3a79eaad244ed0e892e56585a1ee Mon Sep 17 00:00:00 2001
From: Anusha Chaturvedi <anusha.chaturvedi@canonical.com>
Date: Thu, 16 Jul 2026 18:02:33 +0530
Subject: [PATCH] fix(modal): skip hidden elements when setting focus

---
 src/components/Modal/Modal.test.tsx | 56 ++++++++++++++++++++++
 src/components/Modal/Modal.tsx      | 74 ++++++++++++++++++++++-------
 2 files changed, 112 insertions(+), 18 deletions(-)

diff --git a/src/components/Modal/Modal.test.tsx b/src/components/Modal/Modal.test.tsx
index 7a423235e..9a15fe373 100644
--- a/src/components/Modal/Modal.test.tsx
+++ b/src/components/Modal/Modal.test.tsx
@@ -262,4 +262,60 @@ describe("Modal ", () => {
     const proceedButton = screen.getByRole("button", { name: /^proceed$/i });
     expect(proceedButton).toHaveFocus();
   });
+
+  it("excludes hidden focusable elements from the focus trap", async () => {
+    const user = userEvent.setup();
+    const { container } = render(
+      <Modal
+        title="Test"
+        close={jest.fn()}
+        buttonRow={
+          <>
+            <button id="test-cancel">Cancel</button>
+            <button id="test-hidden" style={{ display: "none" }}>
+              Hidden
+            </button>
+          </>
+        }
+      >
+        Bare bones
+      </Modal>,
+    );
+
+    const closeButton = container.querySelector("button.p-modal__close");
+    const cancelButton = container.querySelector("button#test-cancel");
+
+    expect(closeButton).toHaveFocus();
+    await user.tab({ shift: true });
+    expect(cancelButton).toHaveFocus();
+
+    await user.tab();
+    expect(closeButton).toHaveFocus();
+  });
+
+  it("initially focuses the first visible focusable element when the close button is hidden", () => {
+    const style = document.createElement("style");
+    style.innerHTML = ".p-modal__close { display: none; }";
+    document.head.appendChild(style);
+
+    try {
+      const { container } = render(
+        <Modal
+          title="Test"
+          close={jest.fn()}
+          buttonRow={<button id="test-cancel">Cancel</button>}
+        >
+          Bare bones
+        </Modal>,
+      );
+
+      const closeButton = container.querySelector("button.p-modal__close");
+      const cancelButton = container.querySelector("button#test-cancel");
+
+      expect(closeButton).not.toHaveFocus();
+      expect(cancelButton).toHaveFocus();
+    } finally {
+      document.head.removeChild(style);
+    }
+  });
 });
diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx
index c346372d1..aa482dbb2 100644
--- a/src/components/Modal/Modal.tsx
+++ b/src/components/Modal/Modal.tsx
@@ -67,24 +67,46 @@ export const Modal = ({
 
   const modalRef: RefObject<HTMLDivElement> = useRef(null);
   const closeButtonRef: RefObject<HTMLButtonElement> = useRef(null);
-  const handleTabKey = (event: React.KeyboardEvent<HTMLDivElement>) => {
-    const focusableModalElements = modalRef.current.querySelectorAll(
-      focusableElementSelectors,
-    );
-    if (focusableModalElements.length > 0) {
-      const firstElement = focusableModalElements[0];
-      const lastElement =
-        focusableModalElements[focusableModalElements.length - 1];
-
-      if (!event.shiftKey && document.activeElement === lastElement) {
-        (firstElement as HTMLElement).focus();
-        event.preventDefault();
-      }
 
-      if (event.shiftKey && document.activeElement === firstElement) {
-        (lastElement as HTMLElement).focus();
-        return event.preventDefault();
+  // determines whether an element is visible by checking computed styles
+  // fails open: an element is only treated as hidden when getComputedStyle
+  // explicitly reports `display:none` or `visibility:hidden`
+  const isElementVisible = (element: Element): boolean => {
+    let current: Element | null = element;
+    while (current) {
+      const style = window.getComputedStyle(current);
+      if (style.display === "none" || style.visibility === "hidden") {
+        return false;
       }
+      current = current.parentElement;
+    }
+    return true;
+  };
+
+  const getVisibleFocusableElements = (): HTMLElement[] => {
+    if (!modalRef.current) {
+      return [];
+    }
+    return Array.from(
+      modalRef.current.querySelectorAll<HTMLElement>(focusableElementSelectors),
+    ).filter(isElementVisible);
+  };
+
+  const handleTabKey = (event: React.KeyboardEvent<HTMLDivElement>) => {
+    const focusableModalElements = getVisibleFocusableElements();
+    if (focusableModalElements.length === 0) {
+      return;
+    }
+    const firstElement = focusableModalElements[0];
+    const lastElement =
+      focusableModalElements[focusableModalElements.length - 1];
+
+    if (event.shiftKey && document.activeElement === firstElement) {
+      lastElement.focus();
+      event.preventDefault();
+    } else if (!event.shiftKey && document.activeElement === lastElement) {
+      firstElement.focus();
+      event.preventDefault();
     }
   };
 
@@ -103,13 +125,29 @@ export const Modal = ({
     }
   };
 
+  const focusModalWrapper = () => {
+    if (modalRef.current) {
+      modalRef.current.tabIndex = -1;
+      modalRef.current.focus();
+    }
+  };
+
   useEffect(() => {
     if (focusRef?.current) {
       focusRef.current.focus();
     } else if (closeButtonRef.current) {
-      closeButtonRef.current.focus();
+      if (isElementVisible(closeButtonRef.current)) {
+        closeButtonRef.current.focus();
+      } else {
+        const firstFocusable = getVisibleFocusableElements()[0];
+        if (firstFocusable) {
+          firstFocusable.focus();
+        } else {
+          focusModalWrapper();
+        }
+      }
     } else {
-      modalRef.current.focus();
+      focusModalWrapper();
     }
   }, [focusRef]);