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/.changeset/typeahead-tokenizer-busy-indicator.md b/.changeset/typeahead-tokenizer-busy-indicator.md new file mode 100644 index 0000000000000..a4a68cbe515cd --- /dev/null +++ b/.changeset/typeahead-tokenizer-busy-indicator.md @@ -0,0 +1,21 @@ +--- +'@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 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. 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 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/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 35c84871efab8..d9cde7f0c8c5c 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,51 @@ 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)', +}; + +/** + * 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 4acadf6d44a24..2a34d4e791a21 100644 --- a/apps/storybook/stories/Typeahead.stories.tsx +++ b/apps/storybook/stories/Typeahead.stories.tsx @@ -24,6 +24,45 @@ 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 + * 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 +304,90 @@ export const StatusVariantComparison: Story = { ); }, }; + +export const Loading: Story = { + render: () => { + const [value, setValue] = useState(null); + return ( +
+ +
+ ); + }, + 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/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 8f42abebf2761..1db25c3dd4b3a 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,256 @@ 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*="--_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 + // `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( + '--_tokenizer-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( + '--_tokenizer-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/Tokenizer/Tokenizer.tsx b/packages/core/src/Tokenizer/Tokenizer.tsx index 9b2494aaf31b3..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'; @@ -40,6 +41,8 @@ import { type FieldStatusVariant, } from '../Field'; import {Token} from '../Token'; +import {Spinner} from '../Spinner'; +import {useEndLaneReserve} from './useEndLaneReserve'; import {renderIconSlot, type IconType} from '../Icon'; import {OverflowList} from '../OverflowList'; import {useLayer} from '../Layer/useLayer'; @@ -232,6 +235,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 +274,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 +468,19 @@ 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); + 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. + 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; @@ -795,37 +815,45 @@ export function Tokenizer({ ) : ( tokens )} - 0 - ? styles.inputCompact - : undefined - } - /> + {/* 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 => ( ({ disabled={isDisabled} /> ))} - {(endContent || (hasClear && value.length > 0 && !isDisabled)) && ( -
+ {hasEndLane && ( +
+ {isLoading && ( + + )} {endContent} {hasClear && value.length > 0 && !isDisabled && ( ({ + paddingInlineEnd: `calc(${laneInset} + var(${LANE_WIDTH_VAR}, 0px))`, + }), +}); + +/** + * 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. 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 + * 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. 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 laneRef = useCallback((node: HTMLElement | null) => { + if (node == null) { + 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; + + observeResize(node, () => { + // `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 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 + // 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 + // 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, reserveStyles.reserve(laneInset)]; +} diff --git a/packages/core/src/Typeahead/BaseTypeahead.tsx b/packages/core/src/Typeahead/BaseTypeahead.tsx index 2f2b6af08bdf4..aab4d155c78ce 100644 --- a/packages/core/src/Typeahead/BaseTypeahead.tsx +++ b/packages/core/src/Typeahead/BaseTypeahead.tsx @@ -27,12 +27,15 @@ import React, { type RefObject, } from 'react'; import * as stylex from '@stylexjs/stylex'; +import {useBusyIndicatorLane} from './busyIndicatorLane'; 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'; +import {Spinner} from '../Spinner'; import { colorVars, spacingVars, @@ -310,10 +313,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'], }, }); @@ -424,6 +432,40 @@ 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. + // + // 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. + // 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 = busyLane?.onBusyChange; + }, [busyLane]); + 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 +541,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 +576,7 @@ export const BaseTypeahead = function BaseTypeahead({ setHighlightedIndex(-1); } finally { if (searchGenRef.current === gen) { - setIsLoading(false); + setLoading(false); } } }, @@ -545,6 +587,7 @@ export const BaseTypeahead = function BaseTypeahead({ announce, emptySearchResultsText, __queryEntries, + setLoading, t, ], ); @@ -552,7 +595,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 +615,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 +651,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 +688,7 @@ export const BaseTypeahead = function BaseTypeahead({ debounceMs, searchSource, announce, + setLoading, ], ); @@ -673,11 +717,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 +933,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 +968,11 @@ export const BaseTypeahead = function BaseTypeahead({ inputXStyle, )} /> - {isLoading && ( - - + {isLoading && busyLane == 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 +932,7 @@ describe('BaseTypeahead minQueryLength', () => { expect( screen.queryByRole('status', {name: 'Loading'}), ).not.toBeInTheDocument(); + expect(input).not.toHaveAttribute('aria-busy'); expect(input).toHaveAttribute('aria-expanded', 'false'); }); }); @@ -1278,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'); @@ -1445,3 +1456,361 @@ 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( + + {}} + 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('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 + // `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( + + {}} + 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); + }); +}); + +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( + {}} + />, + ); + const field = container.querySelector('.astryx-typeahead'); + const clear = screen.getByRole('button', {name: /clear/i}); + expect(field).toContainElement(clear); + // 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', () => { + // 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 (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*="--_tokenizer-end-lane-width"]'), + ).toBeNull(); + }); + + 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 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 + // 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'); + }); + + 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 4c05dd1579ff8..813de5c2bad3f 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 { @@ -42,6 +44,7 @@ import {Token} from '../Token'; import {useTooltip} from '../Tooltip'; import {renderIconSlot, type IconType} from '../Icon'; import {VisuallyHidden} from '../VisuallyHidden'; +import {Spinner} from '../Spinner'; import {spacingVars, sizeVars} from '../theme/tokens.stylex'; import {groupStyles} from '../InputGroup/groupStyles'; import {useInputGroup} from '../InputGroup/InputGroupContext'; @@ -170,7 +173,12 @@ export interface TypeaheadProps extends Omit< 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 @@ -180,29 +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))`, - }, - clearButton: { - position: 'absolute', - top: `calc((${sizeVars['--size-element-md']} - 20px) / 2 - 1px)`, - insetInlineEnd: `calc((${sizeVars['--size-element-md']} - 20px) / 2 - 1px)`, - height: '20px', + // 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 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', }, - clearButtonSm: { - top: `calc((${sizeVars['--size-element-sm']} - 20px) / 2 - 1px)`, - insetInlineEnd: `calc((${sizeVars['--size-element-sm']} - 20px) / 2 - 1px)`, + // 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, }, + // 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: { - width: 0, - minWidth: 0, - flex: '0 0 0', - padding: 0, 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', }, }); @@ -299,6 +360,10 @@ 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); + const busyLane = useMemo(() => ({onBusyChange: setIsLoading}), []); const [isEditing, setIsEditing] = useState(false); const [editingValue, setEditingValue] = useState(null); @@ -456,56 +521,69 @@ export function Typeahead({ {inputGroup && ( {label} )} - {showToken && ( - - )} - - {hasClear && value && !isDisabled && ( - { - e.stopPropagation(); - handleClear(); - }} - xstyle={[styles.clearButton, size === 'sm' && styles.clearButtonSm]} - /> + {/* 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. */} + +
+ {showToken && ( + + )} + +
+ + {(isLoading || (hasClear && value && !isDisabled)) && ( +
+ {isLoading && ( + + )} + {hasClear && value && !isDisabled && ( + { + e.stopPropagation(); + handleClear(); + }} + /> + )} +
)}
{showsDisabledMessage && 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); +}