fix(popover): use non-passive wheel listener

This commit is contained in:
Brennan Benson
2026-08-24 14:42:09 -07:00
parent b085e67c35
commit 0aa5ac0e6d
6 changed files with 48 additions and 164 deletions
@@ -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 }}
>
<div className="relative w-full" style={{ height: `${virtualizer.getTotalSize()}px` }}>
@@ -25,7 +25,7 @@ function CategoryRoot({
onOpenField: (field: PaletteFilterField) => void
}): React.JSX.Element {
return (
<CommandList className="popover-scroll-content scrollbar-sleek max-h-[280px] py-1">
<CommandList className="scrollbar-sleek max-h-[280px] py-1">
<CommandGroup>
{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"
>
<Command shouldFilter={false} className="bg-transparent">
{activeGroup == null ? (
@@ -1,44 +0,0 @@
import * as React from 'react'
import type { Popover as PopoverPrimitive } from 'radix-ui'
type PopoverContentRef = React.ComponentProps<typeof PopoverPrimitive.Content>['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
}
}
}
@@ -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<HTMLElement>('[data-testid="inner"]')!, 120)
flushFrames()
const event = wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[data-testid="inner"]')!, 120)
flushFrames()
const event = wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[data-testid="viewport"]')!
makeScrollable(viewport, 1000, 400)
wheel(document.querySelector<HTMLElement>('[data-testid="inner"]')!, 120)
flushFrames()
const event = wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[data-testid="inner"]')!, 120)
flushFrames()
const event = wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[data-testid="inner"]')!, 120)
flushFrames()
const event = wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[data-testid="inner"]')!, 120)
flushFrames()
const event = wheel(document.querySelector<HTMLElement>('[data-testid="inner"]')!, 120)
expect(viewport.scrollTop).toBe(120)
expect(event.defaultPrevented).toBe(true)
})
})
@@ -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)
})
})
+29 -30
View File
@@ -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<typeof PopoverPrimitive.Root>) {
return <PopoverPrimitive.Root data-slot="popover" {...props} />
@@ -48,32 +47,37 @@ function PopoverContent({
}: React.ComponentProps<typeof PopoverPrimitive.Content> & {
portalContainer?: HTMLElement | null
}) {
const wheelFrameIdsRef = React.useRef<Set<number>>(new Set())
const cancelWheelFrames = React.useCallback(() => {
for (const frameId of wheelFrameIdsRef.current) {
cancelAnimationFrame(frameId)
}
wheelFrameIdsRef.current.clear()
}, [])
const [content, setContent] = React.useState<HTMLDivElement | null>(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<HTMLDivElement>) => {
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 (
<PopoverPrimitive.Portal container={portalContainer ?? undefined}>
@@ -144,7 +143,7 @@ function PopoverContent({
WebkitAppRegion: 'no-drag'
} as React.CSSProperties
}
onWheel={handleWheel}
onWheel={onWheel}
{...props}
/>
</PopoverPrimitive.Portal>