diff --git a/packages/gamut/src/PopoverContainer/PopoverContainer.tsx b/packages/gamut/src/PopoverContainer/PopoverContainer.tsx index 6f938d86e0..86ba4908bd 100644 --- a/packages/gamut/src/PopoverContainer/PopoverContainer.tsx +++ b/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 = ({ }) => { const popoverRef = useRef(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(); const [targetRect, setTargetRect] = useState(); const parent = containers?.parent; + // Memoize scrolling parents to avoid expensive DOM traversals + const scrollingParents = useScrollingParents( + targetRef as React.RefObject + ); + + // 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 = ({ 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. diff --git a/packages/gamut/src/PopoverContainer/__tests__/PopoverContainer.test.tsx b/packages/gamut/src/PopoverContainer/__tests__/PopoverContainer.test.tsx index 6a18ecf8d0..96658e5d19 100644 --- a/packages/gamut/src/PopoverContainer/__tests__/PopoverContainer.test.tsx +++ b/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); + }); + }); }); diff --git a/packages/gamut/src/PopoverContainer/__tests__/utils.ts b/packages/gamut/src/PopoverContainer/__tests__/utils.ts new file mode 100644 index 0000000000..747a36a718 --- /dev/null +++ b/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 = {} +): HTMLElement => { + const element = document.createElement(tagName); + Object.assign(element.style, styles); + return element; +}; + +export const createScrollableParent = ( + options: Partial & { + 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 & { contentHeight?: string }; + inner?: Partial & { 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); +}; diff --git a/packages/gamut/src/PopoverContainer/hooks.ts b/packages/gamut/src/PopoverContainer/hooks.ts index 36ff21ce26..295591da7c 100644 --- a/packages/gamut/src/PopoverContainer/hooks.ts +++ b/packages/gamut/src/PopoverContainer/hooks.ts @@ -1,4 +1,4 @@ -import { useEffect } from 'react'; +import { useEffect, useMemo } from 'react'; import { findAllAdditionalScrollingParents, findResizingParent } from './utils'; @@ -20,20 +20,14 @@ export const useScrollingParentsEffect = ( setTargetRect(targetRef?.current?.getBoundingClientRect()); }; - // For immediate updates during scroll - const immediateUpdate = () => { - updatePosition(); - }; - const cleanup: (() => void)[] = []; // Add listeners to all scrolling parents (window scroll handled by useWindowScroll) scrollingParents.forEach((parent) => { if (parent.addEventListener) { - // Use immediate update for smoother experience - parent.addEventListener('scroll', immediateUpdate, { passive: true }); + parent.addEventListener('scroll', updatePosition, { passive: true }); cleanup.push(() => - parent.removeEventListener('scroll', immediateUpdate) + parent.removeEventListener('scroll', updatePosition) ); } }); @@ -69,3 +63,20 @@ export const useResizingParentEffect = ( return () => ro.unobserve(resizingParent); }, [targetRef, setTargetRect]); }; + +/** + * Memoizes the list of scrolling parent elements for a target element. + * This avoids expensive DOM traversals and getComputedStyle calls on every render. + * Returns an empty array if the target element is not available. + */ +export const useScrollingParents = ( + targetRef: React.RefObject +): HTMLElement[] => { + return useMemo(() => { + if (!targetRef.current) { + return []; + } + return findAllAdditionalScrollingParents(targetRef.current); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [targetRef.current]); +}; diff --git a/packages/gamut/src/PopoverContainer/utils.ts b/packages/gamut/src/PopoverContainer/utils.ts index 5a40b7ef2d..0a23e70f12 100644 --- a/packages/gamut/src/PopoverContainer/utils.ts +++ b/packages/gamut/src/PopoverContainer/utils.ts @@ -5,22 +5,51 @@ import { import { PopoverPositionConfig, TargetRef } from './types'; -export const findResizingParent = ({ - parentElement, -}: HTMLElement): HTMLElement | null => { - if (parentElement) { - const { overflow, overflowY, overflowX } = getComputedStyle(parentElement); +const getWindowDimensions = () => ({ + height: window.innerHeight || document.documentElement.clientHeight, + width: window.innerWidth || document.documentElement.clientWidth, +}); + +const isRectOutOfBounds = ( + rect: DOMRect, + container: { top: number; bottom: number; left: number; right: number } +): boolean => { + const { top, bottom, left, right } = rect; + const { + top: containerTop, + bottom: containerBottom, + left: containerLeft, + right: containerRight, + } = container; + + return ( + bottom <= containerTop || + top >= containerBottom || + right <= containerLeft || + left >= containerRight + ); +}; + +export const findResizingParent = ( + element: HTMLElement +): HTMLElement | null => { + let currentElement = element.parentElement; + + while (currentElement && currentElement !== document.body) { + const { overflow, overflowY, overflowX } = getComputedStyle(currentElement); if ([overflow, overflowY, overflowX].some((val) => val === 'clip')) { - return parentElement; + return currentElement; } - return findResizingParent(parentElement); // parent of this parent is used via prop destructure + currentElement = currentElement.parentElement; } + return null; }; -/* - * Finds all extra scrolling parents of an element. - * This is useful for detecting scroll events on parents that may not be the direct parent, which should be managed by react-use's useWindowScroll. +/** + * Finds all scrolling parents of an element. + * This is useful for detecting scroll events on parents that may not be the direct parent. + * Window scroll is handled separately by react-use's useWindowScroll. */ export const findAllAdditionalScrollingParents = ( element: HTMLElement @@ -30,11 +59,15 @@ export const findAllAdditionalScrollingParents = ( while (currentElement && currentElement !== document.body) { const { overflow, overflowY, overflowX } = getComputedStyle(currentElement); - if ( - [overflow, overflowY, overflowX].some((val) => - ['scroll', 'auto'].includes(val) - ) - ) { + const isScrollable = [overflow, overflowY, overflowX].some((val) => + ['scroll', 'auto'].includes(val) + ); + + const hasScrollableContent = + currentElement.scrollHeight > currentElement.clientHeight || + currentElement.scrollWidth > currentElement.clientWidth; + + if (isScrollable || hasScrollableContent) { scrollingParents.push(currentElement); } currentElement = currentElement.parentElement; @@ -43,42 +76,62 @@ export const findAllAdditionalScrollingParents = ( return scrollingParents; }; +/** + * Determines if an element is out of view, checking both the window viewport + * and all scrollable parent containers. Returns true if the element is completely + * outside the visible area of any containing scrollable parent or the window viewport. + * Used by closeOnViewportExit to detect when the target element has scrolled out of view. + * @param rect - The DOMRect of the target element + * @param target - The target element (optional) + * @param cachedScrollingParents - Pre-computed list of scrolling parents to avoid expensive DOM traversals (optional) + */ export const isOutOfView = ( rect: DOMRect, - target?: HTMLElement | null + target?: HTMLElement | null, + cachedScrollingParents?: HTMLElement[] ): boolean => { - const windowHeight = - window.innerHeight || document.documentElement.clientHeight; - const windowWidth = window.innerWidth || document.documentElement.clientWidth; - - const outOfViewport = - rect.bottom < 0 || - rect.top > windowHeight || - rect.right < 0 || - rect.left > windowWidth; - - if (outOfViewport || !target) { - return outOfViewport; + if (!target) { + const { height, width } = getWindowDimensions(); + const windowRect = { + top: 0, + left: 0, + bottom: height, + right: width, + }; + return isRectOutOfBounds(rect, windowRect); } - const scrollingParents = findAllAdditionalScrollingParents(target); + const scrollingParents = + cachedScrollingParents ?? findAllAdditionalScrollingParents(target); - for (const parent of scrollingParents) { - const parentRect = parent.getBoundingClientRect(); + const { documentElement } = document; + const isDocumentScrollable = + documentElement.scrollHeight > documentElement.clientHeight || + documentElement.scrollWidth > documentElement.clientWidth; + + const allScrollableContainers = isDocumentScrollable + ? [documentElement, ...scrollingParents] + : scrollingParents; - const intersects = - rect.top < parentRect.bottom && - rect.bottom > parentRect.top && - rect.left < parentRect.right && - rect.right > parentRect.left; + for (const parent of allScrollableContainers) { + if (!parent.contains(target)) { + continue; + } - // If element doesn't intersect with a scrollable parent's visible area, it's out of view - if (!intersects) { + const parentRect = parent.getBoundingClientRect(); + if (isRectOutOfBounds(rect, parentRect)) { return true; } } - return false; + const { height, width } = getWindowDimensions(); + const windowRect = { + top: 0, + left: 0, + bottom: height, + right: width, + }; + return isRectOutOfBounds(rect, windowRect); }; export const ALIGN = { @@ -121,36 +174,42 @@ export const getPosition = ({ const xOffset = width + offset + x; const yOffset = height + offset + y; - const styles = {} as CSSObject; - const alignments = alignment.split('-') as | ['top' | 'bottom' | 'left' | 'right'] | ['top' | 'bottom', 'left' | 'right']; + const styles: CSSObject = {}; + if (alignments.length === 1) { - switch (alignments[0]) { - case 'left': - case 'right': - styles.transform = 'translate(0, -50%)'; - styles.top = top + height / 2; - break; - case 'top': - case 'bottom': - styles.transform = 'translate(-50%, 0)'; - styles.left = left + width / 2; - } + const [direction] = alignments; + const isVertical = direction === 'top' || direction === 'bottom'; + styles.transform = isVertical ? 'translate(-50%, 0)' : 'translate(0, -50%)'; + styles[isVertical ? 'left' : 'top'] = isVertical + ? left + width / 2 + : top + height / 2; } else { const coef = AXIS[invertAxis ?? 'none']; - const [y, x] = alignments; - styles.transform = `translate(${percent(coef[x])}, ${percent(coef[y])})`; + const [yAxis, xAxis] = alignments; + styles.transform = `translate(${percent(coef[xAxis])}, ${percent( + coef[yAxis] + )})`; } + const alignmentOffsets: Record< + 'left' | 'right' | 'top' | 'bottom', + { position: keyof CSSObject; value: number } + > = { + left: { position: 'right', value: right + xOffset }, + right: { position: 'left', value: left + xOffset }, + top: { position: 'bottom', value: bottom + yOffset }, + bottom: { position: 'top', value: top + yOffset }, + }; + alignments.forEach((alignment) => { - if (alignment === 'left') styles.right = right + xOffset; - if (alignment === 'right') styles.left = left + xOffset; - if (alignment === 'top') styles.bottom = bottom + yOffset; - if (alignment === 'bottom') styles.top = top + yOffset; + const { position, value } = alignmentOffsets[alignment]; + styles[position] = value; }); + return styles; }; @@ -159,19 +218,19 @@ export const getContainers = ( inline = false, scroll: { x: number; y: number } ) => { - const boundingClient = target.getBoundingClientRect(); + const viewport = target.getBoundingClientRect(); if (!inline) { - const { width, top, height, left } = boundingClient; + const { width, top, height, left } = viewport; return { - viewport: boundingClient, + viewport, parent: { width, height, top: top + scroll.y, left, right: document.body.offsetWidth - width - left, - bottom: -1 * (top + height + scroll.y), + bottom: -(top + height + scroll.y), }, }; } @@ -182,7 +241,6 @@ export const getContainers = ( offsetLeft: left, offsetTop: top, } = target; - const offsetParent = target?.offsetParent as HTMLElement; return { @@ -190,10 +248,10 @@ export const getContainers = ( width, height, left, - right: offsetParent?.offsetWidth - left - width, + right: (offsetParent?.offsetWidth ?? 0) - left - width, top, - bottom: offsetParent?.offsetHeight - top - height, + bottom: (offsetParent?.offsetHeight ?? 0) - top - height, } as DOMRect, - viewport: boundingClient, + viewport, }; }; diff --git a/packages/styleguide/src/lib/Organisms/Lists & Tables/examples.tsx b/packages/styleguide/src/lib/Organisms/Lists & Tables/examples.tsx index b2fbe8b31c..4a9b19219d 100644 --- a/packages/styleguide/src/lib/Organisms/Lists & Tables/examples.tsx +++ b/packages/styleguide/src/lib/Organisms/Lists & Tables/examples.tsx @@ -100,6 +100,7 @@ const CrewMgmtDropdown: React.FC<{ , }, - { - header: 'Controls', - key: 'name', - size: 'md', - justify: 'right', - type: 'control', - render: (row) => , - }, ] as ColumnConfig<(typeof crew)[number]>[]; export const createDemoTable =