Skip to content

fix: Form primitives spacing - #110

Open
gcgoncalves wants to merge 2 commits into
mainfrom
6508-field-spacing-primitives
Open

fix: Form primitives spacing#110
gcgoncalves wants to merge 2 commits into
mainfrom
6508-field-spacing-primitives

Conversation

@gcgoncalves

Copy link
Copy Markdown
Contributor

Relates to IBM/mcp-context-forge#6508

Three root causes of inconsistent label-to-control gaps across forms:

  • Label: leading-none lived in the CVA base string, so a call-site text-sm silently stripped it via tailwind-merge (line-height utilities are treated as conflicting with font-size ones). Moved to a [data-slot="label"] CSS rule that no className can strip.
  • SelectTrigger: height was set via data-[size=*], an attribute selector that always beats a plain utility class in specificity, so call-site h-10 overrides were silently ignored. Replaced with a CVA size variant so heights merge normally.
  • SelectTrigger defaulted to w-fit while Input defaults to w-full, forcing every call site to re-add w-full. Default is now w-full.

Also introduces Field, a label/control/hint/error stack primitive, so future spacing changes live in one place instead of every form. Form migration to Field is PR2.

Three root causes of inconsistent label-to-control gaps across forms:

- Label: leading-none lived in the CVA base string, so a call-site
  text-sm silently stripped it via tailwind-merge (line-height
  utilities are treated as conflicting with font-size ones). Moved to
  a `[data-slot="label"]` CSS rule that no className can strip.
- SelectTrigger: height was set via `data-[size=*]`, an attribute
  selector that always beats a plain utility class in specificity, so
  call-site `h-10` overrides were silently ignored. Replaced with a
  CVA `size` variant so heights merge normally.
- SelectTrigger defaulted to `w-fit` while Input defaults to `w-full`,
  forcing every call site to re-add `w-full`. Default is now `w-full`.

Also introduces `Field`, a label/control/hint/error stack primitive,
so future spacing changes live in one place instead of every form.
Form migration to `Field` is PR2.

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core fixes are solid.

The new Field primitive has a11y gaps:

  1. [High] field.tsx:~116 — aria-invalid/aria-describedby silently no-op on composite children (e.g. Select) Wrapping <Select><SelectTrigger id="type">...</SelectTrigger></Select> in <Field id="type" error="Required"> clones the aria props onto <Select>, but Select (select.tsx:29-31) just spreads ...props onto SelectPrimitive.Root, a Radix context provider that renders no DOM element — the actual SelectTrigger never receives them. Verified empirically: trigger.getAttribute('aria-invalid') and aria-describedby both came back null. Since select.tsx is touched in this same PR and select-based fields are the next thing scheduled to migrate onto Field ("PR2"), this would silently drop error-state a11y wiring and styling (aria-invalid:border-destructive) on every select field.

  2. [High] field.tsx:~116 — aria-invalid/aria-describedby get cloned onto every child, not just the form control <Field id="name" error="Required"><Input id="name"/><Button>Clear</Button></Field> (a field with a trailing action) marks the Clear button as aria-invalid="true" and aria-describedby="name-error" too. Verified empirically: both Input and the sibling Button received the same attributes. Screen readers will announce the Clear button as invalid and "described by: Required," which is wrong.

  3. [Medium] field.tsx:~124 — hint text has no id and is never wired into aria-describedby <Field id="name" label="Name" hint="Must be unique"><Input id="name"/></Field> renders the hint paragraph with no id, and the aria-describedby injected onto the Input only ever comes from errorId (undefined here) or the child's pre-existing aria-describedby — never the hint. A screen reader user tabbing into the input gets no indication the hint exists, defeating the component's stated purpose.

  4. [Low] field.tsx:~112 — labelProps spreads after the auto-derived htmlFor, silently overriding it <Field id="email" label="Email" labelProps={{ htmlFor: "wrong-id" }}><Input id="email"/></Field> produces <label for="wrong-id"> that no longer targets the actual input, breaking click-to-focus and the screen-reader label association, with no warning.

Four review findings on the Field primitive:

- Cloning aria-invalid/aria-describedby onto a `<Select>` child was a
  no-op: Select's root renders no DOM node and never forwards those
  props to its SelectTrigger. Field now also accepts a render-prop
  child, so composite controls can apply the computed props to their
  actual DOM-facing element.
- Children is now a single ReactElement (or render function) instead
  of ReactNode, so a trailing sibling (e.g. a Clear button) can no
  longer be swept up by React.Children.map and tagged aria-invalid.
- hint text now gets an id and is wired into aria-describedby when
  there's no error; previously it had no id and was never referenced.
- labelProps spreads before htmlFor now (and the type omits htmlFor),
  so a caller can't silently detach the label from its control.

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
@gcgoncalves
gcgoncalves force-pushed the 6508-field-spacing-primitives branch from 17de2f7 to 1680947 Compare September 8, 2026 13:25
@gcgoncalves

Copy link
Copy Markdown
Contributor Author

@marekdano Addressed.

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 src/index.css:256 — line-height fix likely doesn't apply in production

The new line-height: 1 rule for [data-slot="label"] is wrapped in @layer components, but Tailwind's .text-sm utility lives in @layer utilities, declared later. Under CSS Cascade Layers, a rule in a later layer beats a rule in an earlier layer for the same property on the same element, regardless of selector specificity — so .text-sm's line-height (≈1.4286) still wins over this new rule. Since Label's CVA base class always includes text-sm, every rendered <Label> keeps the old line-height — this is the exact bug the PR claims to fix, and it still exists.

Verified against an actual vite build of this branch — the compiled CSS shows:

@layer components{[data-slot=label]{line-height:1}}
@layer utilities{...}.text-sm{font-size:var(--text-sm);line-height:var(--tw-leading,var(--text-sm--line-height))}

Vitest/jsdom tests pass because jsdom doesn't apply real cascade-layer semantics, so this isn't caught by CI.

The fix: declare the rule unlayered instead of inside @layer components, matching the pattern already documented a few lines above it in src/index.css ("Unlayered on purpose: Tailwind emits .overflow-y-auto into @layer utilities, which these rules have to outrank at equal specificity").


🟡 src/components/ui/field.tsx:28 — dual child API adds unnecessary surface (minor)

Field supports both a cloned single ReactElement child and a render-prop function. The render-prop form alone already covers every call site (including plain Input/Textarea), so the cloneElement branch is extra surface to maintain — it has subtly different prop-merging behavior (id always overrides, but aria-invalid/aria-describedby only fall back to the child's own value) and requires two Record<string, unknown> casts.

Suggest dropping the cloneElement branch and always using the render-prop form ({(p) => <Input {...p} />}) — removes ~10 lines and one path callers could pick incorrectly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants