From 27ab459fe0b29e5bc6e4e0af6e3d204f62210ceb Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:33:32 -0700 Subject: [PATCH] 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 | 57 ++++++++++++++++++- src/renderer/src/components/ui/popover.tsx | 11 +++- .../workspace-cleanup-filter-bar.tsx | 10 ++-- 3 files changed, 71 insertions(+), 7 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 79a50f5b46c..c369d3d2d2c 100644 --- a/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx +++ b/src/renderer/src/components/ui/popover-wheel-scroll.test.tsx @@ -77,18 +77,22 @@ describe('PopoverContent wheel shim', () => { renderPopover('popover-scroll-content', true) const content = document.querySelector('[data-slot="popover-content"]')! const viewport = document.querySelector('[data-testid="viewport"]')! - makeScrollable(content, 400, 400) + 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) @@ -107,4 +111,55 @@ describe('PopoverContent wheel shim', () => { 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 449b50edeb3..f76bc2aee8a 100644 --- a/src/renderer/src/components/ui/popover.tsx +++ b/src/renderer/src/components/ui/popover.tsx @@ -33,7 +33,7 @@ function resolvePopoverScroller(target: EventTarget | null, content: HTMLElement } node = node.parentNode } - return content.scrollHeight > content.clientHeight ? content : null + return null } function PopoverContent({ @@ -74,7 +74,14 @@ function PopoverContent({ } const content = event.currentTarget - if (!content.classList.contains('popover-scroll-content')) { + // 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 } 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 cedfd293516..53e732fb3de 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 @@ -70,10 +70,12 @@ export function WorkspaceCleanupFilterBar({ {/* 471px preserves the 420px facet viewport plus the fixed footer at full height. */}