mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
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.
This commit is contained in:
@@ -77,18 +77,22 @@ describe('PopoverContent wheel shim', () => {
|
||||
renderPopover('popover-scroll-content', true)
|
||||
const content = document.querySelector<HTMLElement>('[data-slot="popover-content"]')!
|
||||
const viewport = document.querySelector<HTMLElement>('[data-testid="viewport"]')!
|
||||
makeScrollable(content, 400, 400)
|
||||
makeScrollable(content, 800, 400)
|
||||
makeScrollable(viewport, 1000, 400)
|
||||
|
||||
wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[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<HTMLElement>('[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<HTMLElement>('[data-slot="popover-content"]')!
|
||||
const viewport = document.querySelector<HTMLElement>('[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<HTMLElement>('[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<HTMLElement>('[data-slot="popover-content"]')!
|
||||
const viewport = document.querySelector<HTMLElement>('[data-testid="viewport"]')!
|
||||
content.style.overflowY = 'hidden'
|
||||
viewport.style.overflowY = 'hidden'
|
||||
makeScrollable(content, 1000, 400)
|
||||
makeScrollable(viewport, 1000, 400)
|
||||
|
||||
wheel(document.querySelector<HTMLElement>('[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<HTMLElement>('[data-slot="popover-content"]')!
|
||||
const viewport = document.querySelector<HTMLElement>('[data-testid="viewport"]')!
|
||||
viewport.style.overflowY = 'auto'
|
||||
makeScrollable(content, 400, 400)
|
||||
makeScrollable(viewport, 1000, 400)
|
||||
|
||||
wheel(document.querySelector<HTMLElement>('[data-testid="inner"]')!, 120)
|
||||
flushFrames()
|
||||
|
||||
expect(viewport.scrollTop).toBe(120)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -70,10 +70,12 @@ export function WorkspaceCleanupFilterBar({
|
||||
<PopoverContent
|
||||
align="end"
|
||||
sideOffset={6}
|
||||
// Why popover-scroll-content: this popover portals outside the dialog subtree,
|
||||
// so react-remove-scroll's scroll-lock cancels wheel here and only the
|
||||
// scrollbar drag worked. The class opts into the wheel shim.
|
||||
className="popover-scroll-content flex h-[min(471px,var(--radix-popover-content-available-height))] w-[320px] flex-col p-0"
|
||||
// Why popover-wheel-scroll and not popover-scroll-content: this popover portals
|
||||
// outside the dialog subtree, so react-remove-scroll cancels wheel here and only
|
||||
// the scrollbar drag worked. It needs the shim but not the styling half --
|
||||
// popover-scroll-content caps height at 15rem, which would crush this panel's
|
||||
// 471px flex column.
|
||||
className="popover-wheel-scroll flex h-[min(471px,var(--radix-popover-content-available-height))] w-[320px] flex-col p-0"
|
||||
>
|
||||
{/* 471px preserves the 420px facet viewport plus the fixed footer at full height. */}
|
||||
<ScrollArea className="min-h-0 flex-1">
|
||||
|
||||
Reference in New Issue
Block a user