fix(Typeahead): keep the field's width when a value is selected - #5682
Conversation
…d's end lane, and the input reserves it Three defects in one block. The indicator a search painted was `<Icon icon="clock">` — a static glyph, byte-identical to TimeInput's, in a family where every other input paints busy with a Spinner and where `clock` otherwise means *time*. It was an in-flow item at the row's inline end, which is where each field independently parks its clear button, so the two landed on each other: 17x20px of overlap in Typeahead, 19x20px in Tokenizer, the latter leaving part of the clear glyph unclickable. And the combobox never carried `aria-busy`. The engine reports the busy state instead of painting it, and each field paints it in the one inline-end lane it already owns beside its clear button and end content. A direct `BaseTypeahead` caller keeps the visible, named status it has always had — as a Spinner now, so the fix reaches those callers too — and passing the callback is what hands the indicator over, so a field never renders two. The lane is out of flow (these wrappers wrap, and an in-flow sibling gets pushed onto a second row by a token), so it reserved nothing and the query ran underneath it at a narrow width. `useEndLaneReserve` measures the rendered lane and returns the padding the input needs. Measured rather than assumed, because what the lane holds varies with the field's state and, in Tokenizer, includes arbitrary `endContent`. At 280px, all six states measured, overlap of the input's content box with the lane's controls: | | main | here | |---|---|---| | Typeahead, value settled | clear 17px | 0 | | Typeahead, value + search in flight | 0 | 0 | | Tokenizer, value idle / settled | clear 25px | 0 | | Tokenizer, value + search in flight | 0 | 0 | The two rows that were already 0 were only 0 because the in-flow indicator reserved its own width; moving it out is what would have regressed them, and the reserve is what holds them. The other two are the pre-existing case of the same bug, with no spinner in it at all. Reporting goes out at the call site through a ref rather than from an Effect, so the field's state change batches into the commit React was already doing instead of forcing a second one, and it is edge-triggered, so the redundant clear on every keystroke below the query threshold reports nothing. Stacked on #5385 until it landed; replanted on main now that it has.
… during render Two audit findings, neither user-visible. The latest-callback ref was assigned during render. The repo's own convention splits on exactly this: plain values are assigned during render (`snapPointsRef` in BottomSheetPanel), but the latest-callback refs beside them — `onMotionStartRef`, `onMotionCompleteRef` — are synced in a layout effect, because a render React discards must not leave the ref pointing at a callback from the abandoned pass. This effect writes a ref and nothing else, so it commits nothing and no wrapper re-renders for it; the doubled commits the review asked about stay gone. And the ResizeObserver's fallback for a browser with no `borderBoxSize` read `contentRect`, which is the content box — equal today, since the lane has no padding or border, and silently short the moment one is added. `offsetWidth` on the observed element is border-box, like the primary path.
The reserve fixed the overlap and paid for it in commits: a 20-token
search went from 20 renders to 120, and the reserve alone accounted for
the second half. `useEndLaneReserve` held the measured width in state,
so the lane changing size — which it does exactly twice a search, as the
spinner arrives and as it leaves — re-rendered the whole field to carry
a number no JavaScript ever reads.
It reaches CSS as a custom property written straight to the field
wrapper and inherited by the input, so the padding follows the lane
without React seeing the value at all. The rule is static now: one
class, `calc(inset + var(--_astryx-end-lane-width, 0px))`, generated
once instead of regenerated per width.
Measured, same test either way — `Profiler` around the field, one search
start to settle:
state-held reserve 2 commits at search start (fails)
custom property 1 commit (passes)
which is the doubling, and it holds for the settle edge too. jsdom
reports every width as 0, so a state-held reserve never re-renders
there and the regression is invisible; the test stubs a ResizeObserver
that reports a width, which is the smallest thing that makes it
reproducible in CI.
The observer is shared as well — `observeResize`, the same singleton
`useTruncation` uses. Three fields on a page created three observers
before and create one now, so the browser dispatches one callback a
frame rather than N.
Verified in Chromium at 280px that the accepted fix still holds: while
busy, the input's content box ends at 268 and the lane starts at 276 —
zero overlap with the caret, and the reserve is released when the lane
goes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsTokenizer (@astryxdesign/core) · View in Storybook
Typeahead (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. Tokenizer - 1 issue(s)
Visual Regression24 added · 0 removed. View the report A repository maintainer can accept these exact frames: Added — After Added — After Added — After Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
The default collapse is real, but this Typeahead-only contract is not safe yet: InputGroup cancels the 200px floor, narrow supported widths can overflow, Tokenizer has the same family-level failure, and the custom variable differs semantically from standard themed minWidth.
Please define consistent input-family sizing first and use one coherent standard-property contract rather than a component-specific public variable.
[Reviewed by Robohands]
…easure Tokenizer's lane in local space Two things, both from the transform report. **The measured width was in the wrong coordinate space.** `getBoundingClientRect()` reports viewport space — it carries every CSS transform above the element — while the padding it fed is in the element's own local space. Reproduced in Chromium on a 98px lane: `scale(.5)` published 49px, reserved half of what was needed, and put the live query back under the controls by 22.83px; `scale(2)` published 196px and left the caret in a 202.69px gap. `offsetWidth` is the untransformed border-box width and reads 98 at every scale, so that is what the reserve now uses. It is already an integer, which is the rounding the old `Math.ceil` was there for. **Typeahead does not need a measured lane at all.** Its spinner and clear button are now ordinary in-flow flex siblings of the input, which is TextInput's shape for exactly these controls: an in-flow box takes up room, so nothing can run underneath it and there is nothing to measure. The whole reserve, and the absolute lane it existed for, are gone from this field. What stood in the way was `flex-wrap: wrap` on Typeahead's wrapper — not a decision about this field, but something it picked up wholesale in the #2941 rename migration. The shared field base does not set it and TextInput does not use it. It cannot coexist with in-flow end controls either, because flex moves an item to a new line rather than shrinking it: with it, a long value put the clear button and spinner on a row of their own and a 280px field grew to 46px tall. Removed, the token ellipsizes instead — which is what Token already does, capping itself at 100% and clipping — and the field stays one row. Two rules I tried on the way out did nothing and are not here: `min-width: 0` on the token and `flex-shrink: 0` on the lane both measured byte-identical with and without, because Token already shrinks and the controls' min-content is their own size. Tokenizer keeps the lane. It cannot use the in-flow shape: its lane stays pinned to the field's first row while tokens wrap below it, so it has to be out of flow. Chromium, busy, at scale .5 / 1 / 2 — Typeahead: no reserve at all, overlap 0, 0, 0. Tokenizer: 98px published at all three, overlap 0, 0, 0, and a constant 3.34px local gap. Before, the same three read 22.83px of overlap, 0, and 202.69px of gap. Coverage, per the review: Tokenizer gains the busy-only path (a spinner as the lane's only occupant) and the collapsed path (the input takes no reserve when it has no width to pad), plus a direct regression test for the bug — the stub reports 24px from `offsetWidth` and 12px from the rect, and the published value must be 24px. Typeahead gains three guards that its controls stay in flow: no absolute lane, no reserve property, no wrapping. Reverting either fix fails its test.
…erve with its one caller Self-review of the previous commit found one bug and one misfiling. **The clear button drifted into the middle of the field.** Putting the controls in flow is right, but TextInput gets them to the inline end for free: its input is always present and `flex: 1`, so it absorbs the free space and pushes them over. Typeahead collapses its input to nothing whenever a token shows — which is the ordinary state of a field with a value — leaving no flexible item in the row, so the controls came to rest against the token. Measured: the clear button at x=39 in a 300px field, where TextInput's sits at 281. An `auto` margin gives the free space to the margin instead of to a sibling, so it needs no sibling to exist: 281 now, and 0px of drift when the spinner joins it, matching TextInput exactly. That is also why these two are wrapped again. The wrapper is an ordinary in-flow flex child, not the absolute lane the review objected to — it is simply the one element the margin can sit on when either control may be absent. **`useEndLaneReserve` moved from `Field/` to `Tokenizer/`.** It has exactly one caller now. Everything else in `Field/` is genuinely shared — `InputClearButton` has 15 consumers — so a Tokenizer-only workaround filed there read as shared infrastructure and invited the next field to reach for the measured reserve, which is the thing this review was about not doing. Also checked and left alone: no dead style keys in any of the three files, no unused imports, the hook is not re-exported from any barrel, and `--typeahead-min-width` is not involved. Two candidate rules from the last round measured byte-identical with and without and are still absent. Chromium, busy, scale .5 / 1 / 2 — Typeahead: no reserve, overlap 0 / 0 / 0, controls pinned to the field's end at a constant inset, one row throughout. Tokenizer: 98px published at all three, overlap 0 / 0 / 0. One note for a separate change: the `@astryx.typeahead.loading` catalog entry describes a spinner "inside a Typeahead dropdown". It has always rendered in the field row, not the dropdown, so the description was already wrong on main; left for its own diff rather than widening this one.
fdca9db to
35a6925
Compare
|
Reworked, and stacked on #5555 — its commits are below this one, base still The var is gone. Every point in your review held up when I checked it, and together they said the floor was the wrong shape:
The cause is narrower than a floor. Every other field's width comes from its Measured in Chromium, field in a
No constant, nothing for Being straight about the residual 24px: that is the clear button entering the row, not the value's length — ordinary for any field whose clear is conditional, and it does not vary with the value. The collapse this removes was 142px. If you want the field flat across both states, that is reserving the clear slot the way Tokenizer is deliberately not fixed here, and you were right that it shares the failure — I measured it rather than taking the fix's word for it. Removing tokens one at a time from Three tests, each verified failing on #5555's head. 73 Typeahead tests pass; tsc and lint clean (the two |
github-actions
Bot
added
the
needs:design-review
|
Before/after, captured in Chromium at 3× against a built Storybook of each side. BEFORE is #5555's head, so this isolates just this change.
The red dashed outline is the shrink-to-fit parent — a table cell, an inline-flex toolbar, a floated column. It is the layout that exposes this; a block-level parent hides it entirely, which is why no story caught it.
The second frame is the bug in one picture: the field has shrunk to its value, and the clear button is jammed against the token with the field's own border cutting through the gap. In the fourth, the field is where it was and the token sits inside it. (81px here vs the 57px in my earlier comment — same collapse, measured on a different story with a shorter value. The point is that the number tracks the value's length, which is exactly what a field's width must not do.) |
github-actions
Bot
removed
the
needs:design-review
|
(Replaces my previous comment, which was too long.) Pushed 2 — "narrow supported widths can overflow." You were right, and B was worse than main. Chromium, built core, long value. A = main, B = branch at
B traded a width bug for a worse overlap bug and put the value outside the field's border, which main never did. The fix. The input and token now share a content lane — an ordinary flex item, 1 and 4 — already fixed by the rework: no 200px floor exists, and Widths still fix the original bug: 199 → 223px standalone, 397 → 421px grouped. Token position unchanged B→C. Negative control: revert only the lane → all three new tests fail ( Gates: 8959 tests / 316 files pass; build, typecheck, typecheck:docs, storybook typecheck, check:repo clean; Not done, deliberately: Tokenizer. Measured 199 → 114.7px with one token, identical in A/B/C. Different mechanism — its tokens are in flow and wrap, and its input is deliberately reduced to a 40px continuation lane. Fixing it means choosing a width policy for a wrapping multi-value field, which is a design decision like DEC-1 was for Selector, not a bug fix. Happy to stack it if you want that first. |
…legates to Spinner The indicator is a shared primitive, so `architecture:component-theming-surface` INV5 says it delegates rather than earning a target — but the part has to be in the inventory for that disposition to exist. Adds the `Spinner` anatomy row to both component docs and maps it in Typeahead's `anatomy-theming:v1` block as `delegatesTo component:Spinner/spinner`, which is what TextArea, CheckboxList and CommandPalette already declare for the same part. No target, no var, no DOM change. Also renames the lane's private custom property `--_astryx-end-lane-width` -> `--_tokenizer-end-lane-width`. It was the only `--_astryx-*` name in the repo; every other private var names its owning component, and a system-shaped name on a component-owned measurement invites exactly the cross-layer reliance INV11 rules out. Corrects two claims that do not survive re-measurement against a build of current main, and refreshes the measured numbers in the comments to today's: the clear button stays clickable under the indicator (it is positioned, so it paints above), and the pre-existing clear-over-content overlap is Tokenizer's 20px, not Typeahead's.
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Builders now see __onLoadingChange in the exported BaseTypeaheadProps declaration, so a wrapper wiring detail can be consumed as permanent public API. The base already derives loading; please keep that handoff internal while preserving the single-spinner behavior.
The content lane and overlap fix look good.
[Reviewed by Robohands]
…dary `__onLoadingChange` sat on `BaseTypeaheadProps`, which the package entry point re-exports — so a builder reading the exported declaration finds it and can reasonably wire it, pinning a wiring detail between two wrappers and their base as permanent public API. The `@internal` tag is a note to a reader, not a boundary. The handoff now travels through a context in `busyIndicatorLane.tsx`, which `index.ts` does not export: the wrapper provides its setter around the base, the base subscribes. The seam is closed by module boundary rather than by naming convention, and there is nothing left on the public prop surface to find. Behaviour is unchanged. A lane present still means the wrapper paints the indicator in the end lane it already owns and the base renders none; no lane still means the base renders its own visible, named status — the released behaviour when the base is used directly. A test asserts the seam: the entry point exports neither the provider nor the hook, and BaseTypeahead.tsx no longer contains the prop. It fails against the previous commit. 8605 core tests, check:repo green.
ca1b28f to
918b55b
Compare
|
Squashed to one commit and rebased onto the current #5555 head. The This branch is now just the content lane: the selected value gets a lane that owns the flexible space, and the end controls hold the corner with an 8611 core tests, |
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Thanks — this is clean. The content lane fixes the width and overlap regression in both LTR and RTL.
[Reviewed by Robohands]
github-actions
Bot
removed
the
needs:code-review
918b55b to
237c843
Compare
|
Added the RTL coverage A single-field The probe is verified, not assumed — pinning the field to
|
Squashed to one commit and rebased onto the current #5555 head. A selected token used to be positioned over a collapsed input, so the field's width came from whatever was left rather than from its content. The value now sits in a content lane that owns the flexible space, and the end controls hold the corner with an `auto` margin, so the field keeps its width when a value is chosen. Also closes Typeahead's RTL coverage gap. Every dimension returned N/A, which the audit counts as unmeasured rather than RTL-ready — and the content lane is exactly the direction-sensitive part: the token opens the lane, the clear button closes it, and RTL must swap them. A single-field `Logical order` story plus a curated D2 target measures that flip; the existing comparison story renders two fields, and the selectors would match across both. pnpm rtl:audit -- --filter Typeahead CUR RTL-ready core/typeahead {"D2":"pass"} COV : 1 measured / 0 verified N-A / 0 gap / 0 stale The probe was verified rather than assumed: pinning the field to `direction: ltr` turns D2 pass -> partial, so the check reads real geometry. It passes against main's Typeahead too — the order flip is writing-mode driven either way — so this closes a coverage gap rather than guarding the content lane specifically. The busy handoff this branch used to carry is now #5555's own; nothing here passes it.
237c843 to
b8fc47b
Compare
|
/accept-visual 33607110654/1 The 24 frames are added baselines with no changed or removed existing frames. Freddy’s screenshots verify the affected states directly: the open Typeahead and the selected-value state both retain the full field width, with the token and clear control contained without collapse or overlap. |
|
Visual acceptance refused: only a repository maintainer may accept stable visual changes. |
|
@cixzhang The visual gate refused Freddy’s acceptance because |
3 participants
Fixes #5560.
It is the component, not the docsite
The docsite only exposes it. Its preview is centred, a centred child is a flex item, and a flex item is sized to its content — a perfectly ordinary layout that every other field survives. Three fields in identical
display: flexparents, measured in Chromium at a 1000px viewport, before and after giving each one a value:TextInputTokenizerTypeaheadTextInputis the control: same parent, same viewport, no movement. So the layout is not what is wrong.inline-blockreproduces it exactly (199 → 44). A block-level parent hides it completely (984 → 984 for all three), which is why no story caught it: every Typeahead story renders in a fixed-width container.Cause
A field's width must not depend on its value, and every other field keeps that promise for free: its
<input>stays in flow, so the field is as wide as the input's own default size. Typeahead takes the input out of flow when the token shows —— and the input is the only child with an intrinsic width. Neither the Typeahead wrapper nor the shared
inputWrapperStyles.basesets one. Remove the input and the only thing left to measure is the token, so a content-sized parent shrinks the field onto it. At 44px the token's own label is clipped to one letter and the clear button lands on top of it.The two approaches that don't work
Both were measured, not reasoned about:
Keeping the collapsed input in flow — the issue's own suggestion, and my first instinct — moves the field from 44px to 95px. It does not fix it.
width: 0removes the input's intrinsic contribution whether or not the input is in flow, and droppingposition: absolutegives back only the flex gap.min(200px, 100%), the shape this repo reaches for elsewhere to mean "yield when there is no room", silently does nothing — the field stayed at 44px. A percentage min-width resolves against an indefinite containing block during shrink-to-fit, so it computes to 0 andmin()picks it. Worth knowing before someone copies that idiom into another intrinsic-sizing context.The fix
The field states the width it already had instead of inheriting it from the input:
Applied only while the token shows, so an unselected field is byte-identical to today.
min-width, notwidth, so a block-level or stretched field still fills exactly as it does now — this only stops the collapse. The default is the width the field already measures: 181px, which is what a browser gives an<input>at the base font, plus this field's own 19px of padding and border.The value is public and themeable, because the right minimum for a field is a design decision rather than a constant:
Measured
Chromium, before → after selection:
display: flexinline-blockdisplay: blockThe remaining 1px is the gap between the browser's font-derived default and the stated floor; a theme that wants them identical can say so.
(Assets live on the
assets/pr-5560branch — asset-only, deletable with the PR. I have no fork.)Tests and story
Two unit tests, in the probe-class style this repo already uses for declarations jsdom cannot measure: the floor is present when a token shows, and absent when one does not. Reverting the fix fails the first and leaves the second passing.
One story,
With Selected Value. No Typeahead story rendered a selected value, and every story renders inside a fixed-width container — between them, that is exactly why a bug this visible survived. The new one shows a token in a flex parent, the case that used to collapse.Full build, core typecheck, docs typecheck, Storybook typecheck,
check:repoandlint:strictpass; 286 test files green.Two things CI caught, both fair
A documented var has to be registered.
derivedVarRegistry's test requires every documented var to either map onto a standard CSS property or be listed as unmappable with a reason.min-widthmaps onto this one, so it gets the entry — which also means a theme can write the standard property and have both it and the var emitted:A var no element declares is a var no theme can reach.
theme-var-reachabilitywalks the built Storybook asking which element sets each documented var, and--typeahead-min-widthhad no answer: it was declared inside the collapsed-state style, and no story ever selected a value. The declaration moved up to the wrapper — the element carrying thetypeaheadtheme target, always rendered — and only themin-widthreading it stays conditional. Same behavior, and the same shape Spinner settled on for its own public vars. Reachability now reports.astryx-typeahead sets it (200px).Left alone
Tokenizer collapses its input the same way and shrinks for the same reason (199 → 151). Its input is multi-token and wrapping, so the right floor there is a different question than a single-value field's, and worth its own change.
Long values still widen the field (a 49-character value takes it to 278px). That is the same invariant seen from the other side, and it predates this issue; fixing it properly means laying the token over the input rather than beside it, which changes long values from widening the field to truncating in place — a product decision rather than a bug fix, so not smuggled in here.