diff --git a/packages/codemods/src/migrations.generated.ts b/packages/codemods/src/migrations.generated.ts index 4c4b9f9199..a583bf303a 100644 --- a/packages/codemods/src/migrations.generated.ts +++ b/packages/codemods/src/migrations.generated.ts @@ -6,6 +6,16 @@ import type { MigrationEntry } from "./catalog/types.js"; /** Every migration, newest first. Bodies live in `src/migrations`. */ export const migrations: Omit[] = [ + { + id: "option-value-inferred-from-mixed-children", + since: "1.1.12", + title: "Option: value is inferred from mixed children", + kind: "migration", + action: "none", + remotePackage: true, + apply: + "No code change required for the option itself. An `Option` whose children are text plus an element — text and a `Badge`, text and an icon — now carries that text as its key, where it previously fell back to react-aria's generated `react-aria-N`. Check anywhere such a key was read back: a stored or server-side selection, a `defaultValue`/`value` matched against it, or a test asserting on it. Pass an explicit `value` to pin the key to something other than the display text.", + }, { id: "use-design-tokens-build-metadata-removed", since: "1.0.16", diff --git a/packages/codemods/src/migrations/option-value-inferred-from-mixed-children/entry.md b/packages/codemods/src/migrations/option-value-inferred-from-mixed-children/entry.md new file mode 100644 index 0000000000..fc7e8e05d6 --- /dev/null +++ b/packages/codemods/src/migrations/option-value-inferred-from-mixed-children/entry.md @@ -0,0 +1,60 @@ +--- +since: 1.1.12 +title: "Option: value is inferred from mixed children" +kind: migration +action: none +remotePackage: true +apply: + "No code change required for the option itself. An `Option` whose children are + text plus an element — text and a `Badge`, text and an icon — now carries that + text as its key, where it previously fell back to react-aria's generated + `react-aria-N`. Check anywhere such a key was read back: a stored or + server-side selection, a `defaultValue`/`value` matched against it, or a test + asserting on it. Pass an explicit `value` to pin the key to something other + than the display text." +--- + +An `Option` used to infer its `textValue` — and, through it, its `value` — only +when its children were exactly one text node. Text next to any element left both +undefined, so react-aria assigned the option a key off a render-order counter: + +```tsx + +``` + +| Option | key before | key now | +| ----------------------------- | -------------- | ------------------- | +| `Millennium Falcon` + `Badge` | `react-aria-1` | `Millennium Falcon` | +| `X-Wing` | `X-Wing` | `X-Wing` | + +The key is what the field reports as its selected value and what `defaultValue` +has to match, so the old key made the field submit a meaningless string, could +not be preselected, and shifted when unrelated markup around it changed. + +Only text that is a child itself contributes — text inside an element child does +not, so the option above is `"Millennium Falcon"` and not +`"Millennium Falcon Latest"`. + +An `Option` with no text among its children at all still has no key to infer, +and now says so on the console instead of failing silently: + +```tsx +// logs: An +``` + +Give those options a `value`: + +```diff +- +``` diff --git a/packages/codemods/src/tests/guide.test.ts b/packages/codemods/src/tests/guide.test.ts index c14c19ea1a..d1d5f2b954 100644 --- a/packages/codemods/src/tests/guide.test.ts +++ b/packages/codemods/src/tests/guide.test.ts @@ -24,7 +24,7 @@ describe("the generated migration guide", () => { const order = [...committed.matchAll(/<\/a>/g)].map( (match) => match[1], ); - expect(order[0]).toBe("use-design-tokens-build-metadata-removed"); + expect(order[0]).toBe("option-value-inferred-from-mixed-children"); expect(order.at(-1)).toBe("renamed-css-export"); }); }); diff --git a/packages/codemods/src/tests/remoteScope.test.ts b/packages/codemods/src/tests/remoteScope.test.ts index 8f2a9600c5..884ea3b370 100644 --- a/packages/codemods/src/tests/remoteScope.test.ts +++ b/packages/codemods/src/tests/remoteScope.test.ts @@ -53,6 +53,7 @@ const targets: Record = { "form-resets-after-modal-close": ["Form"], "overlay-controller-add-on-close-return-type": ["OverlayController"], "cartesian-chart-empty-view": ["CartesianChart"], + "option-value-inferred-from-mixed-children": ["Option"], "action-prop-to-on-action": ["Action", "ActionProps"], "button-props-interfaces": [ "ResetButtonProps", diff --git a/packages/components/MIGRATION.md b/packages/components/MIGRATION.md index a47d4e2604..b53897fda3 100644 --- a/packages/components/MIGRATION.md +++ b/packages/components/MIGRATION.md @@ -28,6 +28,68 @@ The CLI's own output does detect it and prints the right form. --- + + +## Option: value is inferred from mixed children + +**Since `1.1.12`** · migration · no code change needed · also applies to +`@mittwald/flow-remote-react-components` + +An `Option` used to infer its `textValue` — and, through it, its `value` — only +when its children were exactly one text node. Text next to any element left both +undefined, so react-aria assigned the option a key off a render-order counter: + +```tsx + +``` + +| Option | key before | key now | +| ----------------------------- | -------------- | ------------------- | +| `Millennium Falcon` + `Badge` | `react-aria-1` | `Millennium Falcon` | +| `X-Wing` | `X-Wing` | `X-Wing` | + +The key is what the field reports as its selected value and what `defaultValue` +has to match, so the old key made the field submit a meaningless string, could +not be preselected, and shifted when unrelated markup around it changed. + +Only text that is a child itself contributes — text inside an element child does +not, so the option above is `"Millennium Falcon"` and not +`"Millennium Falcon Latest"`. + +An `Option` with no text among its children at all still has no key to infer, +and now says so on the console instead of failing silently: + +```tsx +// logs: An +``` + +Give those options a `value`: + +```diff +- +``` + +**Apply:** No code change required for the option itself. An `Option` whose +children are text plus an element — text and a `Badge`, text and an icon — now +carries that text as its key, where it previously fell back to react-aria's +generated `react-aria-N`. Check anywhere such a key was read back: a stored or +server-side selection, a `defaultValue`/`value` matched against it, or a test +asserting on it. Pass an explicit `value` to pin the key to something other than +the display text. + +--- + ## useDesignTokens(): no longer returns style-dictionary build metadata diff --git a/packages/components/src/components/Option/Option.browser.test.tsx b/packages/components/src/components/Option/Option.browser.test.tsx new file mode 100644 index 0000000000..9ff07a4ac6 --- /dev/null +++ b/packages/components/src/components/Option/Option.browser.test.tsx @@ -0,0 +1,123 @@ +import { Badge } from "@/components/Badge"; +import { Label } from "@/components/Label"; +import { Option } from "@/components/Option"; +import { Select } from "@/components/Select"; +import { Text } from "@/components/Text"; +import type { ReactNode } from "react"; +import { expect, test, vi } from "vitest"; +import { page } from "vitest/browser"; +import { render } from "vitest-browser-react"; + +/* + * The shape of the Select Default story: one option whose children are text + * plus a Badge, next to plain-text options. Its key used to fall back to + * react-aria's render-order counter (`react-aria-1`), because neither + * `textValue` nor `value` could be inferred from more than one child (#3028). + */ +const renderSelect = (props?: { + defaultValue?: string; + onChange?: (value: unknown) => void; +}) => + render( + , + ); + +const toggle = page.getByRole("button", { name: "Starship" }); + +const openOptions = async () => { + await toggle.click(); + await expect.element(page.getByRole("listbox")).toBeVisible(); +}; + +const optionKeys = () => + Array.from(document.querySelectorAll("[role='option']")).map((option) => + option.getAttribute("data-key"), + ); + +test("every option gets its text as its key, mixed children included", async () => { + renderSelect(); + await openOptions(); + + expect(optionKeys()).toEqual(["Millennium Falcon", "X-Wing", "TIE Fighter"]); +}); + +test("selecting an option with mixed children reports its text as the value", async () => { + const onChange = vi.fn(); + renderSelect({ onChange }); + await openOptions(); + + await page.getByRole("option", { name: /Millennium Falcon/ }).click(); + + expect(onChange).toHaveBeenCalledWith("Millennium Falcon"); +}); + +test("an option with mixed children can be targeted by defaultValue", async () => { + renderSelect({ defaultValue: "Millennium Falcon" }); + + await expect.element(toggle).toHaveTextContent("Millennium Falcon"); +}); + +test("the inferred textValue leaves out the text of element children", async () => { + renderSelect({ defaultValue: "Millennium Falcon" }); + + await expect.element(toggle).not.toHaveTextContent("Latest"); +}); + +/* + * With no text among its children there is nothing to infer, so `value` stays + * undefined and react-aria generates the key. That used to be silent — the only + * console output was react-aria's warning about `textValue`, which says nothing + * about the key becoming the option's form value. + */ +test("an option whose children carry no text of their own says so", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + render( + , + ); + + await expect + .poll(() => error.mock.calls.flat().join("\n")) + .toMatch(/Option.*no 'value'/s); + + error.mockRestore(); +}); + +test("an explicit value silences the warning and wins over the children", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const onChange = vi.fn(); + + const options: ReactNode = ( + + ); + + render( + , + ); + + await openOptions(); + expect(optionKeys()).toEqual(["mf"]); + + await page.getByRole("option", { name: /Millennium Falcon/ }).click(); + expect(onChange).toHaveBeenCalledWith("mf"); + expect(error.mock.calls.flat().join("\n")).not.toMatch(/Option.*no 'value'/s); + + error.mockRestore(); +}); diff --git a/packages/components/src/components/Option/Option.tsx b/packages/components/src/components/Option/Option.tsx index c25b381435..5254421d69 100644 --- a/packages/components/src/components/Option/Option.tsx +++ b/packages/components/src/components/Option/Option.tsx @@ -1,11 +1,11 @@ import { type PropsWithChildren } from "react"; -import { Children } from "react"; +import { Children, useEffect, useRef } from "react"; import * as Aria from "react-aria-components"; import clsx from "clsx"; import styles from "./Option.module.scss"; import type { FlowComponentProps } from "@/lib/componentFactory/flowComponent"; import { flowComponent } from "@/lib/componentFactory/flowComponent"; -import { extractTextFromFirstChild } from "@/lib/react/remote"; +import { extractTextFromChildren } from "@/lib/react/remote"; import { IconCheck } from "@/components/Icon/components/icons"; export interface OptionProps @@ -13,6 +13,16 @@ export interface OptionProps Omit, PropsWithChildren, FlowComponentProps { + /** + * The value this option contributes to the field's selection, and the key + * `defaultValue` / `value` of the surrounding field target. + * + * Defaults to `textValue`, which itself defaults to the text among the + * option's children — element children such as a `Badge` do not contribute, + * so `` is + * `"Millennium Falcon"`. Pass it explicitly when the value must not follow + * the display text, or when the children carry no text at all. + */ value?: string | number; } @@ -21,12 +31,14 @@ export const Option = flowComponent("Option", (props) => { const { className, children, - textValue = extractTextFromFirstChild(children), + textValue = extractTextFromChildren(children), value = textValue, ref, ...rest } = props; + useWarnMissingValue(value === undefined); + const rootClassName = clsx(styles.option, className); const hasChildren = Children.count(children) >= 1; @@ -46,4 +58,31 @@ export const Option = flowComponent("Option", (props) => { ); }); +/* + * Without an `id`, react-aria gives the item a key off a render-order counter + * (`react-aria-1`). That key is what the field reports as its selected value + * and what `defaultValue` has to match, so an option without a `value` silently + * submits a meaningless string and cannot be preselected — and the key shifts + * when unrelated markup around it changes. react-aria warns about the missing + * `textValue` in this situation but says nothing about the key, which is the + * expensive half (#3028). + * + * Warn in an effect rather than during render, so a double render in StrictMode + * does not log twice; the ref keeps it to once per option. + */ +const useWarnMissingValue = (isMissing: boolean): void => { + const hasWarned = useRef(false); + + useEffect(() => { + if (!isMissing || hasWarned.current) { + return; + } + + hasWarned.current = true; + console.error( + "An