From 0aa5ac0e6d49cb23ff75fcb9f5776395c0100d0b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:42:09 -0700 Subject: [PATCH] fix(popover): use non-passive wheel listener --- .../cmd-j/PaletteFilterFieldOptions.tsx | 2 +- .../components/cmd-j/PaletteFilterMenu.tsx | 4 +- .../src/components/ui/popover-content-ref.ts | 44 -------------- .../ui/popover-wheel-scroll.test.tsx | 47 +++++---------- .../src/components/ui/popover.test.tsx | 56 ------------------ src/renderer/src/components/ui/popover.tsx | 59 +++++++++---------- 6 files changed, 48 insertions(+), 164 deletions(-) delete mode 100644 src/renderer/src/components/ui/popover-content-ref.ts delete mode 100644 src/renderer/src/components/ui/popover.test.tsx diff --git a/src/renderer/src/components/cmd-j/PaletteFilterFieldOptions.tsx b/src/renderer/src/components/cmd-j/PaletteFilterFieldOptions.tsx index cfea2c37f67..95372319a33 100644 --- a/src/renderer/src/components/cmd-j/PaletteFilterFieldOptions.tsx +++ b/src/renderer/src/components/cmd-j/PaletteFilterFieldOptions.tsx @@ -275,7 +275,7 @@ export function PaletteFilterFieldOptions({ aria-label={group.heading} tabIndex={-1} onKeyDown={handleListKeyDown} - className="popover-scroll-content scrollbar-sleek overflow-y-auto py-1" + className="scrollbar-sleek overflow-y-auto py-1" style={{ maxHeight: FILTER_OPTION_LIST_MAX_HEIGHT }} >
diff --git a/src/renderer/src/components/cmd-j/PaletteFilterMenu.tsx b/src/renderer/src/components/cmd-j/PaletteFilterMenu.tsx index d352363e630..d9f8b77ac7a 100644 --- a/src/renderer/src/components/cmd-j/PaletteFilterMenu.tsx +++ b/src/renderer/src/components/cmd-j/PaletteFilterMenu.tsx @@ -25,7 +25,7 @@ function CategoryRoot({ onOpenField: (field: PaletteFilterField) => void }): React.JSX.Element { return ( - + {groups.map((group) => { const selectedCount = group.selected.length @@ -183,7 +183,7 @@ export default function PaletteFilterMenu({ collisionBoundary={portalContainer ?? undefined} onKeyDown={handleKeyDown} onCloseAutoFocus={handleCloseAutoFocus} - className="w-[290px] p-0" + className="popover-wheel-scroll w-[290px] p-0" > {activeGroup == null ? ( diff --git a/src/renderer/src/components/ui/popover-content-ref.ts b/src/renderer/src/components/ui/popover-content-ref.ts deleted file mode 100644 index cbdf783471d..00000000000 --- a/src/renderer/src/components/ui/popover-content-ref.ts +++ /dev/null @@ -1,44 +0,0 @@ -import * as React from 'react' -import type { Popover as PopoverPrimitive } from 'radix-ui' - -type PopoverContentRef = React.ComponentProps['ref'] - -export function updatePopoverContentRef( - forwardedRef: PopoverContentRef | undefined, - node: HTMLDivElement | null, - cancelWheelFrames: () => void -): (() => void) | undefined { - if (node === null) { - cancelWheelFrames() - if (typeof forwardedRef === 'function') { - forwardedRef(null) - } else if (forwardedRef) { - forwardedRef.current = null - } - return undefined - } - - if (typeof forwardedRef === 'function') { - const cleanup = forwardedRef(node) - // Why: React callback refs may return cleanup; wrapping the Radix ref must - // preserve that cleanup instead of replacing it with a null callback. - return () => { - cancelWheelFrames() - if (typeof cleanup === 'function') { - cleanup() - } else { - forwardedRef(null) - } - } - } - - if (forwardedRef) { - forwardedRef.current = node - } - return () => { - cancelWheelFrames() - if (forwardedRef) { - forwardedRef.current = null - } - } -} 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 c369d3d2d2c..dd714241445 100644 --- a/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx +++ b/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx @@ -13,10 +13,12 @@ function makeScrollable(el: HTMLElement, scrollHeight: number, clientHeight: num Object.defineProperty(el, 'clientHeight', { value: clientHeight, configurable: true }) } -function wheel(el: HTMLElement, deltaY: number): void { +function wheel(el: HTMLElement, deltaY: number): WheelEvent { + const event = new WheelEvent('wheel', { deltaY, bubbles: true, cancelable: true }) act(() => { - el.dispatchEvent(new WheelEvent('wheel', { deltaY, bubbles: true, cancelable: true })) + el.dispatchEvent(event) }) + return event } function renderPopover(contentClassName: string, nested: boolean): void { @@ -42,17 +44,8 @@ function renderPopover(contentClassName: string, nested: boolean): void { } describe('PopoverContent wheel shim', () => { - let rafCallbacks: FrameRequestCallback[] - beforeEach(() => { root = null - // The shim defers the scroll write to rAF; capture and flush it explicitly. - rafCallbacks = [] - vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => { - rafCallbacks.push(cb) - return rafCallbacks.length - }) - vi.stubGlobal('cancelAnimationFrame', () => {}) }) afterEach(() => { @@ -63,14 +56,6 @@ describe('PopoverContent wheel shim', () => { vi.unstubAllGlobals() }) - const flushFrames = (): void => { - const pending = rafCallbacks - rafCallbacks = [] - for (const cb of pending) { - cb(0) - } - } - it('scrolls a nested viewport when the content itself cannot scroll', () => { // The workspace-cleanup Filters panel: a flex column whose PopoverContent is // overflow-hidden, with a ScrollArea viewport above a pinned footer. @@ -80,11 +65,11 @@ describe('PopoverContent wheel shim', () => { makeScrollable(content, 800, 400) makeScrollable(viewport, 1000, 400) - wheel(document.querySelector('[data-testid="inner"]')!, 120) - flushFrames() + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) expect(viewport.scrollTop).toBe(120) expect(content.scrollTop).toBe(0) + expect(event.defaultPrevented).toBe(true) }) it('still scrolls the content itself when it is the scroller', () => { @@ -95,10 +80,10 @@ describe('PopoverContent wheel shim', () => { content.style.overflowY = 'auto' makeScrollable(content, 1000, 400) - wheel(document.querySelector('[data-testid="inner"]')!, 120) - flushFrames() + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) expect(content.scrollTop).toBe(120) + expect(event.defaultPrevented).toBe(true) }) it('leaves popovers that did not opt in alone', () => { @@ -106,10 +91,10 @@ describe('PopoverContent wheel shim', () => { const viewport = document.querySelector('[data-testid="viewport"]')! makeScrollable(viewport, 1000, 400) - wheel(document.querySelector('[data-testid="inner"]')!, 120) - flushFrames() + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) expect(viewport.scrollTop).toBe(0) + expect(event.defaultPrevented).toBe(false) }) it('ignores vertically clipped elements and horizontal-only scrollers', () => { @@ -124,11 +109,11 @@ describe('PopoverContent wheel shim', () => { Object.defineProperty(viewport, 'scrollWidth', { value: 1000, configurable: true }) Object.defineProperty(viewport, 'clientWidth', { value: 400, configurable: true }) - wheel(document.querySelector('[data-testid="inner"]')!, 120) - flushFrames() + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) expect(content.scrollTop).toBe(0) expect(viewport.scrollTop).toBe(0) + expect(event.defaultPrevented).toBe(false) }) it('does nothing when no element in the target chain can scroll', () => { @@ -140,11 +125,11 @@ describe('PopoverContent wheel shim', () => { makeScrollable(content, 1000, 400) makeScrollable(viewport, 1000, 400) - wheel(document.querySelector('[data-testid="inner"]')!, 120) - flushFrames() + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) expect(content.scrollTop).toBe(0) expect(viewport.scrollTop).toBe(0) + expect(event.defaultPrevented).toBe(false) }) it('runs for the shim-only marker, which carries no styling', () => { @@ -157,9 +142,9 @@ describe('PopoverContent wheel shim', () => { makeScrollable(content, 400, 400) makeScrollable(viewport, 1000, 400) - wheel(document.querySelector('[data-testid="inner"]')!, 120) - flushFrames() + const event = wheel(document.querySelector('[data-testid="inner"]')!, 120) expect(viewport.scrollTop).toBe(120) + expect(event.defaultPrevented).toBe(true) }) }) diff --git a/src/renderer/src/components/ui/popover.test.tsx b/src/renderer/src/components/ui/popover.test.tsx deleted file mode 100644 index 3d84a1f8e33..00000000000 --- a/src/renderer/src/components/ui/popover.test.tsx +++ /dev/null @@ -1,56 +0,0 @@ -import { describe, expect, it, vi } from 'vitest' -import { updatePopoverContentRef } from './popover-content-ref' - -describe('updatePopoverContentRef', () => { - it('preserves callback ref cleanup when content detaches', () => { - const node = {} as HTMLDivElement - const cancelWheelFrames = vi.fn() - const cleanup = vi.fn() - const ref = vi.fn(() => cleanup) - - const detach = updatePopoverContentRef(ref, node, cancelWheelFrames) - detach?.() - - expect(ref).toHaveBeenCalledTimes(1) - expect(ref).toHaveBeenCalledWith(node) - expect(cancelWheelFrames).toHaveBeenCalledTimes(1) - expect(cleanup).toHaveBeenCalledTimes(1) - }) - - it('clears callback refs that do not return cleanup', () => { - const node = {} as HTMLDivElement - const cancelWheelFrames = vi.fn() - const ref = vi.fn() - - const detach = updatePopoverContentRef(ref, node, cancelWheelFrames) - detach?.() - - expect(ref).toHaveBeenNthCalledWith(1, node) - expect(ref).toHaveBeenNthCalledWith(2, null) - expect(cancelWheelFrames).toHaveBeenCalledTimes(1) - }) - - it('clears object refs when content detaches', () => { - const node = {} as HTMLDivElement - const cancelWheelFrames = vi.fn() - const ref: { current: HTMLDivElement | null } = { current: null } - - const detach = updatePopoverContentRef(ref, node, cancelWheelFrames) - expect(ref.current).toBe(node) - - detach?.() - - expect(ref.current).toBeNull() - expect(cancelWheelFrames).toHaveBeenCalledTimes(1) - }) - - it('cancels pending frames when React supplies a null node', () => { - const cancelWheelFrames = vi.fn() - const ref: { current: HTMLDivElement | null } = { current: {} as HTMLDivElement } - - updatePopoverContentRef(ref, null, cancelWheelFrames) - - expect(ref.current).toBeNull() - expect(cancelWheelFrames).toHaveBeenCalledTimes(1) - }) -}) diff --git a/src/renderer/src/components/ui/popover.tsx b/src/renderer/src/components/ui/popover.tsx index f76bc2aee8a..c8386daec51 100644 --- a/src/renderer/src/components/ui/popover.tsx +++ b/src/renderer/src/components/ui/popover.tsx @@ -4,7 +4,6 @@ import * as React from 'react' import { Popover as PopoverPrimitive } from 'radix-ui' import { cn } from '@/lib/utils' -import { updatePopoverContentRef } from './popover-content-ref' function Popover(props: React.ComponentProps) { return @@ -48,32 +47,37 @@ function PopoverContent({ }: React.ComponentProps & { portalContainer?: HTMLElement | null }) { - const wheelFrameIdsRef = React.useRef>(new Set()) - - const cancelWheelFrames = React.useCallback(() => { - for (const frameId of wheelFrameIdsRef.current) { - cancelAnimationFrame(frameId) - } - wheelFrameIdsRef.current.clear() - }, []) + const [content, setContent] = React.useState(null) const setContentRef = React.useCallback( (node: HTMLDivElement | null) => { - // Why: the wheel shim schedules frames against the content node; cancel - // them when Radix removes that node instead of from a passive Effect. - return updatePopoverContentRef(forwardedRef, node, cancelWheelFrames) + setContent(node) + if (typeof forwardedRef === 'function') { + const cleanup = forwardedRef(node) + if (node !== null && typeof cleanup === 'function') { + return () => { + setContent(null) + cleanup() + } + } + } else if (forwardedRef) { + forwardedRef.current = node + } + return undefined }, - [cancelWheelFrames, forwardedRef] + [forwardedRef] ) - const handleWheel = React.useCallback( - (event: React.WheelEvent) => { - onWheel?.(event) + React.useEffect(() => { + if (!content) { + return + } + + const handleWheel = (event: WheelEvent): void => { if (event.defaultPrevented) { return } - const content = event.currentTarget // 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 @@ -106,19 +110,14 @@ function PopoverContent({ // are portaled outside the dialog subtree, so native wheel scrolling is // swallowed even though the scrollbar can be dragged. if (nextScrollTop !== el.scrollTop) { - const previousScrollTop = el.scrollTop - event.stopPropagation() - const frameId = requestAnimationFrame(() => { - wheelFrameIdsRef.current.delete(frameId) - if (el.scrollTop === previousScrollTop) { - el.scrollTop = nextScrollTop - } - }) - wheelFrameIdsRef.current.add(frameId) + event.preventDefault() + el.scrollTop = nextScrollTop } - }, - [onWheel] - ) + } + + content.addEventListener('wheel', handleWheel, { passive: false }) + return () => content.removeEventListener('wheel', handleWheel) + }, [content]) return ( @@ -144,7 +143,7 @@ function PopoverContent({ WebkitAppRegion: 'no-drag' } as React.CSSProperties } - onWheel={handleWheel} + onWheel={onWheel} {...props} />