Skip to content

Commit 3c1f321

Browse files
os-zhuangclaude
andauthored
fix(form): a tabbed/sectioned modal keeps every tab's values (#2959, #2153) (#2987)
* fix(form): a tabbed/sectioned modal keeps every tab's values (#2959, #2153) The explicit-`sections` path rendered one SchemaRenderer — one react-hook-form instance and one <form> element — PER section, all sharing `formId`. Two failures compounded: the footer submit (`form={formId}`) can only reach the FIRST form, so section 2+ never entered the payload; and in the tabbed variant Radix unmounted the inactive panel, destroying that tab's state outright. ModalForm (stacked + contentLayout:'tabbed') and TabbedForm now render ONE form for all sections, as ObjectForm/DrawerForm already do. Stacked sections use the inline `section-divider` header (which now also renders the section description); tabbed sections go through a new FormSchema.fieldTabs (+ defaultFieldTab, fieldTabsPosition) that the form renderer distributes into force-mounted Radix panels — CSS-hidden, not unmounted, because react-hook-form skips validation for unmounted fields, which is how a required field on an unopened tab sailed past the client and returned as a server 400. Validation now points at the tab: the first rejected field activates its tab and every tab holding one is marked on its trigger, for client rules and server `fields[]` alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(form): keep the fieldTabs schema keys off the DOM, and make tab position work Two follow-ups found by running the tabbed form in a real browser (console preview gallery): - `fieldTabs` / `defaultFieldTab` / `fieldTabsPosition` bled through `...formProps` onto the <form> element ("React does not recognize the `fieldTabs` prop on a DOM element"), the same leak the neighbouring destructure already guards `objectName` / `onDirtyChange` / `fields` against. Added to that list, with a regression test that spreads the form node at the top level the way the SDUI dispatch does. - `fieldTabsPosition: 'bottom' | 'right'` positioned the strip with `order-last`, which does nothing unless the parent is a flex container — the root was only flex in the vertical case (a quirk carried over verbatim from TabbedForm). The Tabs root is now always flex (`flex-col` when horizontal), the strip keeps its content size via `self-start`, and `bottom` swaps its margin to `mt-4`. Re-verified in the browser: single form, panels force-mounted with the inactive ones computed `display: none`, strip content-width above the panel, and no console errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 80edbd4 commit 3c1f321

11 files changed

