From 59e9f5ea2bfafa13c54bc374ed16cb997748c806 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 22:30:31 +0100 Subject: [PATCH] Keep inline drawers from moving the viewport Shared subtabs used scrollIntoView for horizontal visibility, allowing remounted or programmatically changed drawers to scroll the whole application shell. Reuse the horizontal rail controller instead, and restore disclosure focus without scrolling when a detail row closes. Record the shared primitive contract and its registry-approved guardrails, with desktop and narrow Backups browser coverage for visible Overview and Manage states, off-screen tab changes, and non-scrolling focus return. Change-source: pulse-maintainer --- .../subsystems/frontend-primitives.md | 9 +- frontend-modern/browser-verification.json | 39 +++--- .../components/shared/FilterToolbar.test.tsx | 2 + .../shared/InlineDetailTableRow.tsx | 7 +- .../SharedPrimitives.guardrails.test.ts | 1 + .../src/components/shared/Subtabs.tsx | 16 ++- .../__tests__/InlineDetailTableRow.test.tsx | 2 + .../shared/__tests__/Subtabs.test.tsx | 23 +++- ...orkloads-proxmox-refresh-stability.spec.ts | 128 ++++++++++++++++++ 9 files changed, 193 insertions(+), 34 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index 78da33370..30148078b 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -4642,6 +4642,10 @@ container, reset the parent table's `whitespace-nowrap` inheritance, and allow its descendants to shrink, then restore visible overflow for the static desktop layout. Long operator-state copy must wrap inside the shared row border instead of painting beneath adjacent controls or disappearing at the clip edge. +When focused detail content is removed, `InlineDetailTableRow` restores focus +to its current `aria-controls` disclosure with `preventScroll`; live refresh, +collapse, and row replacement must not move the surrounding application +viewport merely to reveal that control. Inline detail section content is registry-backed separately from the row shell. `DetailSectionTable`, `InlineDetailPanel`, and `detailSectionModel.ts` own detail row compaction, section-table rendering, value-tone classes, and the @@ -4961,7 +4965,10 @@ already proven on owning surfaces like operations rather than introducing new variant APIs on the primitive. When that rail overflows on phone widths, `Subtabs` owns visible, accessible edge-scroll controls and keeps them in sync with native scrolling and rail resize; callers must not add drawer-local arrow -overlays or leave clipped tab labels as the only overflow cue. +overlays or leave clipped tab labels as the only overflow cue. Selection +changes reveal the active tab by moving only that horizontal rail through the +shared rail-visibility controller; `Subtabs` must not use `scrollIntoView`, +which can also move page and drawer ancestors vertically. The search-input enhancement surfaces now follow that same owner split. `frontend-modern/src/components/shared/SearchInputEnhancements.tsx` stays the render shell, `frontend-modern/src/components/shared/useSearchInputEnhancements.ts` diff --git a/frontend-modern/browser-verification.json b/frontend-modern/browser-verification.json index 5fb7266f5..c5ae5f224 100644 --- a/frontend-modern/browser-verification.json +++ b/frontend-modern/browser-verification.json @@ -1,41 +1,40 @@ { "version": 1, - "base_sha": "54d58a74062ffb15c14510a24223f21fa6ec2fd3", - "verified_at": "2026-09-03T20:15:00Z", + "base_sha": "93c765a7eca670c46d93684e37af76ea9232686d", + "verified_at": "2026-09-03T21:29:58Z", "result": "passed", "changed_paths": [ - "frontend-modern/src/components/Settings/InfrastructureWorkspace.tsx" + "frontend-modern/src/components/shared/InlineDetailTableRow.tsx", + "frontend-modern/src/components/shared/Subtabs.tsx" ], "content_sha256": { - "frontend-modern/src/components/Settings/InfrastructureWorkspace.tsx": "186324f5ede149574cebe2d758b4984077c9105c5ae926721e262cc26a5e2502" + "frontend-modern/src/components/shared/InlineDetailTableRow.tsx": "e3150f2bb67a6e4324bdc2b5cc6fa9915d4a17a059ace5a4571551169b203e36", + "frontend-modern/src/components/shared/Subtabs.tsx": "0f92b5a0be6904c717c6520ec9576beca02c3db4fb6a54b640d4513e8f3ba28b" }, "routes": [ - "/settings/infrastructure (managed local backend with deterministic Proxmox connection-ledger and node fixtures)" + "/proxmox/backups (managed local backend with deterministic Proxmox and PBS fixtures)" ], "viewports": [ { "width": 1280, - "height": 900 + "height": 720 }, { - "width": 390, - "height": 844 + "width": 393, + "height": 727 } ], "states": [ - "Manage Proxmox VE dialog with Username & Password selected and an unsaved username at desktop width", - "Desktop dialog after a 15-second connection-ledger poll replaced lastSeen and endpoint: live description updated, local endpoint and unsaved username remained unchanged", - "Manage Proxmox VE dialog with a distinct unsaved username at narrow width", - "Narrow dialog after a second 15-second ledger replacement: live description updated, local endpoint and unsaved username remained unchanged", - "Scrollable dialog body with visible header and close control, reachable Save changes footer, and no document-width overflow at both widths", - "Closed dialog with focus returned to the originating Manage button at both widths" + "Expanded PBS server row with Overview selected at desktop and narrow widths", + "Expanded PBS server row with Manage selected at desktop and narrow widths", + "Backup health content in view while the expanded drawer tab strip remained above the viewport", + "Collapsed PBS detail after focus restoration at desktop and narrow widths" ], "interactions": [ - "Opened Manage for a configured Proxmox VE source", - "Selected Username & Password and entered a non-secret unsaved username", - "Waited for the real 15-second ledger poll and observed endpoint/lastSeen replacement without form remount or value loss", - "Scrolled to and verified the Save changes footer remained reachable, then returned to the edited field", - "Pressed Escape and verified dialog dismissal and focus return to Manage", - "Repeated the complete edit, poll, scroll, dismissal, and focus-return journey at 390x844" + "Opened a PBS server detail row from the Backups table", + "Selected Manage and returned to Overview while the drawer was visible", + "Moved below the expanded drawer, selected Manage programmatically, and verified no vertical scrollIntoView request occurred", + "Closed the drawer from focused tab content and verified focus restoration used preventScroll", + "Inspected Overview, Manage, and below-drawer screenshots for placement, clipping, overflow, and fixed-navigation overlap at both widths" ] } diff --git a/frontend-modern/src/components/shared/FilterToolbar.test.tsx b/frontend-modern/src/components/shared/FilterToolbar.test.tsx index c4f367373..ac8df9afb 100644 --- a/frontend-modern/src/components/shared/FilterToolbar.test.tsx +++ b/frontend-modern/src/components/shared/FilterToolbar.test.tsx @@ -43,6 +43,8 @@ describe('FilterHeader', () => { expect(subtabsSource).toContain('scrollTabs(-1)'); expect(subtabsSource).toContain('scrollTabs(1)'); expect(subtabsSource).toContain('sm:hidden'); + expect(subtabsSource).toContain('useActiveHorizontalRailItemVisibility'); + expect(subtabsSource).not.toContain('.scrollIntoView'); }); it('keeps segmented controls on value callbacks while forwarding div attributes', async () => { diff --git a/frontend-modern/src/components/shared/InlineDetailTableRow.tsx b/frontend-modern/src/components/shared/InlineDetailTableRow.tsx index 01bce4a88..5aa7776f6 100644 --- a/frontend-modern/src/components/shared/InlineDetailTableRow.tsx +++ b/frontend-modern/src/components/shared/InlineDetailTableRow.tsx @@ -51,7 +51,12 @@ export function InlineDetailTableRow(props: InlineDetailTableRowProps) { const disclosure = Array.from( document.querySelectorAll('button[aria-controls]'), ).find((button) => button.getAttribute('aria-controls') === cellId); - disclosure?.focus(); + // Closing or replacing a detail row can happen while the surrounding + // live table is well below the top of the page. Restore keyboard focus + // without letting the browser scroll the disclosure into view: the + // caller owns any deliberate reveal, while polling must not move the + // operator's viewport. + disclosure?.focus({ preventScroll: true }); }); }); diff --git a/frontend-modern/src/components/shared/SharedPrimitives.guardrails.test.ts b/frontend-modern/src/components/shared/SharedPrimitives.guardrails.test.ts index 7bea85232..8296f22fe 100644 --- a/frontend-modern/src/components/shared/SharedPrimitives.guardrails.test.ts +++ b/frontend-modern/src/components/shared/SharedPrimitives.guardrails.test.ts @@ -7731,6 +7731,7 @@ describe('shared primitive guardrails', () => { expect(inlineDetailTableRowSource).toContain('min-w-0'); expect(inlineDetailTableRowSource).toContain('lg:overflow-x-visible'); expect(inlineDetailTableRowSource).toContain('event.stopPropagation()'); + expect(inlineDetailTableRowSource).toContain('.focus({ preventScroll: true })'); expect(registeredRule?.requiredConsumers?.map((consumer) => consumer.path)).toEqual( expect.arrayContaining([ 'src/components/Infrastructure/UnifiedResourceHostTableCard.tsx', diff --git a/frontend-modern/src/components/shared/Subtabs.tsx b/frontend-modern/src/components/shared/Subtabs.tsx index 577956365..bd4985031 100644 --- a/frontend-modern/src/components/shared/Subtabs.tsx +++ b/frontend-modern/src/components/shared/Subtabs.tsx @@ -11,6 +11,7 @@ import { Show, splitProps, } from 'solid-js'; +import { useActiveHorizontalRailItemVisibility } from './useActiveHorizontalRailItemVisibility'; export interface SubtabOption { value: string; @@ -58,15 +59,19 @@ export const Subtabs: Component = (props) => { 'tabClass', 'trailing', ]); + const activeItemVisibility = useActiveHorizontalRailItemVisibility({ + active: () => local.value, + rail: () => tablistRef, + activeSelector: '[role="tab"][aria-selected="true"]', + }); createEffect(() => { - // Keep the selected tab discoverable when a narrow, horizontally scrolling - // tablist is opened through a deep link or changed programmatically. + // Selection changes can alter the overflow controls after the shared rail + // helper has moved horizontally. Do not use scrollIntoView here: it also + // scrolls vertical ancestors and can pull an open inline drawer upwards + // when live data recreates its tab strip. void local.value; queueMicrotask(() => { - tablistRef - ?.querySelector('[role="tab"][aria-selected="true"]') - ?.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); updateScrollControls(); }); }); @@ -101,6 +106,7 @@ export const Subtabs: Component = (props) => { const scrollTabs = (direction: -1 | 1) => { const rail = tablistRef; if (!rail) return; + activeItemVisibility.markManualScrollIntent(); rail.scrollBy({ left: direction * Math.max(120, Math.round(rail.clientWidth * 0.7)), behavior: 'smooth', diff --git a/frontend-modern/src/components/shared/__tests__/InlineDetailTableRow.test.tsx b/frontend-modern/src/components/shared/__tests__/InlineDetailTableRow.test.tsx index 81fbefc66..f1ccb0d9c 100644 --- a/frontend-modern/src/components/shared/__tests__/InlineDetailTableRow.test.tsx +++ b/frontend-modern/src/components/shared/__tests__/InlineDetailTableRow.test.tsx @@ -113,10 +113,12 @@ describe('InlineDetailTableRow', () => { render(() => ); const disclosure = screen.getByRole('button', { name: 'Resource details' }); + const disclosureFocus = vi.spyOn(disclosure, 'focus'); const close = screen.getByRole('button', { name: 'Close details' }); close.focus(); await fireEvent.click(close); await waitFor(() => expect(disclosure).toHaveFocus()); + expect(disclosureFocus).toHaveBeenCalledWith({ preventScroll: true }); }); }); diff --git a/frontend-modern/src/components/shared/__tests__/Subtabs.test.tsx b/frontend-modern/src/components/shared/__tests__/Subtabs.test.tsx index b4e180e7f..f159b213a 100644 --- a/frontend-modern/src/components/shared/__tests__/Subtabs.test.tsx +++ b/frontend-modern/src/components/shared/__tests__/Subtabs.test.tsx @@ -32,7 +32,7 @@ describe('Subtabs', () => { expect(historyTab).toHaveClass('min-h-9', 'text-xs', 'sm:min-h-10', 'sm:text-sm'); }); - it('scrolls a newly selected tab into view', async () => { + it('reveals a newly selected tab horizontally without scrolling vertical ancestors', async () => { const previousScrollIntoView = Element.prototype.scrollIntoView; const scrollIntoView = vi.fn(); Object.defineProperty(Element.prototype, 'scrollIntoView', { @@ -55,16 +55,25 @@ describe('Subtabs', () => { /> )); + const tablist = screen.getByRole('tablist', { name: 'Threshold platform' }); + const machines = screen.getByRole('tab', { name: 'Machines' }); + Object.defineProperties(tablist, { + clientWidth: { configurable: true, value: 100 }, + scrollWidth: { configurable: true, value: 300 }, + scrollLeft: { configurable: true, writable: true, value: 0 }, + }); + Object.defineProperties(machines, { + offsetLeft: { configurable: true, value: 220 }, + offsetWidth: { configurable: true, value: 70 }, + }); scrollIntoView.mockClear(); - fireEvent.click(screen.getByRole('tab', { name: 'Machines' })); + fireEvent.click(machines); await waitFor(() => { - expect(screen.getByRole('tab', { name: 'Machines' })).toHaveAttribute( - 'aria-selected', - 'true', - ); - expect(scrollIntoView).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest' }); + expect(machines).toHaveAttribute('aria-selected', 'true'); + expect(tablist.scrollLeft).toBe(198); }); + expect(scrollIntoView).not.toHaveBeenCalled(); } finally { if (previousScrollIntoView) { Object.defineProperty(Element.prototype, 'scrollIntoView', { diff --git a/tests/integration/tests/64-workloads-proxmox-refresh-stability.spec.ts b/tests/integration/tests/64-workloads-proxmox-refresh-stability.spec.ts index ddab0cad2..081a2d821 100644 --- a/tests/integration/tests/64-workloads-proxmox-refresh-stability.spec.ts +++ b/tests/integration/tests/64-workloads-proxmox-refresh-stability.spec.ts @@ -334,4 +334,132 @@ test.describe.serial("Workloads Proxmox refresh stability", () => { await expect(rows.filter({ hasText: "lxc-gamma" })).toHaveCount(0); await expect(detailRow).toBeVisible(); }); + + test("does not request vertical scrolling when an off-screen Backups drawer changes or closes", async ({ + page, + }, testInfo) => { + await ensureMockModeEnabled(page); + + await page.goto("/proxmox/backups", { waitUntil: "domcontentloaded" }); + const serversTable = page.locator('[data-proxmox-backups-table="servers"]'); + await expect(serversTable).toBeVisible({ timeout: 60_000 }); + + const disclosure = serversTable.locator("button[aria-controls]").first(); + const summaryRow = disclosure.locator("xpath=ancestor::tr"); + await summaryRow.click(); + + const detailRow = serversTable.locator( + "tr[data-inline-platform-resource-detail-for]", + ); + await expect(detailRow).toBeVisible(); + const manageTab = detailRow.getByRole("tab", { + name: "Manage", + exact: true, + }); + const overviewTab = detailRow.getByRole("tab", { + name: "Overview", + exact: true, + }); + const closeButton = detailRow.getByRole("button", { + name: /^Collapse .* details$/, + }); + const viewportAnchor = page.getByText("Backup health", { exact: true }); + await expect(viewportAnchor).toBeVisible(); + + await page.screenshot({ + path: testInfo.outputPath("pbs-detail-overview.png"), + }); + await manageTab.click(); + await expect(manageTab).toHaveAttribute("aria-selected", "true"); + await page.screenshot({ + path: testInfo.outputPath("pbs-detail-manage.png"), + }); + await overviewTab.click(); + await expect(overviewTab).toHaveAttribute("aria-selected", "true"); + + // Put the section beneath the expanded drawer in view so the drawer tabs + // are above the viewport, matching an operator reading backup evidence + // below a server row. Tab selection must only move its horizontal rail. + const nextScrollTop = await viewportAnchor.evaluate((element) => { + const shell = document.querySelector(".app-scroll-shell"); + if (shell && shell.contains(element)) { + const shellRect = shell.getBoundingClientRect(); + return Math.max( + 0, + shell.scrollTop + + element.getBoundingClientRect().top - + shellRect.top - + 180, + ); + } + return Math.max( + 0, + window.scrollY + element.getBoundingClientRect().top - 180, + ); + }); + await page.evaluate((top) => { + const shell = document.querySelector(".app-scroll-shell"); + if (shell) shell.scrollTop = top; + else window.scrollTo(0, top); + }, nextScrollTop); + await page.waitForTimeout(150); + + await page.evaluate(() => { + const state = window as Window & { + __pulseScrollIntoViewCalls?: number; + }; + state.__pulseScrollIntoViewCalls = 0; + Element.prototype.scrollIntoView = () => { + state.__pulseScrollIntoViewCalls = + (state.__pulseScrollIntoViewCalls ?? 0) + 1; + }; + }); + await manageTab.evaluate((element) => (element as HTMLElement).click()); + await expect(manageTab).toHaveAttribute("aria-selected", "true"); + await page.waitForTimeout(150); + expect( + await page.evaluate( + () => + (window as Window & { __pulseScrollIntoViewCalls?: number }) + .__pulseScrollIntoViewCalls ?? 0, + ), + ).toBe(0); + + await page.screenshot({ + path: testInfo.outputPath("pbs-detail-anchor.png"), + }); + + // Closing from focused drawer content returns focus to the disclosure, + // but that restoration must not scroll the summary row back into view. + await manageTab.evaluate((element) => + (element as HTMLElement).focus({ preventScroll: true }), + ); + await disclosure.evaluate((element) => { + const state = window as Window & { + __pulseDisclosureFocusOptions?: FocusOptions; + }; + const disclosureElement = element as HTMLElement; + const originalFocus = disclosureElement.focus.bind(disclosureElement); + disclosureElement.focus = (options?: FocusOptions) => { + state.__pulseDisclosureFocusOptions = options; + originalFocus(options); + }; + }); + await closeButton.evaluate((element) => (element as HTMLElement).click()); + await expect(detailRow).toHaveCount(0); + await page.waitForTimeout(150); + expect( + await page.evaluate( + () => + ( + window as Window & { + __pulseDisclosureFocusOptions?: FocusOptions; + } + ).__pulseDisclosureFocusOptions, + ), + ).toEqual({ preventScroll: true }); + if (!testInfo.project.name.startsWith("mobile-")) { + await expect(disclosure).toBeFocused(); + } + }); });