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
2026-09-09 18:15:50 +00:00 · 2026-09-03 22:30:31 +01:00
parent 93c765a7ec
commit 59e9f5ea2b
9 changed files with 193 additions and 34 deletions
@@ -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`
@@ -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"
  ]
 }
@@ -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 () => {
@@ -51,7 +51,12 @@ export function InlineDetailTableRow(props: InlineDetailTableRowProps) {
      const disclosure = Array.from(
        document.querySelectorAll<HTMLButtonElement>('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 });
    });
  });

@@ -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',
@@ -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<SubtabsProps> = (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<HTMLElement>('[role="tab"][aria-selected="true"]')
-        ?.scrollIntoView?.({ block: 'nearest', inline: 'nearest' });
      updateScrollControls();
    });
  });
@@ -101,6 +106,7 @@ export const Subtabs: Component<SubtabsProps> = (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',
@@ -113,10 +113,12 @@ describe('InlineDetailTableRow', () => {

    render(() => <Fixture />);
    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 });
  });
 });
@@ -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', {
@@ -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<HTMLElement>(".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<HTMLElement>(".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();
+    }
+  });
 });