From ca446737eecb173fa89ed9db30edfcf4154102fb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 08:56:14 +0000 Subject: [PATCH] fix(console): a long flow screen or param list keeps Submit / Confirm on screen (objectui#12080) A flat flow screen and the action parameter dialog had no height bound, so a screen or params list taller than the window overflowed both edges of the fixed, scroll-locked overlay: heading above the top, primary action below the bottom, nothing a mouse wheel could move. Both dialogs are now a flex column at most 90vh tall: the header on top, the fields in the one scrolling region, the footer outside it. More than eight declared fields (or shown params) also widens the dialog to the object-form step's sm:max-w-3xl with two columns from sm up; shorter screens and lists keep their width and single column. Claude-Session: https://claude.ai/code/session_01B1gHb9baeX7oioD5sHVm7z Co-authored-by: Claude --- .changeset/12080-screen-dialog-reach.md | 11 ++ ...tionParamDialog.dialogReach-12080.test.tsx | 127 ++++++++++++++++ .../app-shell/src/views/ActionParamDialog.tsx | 36 ++++- packages/app-shell/src/views/FlowRunner.tsx | 119 ++++++++++----- .../FlowRunner.dialogReach-12080.test.tsx | 141 ++++++++++++++++++ 5 files changed, 396 insertions(+), 38 deletions(-) create mode 100644 .changeset/12080-screen-dialog-reach.md create mode 100644 packages/app-shell/src/views/ActionParamDialog.dialogReach-12080.test.tsx create mode 100644 packages/app-shell/src/views/__tests__/FlowRunner.dialogReach-12080.test.tsx diff --git a/.changeset/12080-screen-dialog-reach.md b/.changeset/12080-screen-dialog-reach.md new file mode 100644 index 0000000000..bb9c68800d --- /dev/null +++ b/.changeset/12080-screen-dialog-reach.md @@ -0,0 +1,11 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(console): a long flow screen or action parameter list keeps its Submit / Confirm on screen (objectui#12080) + +A flow `screen` dialog without an object form, and the action parameter dialog, had no height bound. A screen or a `params` list taller than the window overflowed both edges of the fixed overlay: the heading above the top, the primary action below the bottom. The overlay locks page scrolling, so a mouse wheel moved nothing, and only keyboard focus could reach Submit. + +Both dialogs are now at most `90vh` tall. The fields scroll in their own region between the header and a footer that stays on screen. A screen with more than eight declared fields, or a list of more than eight shown params, also gets the wider dialog an object-form step already had, with two columns from the `sm` breakpoint up. Shorter screens and lists keep their previous width and single column. + +No new metadata key: the dialog sizes itself by the number of fields. diff --git a/packages/app-shell/src/views/ActionParamDialog.dialogReach-12080.test.tsx b/packages/app-shell/src/views/ActionParamDialog.dialogReach-12080.test.tsx new file mode 100644 index 0000000000..ac0994f553 --- /dev/null +++ b/packages/app-shell/src/views/ActionParamDialog.dialogReach-12080.test.tsx @@ -0,0 +1,127 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#12080 — the action parameter dialog takes the screen dialog's bound: + * at most 90vh tall, the param list as the one scrolling region, Confirm in a + * footer OUTSIDE it; a long list widens to two columns, a short one keeps the + * default width. + * + * ⚠️ The test DOM computes no layout, so nothing here can say Confirm is ON + * SCREEN — that was measured in Chromium with a real mouse wheel and a real + * click, and the readings are on the pull request. These tests pin the + * contract that measurement rests on, and that the two dialogs share it. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import type { ActionParamDef } from '@object-ui/core'; +import { ActionParamDialog } from './ActionParamDialog'; +import { FlowRunner } from './FlowRunner'; + +const params = (n: number, over: Partial = {}): ActionParamDef[] => + Array.from({ length: n }, (_, i) => ({ name: `p${i}`, label: `Param ${i}`, type: 'text', ...over })); + +function openDialog(list: ActionParamDef[]) { + render( + {}} + />, + ); + return { content: screen.getByRole('dialog'), body: screen.getByTestId('action-param-body') }; +} + +/** `a` comes before `b` in document order. */ +const precedes = (a: Node, b: Node) => (a.compareDocumentPosition(b) & Node.DOCUMENT_POSITION_FOLLOWING) !== 0; + +/** The tokens that make up the bound, on the dialog and on its scrolling body. */ +const CONTENT_BOUND = ['flex', 'flex-col', 'max-h-[90vh]']; +const BODY_SCROLL = ['-mx-6', 'px-6', 'min-h-0', 'flex-1', 'overflow-y-auto']; + +describe('ActionParamDialog — Confirm stays reachable on a long `params` list (objectui#12080)', () => { + it('bounds a long list, scrolls only the list, and keeps Confirm outside the scrolling region', async () => { + const { content, body } = openDialog(params(13)); + for (const token of CONTENT_BOUND) expect(content).toHaveClass(token); + expect(content).not.toHaveClass('overflow-y-auto'); + for (const token of BODY_SCROLL) expect(body).toHaveClass(token); + + const inputs = await screen.findAllByRole('textbox'); + expect(inputs).toHaveLength(13); + for (const input of inputs) expect(body).toContainElement(input); + const heading = screen.getByRole('heading', { name: 'Backfill Executed Contract' }); + const confirm = screen.getByRole('button', { name: /confirm/i }); + const cancel = screen.getByRole('button', { name: /cancel/i }); + for (const outside of [heading, confirm, cancel]) expect(body).not.toContainElement(outside); + expect(precedes(heading, body)).toBe(true); + expect(precedes(body, confirm)).toBe(true); + }); + + it('widens a long list to two columns from `sm` up', () => { + const { content, body } = openDialog(params(13)); + expect(content).toHaveClass('sm:max-w-3xl'); + expect(body).toHaveClass('grid'); + expect(body).toHaveClass('sm:grid-cols-2'); + }); + + it('control: a short list keeps the default width and its single column', () => { + const { content, body } = openDialog(params(2)); + // No width class of its own, so `DialogContent`'s default `max-w-lg` holds. + expect(content.className).not.toMatch(/sm:max-w-/); + expect(content).toHaveClass('max-w-lg'); + expect(body).not.toHaveClass('sm:grid-cols-2'); + expect(content).toHaveClass('max-h-[90vh]'); + expect(body).not.toContainElement(screen.getByRole('button', { name: /confirm/i })); + }); + + it.each([ + [8, false], + [9, true], + ])('switches width at more than eight params: %i params wide=%s', (n, wide) => { + const { content } = openDialog(params(n)); + expect(content.classList.contains('sm:max-w-3xl')).toBe(wide); + }); + + it('counts the params the dialog SHOWS: a param gated off by `visible: false` does not widen it', () => { + const list = [...params(8), { name: 'hidden', label: 'Hidden', type: 'text', visible: 'false' }]; + const { content } = openDialog(list); + expect(content).not.toHaveClass('sm:max-w-3xl'); + }); + + it('is the SAME bound as the flow screen dialog, so the two cannot drift apart unseen', () => { + openDialog(params(13)); + const paramContent = screen.getByRole('dialog'); + // A second modal marks the first `aria-hidden`, so both are read with `hidden: true`. + const paramBody = screen.getByTestId('action-param-body'); + render( + ({ name: `f${i}`, label: `Field ${i}`, type: 'text' })), + }, + }} + authFetch={vi.fn()} + baseUrl="" + onClose={() => {}} + onComplete={() => {}} + />, + ); + const screenContent = screen.getAllByRole('dialog', { hidden: true }).find((d) => d !== paramContent)!; + const screenBody = screen.getByTestId('flow-screen-body'); + for (const token of [...CONTENT_BOUND, 'sm:max-w-3xl']) { + expect(paramContent).toHaveClass(token); + expect(screenContent).toHaveClass(token); + } + for (const token of BODY_SCROLL) { + expect(paramBody).toHaveClass(token); + expect(screenBody).toHaveClass(token); + } + }); +}); diff --git a/packages/app-shell/src/views/ActionParamDialog.tsx b/packages/app-shell/src/views/ActionParamDialog.tsx index 125bd9b2b5..f409340cfe 100644 --- a/packages/app-shell/src/views/ActionParamDialog.tsx +++ b/packages/app-shell/src/views/ActionParamDialog.tsx @@ -22,6 +22,17 @@ * objectui#8672 ruling A for the reference-bearing pickers). * * Returns collected param values or null on cancel. + * + * ## A long `params` list still reaches Confirm (objectui#12080) + * + * The dialog takes the bound `FlowRunner`'s screen dialog has: at most `90vh` + * tall, the param list in the ONE scrolling region, the header above it and + * the footer below it. The upstream `DialogContent` has no height bound, so a + * list taller than the window used to overflow both edges of a fixed, + * scroll-locked overlay — its first required params above the top, Confirm + * below the bottom, and nothing a mouse wheel could move. A list of more than + * {@link NARROW_PARAM_MAX_COUNT} params also widens to two columns from `sm` + * up; a shorter one keeps the default width and its single column. */ import { Suspense, useState, useEffect, useMemo } from 'react'; @@ -37,6 +48,7 @@ import { Collapsible, CollapsibleTrigger, CollapsibleContent, + cn, } from '@object-ui/components'; import { ChevronDown, Lock } from 'lucide-react'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; @@ -75,6 +87,14 @@ interface ActionParamDialogProps { onOpenChange: (open: boolean) => void; } +/** + * The most params the dialog keeps its default single-column width for; a + * longer list gets the wide two-column dialog (objectui#12080 — see the + * header). The same count as `FlowRunner`'s `NARROW_SCREEN_MAX_FIELDS`, so a + * flow screen and an action's params lay out alike. + */ +const NARROW_PARAM_MAX_COUNT = 8; + /** * Filter action params by their optional `visible` predicate, evaluated on the * canonical entry (`evalRowPredicate`) against the host predicate scope @@ -478,11 +498,17 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp setErrors(prev => ({ ...prev, [name]: false })); }; + // A long list widens to two columns (objectui#12080 — see the header). + const isLongList = visibleParams.length > NARROW_PARAM_MAX_COUNT; + return ( { if (!open) handleCancel(); }}> - + {/* Bounded (objectui#12080): a flex column at most 90vh tall, in which + only the body below can give up height (`min-h-0`) and scroll, so the + header and the footer always stay on screen. */} + {state.title || t('actionDialog.title')} {/* `whitespace-pre-line` so a description composed of more than one @@ -494,7 +520,13 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp -
+ {/* The ONE scrolling region (objectui#12080). `-mx-6 px-6` runs it to + the dialog's edges, so the scrollbar sits on the border and a + control's focus ring is not clipped at the sides. */} +
{visibleParams.map((rawParam) => { const param = { ...rawParam, diff --git a/packages/app-shell/src/views/FlowRunner.tsx b/packages/app-shell/src/views/FlowRunner.tsx index 02bcd71f8b..481cb37d67 100644 --- a/packages/app-shell/src/views/FlowRunner.tsx +++ b/packages/app-shell/src/views/FlowRunner.tsx @@ -39,10 +39,11 @@ * a retry that cannot work; the input and the reason stay on screen * together. * 2. **The message had one carrier, and it was the transient one.** The toast - * is kept — it is the console's failure idiom and it is viewport-fixed, so - * it survives a tall `object-form` step scrolled past its own header — and - * an inline destructive `Alert` (`role="alert"`) now carries the same - * sentence inside the dialog, next to the values that produced it. + * is kept — it is the console's failure idiom and it is viewport-fixed — + * and an inline destructive `Alert` (`role="alert"`) now carries the same + * sentence inside the dialog, next to the values that produced it. It sits + * above the scrolling body (objectui#12080), so it stays in view however + * far a tall screen is scrolled. * 3. **Success invalidated the wrong thing.** Both hosts answer `onComplete` * with `notifyDataChanged({ objectName: })`, which is * the record the user is LOOKING at — never the record the flow WROTE. The @@ -143,6 +144,25 @@ * `defaultValue` each key carries, which `check:i18n-keys` pins to its `en` * value. The server's own refusal sentence is passed through untranslated by * design — it is prose the backend composed, not a string with a key. + * + * ## A long screen still reaches its Submit (objectui#12080) + * + * The dialog is height-bounded on every screen, not only on an `object-form` + * step: at most `90vh` tall, with the header (and any notice under it) on top, + * the screen body in the ONE scrolling region, and the footer below it. The + * upstream `DialogContent` has no height bound, so a flat screen taller than + * the window used to overflow both edges of a fixed, scroll-locked overlay — + * the heading above the top, Submit below the bottom, and nothing a mouse + * wheel could move. Keeping the footer OUTSIDE the scrolling region is what + * keeps Submit on screen however long the screen is. + * + * Width follows the screen's length, not a layout key: no spec key asks for + * one, and the renderer can size by content. A flat screen declaring more + * than {@link NARROW_SCREEN_MAX_FIELDS} fields gets the `object-form` step's + * wider dialog and two columns from `sm` up; a shorter one keeps the narrow + * single column it always had. The count is the DECLARED one, so a field a + * `visibleWhen` reveals mid-edit never makes the dialog jump width. + * `ActionParamDialog` takes the same bound for a long `params` list. */ import { Suspense, useEffect, useState } from 'react'; import { @@ -155,6 +175,7 @@ import { DialogTitle, DialogDescription, Button, + cn, } from '@object-ui/components'; import { notifyDataChanged, useObjectTranslation } from '@object-ui/react'; import { @@ -171,6 +192,7 @@ import { isObjectFormScreen, initialScreenValues, screenFieldBoundViolations, + screenFields, visibleScreenFields, type ScreenFieldSpec, type ScreenSpec, @@ -179,6 +201,16 @@ import { interpretFlowResponse } from '../utils/flowResponse.js'; export type { ScreenSpec, ScreenFieldSpec } from './ScreenView.js'; +/** + * The most declared fields a flat screen keeps the narrow single-column dialog + * for; a longer one gets the wide two-column dialog (objectui#12080 — see the + * header). Eight is the most single-line fields the narrow column was + * measured to show whole, with no scrolling, inside the `90vh` bound on a + * 1440×900 window — a reading taken once in Chromium for objectui#12080 and + * recorded on its pull request; nothing here re-derives it. + */ +const NARROW_SCREEN_MAX_FIELDS = 8; + /** The `flows` group of a spec `TranslationData` — typed by the spec, so the address below is checked against it. */ type FlowsTranslation = NonNullable; @@ -402,9 +434,9 @@ export function FlowRunner({ state, authFetch, baseUrl, onClose, onComplete, dat // #31). See utils/flowResponse. const outcome = interpretFlowResponse(res, json, 'Resume'); if (outcome.kind === 'failed') { - // Two carriers on purpose (`c40f3b8ca`): the toast is fixed to the viewport and - // reaches a user scrolled to the bottom of a tall object-form step; the - // inline Alert stays with the values that caused it. + // Two carriers on purpose (`c40f3b8ca`): the toast is the console's + // viewport-fixed failure idiom; the inline Alert stays in the dialog, + // above the scrolling body, with the values that caused it. toast.error(outcome.error); setResumeError({ message: outcome.error, retryable: outcome.retryable }); return; @@ -531,10 +563,17 @@ export function FlowRunner({ state, authFetch, baseUrl, onClose, onComplete, dat // withdrawing the submit affordance is one behaviour with two causes, and // splitting it is how the `object-form` arm of it gets forgotten. const terminal = refused !== null || (resumeError !== null && !resumeError.retryable); + // A long flat screen widens to two columns (objectui#12080 — see the header). + const isLongScreen = !isObjectForm && screenFields(screen).length > NARROW_SCREEN_MAX_FIELDS; return ( { if (!o && !submitting) onClose(); }}> - + {/* Bounded on every screen (objectui#12080): a flex column at most 90vh + tall, in which only the body below can give up height (`min-h-0`) + and scroll, so the header and the footer always stay on screen. */} + {/* The flow being run (objectui#11092), above the step's own heading, which stays the dialog's title. */} @@ -565,34 +604,42 @@ export function FlowRunner({ state, authFetch, baseUrl, onClose, onComplete, dat )} - {/* The screen body pulls in lazily-loaded chunks (an `object-form` step - mounts ObjectForm, whose field widgets are lazy). Without a boundary - HERE, that suspension unwinds to the host's nearest — a - route-level one on some surfaces — which swaps the whole page for a - fallback and destroys the host's state, taking this dialog (and the - run it is driving) with it. */} - {t('common.loading', { defaultValue: 'Loading…' })}
}> - - + {/* The ONE scrolling region (objectui#12080). `-mx-6 px-6` runs it to + the dialog's edges, so the scrollbar sits on the border and a + control's focus ring is not clipped at the sides. */} +
+ {/* The screen body pulls in lazily-loaded chunks (an `object-form` step + mounts ObjectForm, whose field widgets are lazy). Without a boundary + HERE, that suspension unwinds to the host's nearest — a + route-level one on some surfaces — which swaps the whole page for a + fallback and destroys the host's state, taking this dialog (and the + run it is driving) with it. */} + {t('common.loading', { defaultValue: 'Loading…' })}
}> + + +
{!isObjectForm && ( diff --git a/packages/app-shell/src/views/__tests__/FlowRunner.dialogReach-12080.test.tsx b/packages/app-shell/src/views/__tests__/FlowRunner.dialogReach-12080.test.tsx new file mode 100644 index 0000000000..0bd36a6b72 --- /dev/null +++ b/packages/app-shell/src/views/__tests__/FlowRunner.dialogReach-12080.test.tsx @@ -0,0 +1,141 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#12080 — a screen dialog is height-bounded on every screen, its body + * is the one scrolling region, and the footer (Submit) sits OUTSIDE that + * region; a long flat screen widens to two columns, a short one is unchanged. + * + * ⚠️ What this file can and cannot show. The test DOM computes no layout, so + * nothing here can say that Submit is ON SCREEN — that was measured in + * Chromium with a real mouse wheel and a real click, and the readings are on + * the pull request. What these tests pin is the contract that measurement + * rests on: the bound's classes, which element scrolls, and that the primary + * action is not inside it. Moving the footer into the body, or dropping the + * bound, turns them red. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { FlowRunner, type ScreenFieldSpec, type ScreenSpec } from '../FlowRunner'; + +const fields = (n: number): ScreenFieldSpec[] => + Array.from({ length: n }, (_, i) => ({ name: `f${i}`, label: `Field ${i}`, type: 'text' })); + +function renderRunner(screenSpec: ScreenSpec, authFetch = vi.fn(async () => new Response('{}'))) { + render( + {}} + onComplete={() => {}} + />, + ); + const content = screen.getByRole('dialog'); + const body = screen.getByTestId('flow-screen-body'); + return { content, body }; +} + +const flat = (n: number, extra: Partial = {}): ScreenSpec => ({ + nodeId: 'details', + title: 'Contract details', + fields: fields(n), + ...extra, +}); + +/** The ScreenView root the body wraps — the element a long screen lays out as a grid. */ +const screenRoot = (body: HTMLElement) => body.firstElementChild as HTMLElement; + +/** `a` comes before `b` in document order. */ +const precedes = (a: Node, b: Node) => (a.compareDocumentPosition(b) & Node.DOCUMENT_POSITION_FOLLOWING) !== 0; + +describe('FlowRunner — the screen dialog keeps Submit reachable (objectui#12080)', () => { + it('bounds a long screen, scrolls only its body, and keeps Submit outside the scrolling region', () => { + const { content, body } = renderRunner(flat(13)); + + // The bound: a flex column at most 90vh tall. The dialog as a whole does + // not scroll — scrolling it would carry the footer away with the fields. + for (const token of ['flex', 'flex-col', 'max-h-[90vh]']) expect(content).toHaveClass(token); + expect(content).not.toHaveClass('overflow-y-auto'); + // The body is the one region that gives up height and scrolls. + for (const token of ['min-h-0', 'flex-1', 'overflow-y-auto']) expect(body).toHaveClass(token); + + // Every field is in the scrolling body… + const inputs = screen.getAllByRole('textbox'); + expect(inputs).toHaveLength(13); + for (const input of inputs) expect(body).toContainElement(input); + // …and the heading and both footer actions are not. + const heading = screen.getByRole('heading', { name: 'Contract details' }); + const submit = screen.getByRole('button', { name: 'Submit' }); + const cancel = screen.getByRole('button', { name: 'Cancel' }); + for (const outside of [heading, submit, cancel]) expect(body).not.toContainElement(outside); + expect(precedes(heading, body)).toBe(true); + expect(precedes(body, submit)).toBe(true); + }); + + it('widens a long screen to two columns from `sm` up', () => { + const { content, body } = renderRunner(flat(13)); + expect(content).toHaveClass('sm:max-w-3xl'); + expect(content).not.toHaveClass('sm:max-w-md'); + const root = screenRoot(body); + for (const token of ['grid', 'gap-4', 'sm:grid-cols-2', 'space-y-0']) expect(root).toHaveClass(token); + // The stack spacing is replaced, not stacked under the grid's gap. + expect(root).not.toHaveClass('space-y-4'); + }); + + it('control: a short screen keeps the narrow single column it always had', () => { + const { content, body } = renderRunner(flat(3)); + expect(content).toHaveClass('sm:max-w-md'); + expect(content).not.toHaveClass('sm:max-w-3xl'); + const root = screenRoot(body); + expect(root).toHaveClass('space-y-4'); + expect(root).not.toHaveClass('grid'); + expect(root).not.toHaveClass('sm:grid-cols-2'); + // Bounded all the same: a short screen on a short window still keeps Submit. + expect(content).toHaveClass('max-h-[90vh]'); + expect(body).not.toContainElement(screen.getByRole('button', { name: 'Submit' })); + }); + + it.each([ + [8, 'sm:max-w-md'], + [9, 'sm:max-w-3xl'], + ])('switches width at more than eight declared fields: %i fields get %s', (n, width) => { + const { content } = renderRunner(flat(n)); + expect(content).toHaveClass(width); + }); + + it('counts DECLARED fields, so a field `visibleWhen` reveals mid-edit never makes the dialog jump width', () => { + // Nine declared, two of them hidden until a box is ticked: seven on screen. + const nine = fields(9).map((f, i) => (i >= 7 ? { ...f, visibleWhen: 'f0 == "show"' } : f)); + const { content } = renderRunner(flat(9, { fields: nine })); + expect(screen.getAllByRole('textbox')).toHaveLength(7); + expect(content).toHaveClass('sm:max-w-3xl'); + }); + + it('keeps an `object-form` step on its wide dialog, inside the same bound, with no runner footer', () => { + const { content, body } = renderRunner({ nodeId: 'customer', kind: 'object-form', objectName: 'crm_account' }); + for (const token of ['flex', 'flex-col', 'max-h-[90vh]', 'sm:max-w-3xl']) expect(content).toHaveClass(token); + expect(content).not.toHaveClass('overflow-y-auto'); + // No data source in this test, so the step renders its refusal line — in the body. + expect(body).toHaveTextContent('no data source'); + expect(screen.queryByRole('button', { name: 'Submit' })).not.toBeInTheDocument(); + }); + + it('keeps a resume failure ABOVE the scrolling body, so it is in view however far the screen is scrolled', async () => { + const authFetch = vi.fn(async () => + new Response(JSON.stringify({ success: true, data: { success: false, error: "Node 'apply' failed" } }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ); + const { body } = renderRunner(flat(13), authFetch); + await userEvent.setup().click(screen.getByRole('button', { name: 'Submit' })); + const alert = await waitFor(() => { + const el = screen.getAllByRole('alert').find((a) => a.textContent?.includes("Node 'apply' failed")); + expect(el).toBeDefined(); + return el!; + }); + expect(body).not.toContainElement(alert); + expect(precedes(alert, body)).toBe(true); + }); +});