Skip to content

Commit 3e96d81

Browse files
localai-botmudler
andauthored
fix(ui): keep row action menu anchored and stop scroll snap on /app/manage (#10419)
Opening a model row's kebab (ActionMenu) on the Manage dashboard snapped the page scroll to the top and rendered the menu detached from its trigger, making it impossible to operate. Two compounding causes: - The menu auto-focus called el.focus() without preventScroll, so the browser scrolled the focused element into view, yanking the page to the top. - The position:fixed Popover was rendered inline inside the table row. The editorial UI overhaul added hover transforms to rows/cards, and a transformed ancestor re-anchors position:fixed to itself instead of the viewport, so the menu (positioned from the trigger's viewport rect) landed in the wrong place. Fix: portal the Popover to document.body so position:fixed always resolves against the viewport, position it before paint with useLayoutEffect (no {0,0} flash), and pass preventScroll:true to both focus calls. Adds an e2e regression test that reproduces the symptom (scroll jumped from 564 to 0 on the old code) and asserts the menu tracks its trigger. Assisted-by: Claude:claude-opus-4-8 [Claude Code] Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
1 parent 23f2252 commit 3e96d81

3 files changed

Lines changed: 70 additions & 7 deletions

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import { test, expect } from './coverage-fixtures.js'
2+
3+
// Regression: opening a row's kebab (ActionMenu) on /app/manage used to snap
4+
// the page scroll to the top and render the menu detached from its trigger,
5+
// making it impossible to operate. Two causes: the menu auto-focus scrolled
6+
// the page (no preventScroll), and the position:fixed popover was rendered
7+
// inside a row whose hover `transform` re-anchored it. Fix portals the popover
8+
// to document.body, positions it before paint, and focuses without scrolling.
9+
test.describe('Manage Page - Action menu positioning', () => {
10+
test('opening a row menu keeps scroll stable and places the menu by its trigger', async ({ page }) => {
11+
// Small viewport so the page is scrollable and a scroll jump is observable.
12+
await page.setViewportSize({ width: 1024, height: 500 })
13+
await page.goto('/app/manage')
14+
await expect(page.locator('.table')).toBeVisible({ timeout: 10_000 })
15+
16+
const trigger = page.locator('button.action-menu__trigger').first()
17+
await expect(trigger).toBeVisible()
18+
19+
// Bring the trigger into view ourselves first, so the only scroll we then
20+
// measure is the one the menu would (wrongly) cause - not Playwright's own
21+
// scroll-into-view before the click.
22+
await trigger.scrollIntoViewIfNeeded()
23+
const scrollBefore = await page.evaluate(() => window.scrollY)
24+
await trigger.click()
25+
26+
const menu = page.locator('[role="menu"]')
27+
await expect(menu).toBeVisible()
28+
29+
// Behavioural symptom 1: focusing the menu must not yank the page scroll.
30+
const scrollAfter = await page.evaluate(() => window.scrollY)
31+
expect(scrollAfter).toBe(scrollBefore)
32+
33+
// Behavioural symptom 2: the menu must sit next to its trigger, not float
34+
// at the top of the window where it can't be operated.
35+
const triggerBox = await trigger.boundingBox()
36+
const menuBox = await menu.boundingBox()
37+
expect(triggerBox).not.toBeNull()
38+
expect(menuBox).not.toBeNull()
39+
// Menu top is within ~24px of the trigger's bottom (below) or above it
40+
// (flipped) — in all cases it tracks the trigger, never floating at y≈0.
41+
const tracksTrigger =
42+
Math.abs(menuBox.y - (triggerBox.y + triggerBox.height)) < 24 ||
43+
Math.abs((menuBox.y + menuBox.height) - triggerBox.y) < 24
44+
expect(tracksTrigger).toBe(true)
45+
46+
// Mechanism: the popover must be portaled to document.body so position:fixed
47+
// resolves against the viewport, not a transformed ancestor row.
48+
await expect(page.locator('body > .popover')).toHaveCount(1)
49+
})
50+
})

core/http/react-ui/src/components/ActionMenu.jsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,11 @@ export default function ActionMenu({ items, ariaLabel = 'Actions', triggerLabel,
9595
className="action-menu"
9696
onKeyDown={handleMenuKeyDown}
9797
// Capture focus when the menu opens so arrow keys work without the
98-
// user clicking inside first.
98+
// user clicking inside first. preventScroll: the popover is portaled
99+
// and positioned by the trigger rect, so focusing it must not scroll
100+
// the page (that yanked the view to the top before it was placed).
99101
tabIndex={-1}
100-
ref={el => { if (el && open) el.focus() }}
102+
ref={el => { if (el && open) el.focus({ preventScroll: true }) }}
101103
>
102104
{visible.map((item, i) => {
103105
if (item.divider) {

core/http/react-ui/src/components/Popover.jsx

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
1-
import { useEffect, useRef, useState, useCallback } from 'react'
1+
import { useEffect, useLayoutEffect, useRef, useState, useCallback } from 'react'
2+
import { createPortal } from 'react-dom'
23

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

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

7383
if (!open) return null
7484

75-
return (
85+
return createPortal(
7686
<div
7787
ref={popoverRef}
7888
role="dialog"
@@ -81,6 +91,7 @@ export default function Popover({ anchor, open, onClose, children, ariaLabel })
8191
style={{ top: pos.top, left: pos.left }}
8292
>
8393
{children}
84-
</div>
94+
</div>,
95+
document.body
8596
)
8697
}

0 commit comments

Comments
 (0)