Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .changeset/select-multiple-widget-mapping-3986.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
---
'@object-ui/fields': patch
'@object-ui/plugin-form': patch
---

Resolve a `select` field declared `multiple: true` to the `field:multiselect` widget, so the object form's visible label actually names the chip picker it renders (objectui#3986).

`mapFieldTypeToFormType` keyed the widget id on the field's `type` string alone, so an object-schema `{ type: 'select', multiple: true }` picklist — a spec-legal, entirely ordinary shape — became `field:select`. `SelectField` then delegated to `MultiSelectField` on `config.multiple`, so the component that RENDERED was the chip picker while everything keyed on the widget id still answered for the single-value combobox. Above all the label-association declaration (`ComponentMeta.labelling`, objectui#3961), which the form renderer resolves per widget id: the host emitted `<label for>` at the chip row's wrapper `div`, where a `for` is inert — `HTMLLabelElement.control` returns `null`. Visually the field had a label; in the accessibility tree that label named nothing. Measured on the object-form path, `role=group` + accessible name went from 1 for a `multiselect`-typed field (fixed in objectui#3975) to 0 for this one.

Declaring `select` itself `labelling: 'group'` was not available: a single-value select's trigger is a labelable `button[role=combobox]` whose `for` association works, and a bare `select` is a builtin the renderer resolves without consulting the registry at all. The fix is therefore at the producer — the widget id now carries the arity, so one place decides which widget renders and the declaration can no longer be addressed to a widget that is not rendering.

- `mapFieldTypeToFormType(fieldType, config?)` takes an optional second argument — the rest of the field definition, of which only `multiple` is read. Existing single-argument calls are unchanged, and so is every type outside the new table: `select` is the only one whose `multiple` form is a different WIDGET. The spec's multi-capable set is larger (select / lookup / file / image, with `radio` on the select branch and `user` storing like `lookup`), but `LookupField`, `FileField` and `ImageField` each render both arities themselves, so their id — and their labelling declaration — is already right for either.
- The four object-form producers pass the pair: `ObjectForm`, `DrawerForm`, `ModalForm`, and `sectionFields` (Tabbed / Wizard / Split / Drawer / Modal). In `sectionFields` the id is now computed once from the EFFECTIVE pair, after view-level overrides have merged, because `multiple` is itself a spec `FormField` key: a view restating only `multiple: true` over a single-value object field moves the widget too, and `multiple: false` moves it back.
- `SelectField`'s `multiple` delegation is KEPT, not retired. Measured, it stays reachable from three entrances that never consult the alias map: the inline grid editor (`FieldEditWidget` finds `select` in its own table first), `ActionParamDialog` (`resolveFormWidgetType` returns `select` from `fieldWidgetMap` first), and hand-written SDUI addressing `field:select` by name with `multiple` on its metadata.

Read-only rendering of these widgets is untouched (objectui#4005), as is the built-in `Select` branch (objectui#3976).
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,7 +31,7 @@

import { describe, it, expect, beforeAll } from 'vitest';
import { ComponentRegistry } from '@object-ui/core';
import { registerAllFields } from '../index';
import { registerAllFields, mapFieldTypeToFormType } from '../index';

/**
* Every field type whose host label must be associated by IDREF. Two shapes, one
Expand DownExpand Up@@ -111,3 +111,38 @@ describe('field widgets declare how their label must be associated (objectui#396
expect(declared).toEqual([...GROUP_LABELLED].sort());
});
});

/**
* The join between the two halves (objectui#3986). The declaration above is
* per-WIDGET; a form asks for a widget by the id its producer emitted. So the
* declaration only reaches the right widget if the producer names the widget that
* will actually render — and for `select` that depends on `multiple`, not on the
* type string alone.
*
* Asserting the pair here, rather than in either half alone, is what makes the
* failure visible: `mapFieldTypeToFormType`'s own test cannot know what
* `labelling` the id it returns carries, and the declaration test cannot know
* which id a `multiple: true` select resolves to. The gap between those two blind
* spots is exactly where this bug lived.
*/
describe('the widget id the object form emits carries the right declaration (objectui#3986)', () => {
const labellingOf = (formType: string) =>
ComponentRegistry.getMeta(formType.replace(/^field:/, ''), 'field')?.labelling;

it('select + multiple resolves to a widget declared `group`', () => {
// Before the producer fix this resolved to `field:select`, whose declaration
// is (correctly) absent — so the host kept emitting a `for` at the chip row's
// wrapper `div`, and the visible label named nothing.
const formType = mapFieldTypeToFormType('select', { multiple: true });
expect(formType).toBe('field:multiselect');
expect(labellingOf(formType)).toBe('group');
});

it('a single-value select still resolves to an UNDECLARED widget', () => {
// The guard direction. `select`'s trigger is a labelable
// `button[role=combobox]`; declaring it `group` would strip a working `for`.
const formType = mapFieldTypeToFormType('select');
expect(formType).toBe('field:select');
expect(labellingOf(formType)).toBeUndefined();
});
});
81 changes: 81 additions & 0 deletions packages/fields/src/field-type-alias.multiple.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
/**
* 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.
*/

/**
* The widget id is a function of the (type, `multiple`) PAIR, not of the type
* string alone (objectui#3986).
*
* `mapFieldTypeToFormType` mapped `select` to `field:select` unconditionally,
* while `SelectField` delegated to `MultiSelectField` whenever the metadata said
* `multiple`. Three correct pieces, one gap between them: the component that
* RENDERED was `MultiSelectField`, but everything keyed on the widget id — above
* all the label-association declaration (`ComponentMeta.labelling`, resolved via
* `getMeta('select')`) — was still answering for the single-value combobox. The
* host label of a normal `{ type: 'select', multiple: true }` picklist therefore
* emitted a `for` at the chip row's wrapper `div`, where a `for` is inert.
*
* `select` cannot simply be declared `labelling: 'group'` instead: the
* single-value trigger IS labelable, and a bare `select` is a builtin the form
* renderer resolves without the registry at all (pinned in
* `form-group-label-association.test.ts`). The producer has to name the widget
* that renders, which is what this file pins.
*
* Metadata only — nothing renders here, so no widget module is loaded.
*/

import { describe, it, expect } from 'vitest';
import { mapFieldTypeToFormType } from './field-type-alias';

describe('select carries its arity in the widget id (objectui#3986)', () => {
it('maps select + multiple to the multi-value widget', () => {
// The one assertion that was `field:select` before this change.
expect(mapFieldTypeToFormType('select', { multiple: true })).toBe('field:multiselect');
});

it('keeps the single-value widget without multiple, and for multiple: false', () => {
// The guard direction: single-select must not move. Its trigger is a
// labelable `button[role=combobox]` and its `for` association works — taking
// that away would break a field that is fine to fix one that is not.
expect(mapFieldTypeToFormType('select')).toBe('field:select');
expect(mapFieldTypeToFormType('select', {})).toBe('field:select');
expect(mapFieldTypeToFormType('select', { multiple: false })).toBe('field:select');
expect(mapFieldTypeToFormType('select', { multiple: null })).toBe('field:select');
});

it('is idempotent for the type that is already inherently multi', () => {
// `multiselect` is a type in its own right; the arity override must not turn
// it into something else, whichever way the flag is set.
expect(mapFieldTypeToFormType('multiselect')).toBe('field:multiselect');
expect(mapFieldTypeToFormType('multiselect', { multiple: true })).toBe('field:multiselect');
});

/**
* The spec's multi-capable set is larger than the widget-moving set, and the
* difference is the whole reason this override is table-driven rather than a
* `config.multiple` branch sprinkled per type:
*
* `MULTI_CAPABLE_TYPES` = select / lookup / file / image (+ `radio` on the
* select branch, `user` storing like `lookup`)
*
* Every member except `select` renders BOTH arities inside one widget —
* `LookupField`, `FileField`, `ImageField` each read `multiple` themselves — so
* their widget id, and with it their `labelling` declaration, is correct for
* either arity. Moving them would point at widgets that do not exist.
*/
const UNMOVED_MULTI_CAPABLE = ['lookup', 'master_detail', 'user', 'owner', 'file', 'image', 'radio'] as const;

it.each(UNMOVED_MULTI_CAPABLE)('%s resolves identically with and without multiple', (type) => {
expect(mapFieldTypeToFormType(type, { multiple: true })).toBe(mapFieldTypeToFormType(type));
});

it('does not invent a widget for an unknown type flagged multiple', () => {
// The fallback is unchanged: an unknown type is `field:text` at either arity,
// never `field:text-multiple`-anything.
expect(mapFieldTypeToFormType('something-unknown', { multiple: true })).toBe('field:text');
});
});
60 changes: 58 additions & 2 deletions packages/fields/src/field-type-alias.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,15 +6,63 @@
* LICENSE file in the root directory of this source tree.
*/

/**
* The slice of a field definition the widget decision reads beyond `type`.
*
* Structural (not the spec `Field`) for the same reason the spec's own
* `ValueShapeFieldDef` is: every caller — object-schema metadata, a spec
* `FormFieldSchema` override, an action param def — can pass its own trimmed
* shape verbatim. `multiple` is the only key read; nothing else here is
* consulted, so a caller that has the whole metadata object may hand it over
* as-is.
*/
export interface FieldTypeMappingConfig {
/** `FieldSchema.multiple` — the field holds zero-or-more values. */
multiple?: boolean | null;
}

/**
* Field types whose `multiple: true` form is a DIFFERENT registered widget,
* rather than the same widget in another mode (objectui#3986).
*
* `select` is the only member, and the narrowness is measured, not assumed. The
* spec's `MULTI_CAPABLE_TYPES` is larger — select / lookup / file / image, with
* `radio` on the select branch and `user` storing like `lookup` — but every
* other member renders both arities INSIDE one widget: `LookupField`,
* `FileField` and `ImageField` each branch on `multiple` themselves, so their
* registry id, and with it their `labelling` declaration, is the same either
* way. A `select` declared `multiple: true` renders `MultiSelectField` instead:
* a different component, whose labelled surface is a chip row's container that
* a `<label for>` cannot address (it must be named by IDREF, which is what
* `field:multiselect`'s `labelling: 'group'` declares — objectui#3975/#3961).
*
* So the widget id has to carry the arity. When it did not, the host label was
* associated by the declaration registered under `select` — a single-value
* combobox that was NOT rendering — and the visible label of a normal
* `{ type: 'select', multiple: true }` picklist named nothing at all. One place
* decides which widget renders, so the declaration and the render cannot
* disagree again.
*/
const MULTI_VALUE_FORM_TYPES: Record<string, string> = {
select: 'field:multiselect',
};

/**
* Map field type to form component type
*
*
* @param fieldType - The ObjectQL field type identifier to convert
* (for example: `"text"`, `"number"`, `"date"`, `"lookup"`).
* @param config - The rest of the field definition, for the types whose widget
* identity depends on more than the type string. Only `multiple` is read; see
* {@link MULTI_VALUE_FORM_TYPES}. Omitting it maps the single-value form, which
* is what every non-multi-capable type resolves to anyway.
* @returns The normalized form field type string used in the form schema
* (for example: `"input"`, `"textarea"`, `"date-picker"`, `"select"`).
*/
export function mapFieldTypeToFormType(fieldType: string): string {
export function mapFieldTypeToFormType(
fieldType: string,
config?: FieldTypeMappingConfig,
): string {
const typeMap: Record<string, string> = {
// Text-based fields
text: 'field:text',
Expand DownExpand Up@@ -96,5 +144,13 @@ export function mapFieldTypeToFormType(fieldType: string): string {
auto_number: 'field:auto_number',
};

// The arity override comes FIRST and is table-driven: only a type listed in
// `MULTI_VALUE_FORM_TYPES` has a second widget to move to, so `multiple` on
// any other type (a multi `lookup`, a multi `file`) resolves exactly as
// before and keeps reaching the widget that handles both arities itself.
if (config?.multiple && MULTI_VALUE_FORM_TYPES[fieldType]) {
return MULTI_VALUE_FORM_TYPES[fieldType];
}

return typeMap[fieldType] || 'field:text';
}
30 changes: 25 additions & 5 deletions packages/fields/src/widgets/SelectField.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -21,11 +21,31 @@ import { useCascadingOptions } from './useCascadingOptions';
*
* A field declared `multiple: true` selects zero-or-more values (spec:
* `multiple` is valid on `select`), so it renders the multi-value chip picker
* — the same widget the `multiselect` type uses. Delegating here (rather than
* only at a type-resolution layer) means every surface that renders the
* `select` widget — the object form, the inline grid editor, and
* `ActionParamDialog` — inherits multi-select identically, with no drift
* between them. Single-value selects keep the cascading dropdown below.
* — the same widget the `multiselect` type uses. Single-value selects keep the
* cascading dropdown below.
*
* The FORM no longer arrives here with `multiple` (objectui#3986):
* `mapFieldTypeToFormType` now resolves a `select` + `multiple: true` field to
* `field:multiselect`, so the object-form path renders `MultiSelectField`
* directly under its own registry id — which is what carries the
* `labelling: 'group'` declaration the host label needs. Deciding the widget at
* the type-resolution layer is what keeps the declaration and the render from
* disagreeing; a delegation invisible to the resolver could not.
*
* The branch below stays because it is NOT dead — measured entrances that reach
* it with `multiple` set, none of which consult that resolver:
*
* - **the inline grid editor** — `FieldEditWidget` looks `select` up in its own
* `EDIT_WIDGETS` table, which SHORT-CIRCUITS before the alias map is
* consulted, and forwards the whole metadata object as `field`;
* - **`ActionParamDialog`** — `paramToField` resolves through
* `resolveFormWidgetType`, which likewise returns `select` from
* `fieldWidgetMap` before reaching the alias map, and carries
* `multiple: param.multiple` on the field it builds;
* - **hand-written SDUI** — a `{ type: 'field:select' }` node whose metadata
* declares `multiple`, which addresses this widget by name.
*
* All three then inherit multi-select from here identically, with no drift.
*
* Both branches resolve per-option `visibleWhen` cascading / role-gating through
* the shared {@link useCascadingOptions} hook (#2715), so single and multi stay
Expand Down
3 changes: 2 additions & 1 deletion packages/plugin-form/src/DrawerForm.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -322,7 +322,8 @@ export const DrawerForm: React.FC<DrawerFormProps> = ({
generated.push({
name,
label: fieldLabel(schema.objectName, name, field.label || name),
type: mapFieldTypeToFormType(field.type),
// (type, multiple) decides the widget (objectui#3986) — see `sectionFields`.
type: mapFieldTypeToFormType(field.type, { multiple: field.multiple }),
required: field.required || false,
disabled: schema.readOnly || schema.mode === 'view' || field.readonly,
placeholder: field.placeholder,
Expand Down
3 changes: 2 additions & 1 deletion packages/plugin-form/src/ModalForm.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -395,7 +395,8 @@ export const ModalForm: React.FC<ModalFormProps> = ({
generated.push({
name,
label: fieldLabel(schema.objectName, name, field.label || name),
type: mapFieldTypeToFormType(field.type),
// (type, multiple) decides the widget (objectui#3986) — see `sectionFields`.
type: mapFieldTypeToFormType(field.type, { multiple: field.multiple }),
required: field.required || false,
disabled: schema.readOnly || schema.mode === 'view' || field.readonly,
placeholder: field.placeholder,
Expand Down
6 changes: 5 additions & 1 deletion packages/plugin-form/src/ObjectForm.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -564,7 +564,11 @@ const SimpleObjectForm: React.FC<ObjectFormProps> = ({
const formField: FormField = {
name: name,
label: fieldLabel(schema.objectName, name, field.label || fieldName),
type: mapFieldTypeToFormType(field.type),
// (type, multiple) decides the widget, not the type alone: a `select`
// declared `multiple: true` renders the multi-value chip picker, whose
// label must be associated by IDREF — a fact declared per WIDGET, so
// the widget id has to carry the arity (objectui#3986).
type: mapFieldTypeToFormType(field.type, { multiple: field.multiple }),
required: field.required || false,
disabled: schema.readOnly || schema.mode === 'view' || field.readonly || managedBlanketLock,
placeholder: field.placeholder,
Expand Down
Loading
Loading