Skip to content

Review 5418

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

#5418 — feat(TextArea): expose theme targets for the control, slots and counter

freddymeta (contributor; head on facebook/astryx) · OPEN, changes requested · view on GitHub

Verdict: request changes — on one small thing, not on the padding work.

Head reviewed: f5f79ec28a5de4c29cc9f9df73a57e0230493cb0, unmoved across three gates. Reviewed as one of a SET of four rows of #5417 — see Review-5426 for the set-level findings.

Three gates, and the verdict moved twice. Worth recording because the movement is the content:

gate verdict what moved it
1 request changes measured a themed inset stranding the start icon
2 approve the same strand is reachable on plain main with no target from this PR — R13, not his
3 request changes Cindy ruled the stranding is a bug to fix, not a limitation to document; and the investigation found a second, larger defect

Problem

A theme author who wants EPS's typography or counter treatment on a TextArea has nowhere to put it: only the wrapper is themeable, and the <textarea> and the counter are bare stylex.props. The theme ships .astryx-textarea > textarea { … !important } and .astryx-textarea > textarea ~ div { … } — element-and-order selectors that keep parsing and silently stop matching on any restructure.

Solution (1 decision · ~22 runtime lines of 35)

1. two elements inside the wrapper get themeProps: the <textarea>
   (reflecting size) and the character counter — TextArea.tsx:613, :640

The body promises four targets and ships two; the changeset is accurate and explains the trim.

Impact

Nobody today — one class and one data-size, no geometry change. Three of the five rules in EPS's TextArea.css retire once a theme names the keys.

These work in a production build: #5410 merged the same morning (c47e75495d5).

What merging newly exposes, and it is why the verdict came back to request changes: textarea-control carries a size axis whose stated purpose in the body is per-size insets — the one thing the target cannot deliver correctly. The first thing a themer reaches it for is the thing that strands the icon.

API

no API change.

Theme targets

+ astryx-textarea-control  → the <textarea>, reflecting size   (public, permanent)
+ astryx-textarea-counter  → the character-counter <div>       (public, permanent)

Asked for astryx-textarea-control without the size axis.

Ossification

textarea-control matches command-palette-input, pagination-input-total, chat-composer-input — the inner control of an already-targeted wrapper, all landed. textarea-counter sits with empty-state-description and pagination-input-label. Both follow <root-target>-<part>, which holds across all 238 themeProps() call sites on main.

The size axis is the part that fails: a permanent data-* attribute justified by a case another mechanism owns, on an axis that carries one property.

Breaking

  • API — no.
  • Visual — no. No stylex.props argument added, removed or reordered.
  • Theme — no. astryx-textarea and --_textarea-inline-padding untouched.

Performance & resources

Nothing. Two mergeProps calls replacing two bare spreads. No effect, no listener, no observer, no layout read. Not measured for render counts; nothing about identity or dependencies changed.

Visual evidence

Real Chromium, 520px, Storybook dev at the PR head. Frames published to assets/pr-5418 on the fork and embedded in the posted review.

what it shows frame
default — grip 1px off the corner grip__baseline.png
a themed padding: 20px — grip pushed to 21px grip__padding-shorthand.png
this PR's target stranding the icon after__themed-inset.png
the inline axis tracking (the derived entry) control__inline-axis-tracks.png

Probe: probe-kit/textarea-padding-mechanisms-5418.cjs, SB_PORT=6610.

What the investigation found that neither review had

Cindy's ruling was "we will need an internal var and for the padding to derive it", and she asked whether a private var was sufficient or whether expand: 'container' did it better. Driving every spelling answered both and turned up two things:

1. A private var catches one spelling in five. generateThemeRules.ts:426 resolves a vars: entry by exact property-name match. Measured:

paddingInline: 20px    TRACKS      grip intact
padding: 20px          STRANDED    grip 1px → 21px
paddingInlineStart     STRANDED    grip 1px → 21px
paddingBlock: 20px     STRANDED    grip 1px → 21px
paddingTop: 20px       STRANDED    grip 1px → 21px

So it is not a block-axis gap — the inline axis strands too. Both the first posted comment and gate 2 had framed it as block-only. The posted review corrects it explicitly rather than leaving both versions standing.

2. A themed padding breaks the native resize grip. The wrapper zeroes its own padding deliberately, and its own comment at TextArea.tsx:73-78 predicts the failure — "pushing the native resize grip in from the corner" — which is exactly what a raw padding on .astryx-textarea does. Nothing enforces it.

3. expand: 'container' subsumes her ruling rather than competing with it. It is the same idea — an internal var the padding derives — using normalized --astryx-textarea-padding-* tokens instead of one private var, which is what lets it parse all seven logical spellings plus paddingTop/paddingBottom. Measured tracking on all three block spellings. Cited to the author's own merged #5181, which removed from NumberInput the hand-rolled entry TextArea still carries.

