From c14aa98811912225ed93f8cf8230f8378fc26ac8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 20:28:02 +0000 Subject: [PATCH] fix(components): built-in input branch reads the declared ceiling in both spellings (#5201) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The built-in `case 'input'` branch spread its leftover field props onto the element and never resolved the declared ceiling, so `max_length: 50` produced no cap at all plus a stray, inert `max_length="50"` attribute (invalid HTML), while `maxLength: 50` worked only by the coincidence that it names a real DOM attribute. Resolve `maxLength ?? max_length` locally in the branch and keep the legacy key off the DOM, mirroring what the neighbouring `textarea` branch does. The legacy key is destructured locally rather than added to the shared `stripRendererOnlyProps` list, which feeds every other branch. Ceiling only — no counter, no announced limit for the single-line input. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE --- ...builtin-input-max-length-dual-read-5201.md | 40 +++++++ .../form-builtin-input-max-length.test.tsx | 111 ++++++++++++++++++ .../components/src/renderers/form/form.tsx | 60 ++++++++-- 3 files changed, 204 insertions(+), 7 deletions(-) create mode 100644 .changeset/builtin-input-max-length-dual-read-5201.md create mode 100644 packages/components/src/renderers/form/__tests__/form-builtin-input-max-length.test.tsx diff --git a/.changeset/builtin-input-max-length-dual-read-5201.md b/.changeset/builtin-input-max-length-dual-read-5201.md new file mode 100644 index 000000000..9e9f21113 --- /dev/null +++ b/.changeset/builtin-input-max-length-dual-read-5201.md @@ -0,0 +1,40 @@ +--- +'@object-ui/components': patch +--- + +The built-in form `input` branch now honours a declared ceiling in both authored spellings. + +The branch spread its leftover field props straight onto the element and never +read the declared ceiling, so one declaration produced two different outcomes. +Measured on `origin/main`, rendering the built-in branch (no `registerAllFields()`) +and dumping the element's `getAttributeNames()` / `getAttribute('maxlength')`: + +| declaration | `maxlength` on the element | effect | +|---|---|---| +| `maxLength: 50` | `"50"` | capped — but only by the coincidence that `maxLength` names a real DOM attribute | +| `max_length: 50` | `null`, plus a stray `max_length="50"` | no cap at all, and invalid HTML | + +Two distinct defects: the missing cap, and an inert attribute on the DOM that +reads like a working cap to whoever greps the file next. + +`max_length` is a live authoring spelling, not a fossil. The registered +`field:*` widgets have dual-read `maxLength ?? max_length` since framework#1878 +§3, all three producers of a form field normalize it (`ObjectForm`, +`sectionFields`, `EmbeddableForm.applyDefaultMaxLengths`) and `@object-ui/types` +declares it on several field types. Every reader in the repo honoured it except +this branch — which is precisely the one serving a hand-written `FormSchema` fed +straight to the renderer, where no producer sits in between to normalize it and +the author is the producer. This is the same mechanism objectui#3439 resolved +for the built-in `textarea` branch. + +The legacy key is destructured off locally rather than added to the shared +`stripRendererOnlyProps` list: that helper feeds every branch +(`checkbox`/`switch`/`select`/`default` all share `domFieldProps`), so extending +it would change what reaches the DOM for widgets this change neither fixes nor +tests. The neighbouring `textarea` branch strips it the same local way. + +Scope, stated because the sibling card resolved more than this one: the ceiling +only. Whether a single-line input should also carry the visible `{n}/{max}` +counter and the announced limit that the `textarea` branch grew in +objectui#3439 is an independent design trade-off that does not follow from that +card's conclusion, and is deliberately left undecided here. diff --git a/packages/components/src/renderers/form/__tests__/form-builtin-input-max-length.test.tsx b/packages/components/src/renderers/form/__tests__/form-builtin-input-max-length.test.tsx new file mode 100644 index 000000000..49daa7490 --- /dev/null +++ b/packages/components/src/renderers/form/__tests__/form-builtin-input-max-length.test.tsx @@ -0,0 +1,111 @@ +/** + * 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 built-in (unregistered) `input` branch honours a declared ceiling in + * BOTH authored spellings — objectui#5201. + * + * ## What was measured on `origin/main` + * + * The branch spread `stripRendererOnlyProps(fieldProps)` onto the element and + * never read the declared ceiling, so one declaration split into two outcomes + * depending on how it was spelled: + * + * | declaration | `maxlength` on the element | effect | + * |---|---|---| + * | `maxLength: 50` | `"50"` | capped — but by COINCIDENCE: `maxLength` happens to name a real DOM attribute | + * | `max_length: 50` | `null`, plus a stray `max_length="50"` | NO cap at all, and invalid HTML | + * + * `max_length` is a live authoring spelling, not a fossil: the registered + * widgets dual-read `maxLength ?? max_length` (framework#1878 §3), all three + * producers of a form field normalize it (`ObjectForm`, `sectionFields`, + * `EmbeddableForm.applyDefaultMaxLengths`) and `packages/types` declares it on + * several field types. This branch is the one path with no producer in + * between — a hand-written `FormSchema` fed straight to the renderer — so on + * it the author IS the producer and nothing normalizes the spelling. + * + * ## Why the assertions read `getAttributeNames()` / `getAttribute()` + * + * The missing cap and the stray attribute are two DISTINCT defects, and a test + * that only checked the cap would let the invalid attribute survive the fix. + * Reading the attribute NAMES is what makes the stray half observable at all — + * it is how the card measured it. + * + * ## Scope + * + * The ceiling only. Whether a single-line input should also carry the visible + * `{n}/{max}` counter and the announced limit that the built-in `textarea` + * branch grew in objectui#3439 is an independent design trade-off that does + * not follow from that card's conclusion; the #5201 triage ruling explicitly + * left it undecided, so nothing here asserts a counter either way. + */ + +import React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ComponentRegistry } from '@object-ui/core'; +// Module scope, not `beforeAll` — the cold transform must not be billed to +// `hookTimeout`. See object-ui/no-dynamic-import-in-test-hook (objectui#3010). +import '../../../renderers'; + +/** Render the built-in branch: no `registerAllFields()`, so nothing resolves from the registry. */ +function renderForm(fields: any[]) { + const Form = ComponentRegistry.get('form')!; + return render( +
, + ); +} + +const textField = (extra: Record = {}) => ({ + name: 'title', + label: 'Title', + type: 'input', + ...extra, +}); + +const input = () => document.querySelector('input') as HTMLInputElement; + +afterEach(cleanup); + +describe('built-in input — the declared ceiling (objectui#5201)', () => { + it('applies a camelCase maxLength as the native attribute', () => { + // This spelling worked before the fix, by the coincidence that it names a + // real DOM attribute. Pinned so resolving the ceiling explicitly cannot + // break the spelling that accidentally worked. + renderForm([textField({ maxLength: 50 })]); + expect(input().getAttribute('maxlength')).toBe('50'); + }); + + it('applies the LEGACY max_length spelling too — it capped nothing before', () => { + renderForm([textField({ max_length: 50 })]); + expect(input().getAttribute('maxlength')).toBe('50'); + }); + + it('never leaks max_length onto the DOM as a stray attribute', () => { + renderForm([textField({ max_length: 50 })]); + // Not a DOM attribute in any spelling. Left in the pass-through it renders + // invalid HTML that reads like a working cap to the next reader — the + // second, independent half of this defect. + expect(input().getAttributeNames()).not.toContain('max_length'); + }); + + it('lets the canonical spelling win when both are declared', () => { + // `maxLength ?? max_length` — the resolution order every other reader in + // the repo already uses. + renderForm([textField({ maxLength: 50, max_length: 80 })]); + expect(input().getAttribute('maxlength')).toBe('50'); + expect(input().getAttributeNames()).not.toContain('max_length'); + }); + + it('leaves an uncapped field exactly as it was — no attribute either way', () => { + renderForm([textField()]); + expect(input().getAttributeNames()).not.toContain('maxlength'); + expect(input().getAttributeNames()).not.toContain('max_length'); + }); +}); diff --git a/packages/components/src/renderers/form/form.tsx b/packages/components/src/renderers/form/form.tsx index abc371fda..147092ec6 100644 --- a/packages/components/src/renderers/form/form.tsx +++ b/packages/components/src/renderers/form/form.tsx @@ -3005,26 +3005,72 @@ function renderFieldComponent(type: string, props: RenderFieldProps) { const readonlyInputClass = readonly && 'bg-muted/40 cursor-default focus-visible:ring-0'; switch (type) { - case 'input': + case 'input': { + // The declared ceiling is resolved HERE, in BOTH authored spellings, + // rather than left to ride the pass-through onto the DOM + // (objectui#5201). This is the same mechanism the `textarea` branch + // below resolves for the same reason (objectui#3439) — this branch was + // deliberately left out of that card because the COUNTER half is a + // design question for a single-line input; the ceiling half is not. + // + // Measured on `origin/main`, the pass-through answered the two spellings + // differently: a camelCase `maxLength` happened to work because it names + // a real DOM attribute, so the element got `maxlength="50"`; the legacy + // `max_length` reached the same element as a STRAY, inert + // `max_length="50"` attribute and the field had no cap at all — no + // truncation, and invalid HTML that reads like a working cap to whoever + // greps this file next. + // + // `maxLength ?? max_length` is not a tolerance invented at a consumer + // (AGENTS.md #0.1): the registered `field:*` widgets have dual-read it + // since framework#1878 §3, all three producers of a form field do + // (`ObjectForm`, `sectionFields`, `EmbeddableForm.applyDefaultMaxLengths`) + // and `packages/types`' field types declare `max_length`. Every reader in + // the repo honoured it except this branch — which is precisely the one + // serving a hand-authored `FormSchema` handed straight to the renderer, + // where there is no normalizing producer in between and the author IS + // the producer. + // + // The legacy key is destructured off LOCALLY — not added to + // `stripRendererOnlyProps` — because that helper feeds EVERY branch + // (`checkbox`, `switch`, `select` and the `default` fallback all share + // `domFieldProps`), so extending it would change what reaches the DOM + // for widgets this card neither fixes nor tests. The `textarea` branch + // strips it the same local way. + // + // Scope: the CEILING only. Whether a single-line input should also carry + // a visible `{n}/{max}` counter and an announced limit the way the + // `textarea` branch does is an independent design trade-off that does + // NOT follow from #3439's conclusion, and is deliberately not decided + // here (the objectui#5201 triage ruling). + const { max_length: _maxLengthLegacy, ...inputProps } = domFieldProps as any; + const maxLength = (fieldProps as any).maxLength ?? (fieldProps as any).max_length; if (inputType === 'file') { - // File inputs cannot be controlled with value prop - const { value, ...fileProps } = domFieldProps; - return ; + // File inputs cannot be controlled with value prop. No cap applies to a + // file picker, but the stray legacy key must not reach it either — it + // is off `inputProps` already. + const { value, ...fileProps } = inputProps; + return ; } return ( { openNativePickerOnClick(inputType)?.(e); - domFieldProps.onClick?.(e); + inputProps.onClick?.(e); }} readOnly={readonly} - value={domFieldProps.value ?? ''} + value={inputProps.value ?? ''} /> ); + } case 'textarea': { // `mobile_fullscreen` is the flag's ONE spelling (objectui#3303). This