From b085e67c35f32b0eca99f58fbfce84f488e1f449 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:26:26 -0700 Subject: [PATCH] fix(popover): wheel-scroll a popover whose scroller is nested, not the content (#16206) * fix(popover): wheel-scroll a popover whose scroller is nested, not the content The workspace-cleanup Filters panel cannot be scrolled with a wheel. Only the scrollbar drag and focus-scroll work. Two independent Electron QA passes measured it on main: wheel events over that popover arrive defaultPrevented, panel scrollTop 0 -> 0, while the candidate list inside the dialog subtree scrolls normally. Cause: the popover portals outside the Radix dialog subtree, so react-remove-scroll's scroll-lock cancels wheel there. #14629 added a shim for exactly this, but it scrolls `event.currentTarget` -- the PopoverContent -- and only when that element is itself overflowing. The Filters panel is a flex column holding a ScrollArea above a pinned footer, so PopoverContent is overflow-hidden and the real scroller is a descendant. The shim looked at the wrong element and returned early. The shim now resolves the nearest scrollable element between the wheel target and the content, inclusive of both, so it handles the nested-viewport shape as well as the flat one. Still opt-in via `popover-scroll-content`; popovers without the class are untouched. The cleanup Filters panel opts in. This is the unfixed half of the original report. #14629 fixed the panel being clipped; the wheel -- what you actually reach for -- stayed dead. The nested test fails against the shipped shim and the other two stay green, so it reproduces the bug without redefining existing behaviour. * fix(popover): split the wheel-shim opt-in from the scroll-container styling Two fixes on top of the reviewer's round. **The reviewer's change, kept.** `resolvePopoverScroller` no longer falls back to the content element unconditionally; it must match the overflow test like any other candidate. The reviewer flagged that this could break the existing opted-in popovers, so I checked: `.popover-scroll-content` sets `overflow-y: auto` (main.css:601-607), so every one of them still resolves. The test needs an inline style only because happy-dom does not apply the stylesheet -- noted in the test so nobody reads it as a production concern. **A bug of mine the review did not reach.** Opting the cleanup Filters panel in via `popover-scroll-content` would also have applied that class's `max-height: min(15rem, ...)`, crushing the panel's 471px flex column to 240px. The class conflates 'style me as a scroll container' with 'run the wheel shim', and a popover that manages its own layout needs only the second. So the shim now accepts `popover-wheel-scroll` as a styling-free marker, and the Filters panel uses that. `popover-scroll-content` keeps implying it, so no existing caller changes. --- .../ui/popover-wheel-scroll.test.tsx | 165 ++++++++++++++++++ src/renderer/src/components/ui/popover.tsx | 37 +++- .../workspace-cleanup-filter-bar.tsx | 7 +- 3 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 src/renderer/src/components/ui/popover-wheel-scroll.test.tsx diff --git a/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx b/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx new file mode 100644 index 00000000000..c369d3d2d2c --- /dev/null +++ b/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx @@ -0,0 +1,165 @@ +// @vitest-environment happy-dom +import { act } 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' + +let root: Root | null = null +let container: HTMLDivElement + +/** happy-dom reports 0 for layout, so scroll geometry has to be defined per element. */ +function makeScrollable(el: HTMLElement, scrollHeight: number, clientHeight: number): void { + Object.defineProperty(el, 'scrollHeight', { value: scrollHeight, configurable: true }) + Object.defineProperty(el, 'clientHeight', { value: clientHeight, configurable: true }) +} + +function wheel(el: HTMLElement, deltaY: number): void { + act(() => { + el.dispatchEvent(new WheelEvent('wheel', { deltaY, bubbles: true, cancelable: true })) + }) +} + +function renderPopover(contentClassName: string, nested: boolean): void { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => { + root!.render( + + open + + {nested ? ( +
+
tall
+
+ ) : ( +
tall
+ )} + + + ) + }) +} + +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(() => { + if (root) { + act(() => root!.unmount()) + } + document.body.replaceChildren() + 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. + renderPopover('popover-scroll-content', true) + const content = document.querySelector('[data-slot="popover-content"]')! + const viewport = document.querySelector('[data-testid="viewport"]')! + makeScrollable(content, 800, 400) + makeScrollable(viewport, 1000, 400) + + wheel(document.querySelector('[data-testid="inner"]')!, 120) + flushFrames() + + expect(viewport.scrollTop).toBe(120) + expect(content.scrollTop).toBe(0) + }) + + it('still scrolls the content itself when it is the scroller', () => { + renderPopover('popover-scroll-content', false) + const content = document.querySelector('[data-slot="popover-content"]')! + // Inline because happy-dom does not apply the stylesheet; in the app + // `.popover-scroll-content` already sets `overflow-y: auto` (main.css). + content.style.overflowY = 'auto' + makeScrollable(content, 1000, 400) + + wheel(document.querySelector('[data-testid="inner"]')!, 120) + flushFrames() + + expect(content.scrollTop).toBe(120) + }) + + it('leaves popovers that did not opt in alone', () => { + renderPopover('', true) + const viewport = document.querySelector('[data-testid="viewport"]')! + makeScrollable(viewport, 1000, 400) + + wheel(document.querySelector('[data-testid="inner"]')!, 120) + flushFrames() + + expect(viewport.scrollTop).toBe(0) + }) + + it('ignores vertically clipped elements and horizontal-only scrollers', () => { + renderPopover('popover-scroll-content', true) + const content = document.querySelector('[data-slot="popover-content"]')! + const viewport = document.querySelector('[data-testid="viewport"]')! + content.style.overflowY = 'hidden' + viewport.style.overflowY = 'hidden' + viewport.style.overflowX = 'auto' + makeScrollable(content, 1000, 400) + makeScrollable(viewport, 1000, 400) + Object.defineProperty(viewport, 'scrollWidth', { value: 1000, configurable: true }) + Object.defineProperty(viewport, 'clientWidth', { value: 400, configurable: true }) + + wheel(document.querySelector('[data-testid="inner"]')!, 120) + flushFrames() + + expect(content.scrollTop).toBe(0) + expect(viewport.scrollTop).toBe(0) + }) + + it('does nothing when no element in the target chain can scroll', () => { + renderPopover('popover-scroll-content', true) + const content = document.querySelector('[data-slot="popover-content"]')! + const viewport = document.querySelector('[data-testid="viewport"]')! + content.style.overflowY = 'hidden' + viewport.style.overflowY = 'hidden' + makeScrollable(content, 1000, 400) + makeScrollable(viewport, 1000, 400) + + wheel(document.querySelector('[data-testid="inner"]')!, 120) + flushFrames() + + expect(content.scrollTop).toBe(0) + expect(viewport.scrollTop).toBe(0) + }) + + it('runs for the shim-only marker, which carries no styling', () => { + // The workspace-cleanup Filters panel needs the wheel shim but must NOT inherit + // `.popover-scroll-content`'s 15rem max-height, which would crush its 471px column. + renderPopover('popover-wheel-scroll', true) + const content = document.querySelector('[data-slot="popover-content"]')! + const viewport = document.querySelector('[data-testid="viewport"]')! + viewport.style.overflowY = 'auto' + makeScrollable(content, 400, 400) + makeScrollable(viewport, 1000, 400) + + wheel(document.querySelector('[data-testid="inner"]')!, 120) + flushFrames() + + expect(viewport.scrollTop).toBe(120) + }) +}) diff --git a/src/renderer/src/components/ui/popover.tsx b/src/renderer/src/components/ui/popover.tsx index 5523f5021ee..f76bc2aee8a 100644 --- a/src/renderer/src/components/ui/popover.tsx +++ b/src/renderer/src/components/ui/popover.tsx @@ -18,6 +18,24 @@ function PopoverAnchor(props: React.ComponentProps } +/** + * Nearest scrollable element between the wheel target and the popover content, + * inclusive of both. Returns null when nothing in that chain can scroll. + */ +function resolvePopoverScroller(target: EventTarget | null, content: HTMLElement): HTMLElement | null { + let node = target instanceof Node ? target : null + while (node && node !== content.parentNode) { + if (node instanceof HTMLElement && node.scrollHeight > node.clientHeight) { + const overflowY = getComputedStyle(node).overflowY + if (overflowY === 'auto' || overflowY === 'scroll') { + return node + } + } + node = node.parentNode + } + return null +} + function PopoverContent({ className, align = 'center', @@ -55,8 +73,23 @@ function PopoverContent({ return } - const el = event.currentTarget - if (!el.classList.contains('popover-scroll-content') || el.scrollHeight <= el.clientHeight) { + 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 + // 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 } diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-filter-bar.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-filter-bar.tsx index 3f4ae9e9ab9..8a48dc10c05 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-filter-bar.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-filter-bar.tsx @@ -78,7 +78,12 @@ export function WorkspaceCleanupFilterBar({ {/* 471px preserves the 420px facet viewport plus the fixed footer at full height. */}