4. A naive derivation is subtly wrong. The overlays are not at the padding edge: the control's block padding is --spacing-1 (4px) and the start icon's top is --spacing-2 (8px), a deliberate optical offset so the glyph centres against the first line. Reading the padding straight gives text +16px against icon +12px — tracking, and 4px out. Found by making the mistake in the probe.

Judgement

request changes

1. [BLOCKS] `visualProps: ['size']` on `textarea-control` is permanent surface
   whose only stated purpose is per-size insets, which the padding expansion
   will own
   → a theme author sets a per-size inset through `data-size` on the control —
     what the PR's body tells them it is for — and the start icon and end slot
     stay where they were
                     · TextArea.doc.mjs:190 (+ :387 docsZh) · TextArea.tsx:169-175
   confirmed two ways: read the atomics (`sm` and `md` are empty objects, `lg`
   carries only `paddingBlock`), AND measured the strand across five spellings.

2. [not blocking — pre-existing, R13] four of five padding spellings strand the
   overlays, and three push the resize grip 20px in from the corner
   → a theme author writes `padding` on the TextArea and gets a control whose
     icon floats at the top edge and whose resize handle is adrift
                     · TextArea.tsx:135, :142 · derivedVarRegistry.ts:84
   The fix is `expand: 'container'`. The follow-up he already scoped is the
   right home; the ask is the shape, not the work.

3. [not blocking] the body describes four targets; two shipped

The review, as posted

Posted as a CHANGES_REQUESTED review with one inline. It deliberately does not repeat the earlier comment (issuecomment-5400672610), which already carried the expand: 'container' advice, the card/dialog/ context-menu/section precedent, the #5181 citation and the RTL warning (R2j).

One thing to change here before it lands: drop visualProps: ['size'] from textarea-control — inline below. That's the only thing blocking; the padding work stays your separate PR.

Two corrections to what I said above, both measured on this head.

It isn't a block-axis gap — the inline axis strands too. Only the one exact spelling tracks:

a theme writes overlays follow?
paddingInline: 20px yes
padding: 20px no
paddingInlineStart: 20px no
paddingBlock: 20px no
paddingTop: 20px no

And the resize grip isn't only a thing to watch when you do the fix — a themed padding already moves it, from 1px off the corner to 21px:

today padding: 20px
default grip pushed in

One thing that'll save you a round: the icon's top is --spacing-2 against the control's --spacing-1. Keep that 4px offset — reading the padding straight looks like it tracks and lands 4px out.

If you'd rather talk it through, we're in Discord.

[Reviewed by Robohands]

Inline, packages/core/src/TextArea/TextArea.doc.mjs:190:

textareaSizeStyles is sm: {}, md: {}, lg with only paddingBlock — the axis is block padding, which the expansion takes over.

128 prose words against the 150 cap. The size ask's detail moved into the inline rather than being compressed in the body — R2h's shape, and it is what brought a 164-word draft under cap without deleting the build note.

What changed before posting

The verdict, twice (table at the top). The body was rewritten once more at post time to avoid repeating the earlier comment, and the size detail was moved to an inline for length.

For the nightly follow-up

Cindy will pick this up in the nightly. Two things to check, in this order:

  1. Did he drop visualProps: ['size'] from textarea-control? That is the only blocking item. If yes and CI is green, this is an approve-and-merge — nothing else is outstanding. Re-review per R1e against the new head.
  2. Did the padding follow-up appear? He scoped it himself in this PR's changeset. If it has, it is a fresh review and the shape to check is {property: 'padding', expand: 'container'} replacing the hand-rolled paddingInline entry at derivedVarRegistry.ts:84, the overlays reading --astryx-textarea-padding-*, and the 4px optical offset preserved.
  3. If neither has moved in two days, this is the nudge ladder (R24) — one nudge, ever.

The grip defect is reachable on main today and is not tracked by this PR. If #5418 merges without the follow-up, that needs its own issue or it evaporates (R24's second orphan shape).

Not verified

  • Whether expand: 'container' is right for TextArea specifically given the wrapper's deliberate padding: 0. The expansion replaces the raw property with tokens, so the grip survives — measured — but which element reads which token is the follow-up's design.
  • mechanism-b__tracks.png is a hand-written approximation of the token reads, not the shipped component. The numeric table is the faithful evidence.

Re-review — 2026-08-25

Versions: Review Loop unversioned (pre-1.0) · Component Audit Rubric 1.10
Head: e167021d · Verdict: approve; hold merge until required CI reruns

All prior architecture asks are satisfied: the size axis is gone, targets sit only on painting elements, and names follow the component-part convention. Five hundred tests passed; default main/head renders were byte-identical; new targets worked in Chromium.

Review as posted:

Thanks — the requested size axis is gone and the target architecture is right. Please refresh the stale body; syncing main will also pick up the visual-job fix.

[Reviewed by Robohands]

Clone this wiki locally