From 4c52fb36645f383a9db7379980abd873b4277ab5 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:17:23 -0700 Subject: [PATCH] fix(popover): preserve wheel handler cancellation and ref cleanup --- .../ui/popover-wheel-scroll.test.tsx | 128 +++++++++++++- src/renderer/src/components/ui/popover.tsx | 167 +++++++++++------- 2 files changed, 226 insertions(+), 69 deletions(-) diff --git a/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx b/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx index dd714241445..833b631f883 100644 --- a/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx +++ b/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx @@ -1,5 +1,12 @@ // @vitest-environment happy-dom -import { act } from 'react' +import { + act, + createRef, + type Ref, + type RefCallback, + type WheelEvent as ReactWheelEvent, + type WheelEventHandler +} from 'react' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { Popover, PopoverContent, PopoverTrigger } from './popover' @@ -21,7 +28,23 @@ function wheel(el: HTMLElement, deltaY: number): WheelEvent { return event } -function renderPopover(contentClassName: string, nested: boolean): void { +function renderPopover( + contentClassName: string, + nested: boolean, + { + onWheel, + onWheelCapture, + innerOnWheel, + contentRef, + portalContainer + }: { + onWheel?: WheelEventHandler + onWheelCapture?: WheelEventHandler + innerOnWheel?: WheelEventHandler + contentRef?: Ref + portalContainer?: HTMLElement + } = {} +): void { container = document.createElement('div') document.body.appendChild(container) root = createRoot(container) @@ -29,13 +52,23 @@ function renderPopover(contentClassName: string, nested: boolean): void { root!.render( open - + {nested ? (
-
tall
+
+ tall +
) : ( -
tall
+
+ tall +
)} @@ -147,4 +180,89 @@ describe('PopoverContent wheel shim', () => { expect(viewport.scrollTop).toBe(120) expect(event.defaultPrevented).toBe(true) }) + + it('lets a consumer prevent the shim scroll', () => { + const onWheel = vi.fn((event: ReactWheelEvent) => { + Object.defineProperty(event.nativeEvent, 'preventDefault', { value: vi.fn() }) + event.preventDefault() + }) + const portalContainer = document.createElement('div') + document.body.appendChild(portalContainer) + renderPopover('popover-wheel-scroll', true, { onWheel, portalContainer }) + const viewport = document.querySelector('[data-testid="viewport"]')! + viewport.style.overflowY = 'auto' + makeScrollable(viewport, 1000, 400) + + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) + + expect(onWheel).toHaveBeenCalledOnce() + expect(event.defaultPrevented).toBe(false) + expect(viewport.scrollTop).toBe(0) + }) + + it('respects consumer capture cancellation', () => { + const onWheelCapture = vi.fn((event: ReactWheelEvent) => { + Object.defineProperty(event.nativeEvent, 'preventDefault', { value: vi.fn() }) + event.preventDefault() + }) + renderPopover('popover-wheel-scroll', true, { onWheelCapture }) + const viewport = document.querySelector('[data-testid="viewport"]')! + viewport.style.overflowY = 'auto' + makeScrollable(viewport, 1000, 400) + + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) + + expect(onWheelCapture).toHaveBeenCalledOnce() + expect(event.defaultPrevented).toBe(false) + expect(viewport.scrollTop).toBe(0) + }) + + it('respects descendant cancellation', () => { + const innerOnWheel = vi.fn((event: ReactWheelEvent) => event.preventDefault()) + renderPopover('popover-wheel-scroll', true, { innerOnWheel }) + const viewport = document.querySelector('[data-testid="viewport"]')! + viewport.style.overflowY = 'auto' + makeScrollable(viewport, 1000, 400) + + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) + + expect(innerOnWheel).toHaveBeenCalledOnce() + expect(event.defaultPrevented).toBe(true) + expect(viewport.scrollTop).toBe(0) + }) + + it('preserves callback ref cleanup', () => { + const cleanup = vi.fn() + const contentRef: RefCallback = vi.fn(() => cleanup) + renderPopover('', false, { contentRef }) + + expect(contentRef).toHaveBeenCalledTimes(1) + expect(contentRef).toHaveBeenCalledWith( + document.querySelector('[data-slot="popover-content"]') + ) + + act(() => root!.unmount()) + root = null + expect(cleanup).toHaveBeenCalledOnce() + expect(contentRef).toHaveBeenCalledTimes(1) + }) + + it('clears callback refs that do not return cleanup', () => { + const callbackRef = vi.fn((_node: HTMLDivElement | null): void => {}) + renderPopover('', false, { contentRef: callbackRef }) + + act(() => root!.unmount()) + root = null + expect(callbackRef).toHaveBeenLastCalledWith(null) + }) + + it('clears object refs on detach', () => { + const objectRef = createRef() + renderPopover('', false, { contentRef: objectRef }) + expect(objectRef.current).toBeInstanceOf(HTMLDivElement) + + act(() => root!.unmount()) + root = null + expect(objectRef.current).toBeNull() + }) }) diff --git a/src/renderer/src/components/ui/popover.tsx b/src/renderer/src/components/ui/popover.tsx index c8386daec51..c25d2555cc4 100644 --- a/src/renderer/src/components/ui/popover.tsx +++ b/src/renderer/src/components/ui/popover.tsx @@ -5,6 +5,9 @@ import { Popover as PopoverPrimitive } from 'radix-ui' import { cn } from '@/lib/utils' +// React delegates wheel passively, so native defaultPrevented may not reflect synthetic cancellation. +const consumerPreventedWheelEvents = new WeakSet() + function Popover(props: React.ComponentProps) { return } @@ -21,7 +24,10 @@ function PopoverAnchor(props: React.ComponentProps node.clientHeight) { @@ -35,6 +41,75 @@ function resolvePopoverScroller(target: EventTarget | null, content: HTMLElement return null } +function handlePopoverWheel(event: WheelEvent, content: HTMLDivElement): void { + if ( + event.defaultPrevented || + consumerPreventedWheelEvents.has(event) || + !(event.target instanceof Node) || + !content.contains(event.target) + ) { + return + } + + // Why two markers: `popover-scroll-content` also imposes a 15rem max-height and its + // own overflow, while `popover-wheel-scroll` opts into the shim alone. + if ( + !content.classList.contains('popover-scroll-content') && + !content.classList.contains('popover-wheel-scroll') + ) { + return + } + + const el = resolvePopoverScroller(event.target, content) + if (!el) { + return + } + + const delta = + event.deltaMode === WheelEvent.DOM_DELTA_LINE + ? event.deltaY * 16 + : event.deltaMode === WheelEvent.DOM_DELTA_PAGE + ? event.deltaY * el.clientHeight + : event.deltaY + const maxScrollTop = el.scrollHeight - el.clientHeight + const nextScrollTop = Math.max(0, Math.min(maxScrollTop, el.scrollTop + delta)) + + // Why: Radix dialog scroll-lock swallows native wheel scrolling in portaled popovers. + if (nextScrollTop !== el.scrollTop) { + event.preventDefault() + el.scrollTop = nextScrollTop + } +} + +type PopoverContentRef = React.ComponentProps['ref'] + +function attachPopoverContent( + content: HTMLDivElement, + portalContainer: HTMLElement | null | undefined, + forwardedRef: PopoverContentRef | undefined +): () => void { + // React delegates portal events here first, preserving onWheel-before-shim ordering. + const wheelTarget = portalContainer ?? content.ownerDocument.body + const handleWheel = (event: WheelEvent): void => handlePopoverWheel(event, content) + wheelTarget.addEventListener('wheel', handleWheel, { passive: false }) + + const refCleanup = typeof forwardedRef === 'function' ? forwardedRef(content) : undefined + if (forwardedRef && typeof forwardedRef !== 'function') { + forwardedRef.current = content + } + + return () => { + wheelTarget.removeEventListener('wheel', handleWheel) + if (typeof refCleanup === 'function') { + refCleanup() + } else if (typeof forwardedRef === 'function') { + forwardedRef(null) + } else if (forwardedRef) { + forwardedRef.current = null + } + } +} + function PopoverContent({ className, align = 'center', @@ -42,83 +117,46 @@ function PopoverContent({ portalContainer, style, onWheel, + onWheelCapture, ref: forwardedRef, ...props }: React.ComponentProps & { portalContainer?: HTMLElement | null }) { - const [content, setContent] = React.useState(null) + const handleConsumerWheel = React.useCallback( + (event: React.WheelEvent): void => { + onWheel?.(event) + if (event.defaultPrevented) { + consumerPreventedWheelEvents.add(event.nativeEvent) + } + }, + [onWheel] + ) + const handleConsumerWheelCapture = React.useCallback( + (event: React.WheelEvent): void => { + onWheelCapture?.(event) + if (event.defaultPrevented) { + consumerPreventedWheelEvents.add(event.nativeEvent) + } + }, + [onWheelCapture] + ) const setContentRef = React.useCallback( (node: HTMLDivElement | null) => { - setContent(node) + if (node) { + return attachPopoverContent(node, portalContainer, forwardedRef) + } if (typeof forwardedRef === 'function') { - const cleanup = forwardedRef(node) - if (node !== null && typeof cleanup === 'function') { - return () => { - setContent(null) - cleanup() - } - } + forwardedRef(null) } else if (forwardedRef) { - forwardedRef.current = node + forwardedRef.current = null } return undefined }, - [forwardedRef] + [forwardedRef, portalContainer] ) - React.useEffect(() => { - if (!content) { - return - } - - const handleWheel = (event: WheelEvent): void => { - if (event.defaultPrevented) { - return - } - - // Why two markers: `popover-scroll-content` also imposes a 15rem max-height and its - // own overflow, which a popover that manages its own layout (a fixed-height flex - // column over a pinned footer) must not inherit. `popover-wheel-scroll` opts into - // the shim alone. - if ( - !content.classList.contains('popover-scroll-content') && - !content.classList.contains('popover-wheel-scroll') - ) { - return - } - - // Why resolve rather than use currentTarget: a popover whose content is a flex - // column with a nested viewport (a ScrollArea above a pinned footer) is itself - // overflow-hidden, so the scroller is a descendant under the pointer. - const el = resolvePopoverScroller(event.target, content) - if (!el) { - return - } - - const delta = - event.deltaMode === WheelEvent.DOM_DELTA_LINE - ? event.deltaY * 16 - : event.deltaMode === WheelEvent.DOM_DELTA_PAGE - ? event.deltaY * el.clientHeight - : event.deltaY - const maxScrollTop = el.scrollHeight - el.clientHeight - const nextScrollTop = Math.max(0, Math.min(maxScrollTop, el.scrollTop + delta)) - - // Why: issue drawers are Radix dialogs with scroll-lock. These popovers - // are portaled outside the dialog subtree, so native wheel scrolling is - // swallowed even though the scrollbar can be dragged. - if (nextScrollTop !== el.scrollTop) { - event.preventDefault() - el.scrollTop = nextScrollTop - } - } - - content.addEventListener('wheel', handleWheel, { passive: false }) - return () => content.removeEventListener('wheel', handleWheel) - }, [content]) - return (