From 7001872876c5d9702f11af631e985838e65e988d Mon Sep 17 00:00:00 2001 From: freddymeta <170298698+freddymeta@users.noreply.github.com> Date: Wed, 26 Aug 2026 20:24:44 -0700 Subject: [PATCH 1/8] fix(Typeahead,Tokenizer): the busy indicator is a Spinner in the field's end lane, and the input reserves it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in one block. The indicator a search painted was `` — 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. --- .../typeahead-tokenizer-busy-indicator.md | 13 ++ apps/storybook/stories/Tokenizer.stories.tsx | 42 ++++++ apps/storybook/stories/Typeahead.stories.tsx | 42 ++++++ packages/core/src/Field/useEndLaneReserve.ts | 107 ++++++++++++++ packages/core/src/Tokenizer/Tokenizer.tsx | 36 ++++- packages/core/src/Typeahead/BaseTypeahead.tsx | 79 +++++++--- .../core/src/Typeahead/Typeahead.test.tsx | 136 ++++++++++++++++++ packages/core/src/Typeahead/Typeahead.tsx | 71 ++++++--- 8 files changed, 488 insertions(+), 38 deletions(-) create mode 100644 .changeset/typeahead-tokenizer-busy-indicator.md create mode 100644 packages/core/src/Field/useEndLaneReserve.ts diff --git a/.changeset/typeahead-tokenizer-busy-indicator.md b/.changeset/typeahead-tokenizer-busy-indicator.md new file mode 100644 index 0000000000000..ebb82078a6c57 --- /dev/null +++ b/.changeset/typeahead-tokenizer-busy-indicator.md @@ -0,0 +1,13 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] Typeahead, Tokenizer: the busy indicator is a Spinner in the field's end lane, and the input keeps its text out from under it (#5555) + +Three defects in one block. The indicator a search painted was `` — a static glyph, 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: 17×20px of overlap in Typeahead and 19×20px in Tokenizer, the latter leaving part of the ✕ unclickable. And the combobox never carried `aria-busy`, unlike every sibling input. + +The base engine now reports the busy state to the field, which paints it in the one inline-end lane it already owns beside its clear button and end content, and sets `aria-busy` on the input. A caller using `BaseTypeahead` directly is unaffected: it still renders its own visible, named "Loading" status, now a Spinner rather than the clock. + +The lane is absolutely positioned — these wrappers wrap, and an in-flow sibling gets pushed onto a second row by a token — so it reserved no space, and at a narrow width the query ran underneath it. The input now reserves the lane's measured width, which also closes a pre-existing case of the same bug: at 280px with a value selected, the clear button covered 17px of the live query in Typeahead and 25px in Tokenizer before this change, with no spinner involved at all. Both are 0 now. + +@freddymeta diff --git a/apps/storybook/stories/Tokenizer.stories.tsx b/apps/storybook/stories/Tokenizer.stories.tsx index 35c84871efab8..10d7ce7bbb728 100644 --- a/apps/storybook/stories/Tokenizer.stories.tsx +++ b/apps/storybook/stories/Tokenizer.stories.tsx @@ -25,6 +25,27 @@ const userSource: SearchSource = { bootstrap: () => users.slice(0, 5), }; +/** + * A remote source, near enough — the busy state only exists between the + * keystroke and the response, so a synchronous source never shows it. + */ +const slowUserSource: SearchSource = { + search: (query: string) => + new Promise(resolve => { + setTimeout( + () => + resolve( + users.filter(u => + u.label.toLowerCase().includes(query.toLowerCase()), + ), + ), + 1200, + ); + }), + bootstrap: () => + new Promise(resolve => setTimeout(() => resolve(users.slice(0, 5)), 1200)), +}; + const meta: Meta = { title: 'Core/Tokenizer', component: Tokenizer, @@ -454,3 +475,24 @@ export const StatusVariantComparison: Story = { ); }, }; + +export const Loading: Story = { + render: args => { + const [value, setValue] = useState([users[0]]); + return ( + setValue(items)} + hasClear + endContent={{value.length} selected} + /> + ); + }, + args: { + label: 'Team Members', + placeholder: 'Search people...', + }, + name: 'Loading (async source, with clear and end content)', +}; diff --git a/apps/storybook/stories/Typeahead.stories.tsx b/apps/storybook/stories/Typeahead.stories.tsx index 4acadf6d44a24..4fb2d2e5421e2 100644 --- a/apps/storybook/stories/Typeahead.stories.tsx +++ b/apps/storybook/stories/Typeahead.stories.tsx @@ -24,6 +24,28 @@ const fruitSource: SearchSource = { bootstrap: () => fruits.slice(0, 5), }; +/** + * A remote source, near enough. The busy state only exists between the + * keystroke and the response, so a synchronous source never shows it — which + * is why the indicator went unexercised long enough to ship as a clock. + */ +const slowFruitSource: SearchSource = { + search: (query: string) => + new Promise(resolve => { + setTimeout( + () => + resolve( + fruits.filter(f => + f.label.toLowerCase().includes(query.toLowerCase()), + ), + ), + 1200, + ); + }), + bootstrap: () => + new Promise(resolve => setTimeout(() => resolve(fruits.slice(0, 5)), 1200)), +}; + const meta: Meta = { title: 'Core/Typeahead', component: Typeahead, @@ -265,3 +287,23 @@ export const StatusVariantComparison: Story = { ); }, }; + +export const Loading: Story = { + render: () => { + const [value, setValue] = useState(null); + return ( +
+ +
+ ); + }, + name: 'Loading (async source)', +}; diff --git a/packages/core/src/Field/useEndLaneReserve.ts b/packages/core/src/Field/useEndLaneReserve.ts new file mode 100644 index 0000000000000..a26cae5f02bda --- /dev/null +++ b/packages/core/src/Field/useEndLaneReserve.ts @@ -0,0 +1,107 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +'use client'; + +/** + * @file useEndLaneReserve.ts + * @input Uses React + * @output Exports useEndLaneReserve, which measures a field's inline-end + * lane and returns the padding the input needs to clear it. + * @position Shared field internal. Used by Typeahead and Tokenizer, both of + * which park their clear button, end content and busy indicator in one + * absolutely-positioned lane at the field's inline end. + * + * SYNC: When modified, update this header and the two callers: + * - /packages/core/src/Typeahead/Typeahead.tsx + * - /packages/core/src/Tokenizer/Tokenizer.tsx + */ + +import {useCallback, useEffect, useRef, useState} from 'react'; +import * as stylex from '@stylexjs/stylex'; + +// Keep the input's text and caret out from under the lane. +// +// The input's content box already stops one wrapper padding short of the +// border, and the lane is inset from that same border — so what is left for +// the input to clear is the lane's inset plus its width, less the padding it +// already has, plus a padding's worth of gap so the text does not touch the +// glyph. The two paddings cancel, which is why this reads as inset + width. +const reserveStyles = stylex.create({ + reserve: (laneInset: string, laneWidth: number) => ({ + paddingInlineEnd: `calc(${laneInset} + ${laneWidth}px)`, + }), +}); + +/** + * Measure a field's inline-end lane, so the input can keep its text out from + * under it. + * + * The lane is absolutely positioned — it has to be, because these wrappers + * wrap, and an in-flow sibling gets pushed onto a second row by a token — and + * an out-of-flow box reserves no space by definition. There is no CSS that + * makes one do so: the input cannot see a sibling's width, and a custom + * property set on the lane cannot travel sideways to it. So the width is + * measured and handed back, and the caller spends it as padding. + * + * What the lane holds is not a fixed set: a clear button that comes and goes + * with the value, a busy indicator that comes and goes with the search, and, + * in Tokenizer, arbitrary `endContent`. Measuring covers all of it, including + * the combinations, and needs no constant kept in step with what renders. + * + * Takes the lane's inset from the field's inline-end border, as the CSS + * expression that positions it, and returns a ref callback for the lane plus + * the style the input needs — `undefined` when there is no lane to clear. + */ +export function useEndLaneReserve( + laneInset: string, +): [(node: HTMLElement | null) => void, stylex.StyleXStyles] { + const [width, setWidth] = useState(0); + const observerRef = useRef(null); + + useEffect( + () => () => { + observerRef.current?.disconnect(); + observerRef.current = null; + }, + [], + ); + + const laneRef = useCallback((node: HTMLElement | null) => { + observerRef.current?.disconnect(); + observerRef.current = null; + + if (node == null) { + // No lane rendered: nothing to reserve. Not a measurement of zero — the + // lane is gone, and the input takes the room back. + setWidth(0); + return; + } + + // jsdom implements no ResizeObserver, and this runs in every consumer's + // component tests. `offsetWidth` is 0 there too, so the reserve is simply + // absent rather than wrong. + if (typeof ResizeObserver === 'undefined') { + setWidth(node.offsetWidth); + return; + } + + const observer = new ResizeObserver(entries => { + const entry = entries[0]; + if (entry == null) { + return; + } + // Border box: the lane's own padding and border are part of what the + // input has to clear. `borderBoxSize` is the value the observer already + // computed; reading `offsetWidth` here would force a second layout. + const next = entry.borderBoxSize?.[0]?.inlineSize ?? entry.contentRect.width; + // Round up. A fractional width left as-is reserves a hair too little and + // the glyph's last subpixel column still lands on the caret. + setWidth(Math.ceil(next)); + }); + observer.observe(node); + observerRef.current = observer; + setWidth(Math.ceil(node.getBoundingClientRect().width)); + }, []); + + return [laneRef, width > 0 ? reserveStyles.reserve(laneInset, width) : undefined]; +} diff --git a/packages/core/src/Tokenizer/Tokenizer.tsx b/packages/core/src/Tokenizer/Tokenizer.tsx index 9b2494aaf31b3..4ec25f627b342 100644 --- a/packages/core/src/Tokenizer/Tokenizer.tsx +++ b/packages/core/src/Tokenizer/Tokenizer.tsx @@ -40,6 +40,8 @@ import { type FieldStatusVariant, } from '../Field'; import {Token} from '../Token'; +import {Spinner} from '../Spinner'; +import {useEndLaneReserve} from '../Field/useEndLaneReserve'; import {renderIconSlot, type IconType} from '../Icon'; import {OverflowList} from '../OverflowList'; import {useLayer} from '../Layer/useLayer'; @@ -232,6 +234,10 @@ export interface TokenizerProps extends Omit< // Styles // ============================================================================= +// How far the end lane sits from the field's inline-end border, named once +// because the input's reserve is derived from the same value. +const END_LANE_INSET = spacingVars['--spacing-2']; + const styles = stylex.create({ wrapper: { position: 'relative', @@ -267,7 +273,7 @@ const styles = stylex.create({ // Match the field's inline padding (inputWrapperStyles.base uses // spacing-2) so end content (clear button, resultCount) lines up with // the text/start-icon inset instead of hugging the border at ~3px. - insetInlineEnd: spacingVars['--spacing-2'], + insetInlineEnd: END_LANE_INSET, display: 'flex', alignItems: 'center', gap: spacingVars['--spacing-2'], @@ -461,6 +467,13 @@ export function Tokenizer({ })); // Focus-within state for overflow truncation + // Reported by BaseTypeahead so the indicator can live in this field's own + // end lane, beside endContent and the clear button. + const [isLoading, setIsLoading] = useState(false); + // What sits in the end lane varies most here — a spinner, arbitrary + // `endContent`, a clear button, or all three — so its width is measured + // rather than assumed, and the input reserves it. + const [laneRef, laneReserve] = useEndLaneReserve(END_LANE_INSET); const [isFocusedWithin, setIsFocusedWithin] = useState(false); const isTruncated = !isFocusedWithin && tokenOverflowBehavior !== 'none' && value.length > 0; @@ -814,17 +827,21 @@ export function Tokenizer({ ariaDescribedBy={ariaDescribedBy} onChangeQuery={onChangeQuery} __queryEntries={createEntries} + __onLoadingChange={setIsLoading} debounceMs={debounceMs} onKeyDown={handleKeyDown} anchorRef={wrapperRef} size={size} - inputXStyle={ + inputXStyle={[ isAtMax || isTruncated ? styles.inputAtMax : value.length > 0 ? styles.inputCompact - : undefined - } + : undefined, + // Not for the collapsed states above: those give the input no width + // to pad, and `inputAtMax` zeroes its padding outright. + !(isAtMax || isTruncated) && laneReserve, + ]} /> {htmlName != null && value.map(item => ( @@ -838,8 +855,15 @@ export function Tokenizer({ disabled={isDisabled} /> ))} - {(endContent || (hasClear && value.length > 0 && !isDisabled)) && ( -
+ {(isLoading || + endContent || + (hasClear && value.length > 0 && !isDisabled)) && ( +
+ {isLoading && ( + + )} {endContent} {hasClear && value.length > 0 && !isDisabled && ( extends Omit< */ __queryEntries?: (query: string, results: T[]) => T[]; + /** + * Called when a search starts or settles, so a wrapper can paint the busy + * state in the one inline-end lane it already owns for its clear button and + * end content, instead of the base rendering a second indicator competing + * for the same corner. + * + * Passing this takes the indicator over: the base stops rendering its own. + * A caller that does not pass it keeps the visible, named status the base + * has always rendered. + * + * Underscored and `@internal` for the same reason as `__queryEntries` + * above: `BaseTypeaheadProps` is re-exported from the package entry point, + * so anything named on it ships as public API at the next cut, and this is + * a wiring detail between the two wrappers and the base. + * + * @internal + */ + __onLoadingChange?: (isLoading: boolean) => void; + /** * Debounce delay in ms before triggering search after typing. * Set to 0 for synchronous/local search sources that don't need debouncing. @@ -310,10 +330,15 @@ const styles = stylex.create({ fontSize: typeScaleVars['--text-supporting-size'], color: colorVars['--color-text-secondary'], }, - loadingSpinner: { + // The indicator a direct caller gets. In flow, where it has always been, so + // it reserves its own width and the input's text never runs under it. + // Typeahead and Tokenizer take the indicator over and paint it in their own + // inline-end lane instead; this is what renders for everyone else. + loadingStatus: { display: 'inline-flex', alignItems: 'center', justifyContent: 'center', + flexShrink: 0, padding: spacingVars['--spacing-1'], }, }); @@ -389,6 +414,7 @@ export const BaseTypeahead = function BaseTypeahead({ onChangeQuery, onOpenChange, __queryEntries, + __onLoadingChange, inputId: externalInputId, ariaDescribedBy, ariaLabelledBy, @@ -424,6 +450,28 @@ export const BaseTypeahead = function BaseTypeahead({ const [isLoading, setIsLoading] = useState(false); const [hasSearched, setHasSearched] = useState(false); + // Report the busy state to a wrapper that has taken the indicator over. + // + // Through a ref, and at the call site rather than from an effect: an effect + // would run after this component had already committed, so the wrapper's + // own state change landed in a second commit — two renders of the whole + // field per transition, four across a search. Called here, the wrapper's + // setState batches with ours into the one commit that React was already + // doing. The ref keeps the identity of a caller's inline arrow from + // mattering, and the guard makes the report edge-triggered: the redundant + // `false` on every keystroke below the query threshold reports nothing. + const onLoadingChangeRef = useRef(__onLoadingChange); + onLoadingChangeRef.current = __onLoadingChange; + const loadingRef = useRef(false); + const setLoading = useCallback((next: boolean) => { + if (loadingRef.current === next) { + return; + } + loadingRef.current = next; + setIsLoading(next); + onLoadingChangeRef.current?.(next); + }, []); + // Track active pointer to defer popover.show() past click events. // With popover="auto", showing the popover between pointerdown and // pointerup/click causes the browser's light-dismiss to immediately @@ -499,7 +547,7 @@ export const BaseTypeahead = function BaseTypeahead({ // in-flight response for an older query fails the gen check below // instead of overwriting the newer results. const gen = ++searchGenRef.current; - setIsLoading(true); + setLoading(true); setHasSearched(true); try { const searchResults = await searchSource.search(searchQuery); @@ -534,7 +582,7 @@ export const BaseTypeahead = function BaseTypeahead({ setHighlightedIndex(-1); } finally { if (searchGenRef.current === gen) { - setIsLoading(false); + setLoading(false); } } }, @@ -545,6 +593,7 @@ export const BaseTypeahead = function BaseTypeahead({ announce, emptySearchResultsText, __queryEntries, + setLoading, t, ], ); @@ -552,7 +601,7 @@ export const BaseTypeahead = function BaseTypeahead({ // Perform bootstrap const performBootstrap = useCallback(async () => { const gen = ++searchGenRef.current; - setIsLoading(true); + setLoading(true); try { const bootstrapResults = await searchSource.bootstrap(); if (searchGenRef.current !== gen) { @@ -572,10 +621,10 @@ export const BaseTypeahead = function BaseTypeahead({ setResults([]); } finally { if (searchGenRef.current === gen) { - setIsLoading(false); + setLoading(false); } } - }, [searchSource, maxMenuItems, showLayer]); + }, [searchSource, maxMenuItems, showLayer, setLoading]); // Handle query change const handleQueryChange = useCallback( @@ -608,7 +657,7 @@ export const BaseTypeahead = function BaseTypeahead({ // its own `finally` will decline to clear this — so clear it here or // the field spins forever. Backspacing below the threshold on a remote // source is the everyday way to hit that. - setIsLoading(false); + setLoading(false); // Clear any lingering result-count / no-results announcement. announce(''); if (derived.length > 0) { @@ -645,6 +694,7 @@ export const BaseTypeahead = function BaseTypeahead({ debounceMs, searchSource, announce, + setLoading, ], ); @@ -673,11 +723,11 @@ export const BaseTypeahead = function BaseTypeahead({ // Same reason as in handleQueryChange: the invalidated search will not // clear this itself. Selecting a stale result while the next search is // still in flight would otherwise leave the field spinning. - setIsLoading(false); + setLoading(false); popover.hide(); inputRef.current?.focus(); }, - [onChange, popover, searchSource], + [onChange, popover, searchSource, setLoading], ); // Handle focus @@ -889,6 +939,7 @@ export const BaseTypeahead = function BaseTypeahead({ : undefined } aria-autocomplete="list" + aria-busy={isLoading || undefined} aria-describedby={ariaDescribedBy} aria-labelledby={ariaLabelledBy} aria-disabled={isFocusableDisabled ? 'true' : undefined} @@ -923,15 +974,11 @@ export const BaseTypeahead = function BaseTypeahead({ inputXStyle, )} /> - {isLoading && ( - - + {isLoading && __onLoadingChange == null && ( + + )} - {popover.render(
{ await waitFor(() => { expect(screen.getByRole('status', {name: 'Loading'})).toBeInTheDocument(); }); + expect(input).toHaveAttribute('aria-busy', 'true'); fireEvent.change(input, {target: {value: 'Ap'}}); await act(async () => { @@ -928,6 +929,7 @@ describe('BaseTypeahead minQueryLength', () => { expect( screen.queryByRole('status', {name: 'Loading'}), ).not.toBeInTheDocument(); + expect(input).not.toHaveAttribute('aria-busy'); expect(input).toHaveAttribute('aria-expanded', 'false'); }); }); @@ -1445,3 +1447,137 @@ describe('Typeahead statusVariant forwarding', () => { ); }); }); + +describe('busy indicator ownership', () => { + /** A source that stays in flight until the test settles it. */ + const pendingSource = () => { + let settle: (items: SearchableItem[]) => void = () => {}; + return { + source: { + search: async () => + new Promise(resolve => { + settle = resolve; + }), + bootstrap: () => [], + }, + settle: (items: SearchableItem[] = []) => settle(items), + }; + }; + + it('renders its own named status for a direct caller', async () => { + // BaseTypeaheadProps is re-exported from the package entry point, so the + // base has direct callers this repo cannot see. They painted no indicator + // of their own — the base did it for them — so it keeps doing it, and the + // status stays a named one rather than a bare aria-busy that only reaches + // assistive tech. + const {source, settle} = pendingSource(); + render( + {}} + debounceMs={0} + />, + ); + const input = screen.getByRole('combobox'); + + fireEvent.change(input, {target: {value: 'App'}}); + await waitFor(() => { + expect(screen.getByRole('status', {name: 'Loading'})).toBeInTheDocument(); + }); + // A Spinner, not the static clock glyph this used to render: `clock` + // means *time* everywhere else in core, and nothing about it moved while + // a search was out. + expect( + screen.getByRole('status', {name: 'Loading'}).querySelector('svg'), + ).toBeInTheDocument(); + + await act(async () => { + settle(); + await Promise.resolve(); + }); + expect( + screen.queryByRole('status', {name: 'Loading'}), + ).not.toBeInTheDocument(); + }); + + it('hands the indicator over to a field that takes it, and renders none itself', async () => { + // Typeahead and Tokenizer paint the spinner in the one inline-end lane + // they already own. Two indicators in one field is the defect this PR + // exists to fix, so the base must yield rather than add to it. + const {source, settle} = pendingSource(); + const onLoadingChange = vi.fn(); + render( + {}} + __onLoadingChange={onLoadingChange} + debounceMs={0} + />, + ); + const input = screen.getByRole('combobox'); + + fireEvent.change(input, {target: {value: 'App'}}); + await waitFor(() => { + expect(input).toHaveAttribute('aria-busy', 'true'); + }); + // Scoped by name: the announcer for result counts is a role="status" + // live region too, and it is not the indicator. + expect( + screen.queryByRole('status', {name: 'Loading'}), + ).not.toBeInTheDocument(); + expect(onLoadingChange).toHaveBeenLastCalledWith(true); + + await act(async () => { + settle(); + await Promise.resolve(); + }); + expect(onLoadingChange).toHaveBeenLastCalledWith(false); + }); + + it('reports each transition once, and reports nothing when nothing changed', async () => { + // Edge-triggered on purpose. Every keystroke below the query threshold + // clears the flag, and an unconditional report would hand the wrapper a + // `false` per character — each one a state write, and on a field that is + // re-rendering as the user types. + const {source, settle} = pendingSource(); + const onLoadingChange = vi.fn(); + render( + {}} + __onLoadingChange={onLoadingChange} + minQueryLength={3} + debounceMs={0} + />, + ); + const input = screen.getByRole('combobox'); + + // Below the threshold: no search, so nothing to report. + fireEvent.change(input, {target: {value: 'A'}}); + fireEvent.change(input, {target: {value: 'Ap'}}); + await act(async () => { + await Promise.resolve(); + }); + expect(onLoadingChange).not.toHaveBeenCalled(); + + fireEvent.change(input, {target: {value: 'App'}}); + await waitFor(() => { + expect(onLoadingChange).toHaveBeenCalledTimes(1); + }); + expect(onLoadingChange).toHaveBeenCalledWith(true); + + // Back below the threshold: one report out, not one per keystroke. + fireEvent.change(input, {target: {value: 'Ap'}}); + fireEvent.change(input, {target: {value: 'A'}}); + fireEvent.change(input, {target: {value: ''}}); + await act(async () => { + settle(); + await Promise.resolve(); + }); + expect(onLoadingChange).toHaveBeenCalledTimes(2); + expect(onLoadingChange).toHaveBeenLastCalledWith(false); + }); +}); diff --git a/packages/core/src/Typeahead/Typeahead.tsx b/packages/core/src/Typeahead/Typeahead.tsx index 4c05dd1579ff8..77472bc6193db 100644 --- a/packages/core/src/Typeahead/Typeahead.tsx +++ b/packages/core/src/Typeahead/Typeahead.tsx @@ -42,6 +42,8 @@ import {Token} from '../Token'; import {useTooltip} from '../Tooltip'; import {renderIconSlot, type IconType} from '../Icon'; import {VisuallyHidden} from '../VisuallyHidden'; +import {Spinner} from '../Spinner'; +import {useEndLaneReserve} from '../Field/useEndLaneReserve'; import {spacingVars, sizeVars} from '../theme/tokens.stylex'; import {groupStyles} from '../InputGroup/groupStyles'; import {useInputGroup} from '../InputGroup/InputGroupContext'; @@ -167,6 +169,14 @@ export interface TypeaheadProps extends Omit< // Styles // ============================================================================= +// How far the end lane sits from the field's inline-end border. Also what the +// input has to clear, so it is named rather than repeated: the reserve below +// is derived from the same expression that positions the lane. +const LANE_INSET = { + md: `calc((${sizeVars['--size-element-md']} - 20px) / 2 - 1px)`, + sm: `calc((${sizeVars['--size-element-sm']} - 20px) / 2 - 1px)`, +} as const; + const styles = stylex.create({ wrapper: { position: 'relative', @@ -186,15 +196,24 @@ const styles = stylex.create({ // -(8px - 3px) = -5px positions token equidistant from left edge as top. margin: `calc(-1 * (${spacingVars['--spacing-2']} - ${spacingVars['--spacing-1']} + 1px))`, }, - clearButton: { + // One lane for everything that sits at the field's inline end. It is + // absolute rather than in-flow because the wrapper wraps: a selected token + // can push a later in-flow sibling onto a second row, and this has to stay + // on the field's first row. Anything added here is a flex child, so the + // busy indicator and the clear button sit beside each other instead of + // both landing in the same corner. + endLane: { position: 'absolute', - top: `calc((${sizeVars['--size-element-md']} - 20px) / 2 - 1px)`, - insetInlineEnd: `calc((${sizeVars['--size-element-md']} - 20px) / 2 - 1px)`, + display: 'flex', + alignItems: 'center', + gap: spacingVars['--spacing-1'], + top: LANE_INSET.md, + insetInlineEnd: LANE_INSET.md, height: '20px', }, - clearButtonSm: { - top: `calc((${sizeVars['--size-element-sm']} - 20px) / 2 - 1px)`, - insetInlineEnd: `calc((${sizeVars['--size-element-sm']} - 20px) / 2 - 1px)`, + endLaneSm: { + top: LANE_INSET.sm, + insetInlineEnd: LANE_INSET.sm, }, inputHidden: { width: 0, @@ -299,6 +318,14 @@ export function Typeahead({ }); // Edit mode: when the user clicks the token to edit the selected value + // Reported by BaseTypeahead so the indicator can live in this field's own + // end lane, beside the clear button, rather than in the engine's row. + const [isLoading, setIsLoading] = useState(false); + // The lane holds a spinner, a clear button, or both, so what the input has + // to clear changes while the field is in use. Measured rather than assumed. + const [laneRef, laneReserve] = useEndLaneReserve( + size === 'sm' ? LANE_INSET.sm : LANE_INSET.md, + ); const [isEditing, setIsEditing] = useState(false); const [editingValue, setEditingValue] = useState(null); @@ -485,10 +512,11 @@ export function Typeahead({ ariaLabelledBy={ariaLabelledBy} onChangeQuery={onChangeQuery} onOpenChange={onOpenChange} + __onLoadingChange={setIsLoading} debounceMs={debounceMs} anchorRef={wrapperRef} onKeyDown={handleKeyDown} - inputXStyle={showToken ? styles.inputHidden : undefined} + inputXStyle={showToken ? styles.inputHidden : laneReserve} // While the token is shown the input is collapsed (width 0 / // opacity 0) — take it out of the Tab order so keyboard users // don't hit an invisible stop (WCAG 2.4.3 / 2.4.7). It stays @@ -497,15 +525,26 @@ export function Typeahead({ inputTabIndex={showToken ? -1 : undefined} size={size} /> - {hasClear && value && !isDisabled && ( - { - e.stopPropagation(); - handleClear(); - }} - xstyle={[styles.clearButton, size === 'sm' && styles.clearButtonSm]} - /> + {(isLoading || (hasClear && value && !isDisabled)) && ( +
+ {isLoading && ( + + )} + {hasClear && value && !isDisabled && ( + { + e.stopPropagation(); + handleClear(); + }} + /> + )} +
)}
{showsDisabledMessage && From e866238d2be79d3b8e194a480e3cc8939a44d803 Mon Sep 17 00:00:00 2001 From: freddymeta <170298698+freddymeta@users.noreply.github.com> Date: Wed, 26 Aug 2026 21:10:51 -0700 Subject: [PATCH 2/8] refactor(Typeahead): sync the report callback in a layout effect, not during render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/core/src/Field/useEndLaneReserve.ts | 8 ++++++-- packages/core/src/Typeahead/BaseTypeahead.tsx | 12 +++++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/core/src/Field/useEndLaneReserve.ts b/packages/core/src/Field/useEndLaneReserve.ts index a26cae5f02bda..f4c2757f86278 100644 --- a/packages/core/src/Field/useEndLaneReserve.ts +++ b/packages/core/src/Field/useEndLaneReserve.ts @@ -92,8 +92,12 @@ export function useEndLaneReserve( } // Border box: the lane's own padding and border are part of what the // input has to clear. `borderBoxSize` is the value the observer already - // computed; reading `offsetWidth` here would force a second layout. - const next = entry.borderBoxSize?.[0]?.inlineSize ?? entry.contentRect.width; + // computed, so it costs no layout; the fallback for a browser without + // it reads `offsetWidth`, which is border-box too — `contentRect` is + // not, and would under-reserve the moment the lane grows a padding. + const next = + entry.borderBoxSize?.[0]?.inlineSize ?? + (entry.target as HTMLElement).offsetWidth; // Round up. A fractional width left as-is reserves a hair too little and // the glyph's last subpixel column still lands on the caret. setWidth(Math.ceil(next)); diff --git a/packages/core/src/Typeahead/BaseTypeahead.tsx b/packages/core/src/Typeahead/BaseTypeahead.tsx index 119488ff4cff4..fdf48e9d42f5b 100644 --- a/packages/core/src/Typeahead/BaseTypeahead.tsx +++ b/packages/core/src/Typeahead/BaseTypeahead.tsx @@ -30,6 +30,7 @@ import * as stylex from '@stylexjs/stylex'; import type {StyleXStyles} from '@stylexjs/stylex'; import {usePopover} from '../Popover/usePopover'; import {useAnnounce} from '../hooks/useAnnounce'; +import {useIsomorphicLayoutEffect} from '../hooks/useIsomorphicLayoutEffect'; import {isImeKeyEvent} from '../utils/ime'; import {TypeaheadItem} from './TypeaheadItem'; import {Icon} from '../Icon'; @@ -460,8 +461,17 @@ export const BaseTypeahead = function BaseTypeahead({ // doing. The ref keeps the identity of a caller's inline arrow from // mattering, and the guard makes the report edge-triggered: the redundant // `false` on every keystroke below the query threshold reports nothing. + // + // The ref is synced in a layout effect rather than during render, following + // `onMotionStartRef` in BottomSheetPanel — a render that React discards + // must not leave the ref pointing at the callback from the abandoned pass. + // This effect only writes a ref, so it commits nothing and no wrapper + // re-renders for it; every caller of `setLoading` runs from an event or an + // awaited continuation, long after the first commit. const onLoadingChangeRef = useRef(__onLoadingChange); - onLoadingChangeRef.current = __onLoadingChange; + useIsomorphicLayoutEffect(() => { + onLoadingChangeRef.current = __onLoadingChange; + }, [__onLoadingChange]); const loadingRef = useRef(false); const setLoading = useCallback((next: boolean) => { if (loadingRef.current === next) { From b3ea3b1b56cfdb36ac61db4a913dd014be6214b8 Mon Sep 17 00:00:00 2001 From: freddytan Date: Thu, 27 Aug 2026 17:29:03 -0700 Subject: [PATCH 3/8] perf(Typeahead,Tokenizer): keep the lane measurement out of React state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../typeahead-tokenizer-busy-indicator.md | 4 +- packages/core/src/Field/useEndLaneReserve.ts | 112 +++++------ packages/core/src/Tokenizer/Tokenizer.tsx | 11 +- .../core/src/Typeahead/Typeahead.test.tsx | 188 ++++++++++++++++++ packages/core/src/Typeahead/Typeahead.tsx | 13 +- 5 files changed, 264 insertions(+), 64 deletions(-) diff --git a/.changeset/typeahead-tokenizer-busy-indicator.md b/.changeset/typeahead-tokenizer-busy-indicator.md index ebb82078a6c57..b512f9de44b66 100644 --- a/.changeset/typeahead-tokenizer-busy-indicator.md +++ b/.changeset/typeahead-tokenizer-busy-indicator.md @@ -4,10 +4,12 @@ [fix] Typeahead, Tokenizer: the busy indicator is a Spinner in the field's end lane, and the input keeps its text out from under it (#5555) -Three defects in one block. The indicator a search painted was `` — a static glyph, 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: 17×20px of overlap in Typeahead and 19×20px in Tokenizer, the latter leaving part of the ✕ unclickable. And the combobox never carried `aria-busy`, unlike every sibling input. +Three defects in one block. The indicator a search painted was `` — a static glyph, 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: 17×20px of overlap in Typeahead and 19×20px in Tokenizer, the latter leaving part of the ✕ unclickable. And the combobox never carried `aria-busy`, unlike every sibling input. The base engine now reports the busy state to the field, which paints it in the one inline-end lane it already owns beside its clear button and end content, and sets `aria-busy` on the input. A caller using `BaseTypeahead` directly is unaffected: it still renders its own visible, named "Loading" status, now a Spinner rather than the clock. The lane is absolutely positioned — these wrappers wrap, and an in-flow sibling gets pushed onto a second row by a token — so it reserved no space, and at a narrow width the query ran underneath it. The input now reserves the lane's measured width, which also closes a pre-existing case of the same bug: at 280px with a value selected, the clear button covered 17px of the live query in Typeahead and 25px in Tokenizer before this change, with no spinner involved at all. Both are 0 now. +The measurement reaches CSS as a custom property written to the field wrapper, never as React state, so a lane that grows or shrinks repaints without re-rendering the field. Held in state it cost a second commit every time the lane changed size — once as the spinner arrived and once as it left — which doubled the field's commits across a search for a value no JavaScript reads. The observation is shared too, through the same `observeResize` singleton `useTruncation` uses, so a page of fields costs one callback per frame rather than one observer each. + @freddymeta diff --git a/packages/core/src/Field/useEndLaneReserve.ts b/packages/core/src/Field/useEndLaneReserve.ts index f4c2757f86278..940ed0ffc7852 100644 --- a/packages/core/src/Field/useEndLaneReserve.ts +++ b/packages/core/src/Field/useEndLaneReserve.ts @@ -4,9 +4,9 @@ /** * @file useEndLaneReserve.ts - * @input Uses React - * @output Exports useEndLaneReserve, which measures a field's inline-end - * lane and returns the padding the input needs to clear it. + * @input Uses React and the shared ResizeObserver + * @output Exports useEndLaneReserve, which keeps a field's input clear of the + * absolutely-positioned lane at its inline end * @position Shared field internal. Used by Typeahead and Tokenizer, both of * which park their clear button, end content and busy indicator in one * absolutely-positioned lane at the field's inline end. @@ -16,8 +16,17 @@ * - /packages/core/src/Tokenizer/Tokenizer.tsx */ -import {useCallback, useEffect, useRef, useState} from 'react'; +import {useCallback} from 'react'; import * as stylex from '@stylexjs/stylex'; +import {observeResize, unobserveResize} from '../utils/sharedResizeObserver'; + +/** + * The measured lane width, published on the field wrapper and read by the + * input through inheritance. A custom property is the point of the design: it + * carries a measurement into CSS without carrying it through React, so a lane + * that grows or shrinks repaints without re-rendering the field. + */ +const LANE_WIDTH_VAR = '--_astryx-end-lane-width'; // Keep the input's text and caret out from under the lane. // @@ -26,86 +35,75 @@ import * as stylex from '@stylexjs/stylex'; // the input to clear is the lane's inset plus its width, less the padding it // already has, plus a padding's worth of gap so the text does not touch the // glyph. The two paddings cancel, which is why this reads as inset + width. +// +// The width arrives as a variable rather than a number, so this rule is +// static: one class, generated once, never regenerated as the lane changes. +// The `0px` fallback covers the frame before the lane is first measured. const reserveStyles = stylex.create({ - reserve: (laneInset: string, laneWidth: number) => ({ - paddingInlineEnd: `calc(${laneInset} + ${laneWidth}px)`, + reserve: (laneInset: string) => ({ + paddingInlineEnd: `calc(${laneInset} + var(${LANE_WIDTH_VAR}, 0px))`, }), }); /** - * Measure a field's inline-end lane, so the input can keep its text out from - * under it. + * Keep a field's input clear of the lane at its inline end. * * The lane is absolutely positioned — it has to be, because these wrappers * wrap, and an in-flow sibling gets pushed onto a second row by a token — and * an out-of-flow box reserves no space by definition. There is no CSS that - * makes one do so: the input cannot see a sibling's width, and a custom - * property set on the lane cannot travel sideways to it. So the width is - * measured and handed back, and the caller spends it as padding. + * makes one do so: the input cannot see a sibling's width. So the lane is + * measured, and the input spends the measurement as padding. * * What the lane holds is not a fixed set: a clear button that comes and goes * with the value, a busy indicator that comes and goes with the search, and, * in Tokenizer, arbitrary `endContent`. Measuring covers all of it, including * the combinations, and needs no constant kept in step with what renders. * + * **The measurement never enters React state.** It is written to a custom + * property on the field wrapper and inherited by the input. Held in state, it + * cost the field a second commit every time the lane changed size — once when + * the spinner arrived and again when it left — doubling the field's renders + * across a search for a value no JavaScript ever reads. The observation is + * shared as well: `observeResize` batches every field on the page into one + * callback per frame instead of one observer each. + * * Takes the lane's inset from the field's inline-end border, as the CSS - * expression that positions it, and returns a ref callback for the lane plus - * the style the input needs — `undefined` when there is no lane to clear. + * expression that positions it. Returns a ref callback for the lane, and the + * style for the input — which the caller applies only when it renders a lane, + * something it already knows without measuring anything. */ export function useEndLaneReserve( laneInset: string, ): [(node: HTMLElement | null) => void, stylex.StyleXStyles] { - const [width, setWidth] = useState(0); - const observerRef = useRef(null); - - useEffect( - () => () => { - observerRef.current?.disconnect(); - observerRef.current = null; - }, - [], - ); - const laneRef = useCallback((node: HTMLElement | null) => { - observerRef.current?.disconnect(); - observerRef.current = null; - if (node == null) { - // No lane rendered: nothing to reserve. Not a measurement of zero — the - // lane is gone, and the input takes the room back. - setWidth(0); - return; - } - - // jsdom implements no ResizeObserver, and this runs in every consumer's - // component tests. `offsetWidth` is 0 there too, so the reserve is simply - // absent rather than wrong. - if (typeof ResizeObserver === 'undefined') { - setWidth(node.offsetWidth); return; } + // The lane's parent: the field wrapper in both callers, and an ancestor + // of the input either way, which is what inheritance needs. + const host = node.parentElement; - const observer = new ResizeObserver(entries => { - const entry = entries[0]; - if (entry == null) { - return; - } + observeResize(node, () => { // Border box: the lane's own padding and border are part of what the - // input has to clear. `borderBoxSize` is the value the observer already - // computed, so it costs no layout; the fallback for a browser without - // it reads `offsetWidth`, which is border-box too — `contentRect` is - // not, and would under-reserve the moment the lane grows a padding. - const next = - entry.borderBoxSize?.[0]?.inlineSize ?? - (entry.target as HTMLElement).offsetWidth; - // Round up. A fractional width left as-is reserves a hair too little and - // the glyph's last subpixel column still lands on the caret. - setWidth(Math.ceil(next)); + // input has to clear. Rounded up, because a fractional width left as-is + // reserves a hair too little and the glyph's last subpixel column still + // lands on the caret. + host?.style.setProperty( + LANE_WIDTH_VAR, + `${Math.ceil(node.getBoundingClientRect().width)}px`, + ); }); - observer.observe(node); - observerRef.current = observer; - setWidth(Math.ceil(node.getBoundingClientRect().width)); + + // React 19 runs a ref callback's return value as its cleanup, so the + // observer is released exactly when the lane unmounts. + return () => { + unobserveResize(node); + // The room the lane claimed goes back to the input. Removing beats + // setting 0px: the rule's fallback is already that, and this leaves no + // stale property behind on the DOM. + host?.style.removeProperty(LANE_WIDTH_VAR); + }; }, []); - return [laneRef, width > 0 ? reserveStyles.reserve(laneInset, width) : undefined]; + return [laneRef, reserveStyles.reserve(laneInset)]; } diff --git a/packages/core/src/Tokenizer/Tokenizer.tsx b/packages/core/src/Tokenizer/Tokenizer.tsx index 4ec25f627b342..2cfc9eb658291 100644 --- a/packages/core/src/Tokenizer/Tokenizer.tsx +++ b/packages/core/src/Tokenizer/Tokenizer.tsx @@ -474,6 +474,11 @@ export function Tokenizer({ // `endContent`, a clear button, or all three — so its width is measured // rather than assumed, and the input reserves it. const [laneRef, laneReserve] = useEndLaneReserve(END_LANE_INSET); + // Whether a lane renders at all — known from props and state, without + // measuring anything, which is why the reserve costs no extra commit. + const hasEndLane = Boolean( + isLoading || endContent || (hasClear && value.length > 0 && !isDisabled), + ); const [isFocusedWithin, setIsFocusedWithin] = useState(false); const isTruncated = !isFocusedWithin && tokenOverflowBehavior !== 'none' && value.length > 0; @@ -840,7 +845,7 @@ export function Tokenizer({ : undefined, // Not for the collapsed states above: those give the input no width // to pad, and `inputAtMax` zeroes its padding outright. - !(isAtMax || isTruncated) && laneReserve, + !(isAtMax || isTruncated) && hasEndLane && laneReserve, ]} /> {htmlName != null && @@ -855,9 +860,7 @@ export function Tokenizer({ disabled={isDisabled} /> ))} - {(isLoading || - endContent || - (hasClear && value.length > 0 && !isDisabled)) && ( + {hasEndLane && (
diff --git a/packages/core/src/Typeahead/Typeahead.test.tsx b/packages/core/src/Typeahead/Typeahead.test.tsx index 9413dc8e17e45..38b9d0f3a2e99 100644 --- a/packages/core/src/Typeahead/Typeahead.test.tsx +++ b/packages/core/src/Typeahead/Typeahead.test.tsx @@ -17,8 +17,10 @@ import { beforeAll, afterAll, beforeEach, + afterEach, } from 'vitest'; import {render, screen, fireEvent, waitFor, act} from '@testing-library/react'; +import {Profiler} from 'react'; import userEvent from '@testing-library/user-event'; import {Typeahead} from './Typeahead'; import {BaseTypeahead} from './BaseTypeahead'; @@ -1581,3 +1583,189 @@ describe('busy indicator ownership', () => { expect(onLoadingChange).toHaveBeenLastCalledWith(false); }); }); + +describe('Typeahead end-lane reserve — render cost', () => { + // jsdom has no ResizeObserver and reports every width as 0, so the cost + // this guards is invisible without one: a reserve held in React state only + // re-renders when the measurement is non-zero. This stub is the smallest + // thing that makes the regression reproducible in CI — it reports a width + // the moment an element is observed, exactly as a browser would when the + // spinner mounts into the lane and again when it leaves. + class StubResizeObserver { + static instances = 0; + private readonly cb: ResizeObserverCallback; + constructor(cb: ResizeObserverCallback) { + this.cb = cb; + StubResizeObserver.instances++; + } + observe(target: Element) { + const entry = { + target, + borderBoxSize: [{inlineSize: 24, blockSize: 20}], + contentRect: {width: 24, height: 20}, + } as unknown as ResizeObserverEntry; + this.cb([entry], this); + } + unobserve() {} + disconnect() {} + } + + let originalRO: typeof ResizeObserver | undefined; + beforeEach(() => { + originalRO = globalThis.ResizeObserver; + StubResizeObserver.instances = 0; + globalThis.ResizeObserver = + StubResizeObserver; + // A real width, so a state-held reserve would have something to store. + vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({ + width: 24, + height: 20, + top: 0, + left: 0, + right: 24, + bottom: 20, + x: 0, + y: 0, + toJSON: () => ({}), + }); + }); + afterEach(() => { + globalThis.ResizeObserver = originalRO as typeof ResizeObserver; + vi.restoreAllMocks(); + }); + + const pendingSource = () => { + let settle: (items: SearchableItem[]) => void = () => {}; + return { + source: { + search: async () => + new Promise(resolve => { + settle = resolve; + }), + bootstrap: () => [], + }, + settle: (items: SearchableItem[] = []) => settle(items), + }; + }; + + it('costs no commit of its own across a whole search', async () => { + // The lane's width reaches CSS as a custom property written to the DOM, + // never as state, so measuring it cannot re-render the field. The two + // commits below are the ones the search itself owes: the spinner + // arriving, and the spinner leaving. Held in state, the measurement + // doubled that — a second commit for each, carrying a number no + // JavaScript reads. + const {source, settle} = pendingSource(); + const commits: string[] = []; + render( + commits.push(phase)}> + {}} + debounceMs={0} + /> + , + ); + + const input = screen.getByRole('combobox'); + commits.length = 0; + + // Drive the whole cycle without waiting on any DOM signal, so the count + // is of the field's commits and nothing else — and so this reads the + // same against any implementation of the reserve. + await act(async () => { + fireEvent.change(input, {target: {value: 'App'}}); + }); + const afterStart = commits.length; + + await act(async () => { + settle([]); + await Promise.resolve(); + }); + + // One commit for the spinner arriving, one for it leaving. A reserve + // held in state adds a second to each, because the lane changes size + // exactly when it appears and disappears. + expect(afterStart).toBe(1); + expect(commits.length).toBe(2); + }); + + it('shares one observer across every field on the page', () => { + // One observer per lane is the other half of the cost: browsers batch per + // observer instance, so N fields meant N callback dispatches a frame. + render( + <> + {}} + /> + {}} + /> + {}} + /> + , + ); + // Three lanes (each field has a value, so each renders a clear button), + // one observer. + expect(StubResizeObserver.instances).toBeLessThanOrEqual(1); + }); + + it('publishes the measured width for CSS, and takes it back with the lane', async () => { + // The mechanism the two tests above are protecting: the number reaches + // the input as an inherited custom property, so the padding follows the + // lane without React seeing the value at all. + const {source, settle} = pendingSource(); + const {container} = render( + {}} + debounceMs={0} + />, + ); + const input = screen.getByRole('combobox'); + + await act(async () => { + fireEvent.change(input, {target: {value: 'App'}}); + }); + await waitFor(() => { + expect( + container.querySelector('[style*="--_astryx-end-lane-width"]'), + ).not.toBeNull(); + }); + + const host = container.querySelector( + '[style*="--_astryx-end-lane-width"]', + ); + expect(host?.style.getPropertyValue('--_astryx-end-lane-width')).toBe( + '24px', + ); + + await act(async () => { + settle([]); + await Promise.resolve(); + }); + // Lane gone, property gone — the input takes the room back. + await waitFor(() => { + expect( + container.querySelector('[style*="--_astryx-end-lane-width"]'), + ).toBeNull(); + }); + expect( + container.querySelector('[style*="--_astryx-end-lane-width"]'), + ).toBeNull(); + }); +}); diff --git a/packages/core/src/Typeahead/Typeahead.tsx b/packages/core/src/Typeahead/Typeahead.tsx index 77472bc6193db..88cbce20690d0 100644 --- a/packages/core/src/Typeahead/Typeahead.tsx +++ b/packages/core/src/Typeahead/Typeahead.tsx @@ -326,6 +326,9 @@ export function Typeahead({ const [laneRef, laneReserve] = useEndLaneReserve( size === 'sm' ? LANE_INSET.sm : LANE_INSET.md, ); + // Whether a lane renders at all — known from props and state, without + // measuring anything, which is why the reserve costs no extra commit. + const hasEndLane = Boolean(isLoading || (hasClear && value && !isDisabled)); const [isEditing, setIsEditing] = useState(false); const [editingValue, setEditingValue] = useState(null); @@ -516,7 +519,13 @@ export function Typeahead({ debounceMs={debounceMs} anchorRef={wrapperRef} onKeyDown={handleKeyDown} - inputXStyle={showToken ? styles.inputHidden : laneReserve} + inputXStyle={ + showToken + ? styles.inputHidden + : hasEndLane + ? laneReserve + : undefined + } // While the token is shown the input is collapsed (width 0 / // opacity 0) — take it out of the Tab order so keyboard users // don't hit an invisible stop (WCAG 2.4.3 / 2.4.7). It stays @@ -525,7 +534,7 @@ export function Typeahead({ inputTabIndex={showToken ? -1 : undefined} size={size} /> - {(isLoading || (hasClear && value && !isDisabled)) && ( + {hasEndLane && (
Date: Sun, 30 Aug 2026 18:07:56 -0700 Subject: [PATCH 4/8] fix(Typeahead,Tokenizer): put Typeahead's end controls in flow, and measure Tokenizer's lane in local space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../typeahead-tokenizer-busy-indicator.md | 4 +- packages/core/src/Field/useEndLaneReserve.ts | 43 +-- .../core/src/Tokenizer/Tokenizer.test.tsx | 259 +++++++++++++++++- .../core/src/Typeahead/Typeahead.test.tsx | 217 +++------------ packages/core/src/Typeahead/Typeahead.tsx | 82 ++---- 5 files changed, 350 insertions(+), 255 deletions(-) diff --git a/.changeset/typeahead-tokenizer-busy-indicator.md b/.changeset/typeahead-tokenizer-busy-indicator.md index b512f9de44b66..c057d6864cc27 100644 --- a/.changeset/typeahead-tokenizer-busy-indicator.md +++ b/.changeset/typeahead-tokenizer-busy-indicator.md @@ -8,7 +8,9 @@ Three defects in one block. The indicator a search painted was ` { - // Border box: the lane's own padding and border are part of what the - // input has to clear. Rounded up, because a fractional width left as-is - // reserves a hair too little and the glyph's last subpixel column still - // lands on the caret. - host?.style.setProperty( - LANE_WIDTH_VAR, - `${Math.ceil(node.getBoundingClientRect().width)}px`, - ); + // `offsetWidth`, not `getBoundingClientRect().width`. The rect is in + // VIEWPORT space — it carries every CSS transform between this element + // and the root — while the padding it feeds is in the element's own + // LOCAL space. Under `scale(.5)` the rect reads half the lane's real + // width and the input reserves half of what it needs, so the query runs + // under the controls again; under `scale(2)` it reads double and the + // caret sits in a gap twice the lane's width. Measured in Chromium: + // 22.83px of overlap and 202.69px of excess gap on a 98px lane. + // `offsetWidth` is the untransformed border-box width, so it reports 98 + // at every scale — the number this padding is actually denominated in. + // + // It is already an integer, which is the rounding the old `Math.ceil` + // was there for: a fractional width left as-is reserves a hair too + // little and the glyph's last subpixel column lands on the caret. + host?.style.setProperty(LANE_WIDTH_VAR, `${node.offsetWidth}px`); }); // React 19 runs a ref callback's return value as its cleanup, so the diff --git a/packages/core/src/Tokenizer/Tokenizer.test.tsx b/packages/core/src/Tokenizer/Tokenizer.test.tsx index 8f42abebf2761..e3f4c8f139440 100644 --- a/packages/core/src/Tokenizer/Tokenizer.test.tsx +++ b/packages/core/src/Tokenizer/Tokenizer.test.tsx @@ -9,8 +9,18 @@ * SYNC: When Tokenizer.tsx changes, update tests to match */ -import {describe, it, expect, vi, beforeAll, afterAll, afterEach} from 'vitest'; +import { + describe, + it, + expect, + vi, + beforeAll, + beforeEach, + afterAll, + afterEach, +} from 'vitest'; import {render, screen, fireEvent, act, waitFor} from '@testing-library/react'; +import {Profiler} from 'react'; import userEvent from '@testing-library/user-event'; import {Tokenizer} from './Tokenizer'; import {__resetLiveRegionsForTest} from '../hooks/useAnnounce'; @@ -1380,3 +1390,250 @@ describe('Tokenizer disabled theme state', () => { expect(root).not.toHaveAttribute('data-disabled'); }); }); + +describe('Tokenizer end-lane reserve', () => { + // Tokenizer keeps the measured lane that Typeahead no longer needs: its + // lane stays pinned to the field's first row while tokens wrap below it, + // so it has to be out of flow, and an out-of-flow box reserves nothing by + // definition. jsdom performs no layout — it reports every width as 0 and + // has no ResizeObserver — so these stub both, which is what makes the + // mechanism, and the bug it had, reproducible in CI. + class StubResizeObserver { + static instances = 0; + private readonly cb: ResizeObserverCallback; + constructor(cb: ResizeObserverCallback) { + this.cb = cb; + StubResizeObserver.instances++; + } + observe(target: Element) { + this.cb( + [ + { + target, + borderBoxSize: [{inlineSize: 24, blockSize: 20}], + contentRect: {width: 24, height: 20}, + } as unknown as ResizeObserverEntry, + ], + this, + ); + } + unobserve() {} + disconnect() {} + } + + // The lane's true, untransformed width. `offsetWidth` is what reports it. + const LANE_LOCAL_WIDTH = 24; + // What `getBoundingClientRect()` would report for that same lane inside a + // `scale(.5)` subtree: viewport space, so half. Reading this instead is the + // bug — the number is spent as padding, which is in local space. + const LANE_VIEWPORT_WIDTH = 12; + + let originalRO: typeof ResizeObserver | undefined; + beforeEach(() => { + originalRO = globalThis.ResizeObserver; + StubResizeObserver.instances = 0; + globalThis.ResizeObserver = StubResizeObserver; + vi.spyOn(HTMLElement.prototype, 'offsetWidth', 'get').mockReturnValue( + LANE_LOCAL_WIDTH, + ); + vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({ + width: LANE_VIEWPORT_WIDTH, + height: 20, + top: 0, + left: 0, + right: LANE_VIEWPORT_WIDTH, + bottom: 20, + x: 0, + y: 0, + toJSON: () => ({}), + }); + }); + afterEach(() => { + globalThis.ResizeObserver = originalRO as typeof ResizeObserver; + vi.restoreAllMocks(); + }); + + const pendingSource = () => { + let settle: (items: SearchableItem[]) => void = () => {}; + return { + source: { + search: async () => + new Promise(resolve => { + settle = resolve; + }), + bootstrap: () => [], + }, + settle: (items: SearchableItem[] = []) => settle(items), + }; + }; + + const laneHost = (container: HTMLElement) => + container.querySelector('[style*="--_astryx-end-lane-width"]'); + + it('reserves the lane\u2019s untransformed width, not its on-screen width', () => { + // The regression. A CSS transform anywhere above the field scales what + // `getBoundingClientRect()` reports but not what the padding means, so + // the two must not be mixed: measured in Chromium under `scale(.5)` the + // input reserved half what it needed and the live query ran under the + // controls again (22.83px), and under `scale(2)` the caret sat in a + // 202.69px gap. `offsetWidth` is the same number at every scale. + const {container} = render( + {}} + hasClear + />, + ); + expect( + laneHost(container)?.style.getPropertyValue('--_astryx-end-lane-width'), + ).toBe(`${LANE_LOCAL_WIDTH}px`); + }); + + it('reserves the lane when the spinner is the only thing in it', () => { + // Busy-only: no endContent and no clear button, so the lane exists for + // the duration of the search and for nothing else. It still has width, + // and the query still has to clear it. + const {source} = pendingSource(); + const {container} = render( + {}} + hasClear={false} + debounceMs={0} + />, + ); + expect(laneHost(container)).toBeNull(); + + act(() => { + fireEvent.change(screen.getByRole('combobox'), {target: {value: 'Al'}}); + }); + + expect(screen.getByRole('status')).toBeInTheDocument(); + expect( + laneHost(container)?.style.getPropertyValue('--_astryx-end-lane-width'), + ).toBe(`${LANE_LOCAL_WIDTH}px`); + }); + + it('does not pad the input on the collapsed paths', () => { + // Collapsed: with tokens truncated the input is given no width to pad — + // `inputAtMax` zeroes its padding outright — so a reserve there would be + // padding applied to a zero-width box, fighting the collapse. The lane + // still publishes its width; what must not happen is the input claiming + // the reserve class. + const {container} = render( + {}} + tokenOverflowBehavior="unfocusedInline" + />, + ); + const input = screen.getByRole('combobox'); + expect(input.className).not.toBe(''); + // The reserve is the only rule that reads the lane variable; the + // collapsed input must not carry it. + const reserveClasses = new Set( + (laneHost(container)?.className ?? '').split(' '), + ); + expect( + [...input.classList].some(c => reserveClasses.has(c) && c !== ''), + ).toBe(false); + }); + + it('costs no commit of its own across a whole search', async () => { + // The measurement reaches CSS as a custom property written to the DOM, + // never as state, so it cannot re-render the field. The two commits below + // are the ones the search itself owes: the spinner arriving, and the + // spinner leaving. Held in state, the measurement doubled that. + const {source, settle} = pendingSource(); + const commits: string[] = []; + render( + commits.push(phase)}> + {}} + debounceMs={0} + /> + , + ); + const input = screen.getByRole('combobox'); + commits.length = 0; + + await act(async () => { + fireEvent.change(input, {target: {value: 'Al'}}); + }); + const afterStart = commits.length; + + await act(async () => { + settle([]); + await Promise.resolve(); + }); + + expect(afterStart).toBe(1); + expect(commits.length).toBe(2); + }); + + it('shares one observer across every field on the page', () => { + // Browsers batch per observer instance, so one observer per lane meant N + // callback dispatches a frame for N fields. + render( + <> + {}} + hasClear + /> + {}} + hasClear + /> + {}} + hasClear + /> + , + ); + expect(StubResizeObserver.instances).toBeLessThanOrEqual(1); + }); + + it('takes the room back when the lane goes', async () => { + const {source, settle} = pendingSource(); + const {container} = render( + {}} + hasClear={false} + debounceMs={0} + />, + ); + await act(async () => { + fireEvent.change(screen.getByRole('combobox'), {target: {value: 'Al'}}); + }); + expect(laneHost(container)).not.toBeNull(); + + await act(async () => { + settle([]); + await Promise.resolve(); + }); + await waitFor(() => { + expect(laneHost(container)).toBeNull(); + }); + }); +}); diff --git a/packages/core/src/Typeahead/Typeahead.test.tsx b/packages/core/src/Typeahead/Typeahead.test.tsx index 38b9d0f3a2e99..c538ba592f8ad 100644 --- a/packages/core/src/Typeahead/Typeahead.test.tsx +++ b/packages/core/src/Typeahead/Typeahead.test.tsx @@ -17,10 +17,8 @@ import { beforeAll, afterAll, beforeEach, - afterEach, } from 'vitest'; import {render, screen, fireEvent, waitFor, act} from '@testing-library/react'; -import {Profiler} from 'react'; import userEvent from '@testing-library/user-event'; import {Typeahead} from './Typeahead'; import {BaseTypeahead} from './BaseTypeahead'; @@ -1584,188 +1582,61 @@ describe('busy indicator ownership', () => { }); }); -describe('Typeahead end-lane reserve — render cost', () => { - // jsdom has no ResizeObserver and reports every width as 0, so the cost - // this guards is invisible without one: a reserve held in React state only - // re-renders when the measurement is non-zero. This stub is the smallest - // thing that makes the regression reproducible in CI — it reports a width - // the moment an element is observed, exactly as a browser would when the - // spinner mounts into the lane and again when it leaves. - class StubResizeObserver { - static instances = 0; - private readonly cb: ResizeObserverCallback; - constructor(cb: ResizeObserverCallback) { - this.cb = cb; - StubResizeObserver.instances++; - } - observe(target: Element) { - const entry = { - target, - borderBoxSize: [{inlineSize: 24, blockSize: 20}], - contentRect: {width: 24, height: 20}, - } as unknown as ResizeObserverEntry; - this.cb([entry], this); - } - unobserve() {} - disconnect() {} - } - - let originalRO: typeof ResizeObserver | undefined; - beforeEach(() => { - originalRO = globalThis.ResizeObserver; - StubResizeObserver.instances = 0; - globalThis.ResizeObserver = - StubResizeObserver; - // A real width, so a state-held reserve would have something to store. - vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({ - width: 24, - height: 20, - top: 0, - left: 0, - right: 24, - bottom: 20, - x: 0, - y: 0, - toJSON: () => ({}), - }); - }); - afterEach(() => { - globalThis.ResizeObserver = originalRO as typeof ResizeObserver; - vi.restoreAllMocks(); - }); - - const pendingSource = () => { - let settle: (items: SearchableItem[]) => void = () => {}; - return { - source: { - search: async () => - new Promise(resolve => { - settle = resolve; - }), - bootstrap: () => [], - }, - settle: (items: SearchableItem[] = []) => settle(items), - }; - }; - - it('costs no commit of its own across a whole search', async () => { - // The lane's width reaches CSS as a custom property written to the DOM, - // never as state, so measuring it cannot re-render the field. The two - // commits below are the ones the search itself owes: the spinner - // arriving, and the spinner leaving. Held in state, the measurement - // doubled that — a second commit for each, carrying a number no - // JavaScript reads. - const {source, settle} = pendingSource(); - const commits: string[] = []; - render( - commits.push(phase)}> - {}} - debounceMs={0} - /> - , - ); - - const input = screen.getByRole('combobox'); - commits.length = 0; - - // Drive the whole cycle without waiting on any DOM signal, so the count - // is of the field's commits and nothing else — and so this reads the - // same against any implementation of the reserve. - await act(async () => { - fireEvent.change(input, {target: {value: 'App'}}); - }); - const afterStart = commits.length; - - await act(async () => { - settle([]); - await Promise.resolve(); - }); - - // One commit for the spinner arriving, one for it leaving. A reserve - // held in state adds a second to each, because the lane changes size - // exactly when it appears and disappears. - expect(afterStart).toBe(1); - expect(commits.length).toBe(2); - }); - - it('shares one observer across every field on the page', () => { - // One observer per lane is the other half of the cost: browsers batch per - // observer instance, so N fields meant N callback dispatches a frame. - render( - <> - {}} - /> - {}} - /> - {}} - /> - , +describe('end controls stay in flow', () => { + // The fix for the transform bug, expressed as a rule rather than a + // measurement: these controls are ordinary flex siblings of the input, so + // they take up room and nothing has to reserve it for them. jsdom performs + // no layout, so what is asserted is the absence of the two things that + // stopped that being true — an out-of-flow lane, and a padding reserve fed + // by a measured width. The geometry itself is browser-verified in the PR. + it('renders the clear button without taking it out of flow', () => { + const {container} = render( + {}} + />, ); - // Three lanes (each field has a value, so each renders a clear button), - // one observer. - expect(StubResizeObserver.instances).toBeLessThanOrEqual(1); + const field = container.querySelector('.astryx-typeahead'); + const clear = screen.getByRole('button', {name: /clear/i}); + expect(field).toContainElement(clear); + // A direct flex child of the field, not a box positioned over it. + expect(clear.parentElement).toBe(field); + expect(getComputedStyle(clear).position).not.toBe('absolute'); }); - it('publishes the measured width for CSS, and takes it back with the lane', async () => { - // The mechanism the two tests above are protecting: the number reaches - // the input as an inherited custom property, so the padding follows the - // lane without React seeing the value at all. - const {source, settle} = pendingSource(); + it('never reserves room with a measured width', () => { + // The custom property is Tokenizer's mechanism and must not reappear + // here: a width measured in viewport space and spent as local padding is + // wrong under any CSS transform (scale(.5) overlapped the query by + // 22.83px, scale(2) left a 202.69px gap). const {container} = render( {}} - debounceMs={0} />, ); - const input = screen.getByRole('combobox'); - - await act(async () => { - fireEvent.change(input, {target: {value: 'App'}}); - }); - await waitFor(() => { - expect( - container.querySelector('[style*="--_astryx-end-lane-width"]'), - ).not.toBeNull(); - }); - - const host = container.querySelector( - '[style*="--_astryx-end-lane-width"]', - ); - expect(host?.style.getPropertyValue('--_astryx-end-lane-width')).toBe( - '24px', - ); - - await act(async () => { - settle([]); - await Promise.resolve(); - }); - // Lane gone, property gone — the input takes the room back. - await waitFor(() => { - expect( - container.querySelector('[style*="--_astryx-end-lane-width"]'), - ).toBeNull(); - }); expect( container.querySelector('[style*="--_astryx-end-lane-width"]'), ).toBeNull(); }); + + it('keeps the field on one row so the controls cannot be pushed off it', () => { + // `flex-wrap` moves an item to a new line rather than shrinking it, so a + // wrapping field put the controls on a row of their own once the token + // got long. The shared field base does not wrap; this must not either. + const {container} = render( + {}} + />, + ); + const field = container.querySelector('.astryx-typeahead') as HTMLElement; + expect(getComputedStyle(field).flexWrap).not.toBe('wrap'); + }); }); diff --git a/packages/core/src/Typeahead/Typeahead.tsx b/packages/core/src/Typeahead/Typeahead.tsx index 88cbce20690d0..246424225d69b 100644 --- a/packages/core/src/Typeahead/Typeahead.tsx +++ b/packages/core/src/Typeahead/Typeahead.tsx @@ -43,7 +43,6 @@ import {useTooltip} from '../Tooltip'; import {renderIconSlot, type IconType} from '../Icon'; import {VisuallyHidden} from '../VisuallyHidden'; import {Spinner} from '../Spinner'; -import {useEndLaneReserve} from '../Field/useEndLaneReserve'; import {spacingVars, sizeVars} from '../theme/tokens.stylex'; import {groupStyles} from '../InputGroup/groupStyles'; import {useInputGroup} from '../InputGroup/InputGroupContext'; @@ -169,18 +168,15 @@ export interface TypeaheadProps extends Omit< // Styles // ============================================================================= -// How far the end lane sits from the field's inline-end border. Also what the -// input has to clear, so it is named rather than repeated: the reserve below -// is derived from the same expression that positions the lane. -const LANE_INSET = { - md: `calc((${sizeVars['--size-element-md']} - 20px) / 2 - 1px)`, - sm: `calc((${sizeVars['--size-element-sm']} - 20px) / 2 - 1px)`, -} as const; - const styles = stylex.create({ wrapper: { position: 'relative', - flexWrap: 'wrap', + // No `flexWrap`. The shared field base does not wrap and neither does + // TextInput; this field only ever holds one token, so there is no second + // row to wrap to. It also cannot wrap and keep its end controls in flow: + // flex moves an item to a new line rather than shrinking it, so a long + // value put the clear button and spinner on a row of their own (a 280px + // field grew to 46px tall). Unwrapped, the token ellipsizes instead. gap: spacingVars['--spacing-1'], // Standard padding minus border width to prevent height jump // when a token (28px) is added inside the input @@ -196,25 +192,6 @@ const styles = stylex.create({ // -(8px - 3px) = -5px positions token equidistant from left edge as top. margin: `calc(-1 * (${spacingVars['--spacing-2']} - ${spacingVars['--spacing-1']} + 1px))`, }, - // One lane for everything that sits at the field's inline end. It is - // absolute rather than in-flow because the wrapper wraps: a selected token - // can push a later in-flow sibling onto a second row, and this has to stay - // on the field's first row. Anything added here is a flex child, so the - // busy indicator and the clear button sit beside each other instead of - // both landing in the same corner. - endLane: { - position: 'absolute', - display: 'flex', - alignItems: 'center', - gap: spacingVars['--spacing-1'], - top: LANE_INSET.md, - insetInlineEnd: LANE_INSET.md, - height: '20px', - }, - endLaneSm: { - top: LANE_INSET.sm, - insetInlineEnd: LANE_INSET.sm, - }, inputHidden: { width: 0, minWidth: 0, @@ -321,14 +298,6 @@ export function Typeahead({ // Reported by BaseTypeahead so the indicator can live in this field's own // end lane, beside the clear button, rather than in the engine's row. const [isLoading, setIsLoading] = useState(false); - // The lane holds a spinner, a clear button, or both, so what the input has - // to clear changes while the field is in use. Measured rather than assumed. - const [laneRef, laneReserve] = useEndLaneReserve( - size === 'sm' ? LANE_INSET.sm : LANE_INSET.md, - ); - // Whether a lane renders at all — known from props and state, without - // measuring anything, which is why the reserve costs no extra commit. - const hasEndLane = Boolean(isLoading || (hasClear && value && !isDisabled)); const [isEditing, setIsEditing] = useState(false); const [editingValue, setEditingValue] = useState(null); @@ -519,13 +488,7 @@ export function Typeahead({ debounceMs={debounceMs} anchorRef={wrapperRef} onKeyDown={handleKeyDown} - inputXStyle={ - showToken - ? styles.inputHidden - : hasEndLane - ? laneReserve - : undefined - } + inputXStyle={showToken ? styles.inputHidden : undefined} // While the token is shown the input is collapsed (width 0 / // opacity 0) — take it out of the Tab order so keyboard users // don't hit an invisible stop (WCAG 2.4.3 / 2.4.7). It stays @@ -534,26 +497,17 @@ export function Typeahead({ inputTabIndex={showToken ? -1 : undefined} size={size} /> - {hasEndLane && ( -
- {isLoading && ( - - )} - {hasClear && value && !isDisabled && ( - { - e.stopPropagation(); - handleClear(); - }} - /> - )} -
+ {isLoading && ( + + )} + {hasClear && value && !isDisabled && ( + { + e.stopPropagation(); + handleClear(); + }} + /> )}
{showsDisabledMessage && From bdcb7b7eee642ec769dc5de95d5cbfbd2f6b3c33 Mon Sep 17 00:00:00 2001 From: freddymeta Date: Sun, 30 Aug 2026 19:22:52 -0700 Subject: [PATCH 5/8] fix(Typeahead): hold the end controls in the corner, and file the reserve with its one caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/core/src/Tokenizer/Tokenizer.tsx | 2 +- .../{Field => Tokenizer}/useEndLaneReserve.ts | 19 +++++--- .../core/src/Typeahead/Typeahead.test.tsx | 28 ++++++++++-- packages/core/src/Typeahead/Typeahead.tsx | 45 ++++++++++++++----- 4 files changed, 72 insertions(+), 22 deletions(-) rename packages/core/src/{Field => Tokenizer}/useEndLaneReserve.ts (85%) diff --git a/packages/core/src/Tokenizer/Tokenizer.tsx b/packages/core/src/Tokenizer/Tokenizer.tsx index 2cfc9eb658291..80dcdbbc0e752 100644 --- a/packages/core/src/Tokenizer/Tokenizer.tsx +++ b/packages/core/src/Tokenizer/Tokenizer.tsx @@ -41,7 +41,7 @@ import { } from '../Field'; import {Token} from '../Token'; import {Spinner} from '../Spinner'; -import {useEndLaneReserve} from '../Field/useEndLaneReserve'; +import {useEndLaneReserve} from './useEndLaneReserve'; import {renderIconSlot, type IconType} from '../Icon'; import {OverflowList} from '../OverflowList'; import {useLayer} from '../Layer/useLayer'; diff --git a/packages/core/src/Field/useEndLaneReserve.ts b/packages/core/src/Tokenizer/useEndLaneReserve.ts similarity index 85% rename from packages/core/src/Field/useEndLaneReserve.ts rename to packages/core/src/Tokenizer/useEndLaneReserve.ts index e4c00bf910c70..d2018ffe4beda 100644 --- a/packages/core/src/Field/useEndLaneReserve.ts +++ b/packages/core/src/Tokenizer/useEndLaneReserve.ts @@ -7,14 +7,19 @@ * @input Uses React and the shared ResizeObserver * @output Exports useEndLaneReserve, which keeps a field's input clear of the * absolutely-positioned lane at its inline end - * @position Shared field internal. Used by Tokenizer, which parks its clear - * button, end content and busy indicator in one absolutely-positioned lane - * at the field's inline end. + * @position Tokenizer internal. Tokenizer parks its clear button, end content + * and busy indicator in one absolutely-positioned lane at the field's + * inline end, and this keeps the input's text and caret clear of it. * - * Typeahead does NOT use this: its wrapper holds at most one token, so its - * end controls sit in flow as flex siblings the way TextInput's do, and an - * in-flow box needs no reserve. Tokenizer cannot — its lane stays pinned to - * the field's first row while tokens wrap below it. + * It lives here, beside its one caller, rather than in `Field/` with the + * shared field internals — deliberately. A measured reserve is Tokenizer's + * workaround for a constraint only Tokenizer has: its lane must stay pinned + * to the field's first row while tokens wrap below it, so it cannot be in + * flow, and an out-of-flow box reserves nothing. Every other field, this + * one's own Typeahead included, puts its end controls in flow as flex + * siblings the way TextInput does, which needs no measuring at all. Filed + * under `Field/` this read as shared infrastructure and invited the next + * field to reach for it; that is the wrong default. * * SYNC: When modified, update this header and the caller: * - /packages/core/src/Tokenizer/Tokenizer.tsx diff --git a/packages/core/src/Typeahead/Typeahead.test.tsx b/packages/core/src/Typeahead/Typeahead.test.tsx index c538ba592f8ad..4e2736a004e2e 100644 --- a/packages/core/src/Typeahead/Typeahead.test.tsx +++ b/packages/core/src/Typeahead/Typeahead.test.tsx @@ -1601,9 +1601,12 @@ describe('end controls stay in flow', () => { const field = container.querySelector('.astryx-typeahead'); const clear = screen.getByRole('button', {name: /clear/i}); expect(field).toContainElement(clear); - // A direct flex child of the field, not a box positioned over it. - expect(clear.parentElement).toBe(field); - expect(getComputedStyle(clear).position).not.toBe('absolute'); + // The controls sit in a lane that is itself an ordinary in-flow child of + // the field — not a box positioned over it, which is what reserved no + // room and put the input underneath. + const lane = clear.parentElement as HTMLElement; + expect(lane.parentElement).toBe(field); + expect(getComputedStyle(lane).position).not.toBe('absolute'); }); it('never reserves room with a measured width', () => { @@ -1624,6 +1627,25 @@ describe('end controls stay in flow', () => { ).toBeNull(); }); + it('holds the controls at the inline end when a token collapses the input', () => { + // The field pushes its end controls over with an `auto` margin rather + // than by leaning on the input to absorb the free space, because it + // collapses that input to nothing while a token shows. Without it the + // clear button sat against the token in mid-field instead of in the + // corner (measured: x=39 in a 300px field, against TextInput's 281). + const {container} = render( + {}} + />, + ); + const clear = screen.getByRole('button', {name: /clear/i}); + const lane = clear.parentElement as HTMLElement; + expect(getComputedStyle(lane).marginInlineStart).toBe('auto'); + }); + it('keeps the field on one row so the controls cannot be pushed off it', () => { // `flex-wrap` moves an item to a new line rather than shrinking it, so a // wrapping field put the controls on a row of their own once the token diff --git a/packages/core/src/Typeahead/Typeahead.tsx b/packages/core/src/Typeahead/Typeahead.tsx index 246424225d69b..f79044c14f7df 100644 --- a/packages/core/src/Typeahead/Typeahead.tsx +++ b/packages/core/src/Typeahead/Typeahead.tsx @@ -192,6 +192,25 @@ const styles = stylex.create({ // -(8px - 3px) = -5px positions token equidistant from left edge as top. margin: `calc(-1 * (${spacingVars['--spacing-2']} - ${spacingVars['--spacing-1']} + 1px))`, }, + // The busy indicator and the clear button, at the field's inline end. + // + // In flow, as a flex child — an in-flow box takes up room, so the input + // cannot run underneath it and nothing has to be measured. That is + // TextInput's arrangement for the same two controls. + // + // The `auto` margin is what TextInput does not need. Its input is always + // present and `flex: 1`, so it absorbs the free space and pushes these to + // the end on its own. This field collapses its input to nothing while a + // token shows (`inputHidden`), which leaves no flexible item in the row — + // so without the margin the controls sit against the token, mid-field, + // instead of in the corner where they belong. `auto` gives the free space + // to the margin instead of to a sibling, which needs no sibling to exist. + endLane: { + display: 'flex', + alignItems: 'center', + gap: spacingVars['--spacing-1'], + marginInlineStart: 'auto', + }, inputHidden: { width: 0, minWidth: 0, @@ -497,17 +516,21 @@ export function Typeahead({ inputTabIndex={showToken ? -1 : undefined} size={size} /> - {isLoading && ( - - )} - {hasClear && value && !isDisabled && ( - { - e.stopPropagation(); - handleClear(); - }} - /> + {(isLoading || (hasClear && value && !isDisabled)) && ( +
+ {isLoading && ( + + )} + {hasClear && value && !isDisabled && ( + { + e.stopPropagation(); + handleClear(); + }} + /> + )} +
)}
{showsDisabledMessage && From d9b44a00563293ee9987a16837036288149193a5 Mon Sep 17 00:00:00 2001 From: freddymeta Date: Tue, 1 Sep 2026 20:32:05 -0700 Subject: [PATCH 6/8] docs(Typeahead,Tokenizer): file the busy indicator as anatomy that delegates to Spinner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/typeahead-tokenizer-busy-indicator.md | 10 +++++++--- packages/core/src/Tokenizer/Tokenizer.doc.mjs | 2 ++ packages/core/src/Tokenizer/Tokenizer.test.tsx | 12 +++++++++--- packages/core/src/Tokenizer/useEndLaneReserve.ts | 13 ++++++++----- packages/core/src/Typeahead/Typeahead.doc.mjs | 6 ++++++ packages/core/src/Typeahead/Typeahead.spec.md | 8 +++++++- packages/core/src/Typeahead/Typeahead.test.tsx | 11 ++++++----- 7 files changed, 45 insertions(+), 17 deletions(-) diff --git a/.changeset/typeahead-tokenizer-busy-indicator.md b/.changeset/typeahead-tokenizer-busy-indicator.md index c057d6864cc27..a4a68cbe515cd 100644 --- a/.changeset/typeahead-tokenizer-busy-indicator.md +++ b/.changeset/typeahead-tokenizer-busy-indicator.md @@ -4,14 +4,18 @@ [fix] Typeahead, Tokenizer: the busy indicator is a Spinner in the field's end lane, and the input keeps its text out from under it (#5555) -Three defects in one block. The indicator a search painted was `` — a static glyph, 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: 17×20px of overlap in Typeahead and 19×20px in Tokenizer, the latter leaving part of the ✕ unclickable. And the combobox never carried `aria-busy`, unlike every sibling input. +Three defects in one block. The indicator a search painted was `` — a static glyph, 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: 17×20px of overlap in Typeahead and 19×20px in Tokenizer. The overlap is visual, not functional — the clear button is positioned, so it paints above the in-flow indicator and stays clickable across the whole covered band. And the combobox never carried `aria-busy`, unlike every sibling input. The base engine now reports the busy state to the field, which paints it in the one inline-end lane it already owns beside its clear button and end content, and sets `aria-busy` on the input. A caller using `BaseTypeahead` directly is unaffected: it still renders its own visible, named "Loading" status, now a Spinner rather than the clock. -Typeahead puts both controls **in flow**, as ordinary flex siblings of the input, exactly as TextInput does with its own spinner and clear button — an in-flow box takes up room, so the input cannot run under it and there is nothing to measure. Getting there meant dropping `flex-wrap: wrap` from its wrapper, which the shared field base does not set and TextInput does not use: this field holds at most one token, so there is no second row to wrap to, and wrapping is what made an in-flow lane impossible, since flex moves an item to a new line rather than shrinking it. Unwrapped, a long value ellipsizes in the token instead. This also closes a pre-existing case of the same bug: at 280px with a value selected, the clear button covered 17px of the live query before this change, with no spinner involved at all. +Typeahead puts both controls **in flow**, as ordinary flex siblings of the input, exactly as TextInput does with its own spinner and clear button — an in-flow box takes up room, so the input cannot run under it and there is nothing to measure. Getting there meant dropping `flex-wrap: wrap` from its wrapper, which the shared field base does not set and TextInput does not use: this field holds at most one token, so there is no second row to wrap to, and wrapping is what made an in-flow lane impossible, since flex moves an item to a new line rather than shrinking it. Measured in Chromium: with `flex-wrap` restored and a token too wide to share the row, the end controls drop to a second row and a 280px field grows from 32px to 46px tall. Unwrapped, a long value ellipsizes in the token instead. + +Tokenizer's own pre-existing case of the overlap closes with it: at 280px with a token and no search running, its clear button covered 20px of the input's content box, and covers none now. Tokenizer keeps a measured lane, because 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, and an out-of-flow box reserves nothing. Its width is measured with `offsetWidth` rather than `getBoundingClientRect()`. The rect is in viewport space — it carries every CSS transform above the element — while the padding it feeds is in local space, so mixing them broke under any transform: measured in Chromium, `scale(.5)` reserved half of what was needed and put the query back under the controls by 22.83px, and `scale(2)` left the caret in a 202.69px gap. `offsetWidth` is the untransformed border-box width and reports the same number at every scale. -The measurement reaches CSS as a custom property written to the field wrapper, never as React state, so a lane that grows or shrinks repaints without re-rendering the field. Held in state it cost a second commit every time the lane changed size — once as the spinner arrived and once as it left — which doubled the field's commits across a search for a value no JavaScript reads. The observation is shared too, through the same `observeResize` singleton `useTruncation` uses, so a page of fields costs one callback per frame rather than one observer each. +The measurement reaches CSS as a custom property written to the field wrapper, never as React state, so a lane that grows or shrinks repaints without re-rendering the field. Held in state it cost a second commit every time the lane changed size — once as the spinner arrived and once as it left — which doubled the field's commits across a search for a value no JavaScript reads. The observation is shared too, through the same `observeResize` singleton `useTruncation` uses, so a page of fields costs one callback per frame rather than one observer each. The property is `--_tokenizer-end-lane-width`: private and component-named, like every other runtime layout var in the package, and never something a theme writes. + +The busy indicator now appears in each field's documented anatomy, delegating its theming to `component:Spinner` rather than gaining a target of its own — the disposition `TextArea`, `CheckboxList` and `CommandPalette` already use for the same part. @freddymeta diff --git a/packages/core/src/Tokenizer/Tokenizer.doc.mjs b/packages/core/src/Tokenizer/Tokenizer.doc.mjs index d110408a9f994..9f17f62b49985 100644 --- a/packages/core/src/Tokenizer/Tokenizer.doc.mjs +++ b/packages/core/src/Tokenizer/Tokenizer.doc.mjs @@ -258,6 +258,7 @@ export const docs = { {name: 'Token chips', required: false, description: 'Removable chips representing each selected item. Each chip shows a label and a remove button.'}, {name: 'Search input', required: true, description: 'The text input where users type to search the data source. Hides when maxEntries is reached.'}, {name: 'Dropdown menu', required: false, description: 'The search results list that appears below the input as the user types.'}, + {name: 'Spinner', required: false, description: 'Loading indicator shown at the end of the field while a search is in flight.'}, {name: 'End content', required: false, description: 'A trailing slot after the input for action buttons, counts, or other controls.'}, {name: 'Clear button', required: false, description: 'A button that removes all selected tokens at once. Shown when hasClear is true and tokens are present.'}, ], @@ -483,6 +484,7 @@ export const docsZh = { {name: 'Token chips', required: false, description: 'Removable chips representing each selected item. Each chip shows a label and a remove button.'}, {name: 'Search input', required: true, description: 'The text input where users type to search the data source. Hides when maxEntries is reached.'}, {name: 'Dropdown menu', required: false, description: 'The search results list that appears below the input as the user types.'}, + {name: 'Spinner', required: false, description: 'Loading indicator shown at the end of the field while a search is in flight.'}, {name: 'End content', required: false, description: 'A trailing slot after the input for action buttons, counts, or other controls.'}, {name: 'Clear button', required: false, description: 'A button that removes all selected tokens at once. Shown when hasClear is true and tokens are present.'}, ], diff --git a/packages/core/src/Tokenizer/Tokenizer.test.tsx b/packages/core/src/Tokenizer/Tokenizer.test.tsx index e3f4c8f139440..1db25c3dd4b3a 100644 --- a/packages/core/src/Tokenizer/Tokenizer.test.tsx +++ b/packages/core/src/Tokenizer/Tokenizer.test.tsx @@ -1468,7 +1468,9 @@ describe('Tokenizer end-lane reserve', () => { }; const laneHost = (container: HTMLElement) => - container.querySelector('[style*="--_astryx-end-lane-width"]'); + container.querySelector( + '[style*="--_tokenizer-end-lane-width"]', + ); it('reserves the lane\u2019s untransformed width, not its on-screen width', () => { // The regression. A CSS transform anywhere above the field scales what @@ -1487,7 +1489,9 @@ describe('Tokenizer end-lane reserve', () => { />, ); expect( - laneHost(container)?.style.getPropertyValue('--_astryx-end-lane-width'), + laneHost(container)?.style.getPropertyValue( + '--_tokenizer-end-lane-width', + ), ).toBe(`${LANE_LOCAL_WIDTH}px`); }); @@ -1514,7 +1518,9 @@ describe('Tokenizer end-lane reserve', () => { expect(screen.getByRole('status')).toBeInTheDocument(); expect( - laneHost(container)?.style.getPropertyValue('--_astryx-end-lane-width'), + laneHost(container)?.style.getPropertyValue( + '--_tokenizer-end-lane-width', + ), ).toBe(`${LANE_LOCAL_WIDTH}px`); }); diff --git a/packages/core/src/Tokenizer/useEndLaneReserve.ts b/packages/core/src/Tokenizer/useEndLaneReserve.ts index d2018ffe4beda..ee4435f95f076 100644 --- a/packages/core/src/Tokenizer/useEndLaneReserve.ts +++ b/packages/core/src/Tokenizer/useEndLaneReserve.ts @@ -35,7 +35,7 @@ import {observeResize, unobserveResize} from '../utils/sharedResizeObserver'; * carries a measurement into CSS without carrying it through React, so a lane * that grows or shrinks repaints without re-rendering the field. */ -const LANE_WIDTH_VAR = '--_astryx-end-lane-width'; +const LANE_WIDTH_VAR = '--_tokenizer-end-lane-width'; // Keep the input's text and caret out from under the lane. // @@ -99,10 +99,13 @@ export function useEndLaneReserve( // LOCAL space. Under `scale(.5)` the rect reads half the lane's real // width and the input reserves half of what it needs, so the query runs // under the controls again; under `scale(2)` it reads double and the - // caret sits in a gap twice the lane's width. Measured in Chromium: - // 22.83px of overlap and 202.69px of excess gap on a 98px lane. - // `offsetWidth` is the untransformed border-box width, so it reports 98 - // at every scale — the number this padding is actually denominated in. + // caret sits in a gap twice the lane's width. Measured in Chromium on a + // 123px lane in a 280px field: at `scale(.5)` the rect published 61.48px + // and the spinner covered 14px of the input's content box; at `scale(2)` + // it published 245.91px, leaving a 125.95px gap and growing the field + // from 32px to 55px tall. `offsetWidth` is the untransformed border-box + // width, so it reports 123 at every scale — the number this padding is + // actually denominated in. // // It is already an integer, which is the rounding the old `Math.ceil` // was there for: a fractional width left as-is reserves a hair too diff --git a/packages/core/src/Typeahead/Typeahead.doc.mjs b/packages/core/src/Typeahead/Typeahead.doc.mjs index 100a40ba9433e..90012795e8551 100644 --- a/packages/core/src/Typeahead/Typeahead.doc.mjs +++ b/packages/core/src/Typeahead/Typeahead.doc.mjs @@ -32,6 +32,12 @@ const anatomy = [ description: 'Token that presents the selected item while the control is not in edit mode.', }, + { + name: 'Spinner', + required: false, + description: + 'Loading indicator shown at the end of the input surface while a search is in flight.', + }, { name: 'Clear button', required: false, diff --git a/packages/core/src/Typeahead/Typeahead.spec.md b/packages/core/src/Typeahead/Typeahead.spec.md index 01f7bfe5fc336..f1dc7b98db0f3 100644 --- a/packages/core/src/Typeahead/Typeahead.spec.md +++ b/packages/core/src/Typeahead/Typeahead.spec.md @@ -68,6 +68,8 @@ Consumer migration instructions belong in consumer docs and release notes. `token` target. - The Clear button — rendered by Field's InputClearButton and themed through Field's current `input-clear-button` target. +- Loading-indicator presentation — owned by `component:Spinner`. Typeahead + decides only where the indicator sits. - General Icon presentation for an Icon-rendered start icon. - Arbitrary ReactNode start or item content supplied by the caller. - A target/state for the outer Result row, Result group heading, or selected-row @@ -89,7 +91,7 @@ public component documentation. | FR3 | Every result, whether default or caller-rendered, sits inside the same stable outer Result row that owns option semantics, highlight, selected styling, pointer interaction, and keyboard indexing. | Current source and tests | Verified current behavior; no new behavior decided | | FR4 | `typeahead-item` reaches only the standard TypeaheadItem label/supporting-content branch inside a Result row. A custom `renderItem` branch or caller-provided `item.element` does not receive that target, and no branch gives the outer Result row a Typeahead target. | Current source and owner tests | Verified current asymmetry; no normalization | | FR5 | Result group heading and Selected result state are stable visible concepts but no current Typeahead target reaches them. The selected check uses a generic Icon, which does not expose a Typeahead-specific selected-row selector. | Current source | Verified factual reachability gaps | -| FR6 | When present, Field, Selected token, Clear button, and Icon-rendered start icon retain their Field, Token, and Icon theming owners; arbitrary ReactNode start and item content stays caller-owned. | Current composition and owners | Verified current behavior; no target change | +| FR6 | When present, Field, Selected token, Clear button, Spinner, and Icon-rendered start icon retain their Field, Token, Spinner, and Icon theming owners; arbitrary ReactNode start and item content stays caller-owned. | Current composition and owners | Verified current behavior; no target change | ### Allowed variation @@ -140,6 +142,7 @@ listbox, option, live-region, focus, or dismissal behavior. | Icon-rendered start icon | Presents a semantic or component icon through Icon. | Icon component | Supporting | FR6 | | Caller-rendered start content | Presents arbitrary caller content directly in the input surface's start slot. | Caller-supplied content | Context-dependent | FR6 | | Selected token | Presents the selected value outside edit mode through Token. | Token component | Prominent | FR1, FR6 | +| Spinner | Indicates a search in flight, at the end of the input surface. | Spinner component | Supporting | FR6 | | Clear button | Removes the selected value through the shared field clear action. | Field component | Supporting | FR6 | | Dropdown | Paints the anchored listbox surface containing results or Empty state. | Current source and public docs | Prominent | FR2 | | Empty state | Presents the no-results message after a completed empty search. | Current source and public docs | Prominent | FR2 | @@ -174,6 +177,9 @@ normalize targets. "Selected token": { "delegatesTo": {"owner": "component:Token", "target": "token"} }, + "Spinner": { + "delegatesTo": {"owner": "component:Spinner", "target": "spinner"} + }, "Clear button": { "delegatesTo": { "owner": "component:Field", diff --git a/packages/core/src/Typeahead/Typeahead.test.tsx b/packages/core/src/Typeahead/Typeahead.test.tsx index 4e2736a004e2e..e7c02f995710c 100644 --- a/packages/core/src/Typeahead/Typeahead.test.tsx +++ b/packages/core/src/Typeahead/Typeahead.test.tsx @@ -1612,8 +1612,8 @@ describe('end controls stay in flow', () => { it('never reserves room with a measured width', () => { // The custom property is Tokenizer's mechanism and must not reappear // here: a width measured in viewport space and spent as local padding is - // wrong under any CSS transform (scale(.5) overlapped the query by - // 22.83px, scale(2) left a 202.69px gap). + // wrong under any CSS transform (measured on Tokenizer's 123px lane, + // scale(.5) covered 14px of the query and scale(2) left a 125.95px gap). const {container} = render( { />, ); expect( - container.querySelector('[style*="--_astryx-end-lane-width"]'), + container.querySelector('[style*="--_tokenizer-end-lane-width"]'), ).toBeNull(); }); @@ -1632,8 +1632,9 @@ describe('end controls stay in flow', () => { // than by leaning on the input to absorb the free space, because it // collapses that input to nothing while a token shows. Without it the // clear button sat against the token in mid-field instead of in the - // corner (measured: x=39 in a 300px field, against TextInput's 281). - const {container} = render( + // corner: measured in Chromium at a 300px field width, x=49.7 with the + // margin removed against x=271 with it. + render( Date: Tue, 1 Sep 2026 21:26:08 -0700 Subject: [PATCH 7/8] fix(Typeahead,Tokenizer): close the busy handoff behind a module boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `__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. --- packages/core/src/Tokenizer/Tokenizer.tsx | 76 ++++++++++--------- packages/core/src/Typeahead/BaseTypeahead.tsx | 32 ++------ .../core/src/Typeahead/Typeahead.test.tsx | 52 +++++++++---- packages/core/src/Typeahead/Typeahead.tsx | 71 +++++++++-------- .../core/src/Typeahead/busyIndicatorLane.tsx | 52 +++++++++++++ 5 files changed, 177 insertions(+), 106 deletions(-) create mode 100644 packages/core/src/Typeahead/busyIndicatorLane.tsx diff --git a/packages/core/src/Tokenizer/Tokenizer.tsx b/packages/core/src/Tokenizer/Tokenizer.tsx index 80dcdbbc0e752..a82465ab91f3e 100644 --- a/packages/core/src/Tokenizer/Tokenizer.tsx +++ b/packages/core/src/Tokenizer/Tokenizer.tsx @@ -25,6 +25,7 @@ import React, { type ReactNode, } from 'react'; import * as stylex from '@stylexjs/stylex'; +import {BusyIndicatorLaneProvider} from '../Typeahead/busyIndicatorLane'; import type {BaseProps} from '../BaseProps'; import type {SizeValue} from '../utils/types'; import {BaseTypeahead} from '../Typeahead/BaseTypeahead'; @@ -470,6 +471,7 @@ export function Tokenizer({ // Reported by BaseTypeahead so the indicator can live in this field's own // end lane, beside endContent and the clear button. const [isLoading, setIsLoading] = useState(false); + const busyLane = useMemo(() => ({onBusyChange: setIsLoading}), []); // What sits in the end lane varies most here — a spinner, arbitrary // `endContent`, a clear button, or all three — so its width is measured // rather than assumed, and the input reserves it. @@ -813,41 +815,45 @@ export function Tokenizer({ ) : ( tokens )} - 0 - ? styles.inputCompact - : undefined, - // Not for the collapsed states above: those give the input no width - // to pad, and `inputAtMax` zeroes its padding outright. - !(isAtMax || isTruncated) && hasEndLane && laneReserve, - ]} - /> + {/* The base reports its busy state through this lane, so the + indicator lands in the end controls below beside the clear + button rather than as a second one inside the base. */} + + 0 + ? styles.inputCompact + : undefined, + // Not for the collapsed states above: those give the input no width + // to pad, and `inputAtMax` zeroes its padding outright. + !(isAtMax || isTruncated) && hasEndLane && laneReserve, + ]} + /> + {htmlName != null && value.map(item => ( extends Omit< */ __queryEntries?: (query: string, results: T[]) => T[]; - /** - * Called when a search starts or settles, so a wrapper can paint the busy - * state in the one inline-end lane it already owns for its clear button and - * end content, instead of the base rendering a second indicator competing - * for the same corner. - * - * Passing this takes the indicator over: the base stops rendering its own. - * A caller that does not pass it keeps the visible, named status the base - * has always rendered. - * - * Underscored and `@internal` for the same reason as `__queryEntries` - * above: `BaseTypeaheadProps` is re-exported from the package entry point, - * so anything named on it ships as public API at the next cut, and this is - * a wiring detail between the two wrappers and the base. - * - * @internal - */ - __onLoadingChange?: (isLoading: boolean) => void; - /** * Debounce delay in ms before triggering search after typing. * Set to 0 for synchronous/local search sources that don't need debouncing. @@ -415,7 +397,6 @@ export const BaseTypeahead = function BaseTypeahead({ onChangeQuery, onOpenChange, __queryEntries, - __onLoadingChange, inputId: externalInputId, ariaDescribedBy, ariaLabelledBy, @@ -468,10 +449,13 @@ export const BaseTypeahead = function BaseTypeahead({ // This effect only writes a ref, so it commits nothing and no wrapper // re-renders for it; every caller of `setLoading` runs from an event or an // awaited continuation, long after the first commit. - const onLoadingChangeRef = useRef(__onLoadingChange); + // A wrapper that owns the inline-end lane subscribes through context; see + // busyIndicatorLane.tsx for why this is not a prop. + const busyLane = useBusyIndicatorLane(); + const onLoadingChangeRef = useRef(busyLane?.onBusyChange); useIsomorphicLayoutEffect(() => { - onLoadingChangeRef.current = __onLoadingChange; - }, [__onLoadingChange]); + onLoadingChangeRef.current = busyLane?.onBusyChange; + }, [busyLane]); const loadingRef = useRef(false); const setLoading = useCallback((next: boolean) => { if (loadingRef.current === next) { @@ -984,7 +968,7 @@ export const BaseTypeahead = function BaseTypeahead({ inputXStyle, )} /> - {isLoading && __onLoadingChange == null && ( + {isLoading && busyLane == null && ( diff --git a/packages/core/src/Typeahead/Typeahead.test.tsx b/packages/core/src/Typeahead/Typeahead.test.tsx index e7c02f995710c..efbef3ee3dc43 100644 --- a/packages/core/src/Typeahead/Typeahead.test.tsx +++ b/packages/core/src/Typeahead/Typeahead.test.tsx @@ -21,7 +21,10 @@ import { import {render, screen, fireEvent, waitFor, act} from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import {Typeahead} from './Typeahead'; +import {readFile} from 'node:fs/promises'; +import {resolve} from 'node:path'; import {BaseTypeahead} from './BaseTypeahead'; +import {BusyIndicatorLaneProvider} from './busyIndicatorLane'; import type {SearchSource, SearchableItem} from './types'; import {InternationalizationProvider} from '../i18n'; @@ -1508,13 +1511,14 @@ describe('busy indicator ownership', () => { const {source, settle} = pendingSource(); const onLoadingChange = vi.fn(); render( - {}} - __onLoadingChange={onLoadingChange} - debounceMs={0} - />, + + {}} + debounceMs={0} + /> + , ); const input = screen.getByRole('combobox'); @@ -1536,6 +1540,23 @@ describe('busy indicator ownership', () => { expect(onLoadingChange).toHaveBeenLastCalledWith(false); }); + it('keeps the busy handoff out of the exported prop surface', async () => { + // The handoff used to be `__onLoadingChange` on BaseTypeaheadProps, which + // the package entry point re-exports — so a builder reading the exported + // declaration would find it and could reasonably wire it, pinning a detail + // between two wrappers and their base as permanent API. An `@internal` tag + // is a note to a reader; a module boundary is the actual seam. + const entry = await import('./index'); + expect(Object.keys(entry)).not.toContain('BusyIndicatorLaneProvider'); + expect(Object.keys(entry)).not.toContain('useBusyIndicatorLane'); + + const source = await readFile( + resolve(__dirname, 'BaseTypeahead.tsx'), + 'utf8', + ); + expect(source).not.toContain('__onLoadingChange'); + }); + it('reports each transition once, and reports nothing when nothing changed', async () => { // Edge-triggered on purpose. Every keystroke below the query threshold // clears the flag, and an unconditional report would hand the wrapper a @@ -1544,14 +1565,15 @@ describe('busy indicator ownership', () => { const {source, settle} = pendingSource(); const onLoadingChange = vi.fn(); render( - {}} - __onLoadingChange={onLoadingChange} - minQueryLength={3} - debounceMs={0} - />, + + {}} + minQueryLength={3} + debounceMs={0} + /> + , ); const input = screen.getByRole('combobox'); diff --git a/packages/core/src/Typeahead/Typeahead.tsx b/packages/core/src/Typeahead/Typeahead.tsx index f79044c14f7df..337ffabdd71bd 100644 --- a/packages/core/src/Typeahead/Typeahead.tsx +++ b/packages/core/src/Typeahead/Typeahead.tsx @@ -22,10 +22,12 @@ import React, { useCallback, useId, useRef, + useMemo, useState, type ReactNode, } from 'react'; import * as stylex from '@stylexjs/stylex'; +import {BusyIndicatorLaneProvider} from './busyIndicatorLane'; import {BaseTypeahead} from './BaseTypeahead'; import {useSize} from '../SizeContext/SizeContext'; import { @@ -317,6 +319,7 @@ export function Typeahead({ // Reported by BaseTypeahead so the indicator can live in this field's own // end lane, beside the clear button, rather than in the engine's row. const [isLoading, setIsLoading] = useState(false); + const busyLane = useMemo(() => ({onBusyChange: setIsLoading}), []); const [isEditing, setIsEditing] = useState(false); const [editingValue, setEditingValue] = useState(null); @@ -484,38 +487,42 @@ export function Typeahead({ xstyle={styles.token} /> )} - + {/* The base reports its busy state through this lane, so the + indicator lands in the end controls below beside the clear + button rather than as a second one inside the base. */} + + + {(isLoading || (hasClear && value && !isDisabled)) && (
{isLoading && ( diff --git a/packages/core/src/Typeahead/busyIndicatorLane.tsx b/packages/core/src/Typeahead/busyIndicatorLane.tsx new file mode 100644 index 0000000000000..7a2041fd6c361 --- /dev/null +++ b/packages/core/src/Typeahead/busyIndicatorLane.tsx @@ -0,0 +1,52 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +'use client'; + +/** + * @file busyIndicatorLane.tsx + * @input A wrapper's loading setter, provided around BaseTypeahead + * @output Context the base uses to hand its busy state to that wrapper + * @position Package-internal; not exported from the package entry point + */ + +import {createContext, use} from 'react'; + +/** + * How the base hands its busy state to a wrapper that owns the inline-end lane. + * + * Typeahead and Tokenizer both render a clear button and end content in one + * corner, and both want the busy indicator in that same lane rather than a + * second one competing for the corner. The base is the only thing that knows + * when a search starts or settles, so that state has to travel from the base + * out to its wrapper. + * + * A CONTEXT rather than a prop, because a prop cannot be package-internal here: + * `BaseTypeaheadProps` is re-exported from the package entry point, so every + * name on it ships as public API however it is commented — an `@internal` tag + * is a note to a reader, not a boundary. A builder reading the exported + * declaration would find `__onLoadingChange` and could reasonably wire it, + * pinning a handoff between two wrappers and their base as permanent API. + * + * This module is not exported from `index.ts`, so the seam is closed by module + * boundary instead of by naming convention. + */ +export interface BusyIndicatorLane { + /** Called when a search starts or settles. */ + onBusyChange: (isBusy: boolean) => void; +} + +const BusyIndicatorLaneContext = createContext(null); +BusyIndicatorLaneContext.displayName = 'BusyIndicatorLaneContext'; + +export const BusyIndicatorLaneProvider = BusyIndicatorLaneContext.Provider; + +/** + * The wrapper's lane, or null when the base is used directly. + * + * Null is the released behaviour: the base renders its own visible, named + * status. Non-null means a wrapper paints it instead, so the base renders + * nothing and the two cannot both appear. + */ +export function useBusyIndicatorLane(): BusyIndicatorLane | null { + return use(BusyIndicatorLaneContext); +} From b8fc47b91e4290ff3ce74efc7ab0bd432c34fe34 Mon Sep 17 00:00:00 2001 From: freddytan Date: Tue, 1 Sep 2026 21:30:37 -0700 Subject: [PATCH 8/8] fix(Typeahead): bound the selected value with a content lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/typeahead-collapsed-width.md | 76 ++++++++ apps/storybook/rtl-audit/targets.json | 57 +++++- apps/storybook/stories/Tokenizer.stories.tsx | 27 +++ apps/storybook/stories/Typeahead.stories.tsx | 84 +++++++++ .../core/src/Typeahead/Typeahead.test.tsx | 145 ++++++++++++++- packages/core/src/Typeahead/Typeahead.tsx | 166 +++++++++++------- 6 files changed, 486 insertions(+), 69 deletions(-) create mode 100644 .changeset/typeahead-collapsed-width.md diff --git a/.changeset/typeahead-collapsed-width.md b/.changeset/typeahead-collapsed-width.md new file mode 100644 index 0000000000000..8c415a4942167 --- /dev/null +++ b/.changeset/typeahead-collapsed-width.md @@ -0,0 +1,76 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] Typeahead: the field keeps its width when a value is selected, and the value stays out of the end controls (#5560) + +Two halves of one promise from the input-field family contract +(`docs/families/input-fields.md`): **FR1**, a field's available width does not +change because its value did; and **FR2**, a visible end affordance does not have +field content painted under it. + +**FR1 — the input keeps its place.** Every other field in the family gets a +stable width for free: the `` stays in flow, and the field is as wide as +the input's own intrinsic width. Typeahead took the input out of flow and zeroed +its width while a token showed, so the field was left measuring the token. In any +shrink-to-fit parent it snapped to the value's length. Block-level parents hid +it, because they fill their container whatever their content is, which is why no +story caught it. The input now keeps its place in the row and its own width — it +is only made invisible and inert — and the token is painted over that space +rather than beside it. In flow the token would add its own width instead, which +is the same value-dependent sizing from the other direction: a long value would +grow the field. + +**FR2 — the value is bounded by a content lane.** The input and the token share +a content lane: an ordinary flex item, `flex: 1` with `min-width: 0`, that ends +exactly where the end lane begins. That is TextInput's own arrangement — the lane +takes the free space so the end controls sit in the corner, and yields all of it +when the field is narrow, so a narrow field cannot overflow. The token is +anchored at both of the lane's inline edges, so a long value ellipsizes at the +lane's edge instead of reaching the controls. Positioned against the whole field +instead, as the first revision of this change did, it had no idea where those +controls start. + +Measured in Chromium. Widths are the field's border box, field in a `max-content` +parent, `Field.width` otherwise unset: + +| | empty | short value | long value | +| --------------------------------- | ----- | ----------- | ------------ | +| TextInput (family baseline) | 199px | 227px | 227px | +| Typeahead before | 199px | **54.7px** | **224.09px** | +| Typeahead after | 199px | 223px | 223px | +| Typeahead in `InputGroup`, before | 397px | **252.7px** | **422.09px** | +| Typeahead in `InputGroup`, after | 397px | 421px | 421px | + +The 24px between the empty and valued columns is the clear button entering the +row — ordinary for any field whose clear is conditional, it does not vary with the +value, and TextInput's is 28px. + +Overlap is the value's trailing edge past the clear button's leading edge; escape +is how far the value reaches past the field's border. The middle column is this +change's own first revision, which fixed the width and made the overlap worse: + +| field, long value | overlap on main | first revision | now | +| ----------------- | --------------- | -------------- | --------------- | +| shrink-to-fit | 12px | 28.09px | none, 7px clear | +| in `InputGroup` | 12px | 33px | none, 7px clear | +| 220px | 12px | 31.09px | none, 7px clear | +| 180px | 12px | 33px | none, 7px clear | +| 140px | 12px | 33px | none, 7px clear | +| escape, 140–220px | none | up to 4px | none | + +No new API and no constants. An earlier revision floored the field with a +`--typeahead-min-width` public var defaulting to 200px, which review rightly +rejected: it was a second sizing contract beside the documented `Field.width` +prop, it was hand-derived (the empty field measures 199, so the floor overshot by +1), `InputGroup` cancelled it, and it could not help `Tokenizer`. Nothing here +states a width; the lane's `min-width: 0` is the opposite of a floor. + +`Tokenizer` is **not** fixed here. It shares the family promise and breaks it — +199px empty to 114.7px with one token, in the same probe — but by a different +mechanism: its tokens are in flow and wrap, and its input deliberately becomes a +40px continuation lane after them, so what a wrapping multi-value field's width +should be is a design question rather than this bug. Its numbers are identical +before and after this change. + +@freddymeta diff --git a/apps/storybook/rtl-audit/targets.json b/apps/storybook/rtl-audit/targets.json index e45a938c7f1b3..9052f011a610d 100644 --- a/apps/storybook/rtl-audit/targets.json +++ b/apps/storybook/rtl-audit/targets.json @@ -1,4 +1,15 @@ [ + { + "component": "AppShell", + "storyId": "core-appshell--top-nav-with-side-nav", + "dims": [ + "D4" + ], + "selectors": { + "overlay": ".astryx-app-shell-sidenav", + "overlayRoot": ".astryx-app-shell" + } + }, { "component": "ButtonGroup", "storyId": "core-buttongroup--horizontal", @@ -10,6 +21,28 @@ "next": "[role=\"group\"] button:last-child" } }, + { + "component": "CheckboxList", + "storyId": "core-checkboxlist--rich-descriptions", + "dims": [ + "D2" + ], + "selectors": { + "prev": ".astryx-checkbox-input", + "next": "[data-testid=\"checkbox-end-content\"]" + } + }, + { + "component": "RadioList", + "storyId": "core-radiolist--rich-content", + "dims": [ + "D2" + ], + "selectors": { + "prev": "input[aria-label=\"Pro\"]", + "next": "[data-testid=\"radio-end-content\"]" + } + }, { "component": "Calendar", "storyId": "core-calendar--default", @@ -45,5 +78,27 @@ "prev": "button[aria-label=\"Previous\"]", "next": "button[aria-label=\"Next\"]" } + }, + { + "component": "Typeahead", + "storyId": "core-typeahead--logical-order", + "dims": [ + "D2" + ], + "selectors": { + "prev": ".astryx-token", + "next": ".astryx-input-clear-button" + } + }, + { + "component": "Tokenizer", + "storyId": "core-tokenizer--logical-order", + "dims": [ + "D2" + ], + "selectors": { + "prev": ".astryx-token", + "next": ".astryx-input-clear-button" + } } -] \ No newline at end of file +] diff --git a/apps/storybook/stories/Tokenizer.stories.tsx b/apps/storybook/stories/Tokenizer.stories.tsx index 10d7ce7bbb728..d9cde7f0c8c5c 100644 --- a/apps/storybook/stories/Tokenizer.stories.tsx +++ b/apps/storybook/stories/Tokenizer.stories.tsx @@ -496,3 +496,30 @@ export const Loading: Story = { }, name: 'Loading (async source, with clear and end content)', }; + +/** + * Tokens plus a clear-all button — the two ends of the field. Under RTL they + * must swap sides; this is the story the RTL audit measures as a D2 + * layout-order-flip. + */ +export const LogicalOrder: Story = { + render: args => { + const [value, setValue] = useState([users[0], users[2]]); + return ( +
+ setValue(items)} + /> +
+ ); + }, + args: { + label: 'Team Members', + placeholder: 'Add more...', + hasClear: true, + }, + name: 'Logical order', +}; diff --git a/apps/storybook/stories/Typeahead.stories.tsx b/apps/storybook/stories/Typeahead.stories.tsx index 4fb2d2e5421e2..2a34d4e791a21 100644 --- a/apps/storybook/stories/Typeahead.stories.tsx +++ b/apps/storybook/stories/Typeahead.stories.tsx @@ -24,6 +24,23 @@ const fruitSource: SearchSource = { bootstrap: () => fruits.slice(0, 5), }; +/** + * A value longer than the input's own width — the case that used to collapse + * the field onto its value, and then to run under the clear button. + */ +const longFruit: SearchableItem = { + id: '9', + label: 'Elderberry and Blackcurrant Preserve', +}; + +const longFruitSource: SearchSource = { + search: (query: string) => + [...fruits, longFruit].filter(f => + f.label.toLowerCase().includes(query.toLowerCase()), + ), + bootstrap: () => [longFruit, ...fruits.slice(0, 4)], +}; + /** * A remote source, near enough. The busy state only exists between the * keystroke and the response, so a synchronous source never shows it — which @@ -307,3 +324,70 @@ export const Loading: Story = { }, name: 'Loading (async source)', }; + +/** + * The two cases no Typeahead story covered, which is why a bug this visible + * survived: a value selected, and a parent that is sized by its content. + * + * Every other story renders in a fixed-width container, and a block-level + * parent fills its container whatever its content is — so both hid a field + * that sized itself to its value. Here the field is a flex item, so it is + * shrink-to-fit: a table cell, an inline toolbar, a floated column. + * + * Left, a long value: it must not widen the field, and it must ellipsize + * before the clear button rather than under it. Right, an empty field for + * comparison — the two must be the same width. + */ +export const SelectedValueInAContentSizedParent: Story = { + render: () => { + const [a, setA] = useState(longFruit); + const [b, setB] = useState(null); + return ( +
+ + +
+ ); + }, + name: 'Selected value in a content-sized parent', +}; + +/** + * One field with a selected value and a clear button — the two ends of the + * content lane. The token opens the lane and the clear button closes it, so + * under RTL they must swap sides: this is the story the RTL audit measures as + * a D2 layout-order-flip. + * + * A single field on purpose. The comparison story next to it renders two, and + * the audit's selectors would match across both. + */ +export const LogicalOrder: Story = { + render: () => { + const [value, setValue] = useState(longFruit); + return ( +
+ +
+ ); + }, + name: 'Logical order', +}; diff --git a/packages/core/src/Typeahead/Typeahead.test.tsx b/packages/core/src/Typeahead/Typeahead.test.tsx index efbef3ee3dc43..f1497c9869fd1 100644 --- a/packages/core/src/Typeahead/Typeahead.test.tsx +++ b/packages/core/src/Typeahead/Typeahead.test.tsx @@ -1283,7 +1283,13 @@ describe('Typeahead disabledMessage', () => { />, ); - const container = screen.getByRole('combobox').parentElement as HTMLElement; + // The field itself, by its own class: the input no longer sits directly + // inside it — it is in the content lane that bounds the value — and + // `mouseEnter` does not bubble, so the hover has to land on the element + // the tooltip is actually bound to. + const container = document.querySelector( + '.astryx-typeahead', + ) as HTMLElement; const tooltip = screen.getByRole('tooltip', h); expect(tooltip).toHaveTextContent('You need the Editor role'); @@ -1649,13 +1655,12 @@ describe('end controls stay in flow', () => { ).toBeNull(); }); - it('holds the controls at the inline end when a token collapses the input', () => { - // The field pushes its end controls over with an `auto` margin rather - // than by leaning on the input to absorb the free space, because it - // collapses that input to nothing while a token shows. Without it the - // clear button sat against the token in mid-field instead of in the - // corner: measured in Chromium at a 300px field width, x=49.7 with the - // margin removed against x=271 with it. + it('holds the controls at the inline end when a token shows', () => { + // `auto` gives free space to the margin rather than to a sibling, so the + // controls stay in the corner in the states where the content lane is not + // the only flexible item in the row. Without it the clear button sat + // against the token in mid-field instead of in the corner (measured: + // x=39 in a 300px field, against TextInput's 281). render( { const field = container.querySelector('.astryx-typeahead') as HTMLElement; expect(getComputedStyle(field).flexWrap).not.toBe('wrap'); }); + + it('keeps the input in flow while a token shows, so the field keeps its width', () => { + // A field's width must not depend on its value. Every other field in the + // family gets that for free: the `` stays in flow and the field is + // as wide as the input's own intrinsic width. This one used to take the + // input out of flow and zero its width when a token showed, leaving the + // field measuring the token — in a shrink-to-fit parent it snapped to the + // value's length (199px to 57px in Chromium, #5560). Block parents hid it, + // which is why no story caught it. jsdom resolves no layout, so assert the + // mechanism: the input still occupies the row. + render( + {}} + />, + ); + const input = screen.getByRole('combobox'); + const style = getComputedStyle(input); + expect(style.position).not.toBe('absolute'); + expect(style.width).not.toBe('0px'); + expect(style.flex).not.toBe('0 0 0'); + }); + + it('paints the token over the input rather than beside it', () => { + // In flow the token would add its own width to the row — the same + // value-dependent sizing from the other direction, where a long value + // grows the field instead of collapsing it. + const {container} = render( + {}} + />, + ); + const token = container.querySelector('.astryx-token') as HTMLElement; + expect(getComputedStyle(token).position).toBe('absolute'); + }); + + it('lets the token take the pointer that the hidden input would swallow', () => { + // The input still covers that space, so it has to stop intercepting the + // clicks that enter edit mode. + render( + {}} + />, + ); + expect(getComputedStyle(screen.getByRole('combobox')).pointerEvents).toBe( + 'none', + ); + }); +}); + +describe('the value is bounded by the content lane', () => { + // Keeping the input in flow fixed the field's width, but it left the token + // positioned against the whole field, which has no idea where the end + // controls start. A value longer than the input then ran under the clear + // button and out past the field's own border — measured in Chromium at + // 28-33px of overlap and up to 4px outside the border, worse than the 12px + // of overlap on main. The lane is the box the value may occupy: an ordinary + // flex item that ends exactly where the end lane begins. + // + // jsdom resolves no layout, so what is asserted here is the mechanism. The + // geometry is browser-verified in the PR. + const renderWithValue = () => + render( + {}} + hasClear + />, + ); + + it('puts the input and the token in one lane, inside the field', () => { + const {container} = renderWithValue(); + const field = container.querySelector('.astryx-typeahead'); + const input = screen.getByRole('combobox'); + const token = container.querySelector('.astryx-token') as HTMLElement; + + const lane = input.parentElement as HTMLElement; + expect(lane).not.toBe(field); + expect(lane.parentElement).toBe(field); + // The token's containing block is the lane, which is what bounds it. + expect(token.parentElement).toBe(lane); + expect(getComputedStyle(lane).position).toBe('relative'); + }); + + it('lets the lane yield its whole width, so a narrow field cannot overflow', () => { + // `min-width: 0` is the half of this that the earlier `200px` floor got + // wrong: the field states no width of its own, so it still shrinks to + // whatever a narrow parent or an InputGroup gives it. + const {container} = renderWithValue(); + const lane = screen.getByRole('combobox').parentElement as HTMLElement; + const style = getComputedStyle(lane); + expect(style.minWidth).toBe('0'); + expect(style.flexGrow).toBe('1'); + // Nothing states a width: a narrow parent gets all of it back. + expect( + (container.querySelector('.astryx-typeahead') as HTMLElement).style + .minWidth, + ).toBe(''); + }); + + it("anchors the token at the lane's end, not just its start", () => { + // The bound that stops the value reaching the end controls. Anchored at + // one end only, the token is capped by the field's own padding box, which + // is past the clear button. + const {container} = renderWithValue(); + const token = container.querySelector('.astryx-token') as HTMLElement; + const style = getComputedStyle(token); + expect(style.insetInlineEnd).toBe('0'); + // `fit-content` against that pair of insets is what shrink-wraps the + // label yet still caps it at the lane; the `auto` end margin is what + // keeps the pair from being over-constrained and dropping the end inset. + expect(style.width).toBe('fit-content'); + expect(style.marginInlineEnd).toBe('auto'); + }); }); diff --git a/packages/core/src/Typeahead/Typeahead.tsx b/packages/core/src/Typeahead/Typeahead.tsx index 337ffabdd71bd..813de5c2bad3f 100644 --- a/packages/core/src/Typeahead/Typeahead.tsx +++ b/packages/core/src/Typeahead/Typeahead.tsx @@ -188,38 +188,82 @@ const styles = stylex.create({ ':is(:disabled,[aria-disabled="true"])': 'default', }, }, - token: { - // Offset token so it sits 3px from the inner edge (4px from outer edge - // accounting for 1px border). Default inline padding is 8px, so - // -(8px - 3px) = -5px positions token equidistant from left edge as top. - margin: `calc(-1 * (${spacingVars['--spacing-2']} - ${spacingVars['--spacing-1']} + 1px))`, - }, // The busy indicator and the clear button, at the field's inline end. // // In flow, as a flex child — an in-flow box takes up room, so the input // cannot run underneath it and nothing has to be measured. That is // TextInput's arrangement for the same two controls. // - // The `auto` margin is what TextInput does not need. Its input is always - // present and `flex: 1`, so it absorbs the free space and pushes these to - // the end on its own. This field collapses its input to nothing while a - // token shows (`inputHidden`), which leaves no flexible item in the row — - // so without the margin the controls sit against the token, mid-field, - // instead of in the corner where they belong. `auto` gives the free space - // to the margin instead of to a sibling, which needs no sibling to exist. + // The `auto` margin keeps them in the corner in the states where the + // content lane is not the only flexible item in the row (a start icon, a + // grouped row): `auto` gives free space to the margin rather than to a + // sibling, so it needs no sibling to exist. endLane: { display: 'flex', alignItems: 'center', gap: spacingVars['--spacing-1'], marginInlineStart: 'auto', }, - inputHidden: { - width: 0, + // The field's content lane: the input, and the token painted over it. + // + // This is the box the value is allowed to occupy, and it is what makes the + // end lane's space its own — the lane is an ordinary flex item that ends + // exactly where `endLane` begins, so a value bounded by it can never reach + // the clear button or the spinner. Before it existed the token was + // positioned against the whole field, so a value longer than the input ran + // under the end controls and past the field's border (measured 33px of + // overlap and 4px outside the border at a 180px field). + // + // `flex: 1` with `min-width: 0` is TextInput's own arrangement for its + // input: the lane takes the free space so the end controls sit in the + // corner, and yields all of it when the field is narrow, so a narrow field + // never overflows. Stretched, so its padding box matches the field's + // content box in the block direction and the token keeps the vertical + // placement it has always had. + contentLane: { + position: 'relative', + display: 'flex', + alignItems: 'center', + alignSelf: 'stretch', + flex: 1, minWidth: 0, - flex: '0 0 0', - padding: 0, + }, + // While a token shows, the input keeps its place in the row and its own + // intrinsic width — it is only made invisible and inert. That width is what + // sizes the field, exactly as it does for TextInput, so a Typeahead is as + // wide with a value as without one. Collapsing it to nothing instead left + // the field measuring the token: in any shrink-to-fit parent it snapped to + // the value's length (measured 199px to 57px, #5560), and block-level + // parents hid it because they fill their container whatever their content + // is, which is why no story caught it. + inputHidden: { opacity: 0, + // The token is painted over this space and owns the pointer; the input + // must not swallow clicks meant for it, or for the wrapper's own + // click-to-edit. + pointerEvents: 'none', + }, + // Painted over the input rather than beside it. In flow the token would add + // its own width to the row, which is the same value-dependent sizing from + // the other direction — a long value would grow the field. + // + // Bounded by the content lane at both ends. `fit-content` shrink-wraps the + // label but resolves against the space left between the two insets, so a + // long value ellipsizes at the lane's edge instead of running under the end + // controls; the `auto` end margin is what keeps that pair of insets from + // being over-constrained, which would drop the end one and let the token + // overflow again. The negative inline start and the zero block start put it + // where it has always sat: the lane's padding box is the field's content + // box, 3px inside the field's own padding on both axes. + tokenOverlay: { position: 'absolute' as const, + insetBlockStart: 0, + insetInlineStart: `calc(-1 * (${spacingVars['--spacing-1']} - 1px))`, + insetInlineEnd: 0, + width: 'fit-content', + marginBlock: 0, + marginInlineStart: 0, + marginInlineEnd: 'auto', }, }); @@ -477,51 +521,53 @@ export function Typeahead({ {inputGroup && ( {label} )} - {showToken && ( - - )} {/* The base reports its busy state through this lane, so the - indicator lands in the end controls below beside the clear - button rather than as a second one inside the base. */} + indicator lands in the end controls below beside the clear button + rather than as a second one inside the base. */} - +
+ {showToken && ( + + )} + +
{(isLoading || (hasClear && value && !isDisabled)) && (