diff --git a/.changeset/many-glasses-visit.md b/.changeset/many-glasses-visit.md new file mode 100644 index 0000000000..ec807ca8fd --- /dev/null +++ b/.changeset/many-glasses-visit.md @@ -0,0 +1,5 @@ +--- +"@coinbase/onchainkit": patch +--- + +- **feat**: Implement Popover, DismissableLayer, and FocusTrap primitives in SwapSettings. @cpcramer #1856 diff --git a/src/internal/primitives/DismissableLayer.test.tsx b/src/internal/primitives/DismissableLayer.test.tsx index 4fd103ac8f..e0d1274801 100644 --- a/src/internal/primitives/DismissableLayer.test.tsx +++ b/src/internal/primitives/DismissableLayer.test.tsx @@ -1,4 +1,5 @@ import { fireEvent, render, screen } from '@testing-library/react'; +import { useRef } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { DismissableLayer } from './DismissableLayer'; @@ -63,23 +64,73 @@ describe('DismissableLayer', () => { , ); - const innerElement = screen.getByTestId('inner'); - fireEvent.pointerDown(innerElement); + fireEvent.pointerDown(screen.getByTestId('inner')); expect(onDismiss).not.toHaveBeenCalled(); }); - it('does not call onDismiss when clicking outside and disableOutsideClick is true', () => { - render( - -
Test Content
-
, - ); + it('handles trigger clicks with preventTriggerBubbling', () => { + const TestComponent = () => { + const triggerRef = useRef(null); + return ( + <> + + +
Content
+
+ + ); + }; + + render(); + + const event = new Event('pointerdown', { bubbles: true }); + Object.defineProperty(event, 'preventDefault', { value: vi.fn() }); + Object.defineProperty(event, 'stopPropagation', { value: vi.fn() }); + + const trigger = screen.getByTestId('trigger'); + trigger.dispatchEvent(event); + + expect(event.preventDefault).toHaveBeenCalled(); + expect(event.stopPropagation).toHaveBeenCalled(); + expect(onDismiss).not.toHaveBeenCalled(); + }); - fireEvent.pointerDown(document.body); + it('handles trigger clicks without preventTriggerBubbling', () => { + const TestComponent = () => { + const triggerRef = useRef(null); + return ( + <> + + +
Content
+
+ + ); + }; + + render(); + + const event = new Event('pointerdown', { bubbles: true }); + Object.defineProperty(event, 'preventDefault', { value: vi.fn() }); + Object.defineProperty(event, 'stopPropagation', { value: vi.fn() }); + + const trigger = screen.getByTestId('trigger'); + trigger.dispatchEvent(event); + + expect(event.preventDefault).not.toHaveBeenCalled(); + expect(event.stopPropagation).not.toHaveBeenCalled(); expect(onDismiss).not.toHaveBeenCalled(); }); - it('handles case when both disableEscapeKey and disableOutsideClick are true', () => { + it('handles both disableEscapeKey and disableOutsideClick being true', () => { render( { }); it('does not call onDismiss when clicking the trigger button', () => { - render( - <> - - -
Test Content
-
- , - ); + const TestComponent = () => { + const triggerRef = useRef(null); + return ( + <> + + +
Test Content
+
+ + ); + }; + + render(); const triggerButton = screen.getByLabelText('Toggle swap settings'); fireEvent.pointerDown(triggerButton); expect(onDismiss).not.toHaveBeenCalled(); }); + + it('does not call onDismiss when clicking outside and disableOutsideClick is true', () => { + render( + +
Test Content
+
, + ); + + fireEvent.pointerDown(document.body); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + it('handles non-Node event target gracefully', () => { + render( + +
Test Content
+
, + ); + + const event = new Event('pointerdown', { bubbles: true }); + // Create a non-Node object as target + Object.defineProperty(event, 'target', { value: {} }); + document.dispatchEvent(event); + + expect(onDismiss).not.toHaveBeenCalled(); + }); }); diff --git a/src/internal/primitives/DismissableLayer.tsx b/src/internal/primitives/DismissableLayer.tsx index 62c35f3513..d0b8e882bc 100644 --- a/src/internal/primitives/DismissableLayer.tsx +++ b/src/internal/primitives/DismissableLayer.tsx @@ -6,14 +6,26 @@ type DismissableLayerProps = { disableEscapeKey?: boolean; disableOutsideClick?: boolean; onDismiss?: () => void; + /** + * Reference to the trigger element (e.g., button) that opens this layer. + * Prevents the layer from being dismissed when the trigger is clicked, enabling proper toggle behavior. + */ + triggerRef?: React.RefObject; + /** + * When `true`, prevents trigger click events from bubbling up. + * Useful for bottom sheets to prevent unwanted side effects. + */ + preventTriggerEvents?: boolean; }; -// DismissableLayer handles dismissal using outside clicks and escape key events +/** DismissableLayer handles dismissal using outside clicks and escape key events */ export function DismissableLayer({ children, disableEscapeKey = false, disableOutsideClick = false, onDismiss, + triggerRef, + preventTriggerEvents = false, }: DismissableLayerProps) { const layerRef = useRef(null); @@ -22,46 +34,63 @@ export function DismissableLayer({ return; } - const handleKeyDown = (event: KeyboardEvent) => { - if (!disableEscapeKey && event.key === 'Escape') { - onDismiss?.(); + const handleTriggerClick = (event: PointerEvent) => { + if (preventTriggerEvents) { + event.preventDefault(); + event.stopPropagation(); } }; - const handlePointerDown = (event: PointerEvent) => { + const isClickInsideLayer = (target: Node) => { + return layerRef.current?.contains(target); + }; + + // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: TODO Refactor this component + const handlePointerDownCapture = (event: PointerEvent) => { if (disableOutsideClick) { return; } - // If the click is inside the dismissable layer content, don't dismiss - // This prevents the popover from closing when clicking inside it - if (layerRef.current?.contains(event.target as Node)) { + if (!(event.target instanceof Node)) { return; } - // Handling for the trigger button (e.g., settings toggle) - // Without this, clicking the trigger would cause both: - // 1. The button's onClick to fire (toggling isOpen) - // 2. This dismissal logic to fire (forcing close) - // This would create a race condition where the popover rapidly closes and reopens - const isTriggerClick = (event.target as HTMLElement).closest( - '[aria-label="Toggle swap settings"]', - ); - if (isTriggerClick) { + const target = event.target; + + if (triggerRef?.current?.contains(target)) { + handleTriggerClick(event); return; } - onDismiss?.(); + if (!isClickInsideLayer(target)) { + onDismiss?.(); + } + }; + + const handleKeyDown = (event: KeyboardEvent) => { + if (!disableEscapeKey && event.key === 'Escape') { + onDismiss?.(); + } }; + document.addEventListener('pointerdown', handlePointerDownCapture, true); document.addEventListener('keydown', handleKeyDown); - document.addEventListener('pointerdown', handlePointerDown); return () => { + document.removeEventListener( + 'pointerdown', + handlePointerDownCapture, + true, + ); document.removeEventListener('keydown', handleKeyDown); - document.removeEventListener('pointerdown', handlePointerDown); }; - }, [disableOutsideClick, disableEscapeKey, onDismiss]); + }, [ + disableOutsideClick, + disableEscapeKey, + onDismiss, + triggerRef, + preventTriggerEvents, + ]); return (
diff --git a/src/internal/primitives/FocusTrap.tsx b/src/internal/primitives/FocusTrap.tsx index 6b663338c3..b745bea267 100644 --- a/src/internal/primitives/FocusTrap.tsx +++ b/src/internal/primitives/FocusTrap.tsx @@ -9,7 +9,7 @@ interface FocusTrapProps { children?: React.ReactNode; } -// FocusTrap ensures keyboard focus remains within a contained area for accessibility +/** FocusTrap ensures keyboard focus remains within a contained area for accessibility */ export function FocusTrap({ active = true, children }: FocusTrapProps) { const containerRef = useRef(null); const previousFocusRef = useRef(null); diff --git a/src/internal/primitives/Popover.test.tsx b/src/internal/primitives/Popover.test.tsx index 0c847bfb83..dbc6def106 100644 --- a/src/internal/primitives/Popover.test.tsx +++ b/src/internal/primitives/Popover.test.tsx @@ -4,7 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { Popover } from './Popover'; describe('Popover', () => { - let anchorEl: HTMLElement; + let anchor: HTMLElement; beforeEach(() => { Object.defineProperty(window, 'matchMedia', { @@ -21,9 +21,9 @@ describe('Popover', () => { })), }); - anchorEl = document.createElement('button'); - anchorEl.setAttribute('data-testid', 'anchor'); - document.body.appendChild(anchorEl); + anchor = document.createElement('button'); + anchor.setAttribute('data-testid', 'anchor'); + document.body.appendChild(anchor); }); afterEach(() => { @@ -35,7 +35,7 @@ describe('Popover', () => { describe('rendering', () => { it('should not render when isOpen is false', () => { render( - + Content , ); @@ -45,7 +45,7 @@ describe('Popover', () => { it('should render when isOpen is true', () => { render( - + Content , ); @@ -54,9 +54,9 @@ describe('Popover', () => { expect(screen.getByText('Content')).toBeInTheDocument(); }); - it('should handle null anchorEl gracefully', () => { + it('should handle null anchor gracefully', () => { render( - + Content , ); @@ -74,7 +74,7 @@ describe('Popover', () => { it(`should position correctly with position=${position} and align=${align}`, () => { render( { it('should update position on window resize', async () => { render( - + Content , ); @@ -107,7 +107,7 @@ describe('Popover', () => { it('should update position on scroll', async () => { render( - + Content , ); @@ -125,7 +125,7 @@ describe('Popover', () => { .mockReturnValue(undefined); render( - + Content , ); @@ -141,7 +141,7 @@ describe('Popover', () => { it('should not call onClose when clicking inside', async () => { const onClose = vi.fn(); render( - + Content , ); @@ -153,7 +153,7 @@ describe('Popover', () => { it('should call onClose when pressing Escape', async () => { const onClose = vi.fn(); render( - + Content , ); @@ -167,7 +167,7 @@ describe('Popover', () => { it('should have correct ARIA attributes', () => { render( { it('should trap focus when open', async () => { const user = userEvent.setup(); render( - + , @@ -210,7 +210,7 @@ describe('Popover', () => { describe('cleanup', () => { it('should remove event listeners on unmount', () => { const { unmount } = render( - + Content , ); diff --git a/src/internal/primitives/Popover.tsx b/src/internal/primitives/Popover.tsx index a1b82fc60c..239171deda 100644 --- a/src/internal/primitives/Popover.tsx +++ b/src/internal/primitives/Popover.tsx @@ -10,21 +10,25 @@ type Position = 'top' | 'right' | 'bottom' | 'left'; type Alignment = 'start' | 'center' | 'end'; type PopoverProps = { - align?: Alignment; // Determines how the popover aligns with the anchor - anchorEl: HTMLElement | null; + /** Determines how the popover aligns with the anchor */ + align?: Alignment; + /** The element that the popover will be positioned relative to. */ + anchor: HTMLElement | null; children?: React.ReactNode; onClose?: () => void; - offset?: number; // Spacing (in pixels) between the anchor element and the popover content. - position?: Position; // Determines which side of the anchor element the popover will appear. + /** Spacing (in pixels) between the anchor element and the popover content. */ + offset?: number; + /** Determines which side of the anchor element the popover will appear. */ + position?: Position; isOpen?: boolean; + /** Reference to the element that triggered the popover (e.g., a button that opened it). */ + trigger?: React.RefObject; 'aria-label'?: string; 'aria-labelledby'?: string; 'aria-describedby'?: string; }; -/** - * Calculates the initial position of the popover based on the position prop. - */ +/** Calculates the initial position of the popover based on the position prop. */ function getInitialPosition( triggerRect: DOMRect, contentRect: DOMRect, @@ -52,9 +56,7 @@ function getInitialPosition( return { top, left }; } -/** - * Adjusts the initial position based on the alignment prop. - */ +/** Adjusts the initial position based on the alignment prop. */ function adjustAlignment( triggerRect: DOMRect, contentRect: DOMRect, @@ -95,22 +97,21 @@ function adjustAlignment( return { top, left }; } -/** - * Popover primitive that handles: +/** Popover primitive that handles: * - Positioning relative to anchor element * - Focus management * - Click outside and escape key dismissal * - Portal rendering - * - Proper ARIA attributes - */ + * - Proper ARIA attributes */ export function Popover({ children, - anchorEl, + anchor, isOpen, onClose, position = 'bottom', align = 'center', offset = 8, + trigger, 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, 'aria-describedby': ariaDescribedby, @@ -119,11 +120,11 @@ export function Popover({ const componentTheme = useTheme(); const updatePosition = useCallback(() => { - if (!anchorEl || !contentRef.current) { + if (!anchor || !contentRef.current) { return; } - const triggerRect = anchorEl.getBoundingClientRect(); + const triggerRect = anchor.getBoundingClientRect(); const contentRect = contentRef.current?.getBoundingClientRect(); if (!triggerRect || !contentRect) { @@ -146,7 +147,7 @@ export function Popover({ contentRef.current.style.top = `${finalPosition.top}px`; contentRef.current.style.left = `${finalPosition.left}px`; - }, [anchorEl, position, offset, align]); + }, [anchor, position, offset, align]); useEffect(() => { if (!isOpen) { @@ -170,7 +171,7 @@ export function Popover({ const popover = (
- +
({ useBreakpoints: vi.fn(), })); +vi.mock('../../internal/primitives/FocusTrap', () => ({ + FocusTrap: vi.fn(({ children }) => ( +
{children}
+ )), +})); + +vi.mock('../../internal/primitives/DismissableLayer', () => ({ + DismissableLayer: vi.fn(({ children, onDismiss }) => ( +
{ + if (e.key === 'Enter' || e.key === ' ') { + onDismiss(); + } + }} + role="button" + tabIndex={0} + > + {children} +
+ )), +})); + +vi.mock('../../internal/primitives/Popover', () => ({ + Popover: vi.fn(({ children, isOpen, onClose }) => + isOpen ? ( +
{ + if (e.key === 'Enter' || e.key === ' ') { + onClose(); + } + }} + role="button" + tabIndex={0} + > + {children} +
+ ) : null, + ), +})); + const useBreakpointsMock = useBreakpoints as Mock; const renderComponent = (props = {}) => { @@ -57,6 +101,20 @@ const renderComponent = (props = {}) => { describe('SwapSettings', () => { beforeEach(() => { + Object.defineProperty(window, 'matchMedia', { + writable: true, + value: vi.fn().mockImplementation((query) => ({ + matches: false, + media: query, + onchange: null, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn(), + })), + }); + vi.clearAllMocks(); useBreakpointsMock.mockReturnValue('md'); }); @@ -102,14 +160,18 @@ describe('SwapSettings', () => { 'custom-class', ); expect(useIcon).toHaveBeenCalledWith({ icon: 'custom-icon' }); + const button = screen.getByRole('button', { name: /toggle swap settings/i, }); fireEvent.click(button); + await waitFor(() => { expect(screen.getByTestId('ockSwapSettingsDropdown')).toBeInTheDocument(); }); - fireEvent.click(screen.getByTestId('outside')); + + fireEvent.click(screen.getByTestId('mock-popover')); + await waitFor(() => { expect( screen.queryByTestId('ockSwapSettingsDropdown'), @@ -143,44 +205,84 @@ describe('SwapSettings', () => { const button = screen.getByRole('button', { name: /toggle swap settings/i, }); + const initialBottomSheet = screen.getByTestId( 'ockSwapSettingsSlippageLayoutBottomSheet_container', ); - expect(initialBottomSheet).toHaveClass('ease-in-out'); - expect(initialBottomSheet).toHaveClass('group-[]:bottom-0'); + expect(initialBottomSheet).toHaveClass('transition-[bottom]'); + expect(initialBottomSheet).toHaveClass('duration-300'); + expect(initialBottomSheet).toHaveClass('-bottom-[12.875rem]'); + fireEvent.click(button); + await waitFor(() => { const parentDiv = screen .getByTestId('ockSwapSettings_Settings') .querySelector('div'); expect(parentDiv).toHaveClass('group'); + const openBottomSheet = screen.getByTestId( 'ockSwapSettingsSlippageLayoutBottomSheet_container', ); - expect(openBottomSheet).toHaveClass('group-[]:bottom-0'); + expect(openBottomSheet).toHaveClass('bottom-0'); }); + fireEvent.click(button); + await waitFor(() => { const parentDiv = screen .getByTestId('ockSwapSettings_Settings') .querySelector('div'); expect(parentDiv).not.toHaveClass('group'); + const closedBottomSheet = screen.getByTestId( 'ockSwapSettingsSlippageLayoutBottomSheet_container', ); - expect(closedBottomSheet).toHaveClass('ease-in-out'); + expect(closedBottomSheet).toHaveClass('-bottom-[12.875rem]'); }); }); - it('removes event listener on unmount', () => { - const removeEventListenerSpy = vi.spyOn(document, 'removeEventListener'); - const { unmount } = renderComponent(); - unmount(); - expect(removeEventListenerSpy).toHaveBeenCalledWith( - 'click', - expect.any(Function), - { capture: true }, - ); - removeEventListenerSpy.mockRestore(); + it('renders mobile view with FocusTrap and DismissableLayer when breakpoint is "sm"', async () => { + useBreakpointsMock.mockReturnValue('sm'); + renderComponent(); + + const button = screen.getByRole('button', { + name: /toggle swap settings/i, + }); + fireEvent.click(button); + + await waitFor(() => { + expect(screen.getByTestId('mock-focus-trap')).toBeInTheDocument(); + expect(screen.getByTestId('mock-dismissable-layer')).toBeInTheDocument(); + expect(screen.getByTestId('mock-bottom-sheet')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('mock-dismissable-layer')); + await waitFor(() => { + const bottomSheet = screen.getByTestId( + 'ockSwapSettingsSlippageLayoutBottomSheet_container', + ); + expect(bottomSheet).toHaveClass('-bottom-[12.875rem]'); + }); + }); + + it('renders desktop view with Popover when breakpoint is not "sm"', async () => { + useBreakpointsMock.mockReturnValue('md'); + renderComponent(); + + const button = screen.getByRole('button', { + name: /toggle swap settings/i, + }); + fireEvent.click(button); + + await waitFor(() => { + expect(screen.getByTestId('mock-popover')).toBeInTheDocument(); + expect(screen.getByTestId('ockSwapSettingsDropdown')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('mock-popover')); + await waitFor(() => { + expect(screen.queryByTestId('mock-popover')).not.toBeInTheDocument(); + }); }); }); diff --git a/src/swap/components/SwapSettings.tsx b/src/swap/components/SwapSettings.tsx index 7d235591aa..b14a5fb697 100644 --- a/src/swap/components/SwapSettings.tsx +++ b/src/swap/components/SwapSettings.tsx @@ -1,8 +1,10 @@ import { useCallback, useRef, useState } from 'react'; import { useIcon } from '../../core-react/internal/hooks/useIcon'; +import { DismissableLayer } from '../../internal/primitives/DismissableLayer'; +import { FocusTrap } from '../../internal/primitives/FocusTrap'; +import { Popover } from '../../internal/primitives/Popover'; import { background, border, cn, pressable, text } from '../../styles/theme'; import { useBreakpoints } from '../../ui/react/internal/hooks/useBreakpoints'; -import { useOutsideClick } from '../../ui/react/internal/hooks/useOutsideClick'; import type { SwapSettingsReact } from '../types'; import { SwapSettingsSlippageLayout } from './SwapSettingsSlippageLayout'; import { SwapSettingsSlippageLayoutBottomSheet } from './SwapSettingsSlippageLayoutBottomSheet'; @@ -16,14 +18,16 @@ export function SwapSettings({ const breakpoint = useBreakpoints(); const [isOpen, setIsOpen] = useState(false); const dropdownRef = useRef(null); + const triggerRef = useRef(null); - const handleToggle = useCallback(() => { + const handleToggle = useCallback((e: React.MouseEvent) => { + e.stopPropagation(); setIsOpen((prev) => !prev); }, []); - useOutsideClick(dropdownRef, () => { + const handleClose = useCallback(() => { setIsOpen(false); - }); + }, []); const iconSvg = useIcon({ icon }); @@ -38,6 +42,7 @@ export function SwapSettings({ {buttonText && {buttonText}}
{breakpoint === 'sm' ? ( -
- - {children} - -
+ + +
+ + {children} + +
+
+
) : ( - isOpen && ( +
- ) +
)}