diff --git a/.changeset/6909-fieldeditwidget-dom-pass-through.md b/.changeset/6909-fieldeditwidget-dom-pass-through.md new file mode 100644 index 0000000000..3dedb5d414 --- /dev/null +++ b/.changeset/6909-fieldeditwidget-dom-pass-through.md @@ -0,0 +1,60 @@ +--- +'@object-ui/fields': patch +--- + +`FieldEditWidget` now DELIVERS the DOM pass-through block it DECLARES +(objectui#6909). + +Its props are `FieldWidgetComponentProps` — the controlled-input keys +intersected with `FieldWidgetDomProps`, `AriaAttributes` and the open `data-` +family — so a host could always pass `id`, `name`, `autoFocus`, `tabIndex`, +`onBlur`, `onFocus`, `onClick`, any `aria-*` and any `data-*` with no type +error. The body then destructured five keys and rendered the widget with those, +so `autoFocus` was the ONLY survivor of the whole block and everything else was +silently dropped. That is this package's own first-class defect class, named in +`widgets/toDomProps.ts`: a key that type-checks, reads as supported, and +silently never reaches the element (objectui#3290's `aria-required`, +objectui#3222's validation slot). + +Not a widening, and not a contract change. The keys were already declared, and +each widget still re-filters through its own `toDomProps` before anything +reaches a DOM element — what any widget accepts or rejects is unchanged. The +factory was simply the one link in the chain nothing bound to the declaration: +`toDomProps` binds the WIDGET contract to its whitelist with compile-time +assertions in both directions, and the factory sat above them, bound to +neither. + +The fix hands the widget `toDomProps(props)` — this package's own executor — +rather than a second key list written out in the factory. That reuse is the +guard: `toDomProps.ts`'s direction-2 assertion already makes +`keyof FieldWidgetDomProps extends DomPassThroughKey` a compile error to +violate, so a key added to the declared DOM block now reaches the widget +through this factory automatically. One mechanism, one judge — a private list +here would have been free to drift, which is how the factory came to deliver +one key out of seven. + +The forwarded set is a deliberate superset of `FieldWidgetDomProps`: it also +carries `className` and `disabled`, declared on the controlled-input block and +forwarded by the same executor for the reason stated there — withholding them +makes it a silent styling- and interactivity-dropper. The semantic props +(`field`, `value`, `onChange`, `readonly`, and `compact` for the relational +pickers) stay explicit and are applied after the spread, so a host cannot +displace them. + +**No host in this repo changes behaviour.** Measured on all three call sites +before the fix: `ObjectGrid.renderCellEditor` passes `{ field, value, onChange }`, +`InlineFieldInput` passes those plus `autoFocus` (the key that already worked), +and `RequiredFieldsDialog` passes those plus `readonly`. None passes a dropped +key, so this is a plain repair rather than a live regression — but +`RequiredFieldsDialog` had already worked *around* the drop, wrapping each +control in a `label` because "`FieldEditWidget` … takes no `id` to associate +with". It does now. + +Also corrects a comment in `@object-ui/components`' `data-table.tsx` that this +change falsifies. It justified the injected editor's document-level +`pointerdown` listener partly with "`FieldEditWidget` forwards `autoFocus` and +nothing else out of the DOM block, so a host handler could not reach the +control through it even if one were passed" — no longer true. The listener is +still load-bearing for the other half of that reason, which is untouched: the +`renderCellEditor` context object has nowhere to put an `onBlur` in the first +place. Comment only; no behaviour change in that package. diff --git a/packages/components/src/renderers/complex/data-table.tsx b/packages/components/src/renderers/complex/data-table.tsx index 7418fef471..6cf03c612b 100644 --- a/packages/components/src/renderers/complex/data-table.tsx +++ b/packages/components/src/renderers/complex/data-table.tsx @@ -1026,10 +1026,16 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // The listener is still needed, for a different reason: NOTHING EVER HANDS // THE WIDGET ONE. The wrapper below carries `onKeyDown` alone, and the // context object `renderCellEditor` receives — `{ column, row, value, stage, - // commit, cancel }` — has no DOM-props slot to put an `onBlur` in. The - // in-repo factory behind that seam, `@object-ui/fields`' `FieldEditWidget`, - // forwards `autoFocus` and nothing else out of the DOM block, so a host - // handler could not reach the control through it even if one were passed. + // commit, cancel }` — has no DOM-props slot to put an `onBlur` in. + // + // ⚠️ The second half of that reason is GONE (objectui#6909). The in-repo + // factory behind the seam, `@object-ui/fields`' `FieldEditWidget`, used to + // forward `autoFocus` and nothing else out of the DOM block, so a host + // handler could not have reached the control even if one were passed. It now + // hands the widget its whole `toDomProps` set, so a passed `onBlur` WOULD + // arrive. What keeps this listener load-bearing is the FIRST half alone: the + // seam still has nowhere to put one. Widening that context object is a + // `DataTableSchema` contract change, not something to infer from here. // // Note also what the listener is NOT load-bearing for. Its job is exiting // EDIT MODE, not rescuing the value: injected widgets stage on every change diff --git a/packages/fields/src/FieldEditWidget.tsx b/packages/fields/src/FieldEditWidget.tsx index c5a83f4723..70ad0de26e 100644 --- a/packages/fields/src/FieldEditWidget.tsx +++ b/packages/fields/src/FieldEditWidget.tsx @@ -8,6 +8,9 @@ import React from 'react'; import type { FieldWidgetComponentProps } from './widgets/types.js'; +// The package's own executor of the DOM pass-through declaration, reused here +// rather than re-listed — see the note on this component's return statement. +import { toDomProps } from './widgets/toDomProps.js'; // The SAME dedicated widgets the form renders — reused for in-place editing // (e.g. the data grid's inline cell editor) so a select edits as a dropdown, a @@ -249,19 +252,30 @@ const COMPACT_EDIT_TYPES = new Set(['lookup', 'master_detail', 'user']); * `null` for types without a registered widget so the caller can fall back to * a plain editor. * - * `autoFocus` is forwarded because an inline-edit host enters edit mode ON a - * field and expects the caret to land there (objectui#4220 — the detail page's - * delegation): each widget's own `toDomProps` whitelist already carries it onto - * the real focusable control, so nothing here needs to know which element that - * is. A host that passes nothing is unaffected. + * The host's DOM pass-through set is forwarded WHOLE (objectui#6909). This + * component's props are `FieldWidgetComponentProps`, so a caller may already + * pass `id`, `name`, `autoFocus`, `tabIndex`, `onBlur`, `onFocus`, `onClick`, + * any `aria-*` and any `data-*` with no type error — but the body used to + * destructure five keys and render the widget with those, so `autoFocus` was + * the ONLY survivor of the whole block and everything else was silently + * dropped. That is this package's own first-class defect class, named in + * `widgets/toDomProps.ts`: a key that type-checks, reads as supported, and + * silently never reaches the element (objectui#3290's `aria-required`, + * objectui#3222's validation slot). Forwarding is not a widening: the keys were + * already declared, and each widget still re-filters through its own + * `toDomProps` before anything reaches a DOM element. + * + * `autoFocus`' original reason survives inside that set — an inline-edit host + * enters edit mode ON a field and expects the caret to land there + * (objectui#4220, the detail page's delegation) — and so does its property: + * each widget's own whitelist carries these onto the real focusable control, so + * nothing here needs to know which element that is. A host that passes nothing + * is unaffected. */ -export function FieldEditWidget({ - field, - value, - onChange, - readonly, - autoFocus, -}: FieldWidgetComponentProps): React.ReactElement | null { +export function FieldEditWidget( + props: FieldWidgetComponentProps, +): React.ReactElement | null { + const { field, value, onChange, readonly } = props; // A RETIRED spelling never reaches a widget here, whatever the tables say, // and it says so out loud (objectui#4931). This branch is for the caller that // ignores `hasFieldEditWidget` and calls this component directly: without it @@ -291,13 +305,32 @@ export function FieldEditWidget({ // `compact` is a declared widget prop (objectui#3221 closed this type), so // the spread no longer needs an `any` escape hatch to get past it. const compactProps = resolved && COMPACT_EDIT_TYPES.has(resolved) ? { compact: true } : {}; + // `toDomProps` — this package's own runtime executor of the declaration — is + // REUSED rather than re-listed here, and that reuse is the guard. Its + // direction-2 compile-time assertion already makes + // `keyof FieldWidgetDomProps extends DomPassThroughKey` an error to violate, + // so a key added to the declared DOM block now reaches the widget through + // this factory automatically. A private key list written out here would be a + // SECOND judge of the same declaration — exactly what `toDomProps.ts` argues + // against ("one mechanism, two declarations, each bound to the contract it + // executes — not two judges") — and would be free to drift, which is how this + // factory came to deliver one key out of seven in the first place. + // + // The set is a deliberate superset of `FieldWidgetDomProps`: it also carries + // `className` and `disabled`, declared on the controlled-input block and + // forwarded by the same executor for the reason stated there — withholding + // them makes it a silent styling- and interactivity-dropper. + // + // The semantic props stay explicit and come AFTER the spread. They are not in + // the whitelist, so there is no collision to resolve; ordering them this way + // states that this component OWNS them and a host cannot displace them. return ( ); diff --git a/packages/fields/src/__tests__/FieldEditWidget.domPassThrough-6909.test.tsx b/packages/fields/src/__tests__/FieldEditWidget.domPassThrough-6909.test.tsx new file mode 100644 index 0000000000..af1c91bdd6 --- /dev/null +++ b/packages/fields/src/__tests__/FieldEditWidget.domPassThrough-6909.test.tsx @@ -0,0 +1,222 @@ +/** + * 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. + */ + +/** + * `FieldEditWidget` DELIVERS the DOM pass-through block it DECLARES + * (objectui#6909). + * + * ## The defect this pins closed + * + * The factory's props are `FieldWidgetComponentProps`: the controlled-input + * keys intersected with `FieldWidgetDomProps`, `AriaAttributes` and the open + * `data-` family. A host could therefore pass `id` / `name` / `tabIndex` / + * `onBlur` / `onFocus` / `onClick` / any `aria-*` / any `data-*` with no type + * error — and the body destructured `{ field, value, onChange, readonly, + * autoFocus }` and rendered the widget with those five plus `compact`. + * Everything else was silently dropped. `autoFocus` was the ONLY survivor of + * the whole DOM block. + * + * That is this package's own first-class defect class, named in + * `widgets/toDomProps.ts`: "a key that type-checks, reads as supported, and + * silently never reaches the element" (objectui#3290's `aria-required`, + * objectui#3222's validation slot). `toDomProps` binds the WIDGET contract to + * its whitelist with compile-time assertions in both directions; nothing bound + * THIS factory to either, so the factory was the one hole left in the chain. + * + * ## What binds it now — and why this file still exists + * + * The fix hands the widget `toDomProps(props)` — the package's own executor of + * the declaration, not a second key list written out here. That reuse is the + * structural guard: `toDomProps.ts`'s direction-2 assertion already makes + * `keyof FieldWidgetDomProps extends DomPassThroughKey` a compile error to + * violate, so a key added to the declared DOM block now reaches the widget + * through this factory automatically. One mechanism, one judge. + * + * This file pins the half a type cannot: that the forwarded set actually + * ARRIVES on a real control at runtime, and that the forwarding did NOT become + * a bare `{...props}` spread — the shape `toDomProps` exists to prevent. + * + * ## Probe and control + * + * Two measurement points on purpose, because they answer different questions: + * + * - the DOM (`it` #1) — "the host's set reaches a control the user can focus", + * which is the claim the card makes and the only one a host cares about; + * - the FACTORY BOUNDARY (`it` #2) — the exact prop set this component hands + * the widget. The DOM alone cannot see a reopened spread, because each + * widget re-filters through its own `toDomProps` and would quietly rescue + * the mistake. Read at the boundary, an undeclared authored key that the + * factory forwarded is visible immediately. + * + * `FieldEditWidget` is called as a plain function there rather than rendered: + * it uses no hooks, and its return value IS the widget element, so this reads + * the handoff itself with nothing in between. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, fireEvent, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; + +import { FieldEditWidget } from '../FieldEditWidget'; +import type { FieldWidgetComponentProps } from '../widgets/types'; + +afterEach(() => cleanup()); + +/** + * The locator is a `data-*` sentinel: an open family `toDomProps` forwards by + * prefix, and — unlike `id` — nothing downstream rewrites it, so "the sentinel + * is on element X" means "the host set reached element X" and nothing else. + */ +const PROBE = 'data-os6909'; + +/** `text` resolves to `TextField`, which spreads its whole `toDomProps` set onto a real ``. */ +const TEXT_FIELD = { name: 'f', type: 'text', label: 'F' } as never; + +describe('FieldEditWidget delivers its declared DOM pass-through block (objectui#6909)', () => { + it("a host's id / name / tabIndex / aria-* / data-* / onBlur / onFocus / onClick reach the control", () => { + const onBlur = vi.fn(); + const onFocus = vi.fn(); + const onClick = vi.fn(); + + const { container } = render( + {}} + id="host-id" + name="host-name" + tabIndex={3} + onBlur={onBlur} + onFocus={onFocus} + onClick={onClick} + aria-label="host label" + data-os6909="probe" + />, + ); + + const carriers = container.querySelectorAll(`[${PROBE}]`); + expect(carriers.length).toBeGreaterThan(0); + + // A real control, not a wrapper — the host set is only useful where the + // user's focus and pointer actually land. + const control = carriers[0] as HTMLElement; + expect(['INPUT', 'TEXTAREA', 'BUTTON', 'SELECT']).toContain(control.tagName); + + expect(control).toHaveAttribute('id', 'host-id'); + expect(control).toHaveAttribute('name', 'host-name'); + expect(control).toHaveAttribute('tabindex', '3'); + expect(control).toHaveAttribute('aria-label', 'host label'); + expect(control).toHaveAttribute(PROBE, 'probe'); + + fireEvent.blur(control); + expect(onBlur).toHaveBeenCalledTimes(1); + + fireEvent.focus(control); + expect(onFocus).toHaveBeenCalledTimes(1); + + fireEvent.click(control); + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('CONTROL: forwards exactly the declared set — an undeclared authored key is still dropped', () => { + const onBlur = vi.fn(); + const onFocus = vi.fn(); + const onClick = vi.fn(); + const onChange = vi.fn(); + + // `zzcanary` is the control. It is NOT declared on + // `FieldWidgetComponentProps` — passing it is a compile error, which is + // why this object is cast — but an SDUI node or a field config can carry + // exactly such a key at runtime, and putting it on an element is the + // `[object Object]` leak `toDomProps` was written for. It must not survive + // the factory. Without this assertion "everything forwards now" would be + // indistinguishable from having reopened the bare spread. + const props = { + field: TEXT_FIELD, + value: '', + onChange, + readonly: false, + id: 'host-id', + name: 'host-name', + autoFocus: true, + tabIndex: 3, + onBlur, + onFocus, + onClick, + className: 'host-class', + disabled: true, + 'aria-label': 'host label', + [PROBE]: 'probe', + zzcanary: 'CANARY-STR', + } as unknown as FieldWidgetComponentProps; + + const element = FieldEditWidget(props); + expect(element).not.toBeNull(); + + // `ReactElement`'s prop parameter defaults to `unknown` under these React + // typings, so the handoff is read through one explicit narrowing rather + // than `any` — the assertion below is about KEYS, and this keeps that the + // only claim being made about it. + const forwarded = element!.props as Record; + + // Exact set, not a subset. A subset check cannot see the control key, and + // an extra key appearing here is precisely the regression this guards. + // + // If a future key is added to `FieldWidgetDomProps`, `toDomProps.ts`'s + // compile-time assertion forces it into `DOM_PASS_THROUGH_KEYS`, this list + // goes red, and whoever added it confirms delivery through this factory + // too. That red is the point, not a maintenance cost. + expect(Object.keys(forwarded).sort()).toEqual( + [ + // rendered by the factory itself + 'field', + 'value', + 'onChange', + 'readonly', + // the declared DOM pass-through block (`FieldWidgetDomProps`) + 'id', + 'name', + 'autoFocus', + 'tabIndex', + 'onBlur', + 'onFocus', + 'onClick', + // declared controlled-input keys the same executor forwards, because + // withholding them would make it a silent styling / interactivity + // dropper (see `toDomProps.ts`) + 'className', + 'disabled', + // the two open families, matched by prefix + 'aria-label', + PROBE, + ].sort(), + ); + + expect(forwarded).not.toHaveProperty('zzcanary'); + }); + + it('CONTROL: the undeclared key never reaches the DOM either', () => { + const { container } = render( + {}, + [PROBE]: 'probe', + zzcanary: 'CANARY-STR', + } as unknown as FieldWidgetComponentProps)} + />, + ); + + // The probe proves the render really carried a host set through, so the + // absence below is a measurement and not an empty tree. + expect(container.querySelectorAll(`[${PROBE}]`).length).toBeGreaterThan(0); + expect(container.querySelectorAll('[zzcanary]').length).toBe(0); + expect(container.innerHTML).not.toContain('CANARY-STR'); + }); +});