Lines changed: 1313 additions & 470 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
"@object-ui/types": patch
3+
"@object-ui/components": patch
4+
"@object-ui/plugin-form": patch
5+
---
6+
7+
fix(form): a tabbed/sectioned create-edit form no longer loses the tabs you are not looking at (#2959, #2153)
8+
9+
The explicit-`sections` path rendered one `SchemaRenderer` — one react-hook-form
10+
instance and one `<form>` element — **per section**, all sharing the same
11+
`formId`. Two failures compounded:
12+
13+
1. the footer submit button (`form={formId}`) can only be associated with the
14+
**first** of those forms, so section 2+ never reached the payload; and
15+
2. in the `tabbed` variant Radix unmounted the inactive panel, destroying that
16+
tab's form state outright.
17+
18+
Reported flow (HotCRM, 3 tabs, required `description` on tab 3): fill tab 1 →
19+
submit → server 400 `description is required` → switch to tab 3, fill it →
20+
submit → the server now reports `subject; description; status; priority` **all**
21+
missing, because the second submit's body had lost every earlier value.
22+
23+
`ModalForm` (stacked and `contentLayout: 'tabbed'`) and `TabbedForm` now render
24+
ONE form for all sections, matching `ObjectForm` / `DrawerForm`. Stacked sections
25+
use the existing inline `section-divider` header (which now also renders the
26+
section's `description`); tabbed sections go through a new
27+
`FormSchema.fieldTabs` (+ `defaultFieldTab`, `fieldTabsPosition`) that the form
28+
renderer distributes into **force-mounted** Radix panels — CSS-hidden rather
29+
than unmounted, since react-hook-form skips validation for unmounted fields,
30+
which is how a required field on a tab nobody opened used to sail past the
31+
client and come back as a server 400.
32+
33+
Validation feedback now points at the tab: a rejected field activates its tab and
34+
every tab holding one is marked on its trigger, for client-side rules and server
35+
`fields[]` rejections alike.

content/docs/plugins/plugin-form.mdx

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,35 @@ interface FormField {
6262
}
6363
```
6464

65+
### Tabbed field layout (`fieldTabs`)
66+
67+
A sectioned form stays **one** form: declare the tabs on it and the renderer
68+
distributes the fields into panels, instead of rendering a form per section
69+
(which strands every section but the first outside the submit, and lets an
70+
inactive tab unmount along with its values).
71+
72+
```plaintext
73+
{
74+
type: 'form',
75+
fields: [/* every tab's fields, in one flat list */],
76+
fieldTabs: [
77+
{ key: 'basics', label: 'Basics', fields: ['subject', 'status'] },
78+
{ key: 'detail', label: 'Detail', description: 'Anything else', fields: ['description'] },
79+
],
80+
defaultFieldTab?: 'basics', // defaults to the first tab
81+
fieldTabsPosition?: 'top', // 'top' | 'bottom' | 'left' | 'right'
82+
}
83+
```
84+
85+
- Panels are force-mounted and only CSS-hidden, so a tab the user leaves keeps
86+
its values **and** its validation.
87+
- A failed submit activates the tab holding the first offending field and marks
88+
every tab with a rejected field — client rules and server `fields[]` alike.
89+
- Fields no tab claims render above the tab strip rather than disappearing.
90+
- Needs at least two tabs; ignored when the form uses `children`.
91+
92+
`ModalForm` (`contentLayout: 'tabbed'`) and `TabbedForm` build on this.
93+
6594
## Usage
6695

6796
### Auto-registration (Side-effect Import)
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* #2959: `FormSchema.fieldTabs` — a tabbed field layout inside ONE form.
11+
*
12+
* The contract these pin:
13+
* - one `<form>` / react-hook-form instance for every tab;
14+
* - panels are force-mounted (only CSS-hidden), so a tab the user leaves keeps
15+
* its values AND its validation — react-hook-form skips *unmounted* fields,
16+
* which is how a required field on an unopened tab used to reach the server;
17+
* - a failed submit activates the tab holding the first offending field and
18+
* marks every tab that has one;
19+
* - a field no tab claims is still rendered (never silently dropped).
20+
*/
21+
22+
import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest';
23+
import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react';
24+
import { ComponentRegistry } from '@object-ui/core';
25+
import { toast } from '../../../ui/sonner';
26+
27+
beforeAll(async () => {
28+
await import('../../../renderers');
29+
}, 30000);
30+
31+
beforeEach(() => {
32+
vi.spyOn(toast, 'error').mockImplementation(() => 'id' as any);
33+
if (!(Element.prototype as any).scrollIntoView) {
34+
(Element.prototype as any).scrollIntoView = () => {};
35+
}
36+
});
37+
38+
afterEach(() => {
39+
cleanup();
40+
vi.restoreAllMocks();
41+
});
42+
43+
const FIELDS = [
44+
{ name: 'subject', label: 'Subject', type: 'input', required: true },
45+
{ name: 'status', label: 'Status', type: 'input' },
46+
{ name: 'description', label: 'Description', type: 'input', required: true },
47+
];
48+
49+
const TABS = [
50+
{ key: 'basics', label: 'Basics', fields: ['subject', 'status'] },
51+
{ key: 'detail', label: 'Detail', description: 'Anything else', fields: ['description'] },
52+
];
53+
54+
function renderTabbedForm(overrides: Record<string, unknown> = {}) {
55+
const onSubmit = vi.fn();
56+
const Form = ComponentRegistry.get('form')!;
57+
const utils = render(
58+
<Form
59+
schema={{
60+
type: 'form',
61+
mode: 'create',
62+
showSubmit: true,
63+
showCancel: false,
64+
submitLabel: 'Create',
65+
fields: FIELDS,
66+
fieldTabs: TABS,
67+
onSubmit,
68+
...overrides,
69+
}}
70+
/>,
71+
);
72+
return { ...utils, onSubmit };
73+
}
74+
75+
const submit = () => fireEvent.click(screen.getByRole('button', { name: /create/i }));
76+
const fill = (name: string, value: string) => {
77+
const input = document.body.querySelector<HTMLInputElement>(`[data-field="${name}"] input`)!;
78+
fireEvent.change(input, { target: { value } });
79+
};
80+
const tab = (name: RegExp) => screen.getByRole('tab', { name });
81+
82+
describe('form renderer — fieldTabs (#2959)', () => {
83+
it('renders one form and one panel per tab, with every field mounted', () => {
84+
const { container } = renderTabbedForm();
85+
86+
expect(container.querySelectorAll('form')).toHaveLength(1);
87+
expect(screen.getAllByRole('tab')).toHaveLength(2);
88+
// Both panels exist up front — the inactive one is hidden by CSS, not
89+
// unmounted, so its fields are present and registered.
90+
expect(screen.getByTestId('form-tab-panel:basics')).toBeTruthy();
91+
expect(screen.getByTestId('form-tab-panel:detail')).toBeTruthy();
92+
for (const name of ['subject', 'status', 'description']) {
93+
expect(container.querySelector(`[data-field="${name}"]`)).toBeTruthy();
94+
}
95+
// The tab's blurb rides along with its panel.
96+
expect(screen.getByText('Anything else')).toBeTruthy();
97+
});
98+
99+
it('marks the inactive panel with data-state=inactive (CSS-hidden, still mounted)', () => {
100+
renderTabbedForm();
101+
expect(screen.getByTestId('form-tab-panel:basics')).toHaveAttribute('data-state', 'active');
102+
expect(screen.getByTestId('form-tab-panel:detail')).toHaveAttribute('data-state', 'inactive');
103+
expect(screen.getByTestId('form-tab-panel:detail').className).toContain(
104+
'data-[state=inactive]:hidden',
105+
);
106+
});
107+
108+
it('submits values entered across different tabs', async () => {
109+
const { onSubmit } = renderTabbedForm();
110+
111+
fill('subject', 'From tab 1');
112+
fireEvent.click(tab(/Detail/));
113+
fill('description', 'From tab 2');
114+
submit();
115+
116+
await waitFor(() => expect(onSubmit).toHaveBeenCalledTimes(1));
117+
expect(onSubmit.mock.calls[0][0]).toMatchObject({
118+
subject: 'From tab 1',
119+
description: 'From tab 2',
120+
});
121+
});
122+
123+
it('validates fields on tabs the user never opened, and jumps to the offender', async () => {
124+
const { onSubmit } = renderTabbedForm();
125+
126+
fill('subject', 'Only tab 1 filled');
127+
submit();
128+
129+
// `description` is required and lives on a tab that was never opened.
130+
await waitFor(() => expect(tab(/Detail/)).toHaveAttribute('data-state', 'active'));
131+
expect(onSubmit).not.toHaveBeenCalled();
132+
expect(tab(/Detail/)).toHaveAttribute('data-error', 'true');
133+
expect(tab(/Basics/)).not.toHaveAttribute('data-error');
134+
});
135+
136+
it('honors defaultFieldTab', () => {
137+
renderTabbedForm({ defaultFieldTab: 'detail' });
138+
expect(screen.getByTestId('form-tab-panel:detail')).toHaveAttribute('data-state', 'active');
139+
});
140+
141+
it('keeps the tab schema keys off the <form> DOM element', () => {
142+
// Callers/the SDUI dispatch spread the whole form node at the top level too,
143+
// so a FormSchema key that isn't consumed leaks onto the DOM and React logs
144+
// "does not recognize the `fieldTabs` prop on a DOM element".
145+
const Form = ComponentRegistry.get('form')!;
146+
const { container } = render(
147+
<Form
148+
schema={{ type: 'form', fields: FIELDS, fieldTabs: TABS }}
149+
// The top-level spread that reproduces the leak.
150+
fields={FIELDS}
151+
fieldTabs={TABS}
152+
defaultFieldTab="detail"
153+
fieldTabsPosition="top"
154+
/>,
155+
);
156+
const form = container.querySelector('form')!;
157+
for (const attr of ['fieldtabs', 'defaultfieldtab', 'fieldtabsposition']) {
158+
expect(form.hasAttribute(attr)).toBe(false);
159+
}
160+
});
161+
162+
it('renders fields no tab claims instead of dropping them', () => {
163+
const { container } = renderTabbedForm({
164+
fieldTabs: [
165+
{ key: 'basics', label: 'Basics', fields: ['subject'] },
166+
{ key: 'detail', label: 'Detail', fields: ['description'] },
167+
],
168+
});
169+
// `status` belongs to no tab — it must still be in the DOM, outside the panels.
170+
const status = container.querySelector('[data-field="status"]')!;
171+
expect(status).toBeTruthy();
172+
expect(status.closest('[role="tabpanel"]')).toBeNull();
173+
});
174+
175+
it('falls back to a plain field list for a single tab', () => {
176+
renderTabbedForm({ fieldTabs: [{ key: 'only', label: 'Only', fields: ['subject'] }] });
177+
expect(screen.queryAllByRole('tab')).toHaveLength(0);
178+
// …and every field still renders, not just the one the lone tab claimed.
179+
expect(document.body.querySelector('[data-field="description"]')).toBeTruthy();
180+
});
181+
});

0 commit comments

Comments
 (0)