fix(PopoverContainer): Add all scrolling parents to closeOnViewportExit

packages/gamut/src/PopoverContainer/PopoverContainer.tsx

@@ -3,15 +3,15 @@ import { variance } from '@codecademy/variance';
 import styled from '@emotion/styled';
 import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
 import * as React from 'react';
-import {
-  useIsomorphicLayoutEffect,
-  useWindowScroll,
-  useWindowSize,
-} from 'react-use';
+import { useWindowScroll, useWindowSize } from 'react-use';
 
 import { BodyPortal } from '../BodyPortal';
 import { FocusTrap } from '../FocusTrap';
-import { useResizingParentEffect, useScrollingParentsEffect } from './hooks';
+import {
+  useResizingParentEffect,
+  useScrollingParents,
+  useScrollingParentsEffect,
+} from './hooks';
 import { ContainerState, PopoverContainerProps } from './types';
 import { getContainers, getPosition, isOutOfView } from './utils';
 
@@ -42,12 +42,23 @@ export const PopoverContainer: React.FC<PopoverContainerProps> = ({
 }) => {
   const popoverRef = useRef<HTMLDivElement>(null);
   const hasRequestedCloseRef = useRef(false);
+  const onRequestCloseRef = useRef(onRequestClose);
   const { width: winW, height: winH } = useWindowSize();
   const { x: winX, y: winY } = useWindowScroll();
   const [containers, setContainers] = useState<ContainerState>();
   const [targetRect, setTargetRect] = useState<DOMRect>();
   const parent = containers?.parent;
 
+  // Memoize scrolling parents to avoid expensive DOM traversals
+  const scrollingParents = useScrollingParents(
+    targetRef as React.RefObject<HTMLElement | null>
+  );
+
+  // Keep onRequestClose ref up to date
+  useEffect(() => {
+    onRequestCloseRef.current = onRequestClose;
+  }, [onRequestClose]);
+
   const popoverPosition = useMemo(() => {
     if (parent !== undefined) {
       return getPosition({
@@ -98,24 +109,32 @@ export const PopoverContainer: React.FC<PopoverContainerProps> = ({
 
   useResizingParentEffect(targetRef, setTargetRect);
 
-  useIsomorphicLayoutEffect(() => {
+  // Handle closeOnViewportExit with cached scrolling parents for performance
+  useEffect(() => {
     if (!closeOnViewportExit) return;
 
-    if (
-      containers?.viewport &&
-      isOutOfView(containers?.viewport, targetRef?.current as HTMLElement) &&
-      !hasRequestedCloseRef.current
-    ) {
+    const rect = targetRect || containers?.viewport;
+    if (!rect) return;
+
+    const isOut = isOutOfView(
+      rect,
+      targetRef?.current as HTMLElement,
+      scrollingParents
+    );
+
+    if (isOut && !hasRequestedCloseRef.current) {
       hasRequestedCloseRef.current = true;
-      onRequestClose?.();
-    } else if (
-      containers?.viewport &&
-      !isOutOfView(containers?.viewport, targetRef?.current as HTMLElement)
-    ) {
+      onRequestCloseRef.current?.();
+    } else if (!isOut) {
       hasRequestedCloseRef.current = false;
     }
-  }, [containers?.viewport, onRequestClose, targetRef, closeOnViewportExit]);
-
+  }, [
+    targetRect,
+    containers?.viewport,
+    targetRef,
+    closeOnViewportExit,
+    scrollingParents,
+  ]);
   /**
    * Allows targetRef to be or contain a button that toggles the popover open and closed.
    * Without this check it would toggle closed then back open immediately.

packages/gamut/src/PopoverContainer/__tests__/PopoverContainer.test.tsx

@@ -4,6 +4,13 @@ import { fireEvent, screen } from '@testing-library/react';
 
 import { PopoverContainer } from '..';
 import { PopoverContainerProps, TargetRef } from '../types';
+import * as utils from '../utils';
+import {
+  createMockDOMRect,
+  createNestedScrollableParents,
+  createScrollableParent,
+  setupWindowDimensions,
+} from './utils';
 
 // Add the custom matchers provided by '@emotion/jest'
 expect.extend(matchers);
@@ -284,4 +291,108 @@ describe('Popover', () => {
       );
     });
   });
+
+  describe('closeOnViewportExit with scrollable parents', () => {
+    beforeEach(() => {
+      setupWindowDimensions();
+    });
+
+    afterEach(() => {
+      document.body.innerHTML = '';
+      jest.restoreAllMocks();
+    });
+
+    it('findAllAdditionalScrollingParents finds scrollable parent elements', () => {
+      const { target } = createScrollableParent();
+
+      const parents = utils.findAllAdditionalScrollingParents(target);
+
+      expect(parents.length).toBeGreaterThan(0);
+    });
+
+    it('detects target is out of view when completely above scrollable parent viewport', () => {
+      const { parent, target } = createScrollableParent();
+
+      // Target is at y=50, but parent's visible viewport starts at y=200
+      jest
+        .spyOn(target, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(50, 100, 100, 50));
+
+      jest
+        .spyOn(parent, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(200, 0, 500, 200));
+
+      const targetRect = target.getBoundingClientRect();
+      const result = utils.isOutOfView(targetRect, target);
+
+      expect(result).toBe(true);
+    });
+
+    it('detects target is visible when within scrollable parent viewport', () => {
+      const { parent, target } = createScrollableParent();
+
+      // Target is within parent's visible viewport (y=250 is between parent's y=200 and y=400)
+      jest
+        .spyOn(target, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(250, 100, 100, 50));
+
+      jest
+        .spyOn(parent, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(200, 0, 500, 200));
+
+      const targetRect = target.getBoundingClientRect();
+      const result = utils.isOutOfView(targetRect, target);
+
+      expect(result).toBe(false);
+    });
+
+    it('detects target is out of view in nested scrollable parents', () => {
+      const { outerParent, innerParent, target } =
+        createNestedScrollableParents();
+
+      // Target is out of view in inner parent (y=500 is below inner parent's visible viewport at y=250-450)
+      // but might be visible in outer parent
+      jest
+        .spyOn(target, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(500, 100, 100, 50));
+
+      jest
+        .spyOn(innerParent, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(250, 50, 500, 200));
+
+      jest
+        .spyOn(outerParent, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(100, 0, 600, 400));
+
+      const targetRect = target.getBoundingClientRect();
+      const result = utils.isOutOfView(targetRect, target);
+
+      // Should return true because target is out of view in the inner (closer) scrollable parent
+      expect(result).toBe(true);
+    });
+
+    it('detects target is visible when within nested scrollable parents', () => {
+      const { outerParent, innerParent, target } =
+        createNestedScrollableParents();
+
+      // Target is visible in both inner and outer parents
+      jest
+        .spyOn(target, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(300, 100, 100, 50));
+
+      jest
+        .spyOn(innerParent, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(250, 50, 500, 200));
+
+      jest
+        .spyOn(outerParent, 'getBoundingClientRect')
+        .mockReturnValue(createMockDOMRect(100, 0, 600, 400));
+
+      const targetRect = target.getBoundingClientRect();
+      const result = utils.isOutOfView(targetRect, target);
+
+      // Should return false because target is visible in all scrollable parents
+      expect(result).toBe(false);
+    });
+  });
 });

packages/gamut/src/PopoverContainer/__tests__/utils.ts

@@ -0,0 +1,153 @@
+const defaultHeight = '200px';
+const defaultWidth = '500px';
+const defaultContentHeight = '1000px';
+const defaultTop = '200px';
+const defaultLeft = '0px';
+const targetWidth = '100px';
+const targetHeight = '50px';
+const defaultOuterHeight = '400px';
+const defaultOuterWidth = '600px';
+const defaultOuterTop = '100px';
+const defaultOuterContentHeight = '2000px';
+const defaultInnerTop = '50px';
+const defaultInnerLeft = '50px';
+
+export const createMockDOMRect = (
+  top: number,
+  left: number,
+  width: number,
+  height: number
+): DOMRect => ({
+  top,
+  bottom: top + height,
+  left,
+  right: left + width,
+  height,
+  width,
+  x: left,
+  y: top,
+  toJSON: jest.fn(),
+});
+
+export const createStyledElement = (
+  tagName = 'div',
+  styles: Partial<CSSStyleDeclaration> = {}
+): HTMLElement => {
+  const element = document.createElement(tagName);
+  Object.assign(element.style, styles);
+  return element;
+};
+
+export const createScrollableParent = (
+  options: Partial<CSSStyleDeclaration> & {
+    contentHeight?: string;
+    appendToBody?: boolean;
+  } = {}
+): { parent: HTMLElement; target: HTMLElement } => {
+  const {
+    height = defaultHeight,
+    width = defaultWidth,
+    position = 'absolute',
+    top = defaultTop,
+    left = defaultLeft,
+    contentHeight = defaultContentHeight,
+    appendToBody = true,
+    ...restStyles
+  } = options;
+
+  const parent = createStyledElement('div', {
+    overflow: 'auto',
+    height,
+    width,
+    position,
+    top,
+    left,
+    ...restStyles,
+  });
+
+  const content = createStyledElement('div', { height: contentHeight, width });
+  parent.appendChild(content);
+
+  const target = createStyledElement('div', {
+    width: targetWidth,
+    height: targetHeight,
+  });
+  parent.appendChild(target);
+
+  if (appendToBody) {
+    document.body.appendChild(parent);
+  }
+
+  return { parent, target };
+};
+
+export const createNestedScrollableParents = (
+  options: {
+    outer?: Partial<CSSStyleDeclaration> & { contentHeight?: string };
+    inner?: Partial<CSSStyleDeclaration> & { contentHeight?: string };
+  } = {}
+): {
+  outerParent: HTMLElement;
+  innerParent: HTMLElement;
+  target: HTMLElement;
+} => {
+  const { outer = {}, inner = {} } = options;
+
+  const {
+    height: outerHeight = defaultOuterHeight,
+    width: outerWidth = defaultOuterWidth,
+    top: outerTop = defaultOuterTop,
+    left: outerLeft = defaultLeft,
+    contentHeight: outerContentHeight = defaultOuterContentHeight,
+    ...outerRestStyles
+  } = outer;
+
+  const outerParent = createStyledElement('div', {
+    overflow: 'auto',
+    height: outerHeight,
+    width: outerWidth,
+    position: 'absolute',
+    top: outerTop,
+    left: outerLeft,
+    ...outerRestStyles,
+  });
+
+  const outerContent = createStyledElement('div', {
+    height: outerContentHeight,
+    width: outerWidth,
+  });
+  outerParent.appendChild(outerContent);
+
+  const {
+    height: innerHeight = defaultHeight,
+    width: innerWidth = defaultWidth,
+    top: innerTop = defaultInnerTop,
+    left: innerLeft = defaultInnerLeft,
+    contentHeight: innerContentHeight = defaultContentHeight,
+    ...innerRestStyles
+  } = inner;
+
+  const { parent: innerParent, target } = createScrollableParent({
+    height: innerHeight,
+    width: innerWidth,
+    position: 'relative',
+    top: innerTop,
+    left: innerLeft,
+    contentHeight: innerContentHeight,
+    appendToBody: false,
+    ...innerRestStyles,
+  });
+
+  outerParent.appendChild(innerParent);
+  document.body.appendChild(outerParent);
+
+  return { outerParent, innerParent, target };
+};
+
+export const setupWindowDimensions = () => {
+  const dimensions = { writable: true, configurable: true, value: 1000 };
+  Object.defineProperty(window, 'innerHeight', dimensions);
+  Object.defineProperty(window, 'innerWidth', dimensions);
+  Object.defineProperty(document.documentElement, 'clientHeight', dimensions);
+  Object.defineProperty(document.documentElement, 'clientWidth', dimensions);
+};