Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions core/http/react-ui/e2e/manage-action-menu-position.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { test, expect } from './coverage-fixtures.js'

// Regression: opening a row's kebab (ActionMenu) on /app/manage used to snap
// the page scroll to the top and render the menu detached from its trigger,
// making it impossible to operate. Two causes: the menu auto-focus scrolled
// the page (no preventScroll), and the position:fixed popover was rendered
// inside a row whose hover `transform` re-anchored it. Fix portals the popover
// to document.body, positions it before paint, and focuses without scrolling.
test.describe('Manage Page - Action menu positioning', () => {
test('opening a row menu keeps scroll stable and places the menu by its trigger', async ({ page }) => {
// Small viewport so the page is scrollable and a scroll jump is observable.
await page.setViewportSize({ width: 1024, height: 500 })
await page.goto('/app/manage')
await expect(page.locator('.table')).toBeVisible({ timeout: 10_000 })

const trigger = page.locator('button.action-menu__trigger').first()
await expect(trigger).toBeVisible()

// Bring the trigger into view ourselves first, so the only scroll we then
// measure is the one the menu would (wrongly) cause - not Playwright's own
// scroll-into-view before the click.
await trigger.scrollIntoViewIfNeeded()
const scrollBefore = await page.evaluate(() => window.scrollY)
await trigger.click()

const menu = page.locator('[role="menu"]')
await expect(menu).toBeVisible()

// Behavioural symptom 1: focusing the menu must not yank the page scroll.
const scrollAfter = await page.evaluate(() => window.scrollY)
expect(scrollAfter).toBe(scrollBefore)

// Behavioural symptom 2: the menu must sit next to its trigger, not float
// at the top of the window where it can't be operated.
const triggerBox = await trigger.boundingBox()
const menuBox = await menu.boundingBox()
expect(triggerBox).not.toBeNull()
expect(menuBox).not.toBeNull()
// Menu top is within ~24px of the trigger's bottom (below) or above it
// (flipped) — in all cases it tracks the trigger, never floating at y≈0.
const tracksTrigger =
Math.abs(menuBox.y - (triggerBox.y + triggerBox.height)) < 24 ||
Math.abs((menuBox.y + menuBox.height) - triggerBox.y) < 24
expect(tracksTrigger).toBe(true)

// Mechanism: the popover must be portaled to document.body so position:fixed
// resolves against the viewport, not a transformed ancestor row.
await expect(page.locator('body > .popover')).toHaveCount(1)
})
})
6 changes: 4 additions & 2 deletions core/http/react-ui/src/components/ActionMenu.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,11 @@ export default function ActionMenu({ items, ariaLabel = 'Actions', triggerLabel,
className="action-menu"
onKeyDown={handleMenuKeyDown}
// Capture focus when the menu opens so arrow keys work without the
// user clicking inside first.
// user clicking inside first. preventScroll: the popover is portaled
// and positioned by the trigger rect, so focusing it must not scroll
// the page (that yanked the view to the top before it was placed).
tabIndex={-1}
ref={el => { if (el && open) el.focus() }}
ref={el => { if (el && open) el.focus({ preventScroll: true }) }}
>
{visible.map((item, i) => {
if (item.divider) {
Expand Down
21 changes: 16 additions & 5 deletions core/http/react-ui/src/components/Popover.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import { useEffect, useRef, useState, useCallback } from 'react'
import { useEffect, useLayoutEffect, useRef, useState, useCallback } from 'react'
import { createPortal } from 'react-dom'

// Minimal popover: positions itself below-right of the trigger's bounding box,
// flips above when there isn't room below, closes on outside click or Escape,
// returns focus to the trigger. Uses the existing .card surface so it picks
// up theme/border/shadow automatically — no new theming work.
//
// Rendered through a portal on document.body: the popover is position:fixed and
// positioned from the trigger's viewport rect, so it must escape any ancestor
// that establishes a containing block (a row/card with a hover `transform`
// would otherwise re-anchor `position:fixed` to itself, throwing the menu to
// the wrong spot and making it unusable).
//
// Props:
// anchor: ref to the trigger DOMElement (required)
// open: boolean
Expand All @@ -30,7 +37,9 @@ export default function Popover({ anchor, open, onClose, children, ariaLabel })
setPos({ top, left: Math.max(8, left), flipped })
}, [anchor])

useEffect(() => {
// useLayoutEffect so we measure + place the popover before the browser
// paints — otherwise it flashes at its initial {0,0} for a frame.
useLayoutEffect(() => {
if (!open) return
reposition()
window.addEventListener('resize', reposition)
Expand Down Expand Up @@ -65,14 +74,15 @@ export default function Popover({ anchor, open, onClose, children, ariaLabel })
if (!open && anchor?.current) {
// requestAnimationFrame so the close is painted before focus jumps;
// otherwise screen readers announce the trigger mid-transition.
const raf = requestAnimationFrame(() => anchor.current?.focus?.())
// preventScroll: focusing the trigger must not yank the page scroll.
const raf = requestAnimationFrame(() => anchor.current?.focus?.({ preventScroll: true }))
return () => cancelAnimationFrame(raf)
}
}, [open, anchor])

if (!open) return null

return (
return createPortal(
<div
ref={popoverRef}
role="dialog"
Expand All @@ -81,6 +91,7 @@ export default function Popover({ anchor, open, onClose, children, ariaLabel })
style={{ top: pos.top, left: pos.left }}
>
{children}
</div>
</div>,
document.body
)
}
Loading